-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
kvserver: rebalance ranges with one voter using joint configurations
The allocator would add a voter, instead of both adding and removing the existing voter when rebalancing ranges with one replica. Removing the leaseholder replica was not possible prior to #74077, so the addition only was necessary. This restriction is no longer necessary. Allow rebalancing a one voter range between stores using joint configurations, where the lease will be transferred to the incoming voter store, from the outgoing demoting voter. Scattering ranges with one voter will now leave the range with exactly one voter, where previously both the leaseholder voter evaluating the scatter, and the new voter would be left. Before this patch, scattering 1000 ranges with RF=1 on a 5 store cluster: ``` store_id | replica_count | replica_distribution | lease_count | lease_distribution -----------+---------------+----------------------+-------------+--------------------- 1 | 1001 | ########## | 500 | ########## 5 | 291 | ### | 147 | ### 4 | 275 | ### | 137 | ### 3 | 229 | ### | 118 | ### 2 | 206 | ### | 99 | ## ``` After: ``` store_id | replica_count | replica_distribution | lease_count | lease_distribution -----------+---------------+----------------------+-------------+--------------------- 2 | 242 | ########## | 241 | ########## 4 | 227 | ########## | 227 | ########## 5 | 217 | ######### | 216 | ######### 3 | 209 | ######### | 208 | ######### 1 | 106 | ##### | 109 | ##### ``` Fixes: #108420 Fixes: #124171 Release note (bug fix): Scattering a range with replication factor=1, no longer erroneously up-replicates the range to two replicas. Leases will also no longer thrash between nodes when perturbed with replication factor=1.
- Loading branch information
Showing
7 changed files
with
326 additions
and
171 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,188 @@ | ||
| // Copyright 2024 The Cockroach Authors. | ||
| // | ||
| // Use of this software is governed by the Business Source License | ||
| // included in the file licenses/BSL.txt. | ||
| // | ||
| // As of the Change Date specified in that file, in accordance with | ||
| // the Business Source License, use of this software will be governed | ||
| // by the Apache License, Version 2.0, included in the file | ||
| // licenses/APL.txt. | ||
|
|
||
| package plan | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| "github.com/cockroachdb/cockroach/pkg/kv/kvpb" | ||
| "github.com/cockroachdb/cockroach/pkg/kv/kvserver/allocator/allocatorimpl" | ||
| "github.com/cockroachdb/cockroach/pkg/roachpb" | ||
| "github.com/cockroachdb/cockroach/pkg/util/leaktest" | ||
| "github.com/cockroachdb/cockroach/pkg/util/log" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // TestReplicationChangesForRebalance asserts that the replication changes for | ||
| // rebalancing are correct, given a a range descriptor and rebalance target. | ||
| func TestReplicationChangesForRebalance(t *testing.T) { | ||
| defer leaktest.AfterTest(t)() | ||
| defer log.Scope(t).Close(t) | ||
|
|
||
| ctx := context.Background() | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| desc *roachpb.RangeDescriptor | ||
| addTarget, removeTarget roachpb.ReplicationTarget | ||
| rebalanceTargetType allocatorimpl.TargetReplicaType | ||
| expectedChanges []kvpb.ReplicationChange | ||
| expectedPerformingSwap bool | ||
| expectedErrStr string | ||
| }{ | ||
| { | ||
| name: "rf=1 rebalance voter 1->2", | ||
| desc: &roachpb.RangeDescriptor{ | ||
| InternalReplicas: []roachpb.ReplicaDescriptor{ | ||
| {NodeID: 1, StoreID: 1, Type: roachpb.VOTER_FULL}, | ||
| }, | ||
| }, | ||
| addTarget: roachpb.ReplicationTarget{NodeID: 2, StoreID: 2}, | ||
| removeTarget: roachpb.ReplicationTarget{NodeID: 1, StoreID: 1}, | ||
| rebalanceTargetType: allocatorimpl.VoterTarget, | ||
| expectedChanges: []kvpb.ReplicationChange{ | ||
| {ChangeType: roachpb.ADD_VOTER, Target: roachpb.ReplicationTarget{NodeID: 2, StoreID: 2}}, | ||
| {ChangeType: roachpb.REMOVE_VOTER, Target: roachpb.ReplicationTarget{NodeID: 1, StoreID: 1}}, | ||
| }, | ||
| expectedPerformingSwap: false, | ||
| expectedErrStr: "", | ||
| }, | ||
| { | ||
| name: "rf=3 rebalance voter 1->4", | ||
| desc: &roachpb.RangeDescriptor{ | ||
| InternalReplicas: []roachpb.ReplicaDescriptor{ | ||
| {NodeID: 1, StoreID: 1, Type: roachpb.VOTER_FULL}, | ||
| {NodeID: 2, StoreID: 2, Type: roachpb.VOTER_FULL}, | ||
| {NodeID: 3, StoreID: 3, Type: roachpb.VOTER_FULL}, | ||
| }, | ||
| }, | ||
| addTarget: roachpb.ReplicationTarget{NodeID: 4, StoreID: 4}, | ||
| removeTarget: roachpb.ReplicationTarget{NodeID: 1, StoreID: 1}, | ||
| rebalanceTargetType: allocatorimpl.VoterTarget, | ||
| expectedChanges: []kvpb.ReplicationChange{ | ||
| {ChangeType: roachpb.ADD_VOTER, Target: roachpb.ReplicationTarget{NodeID: 4, StoreID: 4}}, | ||
| {ChangeType: roachpb.REMOVE_VOTER, Target: roachpb.ReplicationTarget{NodeID: 1, StoreID: 1}}, | ||
| }, | ||
| expectedPerformingSwap: false, | ||
| expectedErrStr: "", | ||
| }, | ||
| { | ||
| name: "rf=3 rebalance voter 1->3 error: already has a voter", | ||
| desc: &roachpb.RangeDescriptor{ | ||
| InternalReplicas: []roachpb.ReplicaDescriptor{ | ||
| {NodeID: 1, StoreID: 1, Type: roachpb.VOTER_FULL}, | ||
| {NodeID: 2, StoreID: 2, Type: roachpb.VOTER_FULL}, | ||
| {NodeID: 3, StoreID: 3, Type: roachpb.VOTER_FULL}, | ||
| }, | ||
| }, | ||
| addTarget: roachpb.ReplicationTarget{NodeID: 3, StoreID: 3}, | ||
| removeTarget: roachpb.ReplicationTarget{NodeID: 1, StoreID: 1}, | ||
| rebalanceTargetType: allocatorimpl.VoterTarget, | ||
| expectedChanges: nil, | ||
| expectedPerformingSwap: false, | ||
| expectedErrStr: "programming error: store being rebalanced to(3) already has a voting replica", | ||
| }, | ||
| { | ||
| name: "rf=3 rebalance non-voter: 1->4", | ||
| desc: &roachpb.RangeDescriptor{ | ||
| InternalReplicas: []roachpb.ReplicaDescriptor{ | ||
| {NodeID: 1, StoreID: 1, Type: roachpb.NON_VOTER}, | ||
| {NodeID: 2, StoreID: 2, Type: roachpb.VOTER_FULL}, | ||
| {NodeID: 3, StoreID: 3, Type: roachpb.VOTER_FULL}, | ||
| }, | ||
| }, | ||
| addTarget: roachpb.ReplicationTarget{NodeID: 4, StoreID: 4}, | ||
| removeTarget: roachpb.ReplicationTarget{NodeID: 1, StoreID: 1}, | ||
| rebalanceTargetType: allocatorimpl.NonVoterTarget, | ||
| expectedChanges: []kvpb.ReplicationChange{ | ||
| {ChangeType: roachpb.ADD_NON_VOTER, Target: roachpb.ReplicationTarget{NodeID: 4, StoreID: 4}}, | ||
| {ChangeType: roachpb.REMOVE_NON_VOTER, Target: roachpb.ReplicationTarget{NodeID: 1, StoreID: 1}}, | ||
| }, | ||
| expectedPerformingSwap: false, | ||
| expectedErrStr: "", | ||
| }, | ||
| { | ||
| name: "rf=3 rebalance non-voter 1->3 error: already has a voter", | ||
| desc: &roachpb.RangeDescriptor{ | ||
| InternalReplicas: []roachpb.ReplicaDescriptor{ | ||
| {NodeID: 1, StoreID: 1, Type: roachpb.NON_VOTER}, | ||
| {NodeID: 2, StoreID: 2, Type: roachpb.VOTER_FULL}, | ||
| {NodeID: 3, StoreID: 3, Type: roachpb.VOTER_FULL}, | ||
| }, | ||
| }, | ||
| addTarget: roachpb.ReplicationTarget{NodeID: 3, StoreID: 3}, | ||
| removeTarget: roachpb.ReplicationTarget{NodeID: 1, StoreID: 1}, | ||
| rebalanceTargetType: allocatorimpl.NonVoterTarget, | ||
| expectedChanges: nil, | ||
| expectedPerformingSwap: false, | ||
| expectedErrStr: "invalid rebalancing decision: trying to move non-voter to a store that already has a replica", | ||
| }, | ||
| { | ||
| name: "rf=3 rebalance non-voter 1->3 error: already has a non-voter", | ||
| desc: &roachpb.RangeDescriptor{ | ||
| InternalReplicas: []roachpb.ReplicaDescriptor{ | ||
| {NodeID: 1, StoreID: 1, Type: roachpb.NON_VOTER}, | ||
| {NodeID: 2, StoreID: 2, Type: roachpb.VOTER_FULL}, | ||
| {NodeID: 3, StoreID: 3, Type: roachpb.NON_VOTER}, | ||
| }, | ||
| }, | ||
| addTarget: roachpb.ReplicationTarget{NodeID: 3, StoreID: 3}, | ||
| removeTarget: roachpb.ReplicationTarget{NodeID: 1, StoreID: 1}, | ||
| rebalanceTargetType: allocatorimpl.NonVoterTarget, | ||
| expectedChanges: nil, | ||
| expectedPerformingSwap: false, | ||
| expectedErrStr: "invalid rebalancing decision: trying to move non-voter to a store that already has a replica", | ||
| }, | ||
| { | ||
| name: "rf=3 rebalance voter 1->3 swap", | ||
| desc: &roachpb.RangeDescriptor{ | ||
| InternalReplicas: []roachpb.ReplicaDescriptor{ | ||
| {NodeID: 1, StoreID: 1, Type: roachpb.VOTER_FULL}, | ||
| {NodeID: 2, StoreID: 2, Type: roachpb.VOTER_FULL}, | ||
| {NodeID: 3, StoreID: 3, Type: roachpb.NON_VOTER}, | ||
| }, | ||
| }, | ||
| addTarget: roachpb.ReplicationTarget{NodeID: 3, StoreID: 3}, | ||
| removeTarget: roachpb.ReplicationTarget{NodeID: 1, StoreID: 1}, | ||
| rebalanceTargetType: allocatorimpl.VoterTarget, | ||
| expectedChanges: []kvpb.ReplicationChange{ | ||
| {ChangeType: roachpb.ADD_VOTER, Target: roachpb.ReplicationTarget{NodeID: 3, StoreID: 3}}, | ||
| {ChangeType: roachpb.REMOVE_NON_VOTER, Target: roachpb.ReplicationTarget{NodeID: 3, StoreID: 3}}, | ||
| {ChangeType: roachpb.ADD_NON_VOTER, Target: roachpb.ReplicationTarget{NodeID: 1, StoreID: 1}}, | ||
| {ChangeType: roachpb.REMOVE_VOTER, Target: roachpb.ReplicationTarget{NodeID: 1, StoreID: 1}}, | ||
| }, | ||
| expectedPerformingSwap: true, | ||
| expectedErrStr: "", | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| chgs, performingSwap, err := ReplicationChangesForRebalance( | ||
| ctx, | ||
| tc.desc, | ||
| len(tc.desc.Replicas().VoterDescriptors()), | ||
| tc.addTarget, | ||
| tc.removeTarget, | ||
| tc.rebalanceTargetType, | ||
| ) | ||
| require.Equal(t, tc.expectedChanges, chgs) | ||
| require.Equal(t, tc.expectedPerformingSwap, performingSwap) | ||
| if tc.expectedErrStr != "" { | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), tc.expectedErrStr) | ||
| } else { | ||
| require.NoError(t, err) | ||
| } | ||
| }) | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.