sql/logictest: add metamorphic param for global MVCC range tombstone

[erikgrinaker]

kvserver: add GlobalMVCCRangeTombstone testing knob

This patch adds a set of testing knobs to enable writing an MVCC range
tombstone at the bottom of the keyspace during cluster bootstrapping.
This also subsumes the previous mechanism to trigger this from the
envvar COCKROACH_GLOBAL_MVCC_RANGE_TOMBSTONE, for e.g. roachtests and
roachperf benchmarks.

Release justification: non-production code changes

Release note: None

sql/logictest: add metamorphic param for global MVCC range tombstone

This patch adds a metamorphic parameter for the SQL logic tests that
will randomly write a global MVCC range tombstone across the entire user
keyspace during cluster bootstrapping. This should not semantically
affect the test data written above it, but will activate MVCC range
tombstone code paths in the storage layer for testing.

Touches #84042.

Release justification: non-production code changes

Release note: None

[cockroach-teamcity]

This change is 

[erikgrinaker]

Looks like this won't generate the config for 3node-tenant-range-tombstone for some reason. I have to run now, but would appreciate some early input on this @jordanlewis (or whoever else is appropriate), and I'll wrap it up when I'm back from PTO.

[jordanlewis]

@rytaft could you please find an reviewer here?

[yuzefovich]

Looks like this won't generate the config for 3node-tenant-range-tombstone for some reason.

My guess is that because you didn't explicitly specify this new config in any of the logic test files, there is no target that would trigger this generation. You might want to include this new config into ThreeNodeTenantDefaultConfigNames in logictestbase.go so that the new config would run against most logic test files. It's possible that you also need to make some adjustments to pkg/cmd/generate-logictest/main.go to include into ConfigOverrides - not sure about that one.

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

---

pkg/kv/kvserver/client_raft_test.go line 83 at r2 (raw file):

}

func TestXXX(t *testing.T) {

nit: this test seems out of place.

[erikgrinaker]

It's possible that you also need to make some adjustments to pkg/cmd/generate-logictest/main.go to include into ConfigOverrides

This did the trick, thanks!

I think this should be good to go now, but I mostly cargo-culted this so let me know if there are any other configs that would be more appropriate or anything.

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

---

pkg/kv/kvserver/client_raft_test.go line 83 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: this test seems out of place.

Whoops, yeah, this was accidentally committed.

[yuzefovich]

Although it might be worth considering a somewhat different approach - rather than introducing some new configs that explicitly use the range tombstones, why not do that randomly on all existing configs? I.e. with 50% chance for each logic test run put the global range tombstone. This seems like it would give us better coverage (e.g. local and 5node configs would be used too) while also keeping the number of tests smaller.

Reviewed 7 of 9 files at r1, 31 of 31 files at r3, 32 of 32 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @tbg)

[erikgrinaker]

Yeah, I think you're right -- done!

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

[tbg]

Pinged #flaky-test-fighter about version-upgrade failure on this PR.

I'll defer to @yuzefovich on the randomization. It seems to make sense - we don't expect any individual test to choke on these (ok maybe some will find a way to accidentally detect the tombstone), and an opt-out pattern would be preferable.

Ah, you just pushed that.

Reviewed 7 of 9 files at r1, 31 of 31 files at r3, 31 of 31 files at r5, 31 of 31 files at r6, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @erikgrinaker)

---

pkg/server/config.go line 240 at r3 (raw file):

	// key-specific code paths in the storage layer. We'll also have to tweak
	// rangefeeds and batcheval to not choke on it.
	if envutil.EnvOrDefaultBool("COCKROACH_GLOBAL_MVCC_RANGE_TOMBSTONE", false) {

I thought we had some (linted) convention to put this in var declarations in the global scope, but I am evidently wrong since CI doesn't have a failed lint. Carry on, I guess?

[erikgrinaker]

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

---

pkg/server/config.go line 240 at r3 (raw file):

Previously, tbg (Tobias Grieger) wrote…

I thought we had some (linted) convention to put this in var declarations in the global scope, but I am evidently wrong since CI doesn't have a failed lint. Carry on, I guess?

We do the same thing elsewhere, e.g. earlier in this file, so I figured it was fine -- especially since this is called once during server startup. I'd be more inclined to use a global var if we read it multiple times.

[yuzefovich]

Reviewed 31 of 31 files at r5, 30 of 31 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @tbg)

[erikgrinaker]

TFTRs! CI failures are known flakes.

bors r=tbg,yuzefovich

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)