engine/metamorphic: Add engine restarts, compare across runs

[itsbilal]

This PR builds on top of #44458 (all commits before the last one
are from that PR). It adds two things: one, it ensures that whenever
successive engine types are tested, it does a comparison for matching
outputs. This will ensure CI will fail if any change down the line
causes an observed difference in behaviour between rocksdb and pebble.

Furthermore, it adds a new kind of operation: restart. Restarts are
special in that they're added after the specified n number of runs
have happened; so a test run with 3 specified engine types
(so 2 restarts), and n = 10000 will have 3 * 10000 = 30000 operations.
A restart closes all open objects, then closes the engine, and starts
up the next engine in the specified engine sequence. This, combined
with the aforementioned checking functionality across different
engine runs, lets us test for bidirectional compatibility across
different engines.

Fixes #43762 .

Release note: None.

[cockroach-teamcity]

This change is 

[itsbilal]

The refactor to-dos about separating generation and execution that apply to #44458 also apply here. I'll do both refactors early in the week; the extra work in this PR is mostly orthogonal to those suggestions.

That's mostly why this PR is a WIP. That, and the code could do with some more cleanup / comments. But it works.

[itsbilal]

This is ready for review. I've added many more operations, and now I'm starting to see some differences with restarts enabled (eg. around MVCCFindSplitKey that I need to triage). Increasing operation coverage as well as testing more rocksdb/pebble optinos will yield interesting results I'm sure; right now we rarely have more than one non-L0 SST, which leaves a lot of things untested.

[petermattis]

Generally looks good, though I'm eager to see generation and execution detangled.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @itsbilal, @petermattis, and @sumeerbhola)

---

pkg/storage/engine/metamorphic/generator.go, line 36 at r1 (raw file):

		Attrs:           roachpb.Attributes{},
		Dir:             path,
		MustExist:       false,

Nit: there is no need to explicitly set fields to the zero value, though I sometimes do so for clarity. Are you doing so for clarity here? Seems like this initialization and the ones below could be removed. Ditto for Attrs.

---

