server,testutils: remove complexity

[knz]

There is a saying (paraphrasing) that it always takes more work removing unwanted complexity than it takes to add it. This is an example of that.

Prior to this commit, there was an "interesting" propagation of the flag that decides whether or not to define a test tenant for test servers and clusters. In a nutshell, we had:

• an "input" flag in base.TestServerArgs, which remained mostly immutable
• a boolean decided once by ShouldStartDefaultTestTenant() either in:
  • serverutils.StartServerOnlyE
  • or testcluster.Start
• that boolean choice was then propagated to server.testServer via another boolean config flag in server.BaseConfig
• both the 2nd boolean and the original input flag were then again checked when the time came to do the work (in maybeStartDefaultTestTenant).

Additional complexity was then incurred by the need of TestCluster to make the determination just once (and not once per server).

This commit cuts through all the layers of complexity by simply propagating the choice of ShouldStartDefaultTestTenant() back into the TestServerArgs and only ever reading from that subsequently.

Release note: None
Epic: CRDB-18499

[cockroach-teamcity]

This change is 

[srosenberg]

There is a saying (paraphrasing) that it always takes more work removing unwanted complexity than it takes to add it. This is an example of that.

I thought the saying was, "complexity begets complexity"...from which emerges simplicity? :)

[yuzefovich]

Nice simplification!

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DarrylWong, @herkolategan, and @knz)

---

pkg/server/testserver.go line 567 at r1 (raw file):

// DefaultTestTenantDisabled is part of the serverutils.TenantControlInterface.
func (ts *testServer) DefaultTestTenantDisabled() bool {

nit: this method is called only in two places, so let's remove it from the interface altogether and, thus, address another TODO.

Update: I think both of those places are now removed.

---

pkg/testutils/serverutils/test_server_shim.go line 48 at r1 (raw file):

// DefaultTestTenantMessage is a message that is printed when a test is run
// with the default test tenant. This is useful for debugging test failures.
const DefaultTestTenantMessage = `

nit: I think DefaultTestTenantMessage and PreventStartTenantError can now be unexported.

---

pkg/testutils/testcluster/testcluster.go line 283 at r1 (raw file):

	// Find out how to do the default test tenant. Pick the setting
	// from either the first server with explicit args, or the top-level server args.
	defaultTestTenantOptions := tc.clusterArgs.ServerArgs.DefaultTestTenant

nit: should we have some validation here? Like I'm imagining it'd be unexpected for top-level server args to disable the test tenant whereas server-level args to explicitly ask for it. How about we require that default test tenant options are the same at the top level as well as at all servers, and only ignore unset args and unset default test tenant options (if per-server args are set)?

[knz]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DarrylWong, @herkolategan, and @yuzefovich)

---

pkg/server/testserver.go line 567 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: this method is called only in two places, so let's remove it from the interface altogether and, thus, address another TODO.

Update: I think both of those places are now removed.

Done.

---

pkg/testutils/serverutils/test_server_shim.go line 48 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: I think DefaultTestTenantMessage and PreventStartTenantError can now be unexported.

Done.

---

pkg/testutils/testcluster/testcluster.go line 283 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: should we have some validation here? Like I'm imagining it'd be unexpected for top-level server args to disable the test tenant whereas server-level args to explicitly ask for it. How about we require that default test tenant options are the same at the top level as well as at all servers, and only ignore unset args and unset default test tenant options (if per-server args are set)?

Good idea, done.

[herkolategan]

I definitely felt the entanglement in this area as well. Great improvement!
Noted that there are some failures on CI around the cluster server arg validation.

Reviewed 3 of 7 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DarrylWong and @yuzefovich)

[knz]

Noted that there are some failures on CI around the cluster server arg validation.

Yes these appeared after I added the extra consistency check requested by Yahor. They should be fixed now.

[yuzefovich]

Reviewed 3 of 4 files at r2, 6 of 7 files at r3, 3 of 3 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DarrylWong and @sumeerbhola)

[knz]

TFYRs!

bors r=yuzefovich,herkolategan

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)