storage: protect ComputeChecksum commands from replaying

[benesch]

Previously, a ComputeChecksum command could apply twice with the same
ID. Consider the following sequence of events:

1. DistSender sends a ComputeChecksum request to a replica.
2. The request is succesfully evaluated and proposed, but a connection
   error occurs.
3. DistSender retries the request, leaving the checksum ID unchanged!

This would result in two ComputeChecksum commands with the same checksum
ID in the Raft log. Somewhat amazingly, this typically wasn't
problematic. If all replicas were online and reasonably up-to-date,
they'd see the first ComputeChecksum command, compute its checksum, and
store it in the checksums map. When they saw the duplicated
ComputeChecksum command, they'd see that a checksum with that ID already
existed and ignore it. In effect, only the first ComputeChecksum command
for a given checksum ID mattered.

The problem occured when one replica saw one ComputeChecksum command but
not the other. There were two ways this could occur. A replica could go
offline after computing the checksum the first time; when it came back
online, it would have an empty checksum map, and the checksum computed
for the second ComputeChecksum command would be recorded instead. Or a
replica could receive a snapshot that advanced it past one
ComputeChecksum but not the other. In both cases, the replicas could
spuriously fail a consistency check.

A very similar problem occured with range merges because ComputeChecksum
requests are incorrectly ranged (see #29002). That means DistSender
might split a ComputeChecksum request in two. Consider what happens when
a consistency check occurs immediately after a merge: the
ComputeChecksum request is generated using the up-to-date, post-merge
descriptor, but DistSender might have the pre-merge descriptors cached,
and so it splits the batch in two. Both halves of the batch would get
routed to the same range, and both halves would have the same command
ID, resulting in the same duplicated ComputeChecksum command problem.

The fix for these problems is to assign the checksum ID when the
ComputeChecksum request is evaluated. If the request is retried, it will
be properly assigned a new checksum ID. Note that we don't need to worry
about reproposals causing duplicate commands, as the MaxLeaseIndex
prevents proposals from replay.

The version compatibility story here is straightforward. The
ReplicaChecksumVersion is bumped, so v2.0 nodes will turn
ComputeChecksum requests proposed by v2.1 nodes into a no-op, and
vice-versa. The consistency queue will spam some complaints into the log
about this--it will time out while collecting checksums--but this will
stop as soon as all nodes have been upgraded to the new version.†

Note that this commit takes the opportunity to migrate
storagebase.ReplicatedEvalResult.ComputeChecksum from
roachpb.ComputeChecksumRequest to a dedicated
storagebase.ComputeChecksum message. Separate types are more in line
with how the merge/split/change replicas triggers work and avoid
shipping unnecessary fields through Raft. Note that even though this
migration changes logic downstream of Raft, it's safe. v2.1 nodes will
turn any ComputeChecksum commands that were commited by v2.0 nodes into
no-ops, and vice-versa, but the only effect of this will be some
temporary consistency queue spam. As an added bonus, because we're
guaranteed that we'll never see duplicate v2.1-style ComputeChecksum
commands, we can properly fatal if we ever see a ComputeChecksum request
with a checksum ID that we've already computed.

† It would be possible to put the late-ID allocation behind a cluster
version to avoid the log spam, but that amounts to allowing v2.1 to
initiate known-buggy consistency checks. A bit of log spam seems
preferable.

Fix #28995.

[cockroach-teamcity]

This change is 

[tbg]

Reviewed 11 of 11 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/storage/batcheval/cmd_compute_checksum.go, line 49 at r1 (raw file):

	if args.Version != ReplicaChecksumVersion {
		log.Errorf(ctx, "Incompatible versions: e=%d, v=%d", ReplicaChecksumVersion, args.Version)

I would downgrade this from Errorf to Infof and make the message more benign.

log.Infof(ctx, "ignoring checksum request due to version mismatch (request: %d, local: %d)", args.Version, ReplicaChecksumVersin)

[benesch]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/storage/batcheval/cmd_compute_checksum.go, line 49 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

I would downgrade this from Errorf to Infof and make the message more benign.

log.Infof(ctx, "ignoring checksum request due to version mismatch (request: %d, local: %d)", args.Version, ReplicaChecksumVersin)
</blockquote></details>

Good idea. Done.


<!-- Sent from Reviewable.io -->

[benesch]

bors r=tschottdorf

[tbg]

Reviewed 1 of 1 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

[craig]

Merge conflict (retrying...)

[benesch]

Uh, bors crashed.

bors r=tschottdorf

[craig]

Build succeeded

• GitHub CI (Cockroach)