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 cockroachdb#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 cockroachdb#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.

Release note: None

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/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)

pkg/kv/kvserver/merge_queue.go

@@ -304,10 +304,8 @@ func (mq *mergeQueue) process(
 		// these ranges and only try to collocate them if they're not in violation,
 		// which would help us make better guarantees about not transiently
 		// violating constraints during a merge.
-		voterTargets, nonVoterTargets, err := GetTargetsToCollocateRHSForMerge(ctx, lhsDesc.Replicas(), rhsDesc.Replicas())
-		if err != nil {
-			return false, err
-		}
+		voterTargets := lhsDesc.Replicas().Voters().ReplicationTargets()
+		nonVoterTargets := lhsDesc.Replicas().NonVoters().ReplicationTargets()
 
 		// AdminRelocateRange moves the lease to the first target in the list, so
 		// sort the existing leaseholder there to leave it unchanged.

pkg/kv/kvserver/replica_command.go

@@ -20,7 +20,6 @@ import (
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
-	"github.com/cockroachdb/cockroach/pkg/build"
 	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv"
@@ -2467,100 +2466,6 @@ func replicasCollocated(a, b []roachpb.ReplicaDescriptor) bool {
 	return true
 }
 
-// GetTargetsToCollocateRHSForMerge decides the configuration of RHS replicas
-// need before the rhs can be subsumed and then merged into the LHS range. The
-// desired RHS voters and non-voters are returned; together they'll cover the
-// same stores as LHS's replicas, but the configuration of replicas doesn't
-// necessarily match (it doesn't need to match for the merge).
-//
-// We compute the new voter / non-voter targets for the RHS by first
-// bootstrapping our result set with the replicas that are already collocated.
-// We then step through RHS's non-collocated voters and try to move them to
-// stores that already have a voter for LHS. If this is not possible for all the
-// non-collocated voters of RHS (i.e. because the RHS has non-voter(s) on
-// store(s) where the LHS has voter(s)), we may move some RHS voters to targets
-// that have non-voters for LHS. Likewise, we do the same for the non-collocated
-// non-voters of RHS: try to relocate them to stores where the LHS has
-// non-voters, but resort to relocating them to stores where the LHS has voters.
-//
-// TODO(aayush): Can moving a voter replica from RHS to a store that has a
-// non-voter for LHS (or vice versa) can lead to constraint violations? Justify
-// why or why not.
-func GetTargetsToCollocateRHSForMerge(
-	ctx context.Context, leftRepls, rightRepls roachpb.ReplicaSet,
-) (voterTargets, nonVoterTargets []roachpb.ReplicationTarget, _ error) {
-	notInRight := func(desc roachpb.ReplicaDescriptor) bool {
-		return !rightRepls.Contains(desc)
-	}
-
-	// Sets of replicas that exist on the LHS but not on the RHS
-	leftMinusRight := leftRepls.Filter(notInRight)
-	leftMinusRightVoters := leftMinusRight.Voters().Descriptors()
-	leftMinusRightNonVoters := leftMinusRight.NonVoters().Descriptors()
-
-	// We bootstrap our result set by first including the replicas (voting and
-	// non-voting) that _are_ collocated, as these will stay unchanged and will
-	// be no-ops when passed through AdminRelocateRange.
-	finalRightVoters := rightRepls.Voters().Filter(leftRepls.Contains).DeepCopy()
-	finalRightNonVoters := rightRepls.NonVoters().Filter(leftRepls.Contains).DeepCopy()
-
-	needMore := func() bool {
-		return len(finalRightVoters.Descriptors())+len(finalRightNonVoters.Descriptors()) < len(leftRepls.Descriptors())
-	}
-
-	numVoters := len(leftRepls.VoterDescriptors())
-	// We loop through the set of non-collocated replicas and figure out a
-	// suitable configuration to relocate RHS's replicas to. At the end of these
-	// two loops, we will have exhausted `leftMinusRight`.
-	for len(finalRightVoters.Descriptors()) < numVoters && needMore() {
-		// Prefer to relocate voters for RHS to stores that have voters for LHS, but
-		// resort to relocating them to stores with non-voters for LHS if that's not
-		// possible.
-		if len(leftMinusRightVoters) != 0 {
-			finalRightVoters.AddReplica(leftMinusRightVoters[0])
-			leftMinusRightVoters = leftMinusRightVoters[1:]
-		} else if len(leftMinusRightNonVoters) != 0 {
-			finalRightVoters.AddReplica(leftMinusRightNonVoters[0])
-			leftMinusRightNonVoters = leftMinusRightNonVoters[1:]
-		} else {
-			log.Fatalf(ctx, "programming error: unexpectedly ran out of valid stores to relocate RHS"+
-				" voters to; LHS: %s, RHS: %s", leftRepls.Descriptors(), rightRepls.Descriptors())
-		}
-	}
-
-	for needMore() {
-		// Like above, we try to relocate non-voters for RHS to stores that have
-		// non-voters for LHS, but resort to relocating them to stores with voters
-		// for LHS if that's not possible.
-		if len(leftMinusRightNonVoters) != 0 {
-			finalRightNonVoters.AddReplica(leftMinusRightNonVoters[0])
-			leftMinusRightNonVoters = leftMinusRightNonVoters[1:]
-		} else if len(leftMinusRightVoters) != 0 {
-			finalRightNonVoters.AddReplica(leftMinusRightVoters[0])
-			leftMinusRightVoters = leftMinusRightVoters[1:]
-		} else {
-			log.Fatalf(ctx, "programming error: unexpectedly ran out of valid stores to relocate RHS"+
-				" non-voters to; LHS: %s, RHS: %s", leftRepls.Descriptors(), rightRepls.Descriptors())
-		}
-	}
-
-	if len(finalRightVoters.Descriptors()) == 0 {
-		// TODO(aayush): We can end up in this case for scenarios like the
-		// following (the digits represent StoreIDs):
-		//
-		// LHS-> voters: {1, 2, 3}, non-voters: {}
-		// RHS-> voters: {4}, non-voters: {1, 2, 3}
-		//
-		// Remove this error path once we support swapping voters and non-voters.
-		return nil, nil,
-			errors.UnimplementedErrorf(errors.IssueLink{IssueURL: build.MakeIssueURL(58499)},
-				"unsupported configuration of RHS(%s) and LHS(%s) as it requires an atomic swap of a"+
-					" voter and non-voter", rightRepls, leftRepls)
-	}
-
-	return finalRightVoters.ReplicationTargets(), finalRightNonVoters.ReplicationTargets(), nil
-}
-
 func checkDescsEqual(desc *roachpb.RangeDescriptor) func(*roachpb.RangeDescriptor) bool {
 	return func(desc2 *roachpb.RangeDescriptor) bool {
 		return desc.Equal(desc2)

pkg/roachpb/metadata_replicas.go

@@ -334,18 +334,6 @@ func (d ReplicaSet) DeepCopy() ReplicaSet {
 	}
 }
 
-// Contains returns true if the set contains rDesc.
-func (d ReplicaSet) Contains(rDesc ReplicaDescriptor) bool {
-	descs := d.Descriptors()
-	for i := range descs {
-		repl := &descs[i]
-		if repl.StoreID == rDesc.StoreID && repl.NodeID == rDesc.NodeID {
-			return true
-		}
-	}
-	return false
-}
-
 // AddReplica adds the given replica to this set.
 func (d *ReplicaSet) AddReplica(r ReplicaDescriptor) {
 	d.wrapped = append(d.wrapped, r)