diff --git a/pkg/kv/kvserver/allocator_scorer.go b/pkg/kv/kvserver/allocator_scorer.go index 855186c933a8..750ca5eb8607 100644 --- a/pkg/kv/kvserver/allocator_scorer.go +++ b/pkg/kv/kvserver/allocator_scorer.go @@ -884,11 +884,28 @@ func rankedCandidateListForRebalancing( ) continue } + + // `replsOnExemptedStores` is used during non-voting replica rebalancing + // to ignore stores that already have a voting replica for the same range. + // When rebalancing a voter, `replsOnExemptedStores` is empty since voters + // are allowed to "displace" non-voting replicas (we correctly turn such + // actions into non-voter promotions, see replicationChangesForRebalance()). + var exempted bool for _, replOnExemptedStore := range replicasOnExemptedStores { if store.StoreID == replOnExemptedStore.StoreID { - continue + log.VEventf( + ctx, + 6, + "s%d is not a possible rebalance candidate for non-voters because it already has a voter of the range; ignoring", + store.StoreID, + ) + exempted = true + break } } + if exempted { + continue + } constraintsOK, necessary := rebalanceConstraintsChecker(store, existing.store) maxCapacityOK := maxCapacityCheck(store) diff --git a/pkg/kv/kvserver/allocator_test.go b/pkg/kv/kvserver/allocator_test.go index 03c83f6e96c5..cb8849f1e7b6 100644 --- a/pkg/kv/kvserver/allocator_test.go +++ b/pkg/kv/kvserver/allocator_test.go @@ -4090,6 +4090,69 @@ func TestVotersCanRebalanceToNonVoterStores(t *testing.T) { } } +// TestNonVotersCannotRebalanceToVoterStores ensures that non-voting replicas +// cannot rebalance to stores that already have a voting replica for the range. +func TestNonVotersCannotRebalanceToVoterStores(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + ctx, finishAndGetRecording := tracing.ContextWithRecordingSpan( + context.Background(), tracing.NewTracer(), "test", + ) + + stopper, g, _, a, _ := createTestAllocator(2, false /* deterministic */) + defer stopper.Stop(context.Background()) + sg := gossiputil.NewStoreGossiper(g) + + // Create 2 stores. Store 2 has a voting replica and store 1 has a non-voting + // replica. Make it such that store 1 has a full disk so the allocator will + // want to rebalance it away. However, the only possible candidate is store 2 + // which already has a voting replica. Thus, this rebalance attempt should + // fail. + stores := []*roachpb.StoreDescriptor{ + { + StoreID: 1, + Capacity: roachpb.StoreCapacity{ + Capacity: 100, + Available: 0, + }, + }, + { + StoreID: 2, + Capacity: roachpb.StoreCapacity{ + Capacity: 100, + Available: 100, + }, + }, + } + existingNonVoters := replicas(1) + existingVoters := replicas(2) + + sg.GossipStores(stores, t) + var rangeUsageInfo RangeUsageInfo + add, remove, _, ok := a.RebalanceNonVoter( + ctx, + emptySpanConfig(), + nil, + existingVoters, + existingNonVoters, + rangeUsageInfo, + storeFilterThrottled, + a.scorerOptions(), + ) + + require.Falsef( + t, ok, "expected no action; got rebalance from s%d to s%d", remove.StoreID, add.StoreID, + ) + trace := finishAndGetRecording().String() + require.Regexpf( + t, + "it already has a voter", + trace, + "expected the voter store to be explicitly ignored; got %s", + trace, + ) +} + func TestRebalanceCandidatesNumReplicasConstraints(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t)