kvtenant: implement SystemConfigProvider, remove Gossip from SQL-only server

[nvanbenschoten]

Fixes #49445.

This commit introduces a new SystemConfigProvider abstraction, which is capable of providing the SystemConfig, as well as notifying clients of updates to the SystemConfig. Gossip already implements this interface. The commit then updates SQL to use this new dependency in place of Gossip whenever it needs to access the SystemConfig.

After making this change, it then updates the kvtenant.Proxy to implement the new SystemConfigProvider interface. This is powered by the GossipSubscription RPC that was added in #50520. The commit updates the subscription to also match on the "system-db" gossip key, and just like that, it can provide the SystemConfig to SQL [*].

Finally, with the kvtenant.Proxy serving the role of a SystemConfigProvider to SQL when applicable, we're able to remove gossip entirely from StartTenant. SQL-only servers will no longer join the gossip network, which is a nice milestone for all of this work.

[*] there are a few remaining questions about how exactly we want to enforce an access control policy on the system config gossip pattern. See the updated comment in Node.GossipSubscription. For now, we're just returning the entire SystemConfig to the subscription.

[cockroach-teamcity]

This change is 

[tbg]

Nice. As far as I'm concerned, this is ready to go when the fixups I will be pushing shortly are squashed.

Reviewed 1 of 1 files at r1, 15 of 15 files at r2, 21 of 21 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)

---

pkg/ccl/kvccl/kvtenantccl/proxy.go, line 242 at r2 (raw file):

// GetSystemConfig implements the config.SystemConfigProvider interface.
func (p *Proxy) GetSystemConfig() *config.SystemConfig {
	// TODO DURING REVIEW: how does this work with Gossip when the SystemConfig

Yes, I think the current code plays fast and loose here. We typically have Gossip connected by the time SQL needs a system config and when gossip connects, I suppose the system config is among the first things that are immediately received.

In the Connector (i.e. tenant.Proxy), I think we want to wait here:

cockroach/pkg/ccl/kvccl/kvtenantccl/proxy.go

Line 162 in 6e1a56b

close(p.startupC)

until we've populated the system config. Since we call .Start() first thing when starting the SQL server, that effectively means that the system config is always available. Even in this code now, though, we're roughly in the same place as before: we're not guaranteed to have the zone config, but since we can't reach this code before a subscription has received data, it's quite likely that we do have it already.

I tried something new - I made a fixup commit for you and am pushing it to your branch past the review. Let me know if that's something we should do more often.

PS: gh makes this kind of thing quite easy.

---

pkg/ccl/kvccl/kvtenantccl/proxy.go, line 255 at r2 (raw file):

func (p *Proxy) RegisterSystemConfigChannel() <-chan struct{} {
	// Create channel that receives new system config notifications.
	// The channel has a size of 1 to prevent proxy from blocking on it.

nit: "having to block on it" is more precise, right? We never blocking-send into this thing.

Added fixup commit as well.

[tbg]

I actually went back and squashed the fixups myself. That way you can get a green CI when you open this hopefully. Again, let me know if you think this is a good way to go about it, I have a feeling it can avoid time typing things into review and copying ideas from reviewable over to code.

[nvanbenschoten]

TFTR!

bors r+

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

---

pkg/ccl/kvccl/kvtenantccl/proxy.go, line 242 at r2 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Yes, I think the current code plays fast and loose here. We typically have Gossip connected by the time SQL needs a system config and when gossip connects, I suppose the system config is among the first things that are immediately received.

In the Connector (i.e. tenant.Proxy), I think we want to wait here:

cockroach/pkg/ccl/kvccl/kvtenantccl/proxy.go

Line 162 in 6e1a56b

close(p.startupC)

until we've populated the system config. Since we call .Start() first thing when starting the SQL server, that effectively means that the system config is always available. Even in this code now, though, we're roughly in the same place as before: we're not guaranteed to have the zone config, but since we can't reach this code before a subscription has received data, it's quite likely that we do have it already.

I tried something new - I made a fixup commit for you and am pushing it to your branch past the review. Let me know if that's something we should do more often.

PS: gh makes this kind of thing quite easy.

I'm mostly fine with this approach, though it does make things confusing on the PR author's side because now my remote has a newer revision than my local branch and I need to force pull it down. In this case, that's fine, but I often have PRs where I allow my remote branch to lag behind my local branch (mainly due to origin/master rebases), so this kind of thing would cause the two to completely diverge, which would end up creating more work than the alternative. Maybe gh makes this all more seamless, but I don't know if it's really worth the hassle.

It also makes it a bit awkward to determine what actually changed. I'd say if we're going to do this, we should keep the fixups in a separate commit and let the PR author squash them in when they want to. I don't mind the extra CI round on large PRs like this.

---

pkg/ccl/kvccl/kvtenantccl/proxy.go, line 255 at r2 (raw file):

Previously, tbg (Tobias Grieger) wrote…

nit: "having to block on it" is more precise, right? We never blocking-send into this thing.

Added fixup commit as well.

👍 Made the same change in Gossip.RegisterSystemConfigChannel.

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build failed:

• GitHub CI (Cockroach)

[nvanbenschoten]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)