kvserver: update TestSystemZoneConfigs

Fixes cockroachdb#98200. This test was written pre-spanconfig days, and when
enabling spanconfigs by default, opted out of using it. This commit
makes it use spanconfigs after giving up on reproducing/diagnosing the
original flake (this test is notoriously slow -- taking 30+s given it
waits for actual upreplication and replica movement, so not --stress
friendly).

Using spanconfigs here surfaced a rare, latent bug, one this author
incorrectly thought was fixed back in cockroachdb#75939. In very rare cases, right
during cluster bootstrap before the span config reconciler has ever had
a chance to run (i.e. system.span_configurations is empty), it's
possible that the subscriber has subscribed to an empty span config
state (we've only seen this happen in unit tests with 50ms scan
intervals). So it's not been meaningfully "updated" in any sense of the
word, but we still previously set a non-empty last-updated timestamp,
something various components in KV rely on as proof that we have span
configs as of some timestamp. As a result, we saw KV incorrectly merge
away the liveness range into adjacent ranges, and then later split it
off. We don't think we've ever seen this happen outside of tests as it
instantly triggers the following fatal in the raftScheduler, which wants
to prioritize the liveness range above all else:

    panic: priority range ID already set: old=2, new=61, first set at:

Release note: None

pkg/kv/kvserver/client_replica_test.go

@@ -19,6 +19,7 @@ import (
 	"reflect"
 	"sort"
 	"strconv"
+	"strings"
 	"sync"
 	"sync/atomic"
 	"testing"
@@ -2700,9 +2701,6 @@ func TestSystemZoneConfigs(t *testing.T) {
 					DisableLoadBasedSplitting: true,
 				},
 			},
-			// This test was written for the gossip-backed SystemConfigSpan
-			// infrastructure.
-			DisableSpanConfigs: true,
 			// Scan like a bat out of hell to ensure replication and replica GC
 			// happen in a timely manner.
 			ScanInterval: 50 * time.Millisecond,
