kvserver: remove consistency checker diffs

[tbg]

We are discussing supporting larger ranges and making first steps in their implementation, see for example #16954.

The consistency checker on failure currently requests a "diff" but this diff is implemented by sending a whole snapshot through memory (in CollectChecksumResponse). This won't work any more once ranges are allowed to be larger than a small fraction of memory.

gz#9424

Jira issue: CRDB-5898

[petermattis]

In discussion today, @sumeerbhola was wondering how we compute the consistency checker diff. The answer is we buffer the entire Range data and send it in a single response. I tracked down this issue from this comment in the code:

  // TODO(tschottdorf): with larger ranges, this is no longer tenable.
  // See https://github.com/cockroachdb/cockroach/issues/21128.
  roachpb.RaftSnapshotData snapshot = 2;

Cc @ajwerner, @andy-kimball, and @johnrk as this throws a bit of a wrench into larger range sizes.

[tbg]

Just in case this isn't clear, the diff is requested only after a first round of the check already failed. It's still bad.

[petermattis]

I suspect the solution here would be to add a streaming RPC interface.

[ajwerner]

I suspect the solution here would be to add a streaming RPC interface.

This isn't going to be a readily back-portable change. This makes me nervous.

[tbg]

How big are the ranges in 20.1? We can always not send a range data snapshot if it would be too large. It would be unfortunate to see this in practice, but the data checkpoints would be there anyway, so we could reconstruct the diff manually later.

[ajwerner]

How big are the ranges in 20.1?

range_max_size defaults to 512 MiB and range_min_size defaults to 128 MiB. This is likely to be bad on tiny deployments but fine on most deployments.

[petermattis]

This isn't going to be a readily back-portable change. This makes me nervous.

This is a good point. Disabling sending range snapshot data for large ranges might be the only back-portable change. My suggestion was more about a long-term fix. We actually already have a facility for streaming range data that we use for sending snapshots. Doesn't seem like it could be easily adapted here, though.

[erikgrinaker]

We're doing some adjacent work over in #77432, and plan to remove the consistency checker diffs while we're at it, since they're pretty unwieldy at 512 MB. This involves removing the following fields and functionality:

• roachpb.CheckConsistencyRequest.WithDiff
• roachpb.ComputeChecksumRequest.Snapshot
• roachpb.CollectChecksumResponse.Snapshot
• roachpb.RaftSnapshotData

It might be possible to do this without requiring a version gate and keeping the old code around for mixed-version clusters, because the current code will gracefully ignore responses that don't have a snapshot:

cockroach/pkg/kv/kvserver/replica_consistency.go

Lines 160 to 168 in 7f61dfd

	minoritySnap := results[shaToIdxs[minoritySHA][0]].Response.Snapshot
	curSnap := results[shaToIdxs[sha][0]].Response.Snapshot
	if sha != minoritySHA && minoritySnap != nil && curSnap != nil {
	diff := diffRange(curSnap, minoritySnap)
	if report := r.store.cfg.TestingKnobs.ConsistencyTestingKnobs.BadChecksumReportDiff; report != nil {
	report(*r.store.Ident, diff)
	}
	buf.Printf("====== diff(%x, [minority]) ======\n%v", redact.Safe(sha), diff)
	}

However, we'll need some mechanism to obtain such diffs for debugging. We already take Pebble checkpoints when inconsistencies are found, so we'll need to write e.g. a cockroach debug diff-checkpoints CLI commands to compare them between good/bad nodes:

cockroach/pkg/kv/kvserver/replica_consistency.go

Lines 785 to 798 in 7f61dfd

	if cc.Checkpoint {
	sl := stateloader.Make(r.RangeID)
	as, err := sl.LoadRangeAppliedState(ctx, snap)
	if err != nil {
	log.Warningf(ctx, "unable to load applied index, continuing anyway")
	}
	// NB: the names here will match on all nodes, which is nice for debugging.
	tag := fmt.Sprintf("r%d_at_%d", r.RangeID, as.RaftAppliedIndex)
	if dir, err := r.store.checkpoint(ctx, tag); err != nil {
	log.Warningf(ctx, "unable to create checkpoint %s: %+v", dir, err)
	} else {
	log.Warningf(ctx, "created checkpoint %s", dir)
	}
	}

