Merge #62631

62631: kvserver: get rid of bespoke relocation logic used by the mergeQueue r=aayushshah15 a=aayushshah15

This PR contains two main commits:

**kvserver: update `AdminRelocateRange` to leverage explicit swaps of
voters to non-voters**

This commit updates `AdminRelocateRange` to use explicit atomic swaps of
voting replicas with non-voting replicas, that #58627 initially added
support for. The patch does so by generalizing behavior that's already
exercised by the `replicateQueue` when it decides to rebalance replicas.
See #61239.

This allows us, in the next commit, to remove bespoke relocation logic
that's used by the `mergeQueue` to align replica sets for the sake of a
range merge.

Release note: None

**kvserver: get rid of bespoke relocation logic used by the mergeQueue**

This commit removes the relocation logic used by the `mergeQueue` thus
far to align replica sets (added in #56197). This logic previously
existed in order to allow us to align the replica sets of a pair of
ranges (which is required for the range merge to proceed), while
avoiding redundant data movement.

Before #58627 and the previous commit in this PR, `AdminRelocateRange`
couldn't be directly used by the mergeQueue under various configurations
of the LHS and RHS ranges. Furthermore, even when it could be used, it
would involve redundant data movement. This all required us to compute
relocation targets for the RHS of a merge separately, above the call to
`AdminRelocateRange`, for the range merge to proceed.

All these limitations have been resolved by the previous commit which
teaches `AdminRelocateRange` to promote non-voters and demote voters
when needed, and the aforementioned bespoke relocation logic is no
longer needed.

Resolves #62370

Release note: None


Co-authored-by: aayush <aayush@pop-os.localdomain>
Co-authored-by: Aayush Shah <aayush.shah15@gmail.com>

pkg/kv/kvserver/BUILD.bazel

@@ -99,7 +99,6 @@ go_library(
     visibility = ["//visibility:public"],
     deps = [
         "//pkg/base",
-        "//pkg/build",
         "//pkg/clusterversion",
         "//pkg/config",
         "//pkg/config/zonepb",

pkg/kv/kvserver/allocator.go

@@ -184,14 +184,43 @@ const (
 	nonVoterTarget
 )
 
+// AddChangeType returns the roachpb.ReplicaChangeType corresponding to the
+// given targetReplicaType.
+//
+// TODO(aayush): Clean up usages of ADD_{NON_}VOTER. Use
+// targetReplicaType.{Add,Remove}ChangeType methods wherever possible.
+func (t targetReplicaType) AddChangeType() roachpb.ReplicaChangeType {
+	switch t {
+	case voterTarget:
+		return roachpb.ADD_VOTER
+	case nonVoterTarget:
+		return roachpb.ADD_NON_VOTER
+	default:
+		panic(fmt.Sprintf("unknown targetReplicaType %d", t))
+	}
+}
+
+// RemoveChangeType returns the roachpb.ReplicaChangeType corresponding to the
+// given targetReplicaType.
+func (t targetReplicaType) RemoveChangeType() roachpb.ReplicaChangeType {
+	switch t {
+	case voterTarget:
+		return roachpb.REMOVE_VOTER
+	case nonVoterTarget:
+		return roachpb.REMOVE_NON_VOTER
+	default:
+		panic(fmt.Sprintf("unknown targetReplicaType %d", t))
+	}
+}
+
 func (t targetReplicaType) String() string {
-	switch typ := t; typ {
+	switch t {
 	case voterTarget:
 		return "voter"
 	case nonVoterTarget:
 		return "non-voter"
 	default:
-		panic(fmt.Sprintf("unknown targetReplicaType %d", typ))
+		panic(fmt.Sprintf("unknown targetReplicaType %d", t))
 	}
 }
 

pkg/kv/kvserver/client_merge_test.go

@@ -18,7 +18,6 @@ import (
 	"math/rand"
 	"reflect"
 	"regexp"
-	"sort"
 	"strings"
 	"sync"
 	"sync/atomic"
@@ -4175,137 +4174,117 @@ func TestMergeQueueSeesNonVoters(t *testing.T) {
 	type test struct {
 		name                                                   string
 		leftVoters, rightVoters, leftNonVoters, rightNonVoters []int
-		expectedRightVoters, expectedRightNonVoters            []int
 	}
 
 	// NB: The test setup code places a single voter replica on (n1,s1) for both
 	// left and right range, which we remove after setting the test up.
 	tests := []test{
 		{
-			name:                   "collocated-per-type",
-			leftVoters:             []int{2, 3, 4},
-			rightVoters:            []int{2, 3, 4},
-			leftNonVoters:          []int{1},
-			rightNonVoters:         []int{1},
-			expectedRightVoters:    []int{2, 3, 4},
-			expectedRightNonVoters: []int{1},
+			name:           "collocated-per-type",
+			leftVoters:     []int{2, 3, 4},
+			rightVoters:    []int{2, 3, 4},
+			leftNonVoters:  []int{1},
+			rightNonVoters: []int{1},
 		},
 		{
-			name:                   "collocated-overall",
-			leftVoters:             []int{3, 4},
-			rightVoters:            []int{1, 2},
-			leftNonVoters:          []int{1, 2},
-			rightNonVoters:         []int{3, 4},
-			expectedRightVoters:    []int{1, 2},
-			expectedRightNonVoters: []int{3, 4},
+			name:           "collocated-overall",
+			leftVoters:     []int{3, 4},
+			rightVoters:    []int{1, 2},
+			leftNonVoters:  []int{1, 2},
+			rightNonVoters: []int{3, 4},
 		},
 		{
-			name:                   "collocated-voters-only",
-			leftVoters:             []int{3, 4},
-			rightVoters:            []int{3, 4},
-			leftNonVoters:          []int{2},
-			rightNonVoters:         []int{1},
-			expectedRightVoters:    []int{3, 4},
-			expectedRightNonVoters: []int{2},
+			name:           "collocated-voters-only",
+			leftVoters:     []int{3, 4},
+			rightVoters:    []int{3, 4},
+			leftNonVoters:  []int{2},
+			rightNonVoters: []int{1},
 		},
 		{
-			name:                   "collocated-non-voters-only",
-			leftVoters:             []int{3},
-			rightVoters:            []int{4},
-			leftNonVoters:          []int{1, 2},
-			rightNonVoters:         []int{1, 2},
-			expectedRightVoters:    []int{3},
-			expectedRightNonVoters: []int{1, 2},
+			name:           "collocated-non-voters-only",
+			leftVoters:     []int{3},
+			rightVoters:    []int{4},
+			leftNonVoters:  []int{1, 2},
+			rightNonVoters: []int{1, 2},
 		},
 		{
-			name:                   "not-collocated",
-			leftVoters:             []int{3},
-			rightVoters:            []int{4},
-			leftNonVoters:          []int{2},
-			rightNonVoters:         []int{1},
-			expectedRightVoters:    []int{3},
-			expectedRightNonVoters: []int{2},
+			name:           "not-collocated",
+			leftVoters:     []int{3},
+			rightVoters:    []int{4},
+			leftNonVoters:  []int{2},
+			rightNonVoters: []int{1},
 		},
 		{
-			name:                   "partially-collocated-voters-only",
-			leftVoters:             []int{2, 3},
-			rightVoters:            []int{1, 4},
-			leftNonVoters:          []int{1},
-			rightNonVoters:         []int{2},
-			expectedRightVoters:    []int{1, 3},
-			expectedRightNonVoters: []int{2},
+			name:           "partially-collocated-voters-only",
+			leftVoters:     []int{2, 3},
+			rightVoters:    []int{1, 4},
+			leftNonVoters:  []int{1},
+			rightNonVoters: []int{2},
 		},
 		{
-			name:                   "partially-collocated-non-voters-only",
-			leftVoters:             []int{4},
-			rightVoters:            []int{4},
-			leftNonVoters:          []int{1, 3},
-			rightNonVoters:         []int{1, 2},
-			expectedRightVoters:    []int{4},
-			expectedRightNonVoters: []int{1, 3},
+			name:           "partially-collocated-non-voters-only",
+			leftVoters:     []int{4},
+			rightVoters:    []int{4},
+			leftNonVoters:  []int{1, 3},
+			rightNonVoters: []int{1, 2},
 		},
 		{
-			name:                   "partially-collocated",
-			leftVoters:             []int{2},
-			rightVoters:            []int{4},
-			leftNonVoters:          []int{1, 3},
-			rightNonVoters:         []int{1, 2},
-			expectedRightVoters:    []int{3},
-			expectedRightNonVoters: []int{1, 2},
+			name:           "partially-collocated",
+			leftVoters:     []int{2},
+			rightVoters:    []int{4},
+			leftNonVoters:  []int{1, 3},
+			rightNonVoters: []int{1, 2},
 		},
 		{
-			name:                   "collocated-rhs-being-reconfigured-1",
-			leftVoters:             []int{1, 2, 3},
-			rightVoters:            []int{1, 2, 3, 4, 5, 6},
-			leftNonVoters:          []int{4, 5, 6},
-			rightNonVoters:         []int{},
-			expectedRightVoters:    []int{1, 2, 3, 4, 5, 6},
-			expectedRightNonVoters: []int{},
+			name:           "collocated-rhs-being-reconfigured-1",
+			leftVoters:     []int{1, 2, 3},
+			rightVoters:    []int{1, 2, 3, 4, 5, 6},
+			leftNonVoters:  []int{4, 5, 6},
+			rightNonVoters: []int{},
 		},
 		{
-			name:                   "collocated-rhs-being-reconfigured-2",
-			leftVoters:             []int{1, 2, 3},
-			rightVoters:            []int{1, 2, 3, 4},
-			leftNonVoters:          []int{4, 5, 6},
-			rightNonVoters:         []int{},
-			expectedRightVoters:    []int{1, 2, 3, 4},
-			expectedRightNonVoters: []int{5, 6},
+			name:           "collocated-rhs-being-reconfigured-2",
+			leftVoters:     []int{1, 2, 3},
+			rightVoters:    []int{1, 2, 3, 4},
+			leftNonVoters:  []int{4, 5, 6},
+			rightNonVoters: []int{},
 		},
 		{
-			name:                   "collocated-rhs-being-reconfigured-3",
-			leftVoters:             []int{1, 2, 3},
-			rightVoters:            []int{1},
-			leftNonVoters:          []int{4, 5, 6},
-			rightNonVoters:         []int{2, 3, 4, 5, 6},
-			expectedRightVoters:    []int{1},
-			expectedRightNonVoters: []int{2, 3, 4, 5, 6},
+			name:           "collocated-rhs-being-reconfigured-3",
+			leftVoters:     []int{1, 2, 3},
+			rightVoters:    []int{1},
+			leftNonVoters:  []int{4, 5, 6},
+			rightNonVoters: []int{2, 3, 4, 5, 6},
 		},
 		{
-			name:                   "non-collocated-rhs-being-reconfigured",
-			leftVoters:             []int{1, 2, 3},
-			rightVoters:            []int{5},
-			leftNonVoters:          []int{4, 6},
-			rightNonVoters:         []int{},
-			expectedRightVoters:    []int{1, 2, 3},
-			expectedRightNonVoters: []int{4, 6},
+			name:           "non-collocated-rhs-being-reconfigured",
+			leftVoters:     []int{1, 2, 3},
+			rightVoters:    []int{5},
+			leftNonVoters:  []int{4, 6},
+			rightNonVoters: []int{},
 		},
 		{
-			name:                   "partially-collocated-rhs-being-downreplicated",
-			leftVoters:             []int{1, 2, 3},
-			rightVoters:            []int{1, 2, 3, 4, 5, 6},
-			leftNonVoters:          []int{4, 5},
-			rightNonVoters:         []int{},
-			expectedRightVoters:    []int{1, 2, 3, 4, 5},
-			expectedRightNonVoters: []int{},
+			name:           "partially-collocated-rhs-being-downreplicated",
+			leftVoters:     []int{1, 2, 3},
+			rightVoters:    []int{1, 2, 3, 4, 5, 6},
+			leftNonVoters:  []int{4, 5},
+			rightNonVoters: []int{},
 		},
 		{
-			name:                   "partially-collocated-rhs-being-upreplicated",
-			leftVoters:             []int{1, 2, 3},
-			rightVoters:            []int{1},
-			leftNonVoters:          []int{4, 5, 6},
-			rightNonVoters:         []int{},
-			expectedRightVoters:    []int{1, 2, 3},
-			expectedRightNonVoters: []int{4, 5, 6},
+			name:           "partially-collocated-rhs-being-upreplicated",
+			leftVoters:     []int{1, 2, 3},
+			rightVoters:    []int{1},
+			leftNonVoters:  []int{4, 5, 6},
+			rightNonVoters: []int{},
+		},
+		{
+			// This is a subtest that should trigger at least 3 voter<->non-voter
+			// swaps.
+			name:           "lhs-voters-collocated-with-rhs-non-voters",
+			leftVoters:     []int{1, 2, 3},
+			rightVoters:    []int{4},
+			leftNonVoters:  []int{},
+			rightNonVoters: []int{1, 2, 3},
 		},
 	}
 
@@ -4374,24 +4353,6 @@ func TestMergeQueueSeesNonVoters(t *testing.T) {
 			tc.RemoveVotersOrFatal(t, rightDesc.StartKey.AsRawKey(), tc.Target(0))
 			rightDesc = tc.LookupRangeOrFatal(t, rightDesc.StartKey.AsRawKey())
 
-			// Check that we're avoiding superfluous data movement.
-			voterTargets, nonVoterTargets, err := kvserver.GetTargetsToCollocateRHSForMerge(ctx, leftDesc.Replicas(), rightDesc.Replicas())
-			require.NoError(t, err)
-			require.Equal(t, len(subtest.expectedRightVoters), len(voterTargets))
-			require.Equal(t, len(subtest.expectedRightNonVoters), len(nonVoterTargets))
-			sort.Slice(voterTargets, func(i, j int) bool {
-				return voterTargets[i].NodeID < voterTargets[j].NodeID
-			})
-			sort.Slice(nonVoterTargets, func(i, j int) bool {
-				return nonVoterTargets[i].NodeID < nonVoterTargets[j].NodeID
-			})
-			for i := range subtest.expectedRightVoters {
-				require.Equal(t, tc.Target(subtest.expectedRightVoters[i]), voterTargets[i])
-			}
-			for i := range subtest.expectedRightNonVoters {
-				require.Equal(t, tc.Target(subtest.expectedRightNonVoters[i]), nonVoterTargets[i])
-			}
-
 			store.SetMergeQueueActive(true)
 			store.MustForceMergeScanAndProcess()
 			verifyMerged(t, store, leftDesc.StartKey, rightDesc.StartKey)