server: use tenant capability watcher in orchestrator

[stevendanna]

This changes over from polling to using the capability watcher to observe virtual cluster service mode changes.

On the surface, it may seem like this make us less responsive since our poll interval was 1 seconds and by default we will only see an update every 3 seconds from the watcher.

However, when we were using the polling approach, what was likely to happen is that the server would fail to start up until the tenant watcher caught up anyway, since the tenant server does a service-mode check using the capability watcher.

Fixes #107827.
Epic: CRDB-26691

Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

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

[cockroach-teamcity]

This change is 

[stevendanna]

The main thing I don't love about this is that to avoid missing updates while processing updates, we currently block the tenant capability watcher until the callback is done.

An alternative could be some interface that never blocks but lets you know if you likely missed updates so that you could do a new complete scan yourself.....

[knz]

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @stevendanna)

---

-- commits line 16 at r1:
Pro tip: no need to include Epic: xxx in individual commit messages if there's a common epic for the entire PR, which can be mentioned in the PR description.

---

pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/watcher.go line 169 at r1 (raw file):

// OnUpdate is called after a tenant state update is received. The
// provided function must take care to avoid blocking for too long.
func (w *Watcher) OnUpdate(cb onUpdateCallback) func() {

IMHO this is not the right abstraction. There are two problems here:

1. as you've already identified in the commit message, this will block the rangefeed until the callbacks have completed. We don't really want that -- this watcher is also used to propagate data through the tenant connector, and to update the RPC authorizer. We don't want that to become indefinitely stale just because the server controller has a hiccup.

2. we already have standardized a robust asynchronous update notification mechanism across the various watchers; and this OnUpdate protocol you're creating here is diverging from that standard mechanism. For the sake of uniformity (learnability, maintainance burden etc) I recommend we continue to use the standard protocol.
   May I thus recommend you consider reusing this, using the following examples for inspiration:

• GetInfo in this file.
• GetAllTenantOverrides in pkg/server/tenantsettingswatcher/watcher.go
• GetTenantOverrides in that file too

[stevendanna]

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

---

pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/watcher.go line 169 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

IMHO this is not the right abstraction. There are two problems here:

1. as you've already identified in the commit message, this will block the rangefeed until the callbacks have completed. We don't really want that -- this watcher is also used to propagate data through the tenant connector, and to update the RPC authorizer. We don't want that to become indefinitely stale just because the server controller has a hiccup.

2. we already have standardized a robust asynchronous update notification mechanism across the various watchers; and this OnUpdate protocol you're creating here is diverging from that standard mechanism. For the sake of uniformity (learnability, maintainance burden etc) I recommend we continue to use the standard protocol.
   May I thus recommend you consider reusing this, using the following examples for inspiration:

• GetInfo in this file.
• GetAllTenantOverrides in pkg/server/tenantsettingswatcher/watcher.go
• GetTenantOverrides in that file too

:D I started with something like this but convinced myself I was going to miss an update. But, looking at it again I see that I just had some ordering wrong. I've moved this to an GetAllTenants() method that returns all of the tenants and a channel that is closed if anything changes.

[knz]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @stevendanna)

---

pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/watcher.go line 179 at r2 (raw file):

		}
	}
	w.mu.anyChangeChs = append(w.mu.anyChangeChs, updateCh)

You only need 1 channel. No need for a slice. The channel is always defined, and closed-reallocated on every update event.

---

pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/watcher.go line 306 at r2 (raw file):

	if len(updates) > 0 {
		w.closeAnyChangeChs()

This is suspicious. The exijsting pattern is, to notify, to create a new channel after an unconditional close.

---

pkg/server/server_controller_orchestration.go line 48 at r2 (raw file):

		for {

nit: remove empty line.

[stevendanna]

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

---

pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/watcher.go line 179 at r2 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

You only need 1 channel. No need for a slice. The channel is always defined, and closed-reallocated on every update event.

Done.

---

pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/watcher.go line 306 at r2 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

This is suspicious. The exijsting pattern is, to notify, to create a new channel after an unconditional close.

We can elide the length check, but it'll cause unnecessary work. handleUpdate is called with an empty incremental update every time the frontier advances. When updating on incremental updates in handleIncrementalUpdate, we close the relevant channels while iterating the updates.

[knz]

LGTM -- you have an optimization to reduce allocs further but I'll leave it up to you.

Reviewed 2 of 2 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @stevendanna)

---

pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/watcher.go line 306 at r2 (raw file):

Previously, stevendanna (Steven Danna) wrote…

We can elide the length check, but it'll cause unnecessary work. handleUpdate is called with an empty incremental update every time the frontier advances. When updating on incremental updates in handleIncrementalUpdate, we close the relevant channels while iterating the updates.

Yes, I agree the if len(updates) > 0 is useful. I was just picking up on the for loop.

---

pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/watcher.go line 173 at r3 (raw file):

	defer w.mu.RUnlock()

	entries := make([]tenantcapabilities.Entry, 0, len(w.mu.store))

nit: ideally, this slice would be populated during the updates, and simply retrieved here by reference.
(This would need to create a new slice every time the channel is closed/refreshed.)

[stevendanna]

Thanks for the careful review.

I'll do the allocation optimization as a follow up shortly but I'm going to take advantage of a green CI run and merge this for now.

bors r=knz

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)