kvserver: disable {split,replicate,mvccGC} queues until...

[irfansharif]

...subscribed to span configs. Do the same for the store rebalancer. We applied this treatment for the merge queue back in #78122 since the fallback behavior, if not subscribed, is to use the statically defined span config for every operation.

• For the replicate queue this mean obtusely applying a replication factor of 3, regardless of configuration. This was possible typically post node restart before subscription was initially established. We saw this in roachtest: replicate/wide failed #98385. It was possible then for us to ignore configured voter/non-voter/lease constraints.
• For the split queue, we wouldn't actually compute any split keys if unsubscribed, so the missing check was somewhat benign. But we would be obtusely applying the default range sizes [128MiB,512MiB], so for clusters configured with larger range sizes, this could lead to a burst of splitting post node-restart.
• For the MVCC GC queue, it would mean applying the the statically configured default GC TTL and ignoring any set protected timestamps. The latter is best-effort protection but could result in internal operations relying on protection (like backups, changefeeds) failing informatively. For clusters configured with GC TTL greater than the default, post node-restart it could lead to a burst of MVCC GC activity and AOST queries failing to find expected data.
• For the store rebalancer, ignoring span configs could result in violating lease preferences and voter constraints.

Fixes #98421.
Fixes #98385.

Release note (bug fix): It was previously possible for CockroachDB to not respect non-default zone configs. This only happened for a short window after nodes with existing replicas were restarted, and self-rectified within seconds. This manifested in a few ways:

• If num_replicas was set to something other than 3, we would still add or remove replicas to get to 3x replication.
  • If num_voters was set explicitly to get a mix of voting and non-voting replicas, it would be ignored. CockroachDB could possibly remove non-voting replicas.
• If range_min_bytes or range_max_bytes were changed from 128 MiB and 512 MiB respectively, we would instead try to size ranges to be within [128 MiB, 512MiB]. This could appear as an excess amount of range splits or merges, as visible in the Replication Dashboard under "Range Operations".
• If gc.ttlseconds was set to something other than 90000 seconds, we would still GC data only older than 90000s/25h. If the GC TTL was set to something larger than 25h, AOST queries going further back may now start failing. For GC TTLs less than the 25h default, clusters would observe increased disk usage due to more retained garbage.
• If constraints, lease_preferences or voter_constraints were set, they would be ignored. Range data and leases would possibly be moved outside where prescribed. This issues only lasted a few seconds post node-restarts, and any zone config violations were rectified shortly after.

[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 

[kvoli]

This only happened for a short window after nodes with existing replicas were restarted

It seems as though it could possibly occur if the range feed couldn't be established as well? Is that unlikely to occur in practice?

Overall . Some minor nits. Looks like the patch needs crlfmt, update setting names and implement *mockSpanConfigSubscriber.LastUpdated to pass CI.

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

---

-- commits line 16 at r1:
Could you explain this further? If there are no split keys being computed how could there be a burst of splitting post node restart?

Code quote:

  - For the split queue, we wouldn't actually compute any split keys if
    unsubscribed, so the missing check was somewhat benign. But we would
    be obtusely applying the default range sizes [128MiB,512MiB], so for
    clusters configured with larger range sizes, this could lead to a
    burst of splitting post node-restart.

---

-- commits line 27 at r1:
nit: Generally it affected constraints, not just voter.

cockroach/pkg/kv/kvserver/store_rebalancer.go

Lines 1097 to 1109 in 1a58583

	for i := 0; i < len(finalNonVoterTargets); i++ {
	add, remove, _, shouldRebalance := sr.allocator.RebalanceTarget(
	ctx,
	sr.storePool,
	rbCtx.conf,
	rbCtx.candidateReplica.RaftStatus(),
	finalVoterTargets,
	finalNonVoterTargets,
	rbCtx.candidateReplica.RangeUsageInfo(),
	storepool.StoreFilterSuspect,
	allocatorimpl.NonVoterTarget,
	options,
	)

---

pkg/kv/kvserver/store_rebalancer.go line 158 at r1 (raw file):

		storePool = rq.store.cfg.StorePool
	}

nit: empty line added.

---

pkg/kv/kvserver/kvserverbase/base.go line 43 at r1 (raw file):

var ReplicateQueueEnabled = settings.RegisterBoolSetting(
	settings.SystemOnly,
	"kv.replicate.queue_enabled",

nit: change these to be kv.xxxx.queue.enabled

[irfansharif]

I'm only mentioning the node restart point since it's a release note -- rangefeeds are an internal mechanism. But yes, it can also happen if rangefeeds reliably fail to get established, say when there's loss of quorum. But these are centralized system database ranges, and if you've lost quorum there, things are pretty bad. I don't think it's worth mentioning, and I think it's rare given our routine 5x replication.

Repurposed queueConfig.needsSystemConfig to capture this needs-subscription-logic; will wait for CI to get green.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @arulajmani and @kvoli)

---

-- commits line 16 at r1:

Previously, kvoli (Austen) wrote…

Could you explain this further? If there are no split keys being computed how could there be a burst of splitting post node restart?

The burst of splitting would be due to the larger configured range sizes, not due to computed split keys (which BTW, are only used to figure out schema boundaries or under coalesced ranges, the points where adjacent schemas have different span configs). So if the restarted store has 4GiB ranges, post-restart we might try to slice them back down to 512MiB ranges.

[irfansharif]

bors r+

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[arulajmani]

Nice find! I see this is already in the bors queue, so no need to stop the train for me, but please do address comments in a followup. Specifically, the one around expanding our testing.

Reviewed 19 of 19 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @irfansharif and @kvoli)

---

pkg/kv/kvserver/mvcc_gc_queue.go line 686 at r2 (raw file):

) (processed bool, err error) {
	if !mgcq.enabled() {
		log.VEventf(ctx, 2, "skipping mvcc gc: queue has been disabled")

nit: Consider logging louder, but doing so log.Every(10 seconds).

---

pkg/kv/kvserver/store.go line 2066 at r2 (raw file):

}

// GetConfReader exposes access to a configuration reader.

Please add a sentence about returning errSpanConfigUnavailable up here as well.

---

pkg/kv/kvserver/store.go line 2090 at r2 (raw file):

		// surface explicitly that we don't have any span configs instead of
		// falling back to the statically configured one.
		// - enabling range merges would be extremely dangerous -- we could

Instead of having these words here, they're better placed around where you're setting needsSpanConfigs for the respective queues.

---

pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber_client_test.go line 32 at r2 (raw file):

// TestBlockedKVSubscriberDisablesMerges ensures that the merge queue is
// disabled until the KVSubscriber has some snapshot.
func TestBlockedKVSubscriberDisablesMerges(t *testing.T) {

Let's expand this test to cover the other queues we've identified in this commit as well.

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 234bcd0 to blathers/backport-release-22.2-98422: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.

---

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

[irfansharif]

please do address comments in a followup. Specifically, the one around expanding our testing.

See #98807.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)