@@ -2711,10 +2709,21 @@ func TestSystemZoneConfigs(t *testing.T) {
 	defer tc.Stopper().Stop(ctx)
 	log.Info(ctx, "TestSystemZoneConfig: test cluster started")
 
-	expectedSystemRanges, err := tc.Servers[0].ExpectedInitialRangeCount()
+	initialRangeCount, err := tc.Servers[0].ExpectedInitialRangeCount()
 	if err != nil {
 		t.Fatal(err)
 	}
+
+	// The initial splits we have when bootstrapping CRDB, look roughly like so:
+	//   r6:/Table/{0-3} [(n1,s1):1, (n4,s4):2, (n7,s7):3, (n3,s3):4, (n6,s6):5, next=6, gen=8]
+	//   r7:/Table/{3-4} [(n1,s1):1, (n6,s6):2, (n7,s7):3, (n4,s4):4, (n5,s5):5, next=6, gen=8]
+	//
+	// Span configs however collapse the known-empty /Table/{0-3} keyspace into
+	// the adjacent range, so we have:
+	//   r6:/Table/{0-4} [(n1,s1):1, (n4,s4):2, (n3,s3):3, (n2,s2):4, (n5,s5):5, next=6, gen=17]
+	//
+	// Which is why we have this -1 term below.
+	expectedSystemRanges := initialRangeCount - 1
 	expectedUserRanges := 1
 	expectedSystemRanges -= expectedUserRanges
 	systemNumReplicas := int(*zonepb.DefaultSystemZoneConfig().NumReplicas)
@@ -2745,8 +2754,19 @@ func TestSystemZoneConfigs(t *testing.T) {
 		for _, desc := range replicas {
 			totalReplicas += len(desc.Replicas().VoterDescriptors())
 		}
+		sortedDesc := make([]roachpb.RangeDescriptor, 0, len(replicas))
+		for _, desc := range replicas {
+			sortedDesc = append(sortedDesc, desc)
+		}
+		sort.Slice(sortedDesc, func(i, j int) bool {
+			return sortedDesc[i].RangeID < sortedDesc[j].RangeID
+		})
+		var buf strings.Builder
+		for _, desc := range sortedDesc {
+			buf.WriteString(fmt.Sprintf("%s\n", desc))
+		}
 		if totalReplicas != expectedReplicas {
-			return fmt.Errorf("got %d voters, want %d; details: %+v", totalReplicas, expectedReplicas, replicas)
+			return fmt.Errorf("got %d voters, want %d; details: %+v", totalReplicas, expectedReplicas, buf.String())
 		}
 		return nil
 	}

pkg/kv/kvserver/queue_helpers_testutil.go

@@ -12,8 +12,10 @@ package kvserver
 
 import (
 	"context"
+	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/retry"
 	"github.com/cockroachdb/errors"
 )
 
@@ -27,11 +29,33 @@ func (bq *baseQueue) testingAdd(
 }
 
 func forceScanAndProcess(ctx context.Context, s *Store, q *baseQueue) error {
-	// Check that the system config is available. It is needed by many queues. If
-	// it's not available, some queues silently fail to process any replicas,
-	// which is undesirable for this method.
-	if _, err := s.GetConfReader(ctx); err != nil {
-		return errors.Wrap(err, "unable to retrieve conf reader")
+	if q.queueConfig.needsSpanConfigs {
+		// Check that the system config is available. It is needed by many
+		// queues. If it's not available, some queues silently fail to process
+		// any replicas, which is undesirable for this method. Retry moderately
+		// to allow the span config reconciler to get started.
+		opts := retry.Options{
+			InitialBackoff: time.Millisecond * 10,
+			MaxBackoff:     time.Millisecond * 100,
+			Multiplier:     2,
+			MaxRetries:     100,
+		}
+		var lastErr error
+		gotConfReader := false
+		for r := retry.Start(opts); r.Next(); {
+			if _, lastErr = s.GetConfReader(ctx); lastErr != nil {
+				if lastErr == errSpanConfigsUnavailable {
+					continue // retry
+				}
+				return errors.Wrap(lastErr, "unable to retrieve conf reader")
+			}
+
+			gotConfReader = true
+			break
+		}
+		if !gotConfReader {
+			return errors.Wrap(lastErr, "unable to retrieve conf reader despite retrying")
+		}
 	}
 
 	newStoreReplicaVisitor(s).Visit(func(repl *Replica) bool {

pkg/spanconfig/spanconfig.go

@@ -84,20 +84,17 @@ type KVAccessor interface {
 // for the span[3].
 //
 // [1]: The contents of the StoreReader and ProtectedTSReader at t1 corresponds
-//
-//	exactly to the contents of the global span configuration state at t0
-//	where t0 <= t1. If the StoreReader or ProtectedTSReader is read from at
-//	t2 where t2 > t1, it's guaranteed to observe a view of the global state
-//	at t >= t0.
+// exactly to the contents of the global span configuration state at t0
+// where t0 <= t1. If the StoreReader or ProtectedTSReader is read from at
+// t2 where t2 > t1, it's guaranteed to observe a view of the global state
+// at t >= t0.
 //
 // [2]: For the canonical KVSubscriber implementation, this is typically lagging
-//
-//	by the closed timestamp target duration.
+// by the closed timestamp target duration.
 //
 // [3]: The canonical KVSubscriber implementation is bounced whenever errors
-//
-//	occur, which may result in the re-transmission of earlier updates
-//	(typically through a coarsely targeted [min,max) span).
+// occur, which may result in the re-transmission of earlier updates
+// (typically through a coarsely targeted [min,max) span).
 type KVSubscriber interface {
 	StoreReader
 	ProtectedTSReader

pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go

@@ -401,7 +401,19 @@ func (s *KVSubscriber) handleCompleteUpdate(
 	}
 	s.mu.Lock()
 	s.mu.internal = freshStore
-	s.setLastUpdatedLocked(ts)
+	if len(events) != 0 {
+		// In very rare cases, right during cluster bootstrap before the span
+		// config reconciler has ever had a chance to run (i.e.
+		// system.span_configurations is empty), it's possible that the
+		// subscriber has subscribed to an empty span config state. We've only
+		// seen this happen in unit tests with 50ms scan intervals. So it's not
+		// been meaningfully "updated" in any sense of the word -- we'll perhaps
+		// get the initial state in the next partial update. We don't set the
+		// last-updated timestamp until then -- various components in KV rely on
+		// this timestamp to be non-empty as proof we have span configs as of
+		// some timestamp.
+		s.setLastUpdatedLocked(ts)
+	}
 	handlers := s.mu.handlers
 	s.mu.Unlock()
 	for i := range handlers {
@@ -426,7 +438,12 @@ func (s *KVSubscriber) handlePartialUpdate(
 		// avoid this mutex.
 		s.mu.internal.Apply(ctx, false /* dryrun */, ev.(*bufferEvent).Update)
 	}
-	s.setLastUpdatedLocked(ts)
+	if len(events) != 0 || !s.mu.lastUpdated.IsEmpty() {
+		// We only set the last-updated timestamp once we know we have some
+		// valid span config state, either by a set of non-empty events or
+		// having already seen it earlier.
+		s.setLastUpdatedLocked(ts)
+	}
 	handlers := s.mu.handlers
 	s.mu.Unlock()