serverutils: clean up some interfaces

[knz]

Epic: CRDB-18499
Part of solving #107058.
Informs #106772.

In this commit:

• TestTenantInterface renamed to ApplicationLayerInterface.

• TestServerInterface split into:

  • StorageLayerInterface containing most of the methods previously
    in TestServerInterface.

  • TenantControlInterface with just the methods relating to
    starting the service for secondary tenants.

• Relevant methods moved from TestServerInterface to
  ApplicationLayerInterface:

  • ServingSQLAddr() - renamed to AdvSQLAddr()
  • RPCAddr()
  • LeaseManager()
  • DistSenderI()
  • MigrationServer()
  • SetDistSQLSpanResolver()
  • CollectionFactory()
  • SystemTableIDResolver()

• Removed HostSQLAddr() - use .SystemLayer().AdvSQLAddr() instead.

• AdvSQLAddr() (previously ServingSQLAddr()) was
  simplified to always return the advertised SQL address for the
  current layer.

• Updated comments/docstrings for the interfaces.

Release note: None

[cockroach-teamcity]

This change is 

[stevendanna]

I like the rename of TenantOrServer for sure and I also like the addition of SystemLayer().

I could see an argument for keeping an embedded ApplicationLayerInterface if we find transitioning tests to this API too difficult, but I'd prefer that we get to this more explicit access if we can.

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling, @herkolategan, @shermanCRL, @smg260, and @yuzefovich)

---

-- commits line 29 at r4:

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: probably worth calling out the change in ServingSQLAddr (which was the only place that had different logic when default test tenant was started), removal of HostSQLAddr method, and inclusion of QueryTableID and friends methods.

Done.

---

-- commits line 50 at r7:

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: did you mean "important interfaces"?

Indeed. Thanks

---

pkg/server/testserver.go line 462 at r4 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: XXes on SQLInstanceID and StatusServer in the first commit.

Fixed.

---

pkg/testutils/serverutils/api.go line 466 at r7 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: should TenantCapabilitiesReader be in TenantControlInterface?

I don't feel it should - it's not a control interface.

---

pkg/testutils/serverutils/test_server_shim.go line 161 at r4 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: perhaps for the follow-up, but should we rename base.TestTenantArgs and alike too?

Let's do it in a different PR.

[herkolategan]

Lovely separation of concerns, and a welcome improvement. I think this will help overall readability in the tests as well. Didn't see anything I don't agree with. Thanks for doing this!

Reviewed 70 of 102 files at r3, 3 of 20 files at r4, 3 of 117 files at r8, 115 of 115 files at r12, 1 of 1 files at r13, 112 of 112 files at r14, 2 of 2 files at r15, 4 of 4 files at r16, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling, @shermanCRL, @smg260, and @yuzefovich)

[yuzefovich]

Nice work!

Reviewed 1 of 117 files at r8, 7 of 115 files at r12.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ericharmeling, @shermanCRL, and @smg260)

[knz]

Let's try it!
But i'll avoid batching to ensure if it fails noone else is inconvenienced.

bors r=yuzefovich,stevendanna single on

[yuzefovich]

Looks like this PR is still included in a batch with others. Perhaps "bors r=..." and "single on" don't work together ref.

bors r+ single on

[craig]

Already running a review

[yuzefovich]

bors r-

[craig]

Canceled.

[yuzefovich]

bors r+ single on

[craig]

Timed out.

[knz]

bors r+ p=99 single on

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)