kvserver: avoid clobbering replica conf

[irfansharif]

Fixes #75109. There are two ways to read the configuration applied over
a given replica:
(a) the copy embedded in each replica struct
(b) spanconfig.StoreReader, hanging off the store struct

The interface in (b) is implemented by both the span configs
infrastructure and the legacy system config span it's designed to
replace; it's typically used by KV queues (see #69172). When switching
between subsystems in KV (controlled by spanconfig.store.enabled), for
we transparently switch the source for (b). We also use then use the
right source to refresh (a) for every replica. Incremental updates
thereafter mutate (a) directly. As you'd expect, we need to take special
care that only one subsystem is writing to (a) at a given point in time,
a guarantee that was not upheld before this commit. This bug manifested
after we enabled span configs by default (see #73876), and likely
affected many tests.

To avoid the system config span from clobbering (a) when span configs
are enabled, we just need to check what spanconfig.store.enabled
holds at the right spot. To repro:

# Reliably fails with 1-2m on my GCE worker before this patch,
# doesn't after.
dev test pkg/kv/kvserver \
  -f TestBackpressureNotAppliedWhenReducingRangeSize \
  -v --show-logs --timeout 15m --stress

Release note: None

[cockroach-teamcity]

This change is 

[irfansharif]

PS: Looking at the CI failures here, merging this bug fix will probably surface more CI failures/improper test assumptions that were masked until now. Take a look at the following for e.g., definitely does not hold true anymore.

cockroach/pkg/kv/kvserver/replicate_queue_test.go

Lines 264 to 277 in 5359e54

	// Disable the replication queues so that the range we're about to create
	// doesn't get down-replicated too soon.
	tc.ToggleReplicateQueues(false)
	testKey := tc.ScratchRange(t)
	desc := tc.LookupRangeOrFatal(t, testKey)
	// At the end of StartTestCluster(), all ranges have 5 replicas since they're
	// all "system ranges". When the ScratchRange() splits its range, it also
	// starts up with 5 replicas. Since it's not a system range, its default zone
	// config asks for 3x replication, and the replication queue will
	// down-replicate it.
	require.Len(t, desc.Replicas().Descriptors(), 5)
	// Re-enable the replication queue.
	tc.ToggleReplicateQueues(true)

[irfansharif]

Outstanding CI failure is #75212.

[arulajmani]

I have a couple questions for the second commit but otherwise everything else looks good.

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 3 of 3 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif and @nvanbenschoten)

---

-- commits, line 38 at r2:
It's worth adding more words around what those assumptions were here.

---

pkg/kv/kvserver/testing_knobs.go, line 378 at r4 (raw file):

	// is used only for (old) tests written with the system config span in mind.
	//
	// TODO(irfansharif): Get rid of this knob, maybe by first move

"maybe by first move"

---

pkg/kv/kvserver/client_store_test.go, line 30 at r3 (raw file):

// TestStoreSetRangesMaxBytes creates a set of ranges via splitting and then
// sets the config zone to a custom max bytes value to verify the ranges' max
// bytes are updated appropriately. // XXX:

// XXX: ?

---

pkg/kv/kvserver/replicate_queue_test.go, line 275 at r2 (raw file):

	// config asks for 3x replication, and the replication queue will
	// down-replicate it.
	require.Len(t, desc.Replicas().Descriptors(), 5)

Do we know why this worked before and doesn't work now? Is that something we want to address or are we happy changing this test?

[irfansharif]

Thanks, PTAL.

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

---

-- commits, line 38 at r2:

Previously, arulajmani (Arul Ajmani) wrote…

It's worth adding more words around what those assumptions were here.

Done.

---

pkg/kv/kvserver/testing_knobs.go, line 378 at r4 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

"maybe by first move"

Done.

---

pkg/kv/kvserver/client_store_test.go, line 30 at r3 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

// XXX: ?

Woops, forgot to remove. Done.

---

pkg/kv/kvserver/replicate_queue_test.go, line 275 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Do we know why this worked before and doesn't work now? Is that something we want to address or are we happy changing this test?

Yes, added to the commit message. Basically this test relied on the the internal behavior of the system config span where scratch range was split off from the system table ranges, and it "originally" applying the system database zone/span config. With "ConfigureScratchRange", we don't have any such lineage.

[arulajmani]

Reviewed 5 of 5 files at r5, 3 of 3 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @nvanbenschoten)

[irfansharif]

CI failures are #75227 (unsure if span configs fallout fixed by #75300) and #75080 (a bit surprising, this PR is rebased on top of #75146; will have a look soon).

bors r+

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)