From d7a7a49a6ca48758fb979c98a2aa04e358effab2 Mon Sep 17 00:00:00 2001 From: Aayush Shah Date: Thu, 25 Mar 2021 20:28:58 -0400 Subject: [PATCH] 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. Release note: None --- pkg/kv/kvserver/BUILD.bazel | 1 - pkg/kv/kvserver/client_merge_test.go | 197 +++++++++++---------------- pkg/kv/kvserver/merge_queue.go | 6 +- pkg/kv/kvserver/replica_command.go | 95 ------------- pkg/roachpb/metadata_replicas.go | 12 -- 5 files changed, 81 insertions(+), 230 deletions(-) diff --git a/pkg/kv/kvserver/BUILD.bazel b/pkg/kv/kvserver/BUILD.bazel index 726abae8625a..8c0034adab20 100644 --- a/pkg/kv/kvserver/BUILD.bazel +++ b/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", diff --git a/pkg/kv/kvserver/client_merge_test.go b/pkg/kv/kvserver/client_merge_test.go index 8feb60467878..a104540db7ed 100644 --- a/pkg/kv/kvserver/client_merge_test.go +++ b/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) diff --git a/pkg/kv/kvserver/merge_queue.go b/pkg/kv/kvserver/merge_queue.go index 924d7e425dee..81c4bdd57078 100644 --- a/pkg/kv/kvserver/merge_queue.go +++ b/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. diff --git a/pkg/kv/kvserver/replica_command.go b/pkg/kv/kvserver/replica_command.go index 5e5afd260fb3..b5347fea3b43 100644 --- a/pkg/kv/kvserver/replica_command.go +++ b/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) diff --git a/pkg/roachpb/metadata_replicas.go b/pkg/roachpb/metadata_replicas.go index cae6035c4c2d..77e61e9bad9e 100644 --- a/pkg/roachpb/metadata_replicas.go +++ b/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)