This could reuse the existing diff code:

cockroach/pkg/kv/kvserver/replica_consistency_diff.go

Lines 82 to 91 in ee57d72

	// diffs the two kv dumps between the lease holder and the replica.
	func diffRange(l, r *roachpb.RaftSnapshotData) ReplicaSnapshotDiffSlice {
	if l == nil || r == nil {
	return nil
	}
	var diff ReplicaSnapshotDiffSlice
	diff = append(diff, diffKVs(l.KV, r.KV)...)
	diff = append(diff, diffRangeKeys(l.RangeKV, r.RangeKV)...)
	return diff
	}

[sumeerbhola]

and plan to remove the consistency checker diffs while we're at it, since they're pretty unwieldy at 512 MB

Are we going to lose diff support?

[pav-kv]

Likely yes, in its current form (an API that transfers the entire range). But we want diffs to be available in some form out-of-band, e.g. have tooling for constructing it from the checkpoints saved on replicas.

We could also explore some forms of partial/sampled diffing, e.g. detect one sub-range with a mismatch, and diff only that part.

[pav-kv]

One option for a partial diff:

• Have each replica compute a sequence of hashes instead of one hash, spaced out (deterministically) across the range so that there aren't too many of them, and each hash spans a relatively small size. ~10-100 would do.
• Compare these sequences of hashes in the first phase of the diff algo. (where we previously compared a single hash)
• When there is a mismatch, drill down into the first hash that mismatches: exchange the corresponding spans of keys and diff them. (like we did before for the whole range)

