serverutils: probabilistic shared process virtual cluster

[herkolategan]

This PR contain several commits that ultimately enables the probabilistic selection of a shared process test virtual cluster that tests run against, iff the test also probabilistically decided to run with a test tenant. Previously only an external process virtual cluster would be available as an option when deciding to run with a test virtual cluster.

Additionally some API changes have been made to serverutils to encourage the use of the test interface layers when working with a test server or cluster. Tests that use other utility methods to connect or create connection URLs may not be aware of a test virtual cluster and lack required information when interacting with the cluster resulting in flakiness and failures.

Finally this change also disables selecting a shared process virtual cluster instead of an external process virtual cluster for tests which fail with a shared process virtual cluster. An issue has been created to track these tests: #112857
Issues linked to this issue will contain more detail relating to the failure. And the issue has been referenced in the code itself when disabling a shared process virtual cluster for a specific test.

Resolves: #111134

[cockroach-teamcity]

This change is 

[herkolategan]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @DarrylWong, @dhartunian, @jayshrivastava, @srosenberg, and @yuzefovich)

---

pkg/server/testserver.go line 743 at r3 (raw file):

Previously, herkolategan (Herko Lategan) wrote…

I agree and I had the same thought, my only rotational was that I didn't fully comprehend the differences in capabilities between external and shared. But it's clear now that it's the same, except for I think can debug process works on external without explicitly setting it here. Busy merging these two, as well as taking the comment around selective capabilities into account.

Merged the two, but there is still a bit of work left I intend to complete in a follow-up PR: #107695, #113187

---

pkg/server/testserver.go line 763 at r3 (raw file):

Previously, herkolategan (Herko Lategan) wrote…

Will be done in addition to removing the duplication of process capabilities. I'll just ask for a second look once I'm done.

I've added the check you have mentioned as well.

[yuzefovich]

I have a few more comments, but it It might still be worth it to wait for @stevendanna to take a look at least at the testserver changes.

Reviewed 48 of 48 files at r6, 4 of 4 files at r7, 1 of 1 files at r8, 25 of 25 files at r9, 21 of 21 files at r10, 1 of 1 files at r11, 2 of 2 files at r12, 2 of 2 files at r13, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @DarrylWong, @dhartunian, @herkolategan, @jayshrivastava, and @srosenberg)

---

pkg/server/testserver.go line 762 at r12 (raw file):

	}

	// Waiting for capabilities can time To avoid paying this cost in all

nit: s/can time/can time./.

---

pkg/testutils/serverutils/test_server_shim.go line 105 at r6 (raw file):

		shared = false
	default:
		shared = util.ConstantWithMetamorphicTestBoolWithoutLogging("test-tenant-shared-process", false)

nit: what's the rationale for using metamorphic helper here? I'd have expected that here we'd give each mode 50% probability, and the current way will pick the external process more frequently.

---

pkg/testutils/serverutils/test_server_shim.go line 115 at r6 (raw file):

	// Obey the env override if present.
	if str, present := envutil.EnvString("COCKROACH_TEST_TENANT", 0); present {

It might be nice to have a way to force the default test tenant with a particular process mode. We could either adjust the existing env var to take in a string or something, or introduce another bool env var that controls whether the shared process is picked (I'd probably prefer the latter option slightly), or perhaps in some other fashion. This doesn't have to be in this PR, but please leave a TODO for this if it's not done.

---

pkg/sql/tests/show_commit_timestamp_test.go line 80 at r9 (raw file):

	testutils.RunTrueAndFalse(t, "pgx batch; simple", func(t *testing.T, simple bool) {
		resetTable(t)
		pgURL, cleanup := s.PGUrl(t)

nit: add .ApplicationLayer() here and below.

[blathers-crl]

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[herkolategan]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @adityamaru, @DarrylWong, @jayshrivastava, @srosenberg, and @yuzefovich)

---

pkg/server/testserver.go line 762 at r12 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: s/can time/can time./.

done.

---

pkg/testutils/serverutils/test_server_shim.go line 105 at r6 (raw file):

metamorphicBuildProbability = 0.8

Ah right, metamorphic eligible builds don't always run metamorphic, so the 50% bool selection is slanted ~40% for the default case.
I'll update this to use the randutil.

---

pkg/testutils/serverutils/test_server_shim.go line 115 at r6 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

It might be nice to have a way to force the default test tenant with a particular process mode. We could either adjust the existing env var to take in a string or something, or introduce another bool env var that controls whether the shared process is picked (I'd probably prefer the latter option slightly), or perhaps in some other fashion. This doesn't have to be in this PR, but please leave a TODO for this if it's not done.

Good idea; adding a TODO.

---

pkg/sql/tests/show_commit_timestamp_test.go line 80 at r9 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: add .ApplicationLayer() here and below.

good catch, done.

[stevendanna]

I really like where this landed. Thanks for pushing this forward and working to integrate it nicely into the existing test-server setup.

[herkolategan]

TFTRs!

bors r=yuzefovich,stevendanna

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)