kvserver: deflake acceptance/version-upgrade

[irfansharif]

Fixes #52627.

In #50938 we were careful to not transmit non-zero MVCCStats through the
replica proposal codepaths, but there seems to have been another
instance where we end up transmitting MVCCStats across replicas - during
splits. We similarly zero AbortSpanBytes out when VersionAbortSpanBytes
is inactive to avoid replica divergence.

Release note: None

[cockroach-teamcity]

This change is 

[irfansharif]

I couldn't actually repro the failure observed, but I'm pretty sure this is what's needed. I can try stressing again tomorrow with and without this patch to confirm. I tried mixed version clusters and generating splits manually but still was unable to. I probably wasn't creating abort span bytes in doing so. Was trying:

 make buildshort; 
 roachprod destroy local; 
 roachprod create local -n 4; 
 roachprod put local:1-3 (which cockroach20.1) ./cockroach; 
 roachprod put local:4 bin/workload ./workload; 
 roachprod start local:1-3
 roachprod run local:4 './workload run kv --init --splits 10000 --tolerate-errors {pgurl:2}' &
 echo "sleeping..";
 sleep 3;
 roachprod put local:2 ./cockroachshort ./cockroach; 
 roachprod stop local:2;
 roachprod start local:2;

[tbg]

Are you sure? If my reading of the callers of this code is correct, deltaPostSplitLeft will essentially be ms, meaning that the existing zeroing out should override it just fine. Don't you want to massage the RHSDelta above? That would make more sense to me as it is used to seed the stats for the RHS, and will not go through the existing check.

[irfansharif]

Looking at the snippet @chrisseto posted:

I200812 19:15:29.100791 4262 kv/kvserver/replica_command.go:414  [n1,s1,r11/1:/Table/1{5-6}] initiating a split of this range at key /Table/15/3 [r997] (manual)
...
E200812 19:16:07.986964 6820 kv/kvserver/replica_consistency.go:162  [n1,consistencyChecker,s1,r997/1:/Table/1{5/3-6}] (n1,s1):1: checksum a67704dbec62d0c7a063e8528bf2cb2e4dc3ce4f83d0b700b16c271f27afe7e52b7533774d9607c33ce5c8d8d7f3ec91cdf7571a3d7d7b0ed197c7df4a0d4214 [minority]

I'm inclined to say this is the patch we're looking for.

[tbg]

Oh, tricky! Good catch. I think you got the wrong side of the split, but this does seem like a problem. I hope you can reproduce before and after as this is all tricky enough that there could be yet another problem.

[irfansharif]

Hm, I think we need both. Something like:

@@ -1036,6 +1033,10 @@ func splitTriggerHelper(
        var pd result.Result
        pd.Replicated.Split = &kvserverpb.Split{
          SplitTrigger: *split,
          // NB: the RHSDelta is identical to the stats for the newly created right
          // hand side range (i.e. it goes from zero to its stats).
          RHSDelta: *h.AbsPostSplitRight(),
        }

        deltaPostSplitLeft := h.DeltaPostSplitLeft()
        if !rec.ClusterSettings().Version.IsActive(ctx, clusterversion.VersionContainsEstimatesCounter) {
          deltaPostSplitLeft.ContainsEstimates = 0
        }

        if !rec.ClusterSettings().Version.IsActive(ctx, clusterversion.VersionContainsEstimatesCounter) {
                deltaPostSplitLeft.ContainsEstimates = 0
        }
+       if !rec.ClusterSettings().Version.IsActive(ctx, clusterversion.VersionAbortSpanBytes) {
+               deltaPostSplitLeft.AbortSpanBytes = 0
+               pd.Replicated.Split.RHSDelta.AbortSpanBytes = 0
+       }
        return deltaPostSplitLeft, pd, nil
 }

But yes, I'll have to repro instead of wildly speculating.

[irfansharif]

@tbg Added a (contrived) test that confirms the patch. Happy to not check-in the test into our repo, though I did try to generalize it a bit.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @madelineliao and @tbg)

---

pkg/kv/kvserver/batcheval/cmd_end_transaction.go, line 1039 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Are you sure? If my reading of the callers of this code is correct, deltaPostSplitLeft will essentially be ms, meaning that the existing zeroing out should override it just fine. Don't you want to massage the RHSDelta above? That would make more sense to me as it is used to seed the stats for the RHS, and will not go through the existing check.

Done.

[tbg]

Hm, I think we need both.

Interesting, but why? deltaPostSplitLeft is newMS in this code here:

cockroach/pkg/kv/kvserver/batcheval/cmd_end_transaction.go

Lines 596 to 599 in 06cd1cb

	newMS, trigger, err := splitTrigger(
	ctx, rec, batch, *ms, ct.SplitTrigger, txn.WriteTimestamp,
	)
	*ms = newMS

so we use it as the "main" stats (which makes sense), so its AbortSpanBytes should be zeroed out by the existing check before proposal. Hmm, your code only zeroes the RHS, so you've come around then?

For the test, I wonder if we can "amend" the existing main version-upgrade test instead of adding a new one? I think it's a good idea to have some more generic splitting going on in that one.

Thanks for the fix!

Reviewed 7 of 7 files at r2, 2 of 2 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif and @madelineliao)

---

pkg/kv/kvserver/batcheval/cmd_end_transaction.go, line 1034 at r3 (raw file):

	deltaPostSplitLeft := h.DeltaPostSplitLeft()
	if !rec.ClusterSettings().Version.IsActive(ctx, clusterversion.VersionContainsEstimatesCounter) {
		deltaPostSplitLeft.ContainsEstimates = 0

Heh, by my same reasoning, this is not necessary but it's necessary on the right. I don't think splits ever come out with ContainsEstimates set, on the RHS, though. Too late now anyway. :-)

[irfansharif]

Interesting, but why?

Yea I was mistaken, I walked through the codepaths after and realized what you were saying. I'll try seeing what the amended version of version-upgrade looks like and abandon this other test, but I might also defer to later given it's holding up the release. We could probably run workload kv against it in the same way I did here. TFTR!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif and @madelineliao)

[irfansharif]

bors r+

[craig]

🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

[irfansharif]

bors r+

[craig]

Build succeeded:

• Compile Build (Cockroach)