We could purposefully limit this to 1 mismatching key (it's the worst case anyway, if there is one large k/v entry), or to some reasonable byte-size worth of keys, and leave full analysis to the offline tooling.

One advantage of retaining some inline diffing (instead of fully removing it) is in having somewhat more informative consistency check failures should they occur in tests like #87106. @tbg How would a human debug those things? I imagine that I would maybe look at the first few mismatching entries in this diff and try to understand what's up with them, but there is probably benefit in seeing larger patterns in it too.

[tbg]

A human would always investigate the checkpoints. The diff would serve the guide them as to which keys are of interest, but there isn't a huge upshot in having the diff available upfront (and if there were we could build the retrieval into roachtest). The diff alone isn't that useful, though it would be useful, but not at the expense of lots of engineering.

[pav-kv]

A human would always investigate the checkpoints.

SGTM.

I’m thinking of the following flow when an inconsistency happens:

1. Like today, crash suspicious nodes (those in the minority when SHA mismatches).
   • This will get surfaced to admins via some crash reporting mechanism.
   • The crash message should have enough information to guide the investigation.
2. Checkpoint all replicas (we need at least one good, for diffing and analysis; but it’s better to have all of them, because even the majority can be “wrong” - e.g. leader may pass its corrupted snapshot to some followers).
3. Admin uses cockroach debug tool to analyse and eventually remove the checkpoints.

Issues:

1. Checkpoints become expensive over time, so they should be deleted timely (on the good nodes).
2. This may not scale if there are too many failures like this.
   • Could be solved with some automatic checkpoint GC.
3. A minority node may never learn that it needs to crash because of an inconsistency (it may crash for another reason, get stuck, etc). So the admins may miss this event.
4. The thing that I still don’t like about the checkpoint is that it’s not granular per range, so checkpointing good replicas is costly.
   • We may want to save "lightweight" checkpoints (specific to the range) on all the nodes that are not killed.

[erikgrinaker]

Checkpoints become expensive over time, so they should be deleted timely (on the good nodes).

We should clearly call this out in the crash message, so that admins know they'll have to remove this when they no longer need it. I think that should be sufficient for now. We may want to e.g. log a message on startup if there are any checkpoints too. It's worth pinging the SREs real quick about how they'd like this to be surfaced.

@joshimhoff When a consistency check fails, we leave behind a checkpoint of the current Pebble state on the relevant nodes (with replicas for that range). These are needed to debug the inconsistency. These are never removed, so over time they'll essentially be an old copy of the node's database. How would SREs prefer to be notified about these checkpoints, which will need to be removed manually when no longer needed for debugging? Currently, the inconsistent nodes go into a panic loop with a log message, so the simplest solution is to just call this out in the crash message -- but we'll also keep checkpoints on the good nodes that will need to be removed. Is that sufficient?

This may not scale if there are too many failures like this.

We essentially never see these failures, so let's not optimize for it.

The thing that I still don’t like about the checkpoint is that it’s not granular per range, so checkpointing good replicas is costly.

True, but they're cheap to take -- they're only expensive to keep for a long time. Let's leave the GC responsibility to admins for now, and not optimize for it.

[joshimhoff]

To answer your Q, I need to ask some Qs first:

If a consistency check fails, will we definitely get a panic crash loop that doesn't end until some operator takes an action? Or could we get a small number of crashes that go away without some operator taking action? I see you say "panic loop" but want to make sure I get what that means exactly.

If an operator will need to take an action, what actions might they take?

[erikgrinaker]

If a consistency check fails, will we definitely get a panic crash loop that doesn't end until some operator takes an action?

The node will keep crashing forever, by design. The node is borked and must be replaced.

However, only the node(s) with the inconsistencies will crash. We'll also leave checkpoints behind on the nodes with correct replicas.

If an operator will need to take an action, what actions might they take?

1. Make copies of the checkpoints, and either give them to CRL staff for debugging, or run some tools that produce a report to give to CRL staff.
2. Replace the crashing nodes that have inconsistencies.
3. Remove the checkpoints from all healthy nodes that have them.

[erikgrinaker]

The thing that I still don’t like about the checkpoint is that it’s not granular per range, so checkpointing good replicas is costly.

@pavelkalinnikov One consideration here is that we may not want to have to copy the entire checkpoint around to run the diff tool. It may be better to have a two-stage process: one command to extract replica data from a checkpoint, then copy that replica data somewhere and run a tool to diff them.

[joshimhoff]

The node will keep crashing forever, by design. The node is borked and must be replaced.

Ack! Then I'm confident SRE will notice the inconsistency, if the alerting stack functions as designed.

How would SREs prefer to be notified about these checkpoints, which will need to be removed manually when no longer needed for debugging?

Here are some options, in order of how likely it is that CRL actually cleans up the checkpoints in a cloud context:

1. Implement GC of checkpoints in CRDB itself.
2. Increment a prometheus gauge or similar that indicates when there are checkpoints on disk to clean up. SRE adds an alert in case the gauges fires. The alert links to a playbook that says to clean up the checkpoints, if they are no longer needed.
3. Just mention the checkpoints in CRDB logs. Also have an SRE playbook re: corruption that mentions the need to clean up the checkpoints.

If we go with 3, I'm not confident we will clean up checkpoints, but maybe that is okay right now, given that a corruption crash loop has never happened in cloud according to my memory, and given that the checkpoints just increase disk utilization.

[tbg]

A gauge seems doable and maybe a good way to solve it reliably.

diff --git a/pkg/kv/kvserver/store.go b/pkg/kv/kvserver/store.go
index fceda5df42..c6cd380766 100644
--- a/pkg/kv/kvserver/store.go
+++ b/pkg/kv/kvserver/store.go
@@ -3303,11 +3303,15 @@ func (s *Store) updateReplicationGauges(ctx context.Context) error {
 	return nil
 }
 
+func (s *Store) checkpointDir() string {
+	return filepath.Join(s.engine.GetAuxiliaryDir(), "checkpoints")
+}
+
 // checkpoint creates a RocksDB checkpoint in the auxiliary directory with the
 // provided tag used in the filepath. The filepath for the checkpoint directory
 // is returned.
 func (s *Store) checkpoint(ctx context.Context, tag string) (string, error) {
-	checkpointBase := filepath.Join(s.engine.GetAuxiliaryDir(), "checkpoints")
+	checkpointBase := s.checkpointDir()
 	_ = s.engine.MkdirAll(checkpointBase)
 
 	checkpointDir := filepath.Join(checkpointBase, tag)
@@ -3337,6 +3341,19 @@ func (s *Store) ComputeMetrics(ctx context.Context, tick int) error {
 	m := s.engine.GetMetrics()
 	s.metrics.updateEngineMetrics(m)
 
+	{
+		cpDirs, err := s.engine.List(s.checkpointDir())
+		var n int64
+		if err != nil {
+			// TODO probably need to tolerate NotFound here,
+			n = math.MaxInt64
+		} else {
+			n = int64(len(cpDirs))
+		}
+
+		someGauge.Update(n)
+	}
+
 	// Get engine Env stats.
 	envStats, err := s.engine.GetEnvStats()
 	if err != nil {

[erikgrinaker]

Yeah, let's do a gauge and a log message (on crash and on node startup). GC is going to be more work than it's worth for something that's very rare.

[joshimhoff]

Works for me! Send me the PR adding the gauge, and I'll make sure we add an alert on the CC side.

[pav-kv]

SGTM, thank you all for the input. A gauge seems strictly more reliable because of the issue that I pointed out above:

A minority node may never learn that it needs to crash because of an inconsistency (it may crash for another reason, get stuck, etc). So the admins may miss this event.

A gauge will be increased on all the alive replicas, so this signal will propagate.

[pav-kv]

It may be better to have a two-stage process: one command to extract replica data from a checkpoint, then copy that replica data somewhere and run a tool to diff them.

For the replica extraction step, it looks like one of SSTWriter variants would be reusable:

cockroach/pkg/storage/sst_writer.go

Lines 74 to 76 in fdbeda5

	// MakeBackupSSTWriter creates a new SSTWriter tailored for backup SSTs which
	// are typically only ever iterated in their entirety.
	func MakeBackupSSTWriter(ctx context.Context, cs *cluster.Settings, f io.Writer) SSTWriter {

cockroach/pkg/storage/sst_writer.go

Lines 102 to 105 in fdbeda5

	// MakeIngestionSSTWriter creates a new SSTWriter tailored for ingestion SSTs.
	// These SSTs have bloom filters enabled (as set in DefaultPebbleOptions) and
	// format set to RocksDBv2.
	func MakeIngestionSSTWriter(

Are there any other options? How are range snapshots encoded, is it by MakeIngestionSSTWriter or something else?

Between "ingestion" and "backup", I will probably start with the latter. The former is interesting though if we imagine wanting to recover inconsistent replicas after the investigation. We can look at this option later, as for now the recovery/upreplication will happen as a result of some nodes crashing.

[erikgrinaker]

I'd go with MakeIngestionSSTWriter, since it's more general purpose and uses typical options (it e.g. has bloom filters enabled and smaller blocks for faster seeks). Range snapshots use MakeIngestionSSTWriter.

[sumeerbhola]

Does the node crashing get triggered by a raft command on a replica? That would make that replica internally consistent in its state machine RaftAppliedIndex+state machine state, but not necessarily for other replicas that may be in the middle of doing a sequence of (atomic ingest of AddSSTable, atomic update of RaftAppliedIndex). Is the offline tool going to try to diff other replicas too?

[erikgrinaker]

Does the node crashing get triggered by a raft command on a replica

Yes.

Is the offline tool going to try to diff other replicas too?

No. As you point out, only this range's replicas are guaranteed to be consistent.