storage: conditional puts do not respect the optional nature of value checksums

[couchand]

Upgrading a cluster across 75cbeb8 causes a never-ending stream of heartbeat failed on epoch increment errors, and the cluster is unable to serve requests.

Steps to reproduce:

• git checkout 75cbeb84848c807437ce84dcb93229d17fbb4a7e~ (note the ~)
• make
• start a cluster
• git checkout 75cbeb84848c807437ce84dcb93229d17fbb4a7e
• make
• restart that cluster

[bdarnell]

We see the same symptom in #22581.

[nvanbenschoten]

I just ran into this with local testing as well. I'm guessing one of the liveness CPuts is causing issues. Investigating now.

[nvanbenschoten]

Yeah, we're going to need CPut to ignore the checksum when performing its existing value comparison in mvccConditionalPutUsingIter. This normalization will need to be in place on all nodes before we can remove checksums from roachpb.Value across the board. I think we'll want to take the following steps:

1. revert 75cbeb8
2. add the normalization in mvccConditionalPutUsingIter to 2.0, along with the same normalization in other places where we compare roachpb.Value.RawBytes directly
3. reintroduce 75cbeb8 behind a 2.0 isMinVersion check
4. remove isMinVersion in 2.1

Even if we decide not to pursue steps 3 and 4, I think step 2 is still important. The reason for this is that even before 75cbeb8, value checksums were meant to be optional, so comparing rawBytes directly like we do in mvccConditionalPutUsingIter is violating this contract. This is why we need code like

cockroach/pkg/sql/sqlbase/rowfetcher.go

Lines 1060 to 1070 in b0b6553

	// We ignore the first 4 bytes of the values. These bytes are a
	// checksum which are not set by EncodeSecondaryIndex.
	if !indexEntry.Key.Equal(rf.rowReadyTable.lastKV.Key) {
	return scrub.WrapError(scrub.IndexKeyDecodingError, errors.Errorf(
	"secondary index key failed to round-trip encode. expected %#v, got: %#v",
	rf.rowReadyTable.lastKV.Key, indexEntry.Key))
	} else if !bytes.Equal(indexEntry.Value.RawBytes[4:], table.lastKV.Value.RawBytes[4:]) {
	return scrub.WrapError(scrub.IndexValueDecodingError, errors.Errorf(
	"secondary index value failed to round-trip encode. expected %#v, got: %#v",
	rf.rowReadyTable.lastKV.Value.RawBytes[4:], indexEntry.Value.RawBytes[4:]))
	}

[benesch]

Haven't thought this through entirely, but why can't you reintroduce 75cbeb8 behind a 2.0 isMinVersion check?

[nvanbenschoten]

Haven't thought this through entirely, but why can't you reintroduce 75cbeb8 behind a 2.0 isMinVersion check?

Yeah if we get step 2 in place by the 2.0 release then you're right that we can put it behind a 2.0 isMinVersion check.

[BramGruneir]

Adriatic is currently suffering the same fate.

[benesch]

Should be fixed if you pick up #22637.

[nvanbenschoten]

@BramGruneir Adriatic is no longer seeing this after #22637, right?

[BramGruneir]

Yep, all good. Wanted to let it bake a while and Adriatic is looking great right now.

[nvanbenschoten]

For posterity, I'm kind of skeptical that we can follow through with step 4:

4. remove isMinVersion in 2.1

The reason is that checksums are used so prevalently, so early into node startup, and at such low levels (for instance, in liveness heartbeats). Because of this, I don't know if we can ever rely on the check that clusters are upgraded through every major release to "bake in" the knowledge that a 2.1 node is talking to at least 2.0 nodes like we do for "baked in" SQL migrations.

[bdarnell]

We check versions very early in the RPC pipeline, so we should be confident that a 2.1 node will never connect to a pre-2.0 node.