From f6819fafcc3e3b028ce346bb175041795eed2860 Mon Sep 17 00:00:00 2001 From: Tobias Grieger Date: Thu, 30 Oct 2025 16:44:02 +0000 Subject: [PATCH 1/7] Revert "kvserver: deflake TestPromoteNonVoterInAddVoter" This reverts commit a884f8f7b25cd3e7d0af26cc703a5dd8a891b9c8 (#155861). As far as I understand, the cluster settings in this commit had the effect of making sure that the span config for the table in the test was exactly `[/Table/N,/Table/(N+1))`, i.e. that it wouldn't merge with adjacent configs. But in a recent flake we see that the real problem is that the range containing our table sometimes extends past the span config (in a recent example, it reached the end of the keyspace and wasn't split down in time for the test to pass). Since commits in this PR ensure that the splitting happens proactively, it should not matter whether span configs are coalesced or not, and we back out that attempt at a fix as it's unclear what positive effect it may have. @arulajmani: I'm probably missing something here. Happy to update the description if so. --- pkg/kv/kvserver/replicate_queue_test.go | 8 -------- pkg/spanconfig/spanconfigstore/span_store.go | 6 +++--- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/pkg/kv/kvserver/replicate_queue_test.go b/pkg/kv/kvserver/replicate_queue_test.go index c3c0e5671db0..bfdaed302ee1 100644 --- a/pkg/kv/kvserver/replicate_queue_test.go +++ b/pkg/kv/kvserver/replicate_queue_test.go @@ -41,7 +41,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/server/serverpb" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/spanconfig" - "github.com/cockroachdb/cockroach/pkg/spanconfig/spanconfigstore" "github.com/cockroachdb/cockroach/pkg/sql/catalog/bootstrap" "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/testutils/skip" @@ -2189,12 +2188,6 @@ func TestPromoteNonVoterInAddVoter(t *testing.T) { defer testutils.StartExecTrace(t, scope.GetDirectory()).Finish(t) ctx := context.Background() - st := cluster.MakeTestingClusterSettings() - // NB: Ensure that tables created by the test start off on their own range. - // This ensures that any span config changes made by the test don't have to - // first induce a split, which is a known source of flakiness. - spanconfigstore.StorageCoalesceAdjacentSetting.Override(ctx, &st.SV, false) - spanconfigstore.TenantCoalesceAdjacentSetting.Override(ctx, &st.SV, false) // Create 7 stores: 3 in Region 1, 2 in Region 2, and 2 in Region 3. const numNodes = 7 @@ -2202,7 +2195,6 @@ func TestPromoteNonVoterInAddVoter(t *testing.T) { regions := [numNodes]int{1, 1, 1, 2, 2, 3, 3} for i := 0; i < numNodes; i++ { serverArgs[i] = base.TestServerArgs{ - Settings: st, Locality: roachpb.Locality{ Tiers: []roachpb.Tier{ { diff --git a/pkg/spanconfig/spanconfigstore/span_store.go b/pkg/spanconfig/spanconfigstore/span_store.go index 59a4b7422017..ab9c8e66c545 100644 --- a/pkg/spanconfig/spanconfigstore/span_store.go +++ b/pkg/spanconfig/spanconfigstore/span_store.go @@ -19,10 +19,10 @@ import ( "github.com/cockroachdb/errors" ) -// TenantCoalesceAdjacentSetting is a public cluster setting that +// tenantCoalesceAdjacentSetting is a public cluster setting that // controls whether we coalesce adjacent ranges across all secondary // tenant keyspaces if they have the same span config. -var TenantCoalesceAdjacentSetting = settings.RegisterBoolSetting( +var tenantCoalesceAdjacentSetting = settings.RegisterBoolSetting( settings.SystemOnly, "spanconfig.tenant_coalesce_adjacent.enabled", `collapse adjacent ranges with the same span configs across all secondary tenant keyspaces`, @@ -157,7 +157,7 @@ func (s *spanConfigStore) computeSplitKey( return roachpb.RKey(match.span.Key), nil } } else { - if !TenantCoalesceAdjacentSetting.Get(&s.settings.SV) { + if !tenantCoalesceAdjacentSetting.Get(&s.settings.SV) { return roachpb.RKey(match.span.Key), nil } } From 5aa363958bebd961e849c9ead3d3f389448516e4 Mon Sep 17 00:00:00 2001 From: Tobias Grieger Date: Thu, 30 Oct 2025 14:17:23 +0000 Subject: [PATCH 2/7] kvserver: fix typo --- pkg/kv/kvserver/lease_queue.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/kv/kvserver/lease_queue.go b/pkg/kv/kvserver/lease_queue.go index 70e43ca5073d..3cf2457542b8 100644 --- a/pkg/kv/kvserver/lease_queue.go +++ b/pkg/kv/kvserver/lease_queue.go @@ -137,7 +137,7 @@ func (lq *leaseQueue) process( if transferOp, ok := change.Op.(plan.AllocationTransferLeaseOp); ok { lease, _ := repl.GetLease() - log.KvDistribution.Infof(ctx, "transferring lease to s%d usage=%v, lease=[%v type=%v]", transferOp.Target, transferOp.Usage, lease, lease.Type()) + log.KvDistribution.Infof(ctx, "transferring lease to %d usage=%v, lease=[%v type=%v]", transferOp.Target, transferOp.Usage, lease, lease.Type()) lq.lastLeaseTransfer.Store(timeutil.Now()) changeID := lq.as.NonMMAPreTransferLease( desc, From 9ad7750f82c64c19d17a093f1a63323aa751dd4a Mon Sep 17 00:00:00 2001 From: Tobias Grieger Date: Thu, 30 Oct 2025 14:17:35 +0000 Subject: [PATCH 3/7] kvserver: log rangeID of interest in a test --- pkg/kv/kvserver/replicate_queue_test.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/kv/kvserver/replicate_queue_test.go b/pkg/kv/kvserver/replicate_queue_test.go index bfdaed302ee1..bc0d4d9cfe3a 100644 --- a/pkg/kv/kvserver/replicate_queue_test.go +++ b/pkg/kv/kvserver/replicate_queue_test.go @@ -2263,14 +2263,14 @@ SELECT * FROM ( t *testing.T, tc *testcluster.TestCluster, db *gosql.DB, - ) (numVoters, numNonVoters int, err error) { + ) (numVoters, numNonVoters int, _ roachpb.RangeID, err error) { if err := forceScanOnAllReplicationQueues(tc); err != nil { - return 0, 0, err + return 0, 0, 0, err } var rangeID roachpb.RangeID if err := db.QueryRow("SELECT range_id FROM [SHOW RANGES FROM TABLE t] LIMIT 1").Scan(&rangeID); err != nil { - return 0, 0, err + return 0, 0, 0, err } iterateOverAllStores(t, tc, func(s *kvserver.Store) error { if replica, err := s.GetReplica(rangeID); err == nil && replica.OwnsValidLease(ctx, replica.Clock().NowAsClockTimestamp()) { @@ -2280,18 +2280,18 @@ SELECT * FROM ( } return nil }) - return numVoters, numNonVoters, nil + return numVoters, numNonVoters, rangeID, nil } // Ensure we are meeting our ZONE survival configuration. testutils.SucceedsSoon(t, func() error { - numVoters, numNonVoters, err := computeNumberOfReplicas(t, tc, db) + numVoters, numNonVoters, rangeID, err := computeNumberOfReplicas(t, tc, db) require.NoError(t, err) if numVoters != 3 { - return errors.Newf("expected 3 voters; got %d", numVoters) + return errors.Newf("expected 3 voters for r%d; got %d", rangeID, numVoters) } if numNonVoters != 2 { - return errors.Newf("expected 2 non-voters; got %v", numNonVoters) + return errors.Newf("expected 2 non-voters for r%d; got %v", rangeID, numNonVoters) } return nil }) @@ -2308,13 +2308,13 @@ SELECT * FROM ( // Ensure we are meeting our REGION survival configuration. testutils.SucceedsSoon(t, func() error { - numVoters, numNonVoters, err := computeNumberOfReplicas(t, tc, db) + numVoters, numNonVoters, rangeID, err := computeNumberOfReplicas(t, tc, db) require.NoError(t, err) if numVoters != 5 { - return errors.Newf("expected 5 voters; got %d", numVoters) + return errors.Newf("expected 5 voters for r%d; got %d", rangeID, numVoters) } if numNonVoters != 0 { - return errors.Newf("expected 0 non-voters; got %v", numNonVoters) + return errors.Newf("expected 0 non-voters for r%d; got %v", rangeID, numNonVoters) } return nil }) From 6d392c9758bbfb4f880b5b1867ae20e3ee1364a1 Mon Sep 17 00:00:00 2001 From: Tobias Grieger Date: Thu, 30 Oct 2025 14:36:01 +0000 Subject: [PATCH 4/7] kvserver: don't swallow errors in forceScanOnAllReplicationQueues --- pkg/kv/kvserver/replicate_queue_test.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/pkg/kv/kvserver/replicate_queue_test.go b/pkg/kv/kvserver/replicate_queue_test.go index bc0d4d9cfe3a..a581f253a66e 100644 --- a/pkg/kv/kvserver/replicate_queue_test.go +++ b/pkg/kv/kvserver/replicate_queue_test.go @@ -1786,11 +1786,13 @@ func toggleReplicationQueues(tc *testcluster.TestCluster, active bool) { func forceScanOnAllReplicationQueues(tc *testcluster.TestCluster) (err error) { for _, s := range tc.Servers { - err = s.GetStores().(*kvserver.Stores).VisitStores(func(store *kvserver.Store) error { + if err := s.GetStores().(*kvserver.Stores).VisitStores(func(store *kvserver.Store) error { return store.ForceReplicationScanAndProcess() - }) + }); err != nil { + return err + } } - return err + return nil } func toggleSplitQueues(tc *testcluster.TestCluster, active bool) { From 2afeed4255b06f2c2446f0af939c1270937016a0 Mon Sep 17 00:00:00 2001 From: Tobias Grieger Date: Thu, 30 Oct 2025 14:48:33 +0000 Subject: [PATCH 5/7] kvserver: also force split queue run in forceScanOnAllReplicationQueues This (hopefully) avoids the timeout in TestPromoteNonVoterInAddVoter that could previously result if an ill-timed lease transfer prevented reactively splitting in response to the zone configuration (which would then delay application of the replication factor in said zone config). Fixes #156530. --- pkg/kv/kvserver/replicate_queue_test.go | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/pkg/kv/kvserver/replicate_queue_test.go b/pkg/kv/kvserver/replicate_queue_test.go index a581f253a66e..c63f087141c7 100644 --- a/pkg/kv/kvserver/replicate_queue_test.go +++ b/pkg/kv/kvserver/replicate_queue_test.go @@ -617,7 +617,7 @@ func checkReplicaCount( rangeDesc *roachpb.RangeDescriptor, voterCount, nonVoterCount int, ) (bool, error) { - err := forceScanOnAllReplicationQueues(tc) + err := forceScanOnAllReplicationAndSplitQueues(tc) if err != nil { log.KvDistribution.Infof(ctx, "store.ForceReplicationScanAndProcess() failed with: %s", err) return false, err @@ -1543,7 +1543,7 @@ func TestReplicateQueueSwapVotersWithNonVoters(t *testing.T) { " num_replicas=5, num_voters=1") require.NoError(t, err) testutils.SucceedsSoon(t, func() error { - if err := forceScanOnAllReplicationQueues(tc); err != nil { + if err := forceScanOnAllReplicationAndSplitQueues(tc); err != nil { return err } scratchRange := tc.LookupRangeOrFatal(t, scratchKey) @@ -1558,7 +1558,7 @@ func TestReplicateQueueSwapVotersWithNonVoters(t *testing.T) { checkRelocated := func(t *testing.T, voterStores, nonVoterStores []roachpb.StoreID) { testutils.SucceedsSoon(t, func() error { - if err := forceScanOnAllReplicationQueues(tc); err != nil { + if err := forceScanOnAllReplicationAndSplitQueues(tc); err != nil { return err } scratchRange := tc.LookupRangeOrFatal(t, scratchKey) @@ -1664,7 +1664,7 @@ func TestReplicateQueueShouldQueueNonVoter(t *testing.T) { // Make sure that the range has conformed to the constraints we just set // above. require.Eventually(t, func() bool { - if err := forceScanOnAllReplicationQueues(tc); err != nil { + if err := forceScanOnAllReplicationAndSplitQueues(tc); err != nil { log.KvDistribution.Warningf(ctx, "received error while forcing a replicateQueue scan: %s", err) return false } @@ -1784,10 +1784,19 @@ func toggleReplicationQueues(tc *testcluster.TestCluster, active bool) { } } -func forceScanOnAllReplicationQueues(tc *testcluster.TestCluster) (err error) { +func forceScanOnAllReplicationAndSplitQueues(tc *testcluster.TestCluster) (err error) { + // We force scans on the replication and split queues because sometimes splits + // are necessary to apply zone config changes, which then lead to further + // replication decisions. See #156530 for an example of this. for _, s := range tc.Servers { if err := s.GetStores().(*kvserver.Stores).VisitStores(func(store *kvserver.Store) error { - return store.ForceReplicationScanAndProcess() + if err := store.ForceReplicationScanAndProcess(); err != nil { + return err + } + if err := store.ForceSplitScanAndProcess(); err != nil { + return err + } + return nil }); err != nil { return err } @@ -2231,7 +2240,7 @@ func TestPromoteNonVoterInAddVoter(t *testing.T) { setConstraintFn("RANGE meta", 7, 7, "") setConstraintFn("RANGE default", 7, 7, "") testutils.SucceedsSoon(t, func() error { - if err := forceScanOnAllReplicationQueues(tc); err != nil { + if err := forceScanOnAllReplicationAndSplitQueues(tc); err != nil { return err } s, err := sqlutils.RowsToDataDrivenOutput(sqlutils.MakeSQLRunner(tc.Conns[0]).Query(t, ` @@ -2266,7 +2275,7 @@ SELECT * FROM ( tc *testcluster.TestCluster, db *gosql.DB, ) (numVoters, numNonVoters int, _ roachpb.RangeID, err error) { - if err := forceScanOnAllReplicationQueues(tc); err != nil { + if err := forceScanOnAllReplicationAndSplitQueues(tc); err != nil { return 0, 0, 0, err } From 21de200cc8246862712384028887cd81468cefdd Mon Sep 17 00:00:00 2001 From: Tobias Grieger Date: Thu, 30 Oct 2025 15:52:38 +0000 Subject: [PATCH 6/7] kvserver: log final rangeID in test It tends to change as splits happen. --- pkg/kv/kvserver/replicate_queue_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/kv/kvserver/replicate_queue_test.go b/pkg/kv/kvserver/replicate_queue_test.go index c63f087141c7..38ff4dc66fc3 100644 --- a/pkg/kv/kvserver/replicate_queue_test.go +++ b/pkg/kv/kvserver/replicate_queue_test.go @@ -2304,6 +2304,7 @@ SELECT * FROM ( if numNonVoters != 2 { return errors.Newf("expected 2 non-voters for r%d; got %v", rangeID, numNonVoters) } + t.Logf("success: %d has %d voters and %d non-voters", rangeID, numVoters, numNonVoters) return nil }) From d52e7e75585af64e385ce4fd687b429673dd8949 Mon Sep 17 00:00:00 2001 From: Tobias Grieger Date: Thu, 30 Oct 2025 16:33:24 +0000 Subject: [PATCH 7/7] kvserver: log active spanconfigs when test takes long time --- pkg/kv/kvserver/replicate_queue_test.go | 55 ++++++++++++++++++------- 1 file changed, 41 insertions(+), 14 deletions(-) diff --git a/pkg/kv/kvserver/replicate_queue_test.go b/pkg/kv/kvserver/replicate_queue_test.go index 38ff4dc66fc3..f89f17f45fd4 100644 --- a/pkg/kv/kvserver/replicate_queue_test.go +++ b/pkg/kv/kvserver/replicate_queue_test.go @@ -2274,37 +2274,64 @@ SELECT * FROM ( t *testing.T, tc *testcluster.TestCluster, db *gosql.DB, - ) (numVoters, numNonVoters int, _ roachpb.RangeID, err error) { + ) (numVoters, numNonVoters int, _ roachpb.RangeDescriptor, err error) { if err := forceScanOnAllReplicationAndSplitQueues(tc); err != nil { - return 0, 0, 0, err + return 0, 0, roachpb.RangeDescriptor{}, err } - var rangeID roachpb.RangeID - if err := db.QueryRow("SELECT range_id FROM [SHOW RANGES FROM TABLE t] LIMIT 1").Scan(&rangeID); err != nil { - return 0, 0, 0, err + var key roachpb.Key + require.NoError(t, + db.QueryRow(` +SELECT start_key from crdb_internal.ranges_no_leases WHERE range_id IN +(SELECT range_id FROM [SHOW RANGES FROM TABLE t] LIMIT 1); +`).Scan(&key)) + desc, err := tc.LookupRange(key) + if err != nil { + return 0, 0, roachpb.RangeDescriptor{}, err } + iterateOverAllStores(t, tc, func(s *kvserver.Store) error { - if replica, err := s.GetReplica(rangeID); err == nil && replica.OwnsValidLease(ctx, replica.Clock().NowAsClockTimestamp()) { + if replica, err := s.GetReplica(desc.RangeID); err == nil && replica.OwnsValidLease(ctx, + replica.Clock().NowAsClockTimestamp()) { desc := replica.Desc() numVoters = len(desc.Replicas().VoterDescriptors()) numNonVoters = len(desc.Replicas().NonVoterDescriptors()) } return nil }) - return numVoters, numNonVoters, rangeID, nil + return numVoters, numNonVoters, desc, nil } // Ensure we are meeting our ZONE survival configuration. + logMore := time.After(15 * time.Second) testutils.SucceedsSoon(t, func() error { - numVoters, numNonVoters, rangeID, err := computeNumberOfReplicas(t, tc, db) + numVoters, numNonVoters, desc, err := computeNumberOfReplicas(t, tc, db) + require.NoError(t, err) + select { + default: + case <-logMore: + // If the retry loop has been stuck for a while, log the span config + // as seen by each store. For it to apply, the range's span needs to + // be contained in the start key's span config. If this is not the + // case, it can explain why the replication changes are not being made. + iterateOverAllStores(t, tc, func(s *kvserver.Store) error { + cfg, sp, err := s.GetStoreConfig().SpanConfigSubscriber.GetSpanConfigForKey(ctx, desc.StartKey) + if err != nil { + return err + } + t.Logf("s%d: r%d %s -> span config %s %s", s.StoreID(), desc.RangeID, desc.RSpan(), sp, &cfg) + return nil + }) + } + if numVoters != 3 { - return errors.Newf("expected 3 voters for r%d; got %d", rangeID, numVoters) + return errors.Newf("expected 3 voters for r%d; got %d", desc.RangeID, numVoters) } if numNonVoters != 2 { - return errors.Newf("expected 2 non-voters for r%d; got %v", rangeID, numNonVoters) + return errors.Newf("expected 2 non-voters for r%d; got %v", desc.RangeID, numNonVoters) } - t.Logf("success: %d has %d voters and %d non-voters", rangeID, numVoters, numNonVoters) + t.Logf("success: %d has %d voters and %d non-voters", desc.RangeID, numVoters, numNonVoters) return nil }) @@ -2320,13 +2347,13 @@ SELECT * FROM ( // Ensure we are meeting our REGION survival configuration. testutils.SucceedsSoon(t, func() error { - numVoters, numNonVoters, rangeID, err := computeNumberOfReplicas(t, tc, db) + numVoters, numNonVoters, desc, err := computeNumberOfReplicas(t, tc, db) require.NoError(t, err) if numVoters != 5 { - return errors.Newf("expected 5 voters for r%d; got %d", rangeID, numVoters) + return errors.Newf("expected 5 voters for r%d; got %d", desc.RangeID, numVoters) } if numNonVoters != 0 { - return errors.Newf("expected 0 non-voters for r%d; got %v", rangeID, numNonVoters) + return errors.Newf("expected 0 non-voters for r%d; got %v", desc.RangeID, numNonVoters) } return nil })