pkg/storage/engine/metamorphic/generator.go, line 175 at r1 (raw file):

	deck := newDeck(m.rng, m.weights...)

	for numRestart := 0; numRestart <= m.numRestarts; numRestart++ {

This is a bit odd. Instead of running n operations, you'll be running n*(m.numRestarts+1) operations. And the restarts are not inserted randomly but performed after exactly n operations have been performed. I think this is another artifact of having generation tightly coupled to execution.

---

pkg/storage/engine/metamorphic/meta_test.go, line 163 at r1 (raw file):

	for _, seed := range seeds {
		t.Run(fmt.Sprintf("seed=%d", seed), func(t *testing.T) {
			// 2 engine sequences with 1 restart each.

Rather than exhaustively listing out the possibilities, I was thinking we'd use a bit of randomness here. Give the test the set of engines it can choose from and select a random engine from that set on each restart. Then you could test with RocksDB-only, Pebble-only, and RocksDB+Pebble. This would also work better in conjunction with generating a random number of restarts, rather than using the number of engines.

---

pkg/storage/engine/metamorphic/operations.go, line 775 at r1 (raw file):

			m.restart()
			// TODO(itsbilal): Write the old and new engine names here. Would be
			// helpful if the checker could ignore this.

The Pebble metamorphic test ignores "comments" in the output. The presence of comments has been very useful in diagnosing problems.

---

pkg/storage/engine/metamorphic/operations.go, line 781 at r1 (raw file):

		// The restart operation is special; it's added by the metaTestRunner
		// manually. To prevent it from being inadvertently added by the deck, set
		// weight to 0.

Is there a reason not to be adding it randomly? That's what we do in the Pebble metamorphic test.

[itsbilal]

TFTR! I've implemented random generation of restarts now. Also added comments, which the restart operation uses to print out before/after engine types.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @petermattis and @sumeerbhola)

---

pkg/storage/engine/metamorphic/generator.go, line 36 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Nit: there is no need to explicitly set fields to the zero value, though I sometimes do so for clarity. Are you doing so for clarity here? Seems like this initialization and the ones below could be removed. Ditto for Attrs.

Yeah, there's no point in doing this. Done.

---

pkg/storage/engine/metamorphic/generator.go, line 175 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

This is a bit odd. Instead of running n operations, you'll be running n*(m.numRestarts+1) operations. And the restarts are not inserted randomly but performed after exactly n operations have been performed. I think this is another artifact of having generation tightly coupled to execution.

Done. Made the restart operation yet another randomly-scheduled operation.

---

pkg/storage/engine/metamorphic/meta_test.go, line 163 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Rather than exhaustively listing out the possibilities, I was thinking we'd use a bit of randomness here. Give the test the set of engines it can choose from and select a random engine from that set on each restart. Then you could test with RocksDB-only, Pebble-only, and RocksDB+Pebble. This would also work better in conjunction with generating a random number of restarts, rather than using the number of engines.

Done.

---

pkg/storage/engine/metamorphic/operations.go, line 775 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

The Pebble metamorphic test ignores "comments" in the output. The presence of comments has been very useful in diagnosing problems.

Done. TODO removed as well.

---

pkg/storage/engine/metamorphic/operations.go, line 781 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Is there a reason not to be adding it randomly? That's what we do in the Pebble metamorphic test.

Done. No good reason for it, other than it made the restarts more controllable at the test runner level. But in practice it looks like random is the way to go; there are too many combinations, and manually iterating them then running each iteration is slower in practice.

[petermattis]

modulo a few minor comments.

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

---

pkg/storage/engine/metamorphic/generator.go, line 181 at r2 (raw file):

	m.closeAll()
	oldEngineName := m.engineImpls[m.curEngine].name
	m.curEngine++

I wonder if this should randomly chose the engine to restart into, rather than rotating amongst the engines.

---

pkg/storage/engine/metamorphic/meta_test.go, line 35 at r2 (raw file):

)

func runMetaTestForEngines(ctx context.Context, t *testing.T, seed int64, checkFile io.Reader, outputFile io.Writer, restarts bool, engineImpls []engineImpl) {

The number of arguments is getting difficult to read and error prone. You might want to define a struct to hold them.

---

pkg/storage/engine/metamorphic/operations.go, line 773 at r2 (raw file):

		name: "restart",
		run: func(ctx context.Context, m *metaTestRunner, args ...operand) string {
			if !m.restarts {

This seems oddly specific. When generation is untangled from execution, a better way to accomplish this is to be be able to define a different set of weights for a test which has a weight of 0 for restarts.

[itsbilal]

Thanks for the review! Also looking forward to cleaning up and separating generation/execution, once I'm through the issues this test is finding.

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

---

pkg/storage/engine/metamorphic/generator.go, line 181 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I wonder if this should randomly chose the engine to restart into, rather than rotating amongst the engines.

That would probably give interesting results. For now the guaranteed toggle is finding enough issues as-is. I can add a random restart mode down the line.

---

pkg/storage/engine/metamorphic/meta_test.go, line 35 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

The number of arguments is getting difficult to read and error prone. You might want to define a struct to hold them.

Done.

---

pkg/storage/engine/metamorphic/operations.go, line 773 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

This seems oddly specific. When generation is untangled from execution, a better way to accomplish this is to be be able to define a different set of weights for a test which has a weight of 0 for restarts.

Noted. It's possible to dynamically set the weight to 0 in metaTestRunner.init already, but the one somewhat nice side effect of no-op-ing this operation at runtime (like we do right now) is you can pass an output file from a run with restarts to a run without restarts to see if it finds a difference.

[petermattis]

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

---

pkg/storage/engine/metamorphic/generator.go, line 181 at r2 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

That would probably give interesting results. For now the guaranteed toggle is finding enough issues as-is. I can add a random restart mode down the line.

Ack. This is fine for now, though you could add a TODO about adding randomization in the future.

---

pkg/storage/engine/metamorphic/operations.go, line 773 at r2 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

Noted. It's possible to dynamically set the weight to 0 in metaTestRunner.init already, but the one somewhat nice side effect of no-op-ing this operation at runtime (like we do right now) is you can pass an output file from a run with restarts to a run without restarts to see if it finds a difference.

Good point about running the same set of operations with and without restarts. I think you should encapsulate these test-run specific options in some sort of metaTestOpts. The Pebble metamorphic test provides something similar in its testOpts structure, which is persisted to disk. This is a suggestion for the future. The current code is fine for this PR.

[itsbilal]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @itsbilal, @petermattis, and @sumeerbhola)

---

pkg/storage/engine/metamorphic/generator.go, line 181 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Ack. This is fine for now, though you could add a TODO about adding randomization in the future.

Done.

[itsbilal]

Reduced operation count to 1000 by default; otherwise, make stress was timing out in CI. The count can now be configured with --operations <count>.

[itsbilal]

TFTR!

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)