diff --git a/pkg/storage/allocator_scorer.go b/pkg/storage/allocator_scorer.go index 7746cfc440fe..e9fb1be0976e 100644 --- a/pkg/storage/allocator_scorer.go +++ b/pkg/storage/allocator_scorer.go @@ -321,7 +321,7 @@ func allocateCandidates( if !maxCapacityCheck(s) { continue } - diversityScore := diversityScore(s, existingNodeLocalities) + diversityScore := diversityAllocateScore(s, existingNodeLocalities) balanceScore := balanceScore(sl, s.Capacity, rangeInfo, options) candidates = append(candidates, candidate{ store: s, @@ -519,7 +519,7 @@ func rebalanceCandidates( s, rangeInfo, balanceScore, sl) continue } - diversityScore := rebalanceToDiversityScore(s, existingNodeLocalities) + diversityScore := diversityRebalanceScore(s, existingNodeLocalities) candidates = append(candidates, candidate{ store: s, valid: true, @@ -697,63 +697,98 @@ func constraintCheck(store roachpb.StoreDescriptor, constraints config.Constrain return true, positive } -// diversityScore returns a score between 1 and 0 where higher scores are stores -// with the fewest locality tiers in common with already existing replicas. -func diversityScore( +// diversityAllocateScore returns a value between 0 and 1 based on how +// desirable it would be to add a replica to store. A higher score means the +// store is a better fit. +func diversityAllocateScore( store roachpb.StoreDescriptor, existingNodeLocalities map[roachpb.NodeID]roachpb.Locality, ) float64 { - minScore := roachpb.MaxDiversityScore + var sumScore float64 + var numSamples int + // We don't need to calculate the overall diversityScore for the range, just + // how well the new store would fit, because for any store that we might + // consider adding the pairwise average diversity of the existing replicas + // is the same. for _, locality := range existingNodeLocalities { - if newScore := store.Node.Locality.DiversityScore(locality); newScore < minScore { - minScore = newScore - } + newScore := store.Node.Locality.DiversityScore(locality) + sumScore += newScore + numSamples++ } - return minScore + // If the range has no replicas, any node would be a perfect fit. + if numSamples == 0 { + return 1.0 + } + return sumScore / float64(numSamples) } -// diversityRemovalScore is the same as diversityScore, but for a node that's -// already present in existingNodeLocalities. It works by calculating the -// diversityScore for nodeID as if nodeID didn't already have a replica. -// As with diversityScore, a higher score indicates that the node is a better -// fit for the range (i.e. keeping it around is good for diversity). +// diversityRemovalScore returns a value between 0 and 1 based on how desirable +// it would be to remove a node's replica of a range. A higher score indicates +// that the node is a better fit (i.e. keeping it around is good for diversity). func diversityRemovalScore( nodeID roachpb.NodeID, existingNodeLocalities map[roachpb.NodeID]roachpb.Locality, ) float64 { - minScore := roachpb.MaxDiversityScore + var sumScore float64 + var numSamples int locality := existingNodeLocalities[nodeID] + // We don't need to calculate the overall diversityScore for the range, because the original overall diversityScore + // of this range is always the same. for otherNodeID, otherLocality := range existingNodeLocalities { if otherNodeID == nodeID { continue } - if newScore := otherLocality.DiversityScore(locality); newScore < minScore { - minScore = newScore - } + newScore := locality.DiversityScore(otherLocality) + sumScore += newScore + numSamples++ } - return minScore + if numSamples == 0 { + return 1.0 + } + return sumScore / float64(numSamples) } -// rebalanceToDiversityScore is like diversityScore, but it returns what -// the diversity score would be if the given store was added and one of the -// existing stores was removed. This is equivalent to the second lowest score. -// -// This is useful for considering rebalancing a range that already has enough -// replicas - it's perfectly fine to add a replica in the same locality as an -// existing replica in such cases. -func rebalanceToDiversityScore( +// diversityRebalanceScore returns a value between 0 and 1 based on how +// desirable it would be to rebalance away from an existing store to the target +// store. Because the store to be removed isn't specified, this assumes that +// the worst-fitting store from a diversity perspective will be removed. A +// higher score indicates that the provided store is a better fit for the +// range. +func diversityRebalanceScore( store roachpb.StoreDescriptor, existingNodeLocalities map[roachpb.NodeID]roachpb.Locality, ) float64 { - minScore := roachpb.MaxDiversityScore - nextMinScore := roachpb.MaxDiversityScore - for _, locality := range existingNodeLocalities { - newScore := store.Node.Locality.DiversityScore(locality) - if newScore < minScore { - nextMinScore = minScore - minScore = newScore - } else if newScore < nextMinScore { - nextMinScore = newScore + var numSamples int + var maxSumScore float64 + // For every existing node, calculate what the diversity score would be if we + // remove that node's replica to replace it with one on the provided store. + for removedNodeID := range existingNodeLocalities { + // Compute the pairwise diversity score of all remaining replicas, including + // the store we're considering adding. + numSamples = 0 + var sumScore float64 + for nodeID, locality := range existingNodeLocalities { + if nodeID == removedNodeID { + continue + } + newScore := store.Node.Locality.DiversityScore(locality) + sumScore += newScore + numSamples++ + for otherNodeID, otherLocality := range existingNodeLocalities { + if otherNodeID == nodeID || otherNodeID == removedNodeID { + continue + } + newScore := locality.DiversityScore(otherLocality) + sumScore += newScore + numSamples++ + } + } + if sumScore > maxSumScore { + maxSumScore = sumScore } } - return nextMinScore + // If the range has zero or one replicas, any node would be a perfect fit. + if numSamples == 0 { + return 1.0 + } + return maxSumScore / float64(numSamples) } type rangeCountStatus int diff --git a/pkg/storage/allocator_scorer_test.go b/pkg/storage/allocator_scorer_test.go index 1efb3cdb36dc..728626b5d7b5 100644 --- a/pkg/storage/allocator_scorer_test.go +++ b/pkg/storage/allocator_scorer_test.go @@ -29,6 +29,17 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/leaktest" ) +type storeScore struct { + storeID roachpb.StoreID + score float64 +} + +type storeScores []storeScore + +func (s storeScores) Len() int { return len(s) } +func (s storeScores) Less(i, j int) bool { return s[i].score < s[j].score } +func (s storeScores) Swap(i, j int) { s[i], s[j] = s[j], s[i] } + func TestOnlyValid(t *testing.T) { defer leaktest.AfterTest(t)() @@ -488,72 +499,62 @@ func TestConstraintCheck(t *testing.T) { } } -func TestDiversityScore(t *testing.T) { +func TestAllocateDiversityScore(t *testing.T) { defer leaktest.AfterTest(t)() + // Given a range that's located on stores, rank order which of the testStores + // are the best fit for allocating a new replica on. testCases := []struct { name string - existing []roachpb.NodeID - expected map[roachpb.StoreID]float64 + stores map[roachpb.StoreID]struct{} + expected []roachpb.StoreID }{ { - name: "no existing replicas", - expected: map[roachpb.StoreID]float64{ - testStoreUSa15: 1, - testStoreUSa15Dupe: 1, - testStoreUSa1: 1, - testStoreUSb: 1, - testStoreEurope: 1, - }, + name: "no existing replicas", + expected: []roachpb.StoreID{testStoreUSa15, testStoreUSa15Dupe, testStoreUSa1, testStoreUSb, testStoreEurope}, }, { - name: "one existing replica", - existing: []roachpb.NodeID{ - roachpb.NodeID(testStoreUSa15), - }, - expected: map[roachpb.StoreID]float64{ - testStoreUSa15: 0, - testStoreUSa15Dupe: 0, - testStoreUSa1: 1.0 / 4.0, - testStoreUSb: 1.0 / 2.0, - testStoreEurope: 1, + name: "one existing replicas", + stores: map[roachpb.StoreID]struct{}{ + testStoreUSa15: {}, }, + expected: []roachpb.StoreID{testStoreEurope, testStoreUSb, testStoreUSa1, testStoreUSa15Dupe}, }, { name: "two existing replicas", - existing: []roachpb.NodeID{ - roachpb.NodeID(testStoreUSa15), - roachpb.NodeID(testStoreEurope), - }, - expected: map[roachpb.StoreID]float64{ - testStoreUSa15: 0, - testStoreUSa15Dupe: 0, - testStoreUSa1: 1.0 / 4.0, - testStoreUSb: 1.0 / 2.0, - testStoreEurope: 0, + stores: map[roachpb.StoreID]struct{}{ + testStoreUSa15: {}, + testStoreEurope: {}, }, + expected: []roachpb.StoreID{testStoreUSb, testStoreUSa1, testStoreUSa15Dupe}, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { existingNodeLocalities := make(map[roachpb.NodeID]roachpb.Locality) - for _, nodeID := range tc.existing { - for _, s := range testStores { - if s.Node.NodeID == nodeID { - existingNodeLocalities[s.Node.NodeID] = s.Node.Locality - } + for _, s := range testStores { + if _, ok := tc.stores[s.StoreID]; ok { + existingNodeLocalities[s.Node.NodeID] = s.Node.Locality } } + var scores storeScores for _, s := range testStores { - actualScore := diversityScore(s, existingNodeLocalities) - expectedScore, ok := tc.expected[s.StoreID] - if !ok { - t.Fatalf("no expected score found for storeID %d", s.StoreID) + if _, ok := tc.stores[s.StoreID]; ok { + continue } - if actualScore != expectedScore { - t.Errorf("store %d expected diversity score: %.2f, actual %.2f", s.StoreID, expectedScore, actualScore) + var score storeScore + actualScore := diversityAllocateScore(s, existingNodeLocalities) + score.storeID = s.StoreID + score.score = actualScore + scores = append(scores, score) + } + sort.Sort(sort.Reverse(scores)) + for i := 0; i < len(scores); { + if scores[i].storeID != tc.expected[i] { + t.Fatalf("expected the result store order to be %v, but got %v", tc.expected, scores) } + i++ } }) } @@ -562,192 +563,167 @@ func TestDiversityScore(t *testing.T) { func TestRebalanceToDiversityScore(t *testing.T) { defer leaktest.AfterTest(t)() + // Given a range that's located on stores, rank order which of the testStores + // are the best fit for rebalancing to. testCases := []struct { name string - existing []roachpb.NodeID - expected map[roachpb.StoreID]float64 + stores map[roachpb.StoreID]struct{} + expected []roachpb.StoreID }{ { - name: "no existing replicas", - expected: map[roachpb.StoreID]float64{ - testStoreUSa15: 1, - testStoreUSa15Dupe: 1, - testStoreUSa1: 1, - testStoreUSb: 1, - testStoreEurope: 1, - }, + name: "no existing replicas", + expected: []roachpb.StoreID{testStoreUSa15, testStoreUSa15Dupe, testStoreUSa1, testStoreUSb, testStoreEurope}, }, { name: "one existing replica", - existing: []roachpb.NodeID{ - roachpb.NodeID(testStoreUSa15), - }, - expected: map[roachpb.StoreID]float64{ - testStoreUSa15: 1, - testStoreUSa15Dupe: 1, - testStoreUSa1: 1, - testStoreUSb: 1, - testStoreEurope: 1, + stores: map[roachpb.StoreID]struct{}{ + testStoreUSa15: {}, }, + expected: []roachpb.StoreID{testStoreUSa15Dupe, testStoreUSa1, testStoreUSb, testStoreEurope}, }, { name: "two existing replicas", - existing: []roachpb.NodeID{ - roachpb.NodeID(testStoreUSa15), - roachpb.NodeID(testStoreUSa1), - }, - expected: map[roachpb.StoreID]float64{ - testStoreUSa15: 1.0 / 4.0, - testStoreUSa15Dupe: 1.0 / 4.0, - testStoreUSa1: 1.0 / 4.0, - testStoreUSb: 1.0 / 2.0, - testStoreEurope: 1, + stores: map[roachpb.StoreID]struct{}{ + testStoreUSa15: {}, + testStoreUSa1: {}, }, + expected: []roachpb.StoreID{testStoreEurope, testStoreUSb, testStoreUSa15Dupe}, }, { name: "three existing replicas", - existing: []roachpb.NodeID{ - roachpb.NodeID(testStoreUSa15), - roachpb.NodeID(testStoreUSa1), - roachpb.NodeID(testStoreEurope), - }, - expected: map[roachpb.StoreID]float64{ - testStoreUSa15: 1.0 / 4.0, - testStoreUSa15Dupe: 1.0 / 4.0, - testStoreUSa1: 1.0 / 4.0, - testStoreUSb: 1.0 / 2.0, - testStoreEurope: 1.0, + stores: map[roachpb.StoreID]struct{}{ + testStoreUSa15: {}, + testStoreUSa1: {}, + testStoreEurope: {}, }, + expected: []roachpb.StoreID{testStoreUSb, testStoreUSa15Dupe}, }, { name: "three existing replicas with duplicate", - existing: []roachpb.NodeID{ - roachpb.NodeID(testStoreUSa15), - roachpb.NodeID(testStoreUSa15Dupe), - roachpb.NodeID(testStoreUSa1), - }, - expected: map[roachpb.StoreID]float64{ - testStoreUSa15: 0, - testStoreUSa15Dupe: 0, - testStoreUSa1: 1.0 / 4.0, - testStoreUSb: 1.0 / 2.0, - testStoreEurope: 1.0, + stores: map[roachpb.StoreID]struct{}{ + testStoreUSa15: {}, + testStoreUSa15Dupe: {}, + testStoreUSa1: {}, }, + expected: []roachpb.StoreID{testStoreEurope, testStoreUSb}, }, { name: "four existing replicas", - existing: []roachpb.NodeID{ - roachpb.NodeID(testStoreUSa15), - roachpb.NodeID(testStoreUSa1), - roachpb.NodeID(testStoreUSb), - roachpb.NodeID(testStoreEurope), - }, - expected: map[roachpb.StoreID]float64{ - testStoreUSa15: 1.0 / 4.0, - testStoreUSa15Dupe: 1.0 / 4.0, - testStoreUSa1: 1.0 / 4.0, - testStoreUSb: 1.0 / 2.0, - testStoreEurope: 1.0, + stores: map[roachpb.StoreID]struct{}{ + testStoreUSa15: {}, + testStoreUSa1: {}, + testStoreUSb: {}, + testStoreEurope: {}, }, + expected: []roachpb.StoreID{testStoreUSa15Dupe}, }, { name: "four existing replicas with duplicate", - existing: []roachpb.NodeID{ - roachpb.NodeID(testStoreUSa15), - roachpb.NodeID(testStoreUSa15Dupe), - roachpb.NodeID(testStoreUSa1), - roachpb.NodeID(testStoreUSb), - }, - expected: map[roachpb.StoreID]float64{ - testStoreUSa15: 0, - testStoreUSa15Dupe: 0, - testStoreUSa1: 1.0 / 4.0, - testStoreUSb: 1.0 / 2.0, - testStoreEurope: 1.0, + stores: map[roachpb.StoreID]struct{}{ + testStoreUSa15: {}, + testStoreUSa15Dupe: {}, + testStoreUSa1: {}, + testStoreUSb: {}, }, + expected: []roachpb.StoreID{testStoreEurope}, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { existingNodeLocalities := make(map[roachpb.NodeID]roachpb.Locality) - for _, nodeID := range tc.existing { - for _, s := range testStores { - if s.Node.NodeID == nodeID { - existingNodeLocalities[s.Node.NodeID] = s.Node.Locality - } + for _, s := range testStores { + if _, ok := tc.stores[s.StoreID]; ok { + existingNodeLocalities[s.Node.NodeID] = s.Node.Locality } } + var scores storeScores for _, s := range testStores { - actualScore := rebalanceToDiversityScore(s, existingNodeLocalities) - expectedScore, ok := tc.expected[s.StoreID] - if !ok { - t.Fatalf("no expected score found for storeID %d", s.StoreID) + if _, ok := tc.stores[s.StoreID]; ok { + continue } - if actualScore != expectedScore { - t.Errorf("store %d expected diversity score: %.2f, actual %.2f", s.StoreID, expectedScore, actualScore) + var score storeScore + actualScore := diversityRebalanceScore(s, existingNodeLocalities) + score.storeID = s.StoreID + score.score = actualScore + scores = append(scores, score) + } + sort.Sort(sort.Reverse(scores)) + for i := 0; i < len(scores); { + if scores[i].storeID != tc.expected[i] { + t.Fatalf("expected the result store order to be %v, but got %v", tc.expected, scores) } + i++ } }) } } -func TestDiversityRemovalScore(t *testing.T) { +func TestRemovalDiversityScore(t *testing.T) { defer leaktest.AfterTest(t)() + // Given a range that's located on stores, rank order which of the replicas + // should be removed. testCases := []struct { name string - expected map[roachpb.StoreID]float64 + stores map[roachpb.StoreID]struct{} + expected []roachpb.StoreID }{ { name: "four existing replicas", - expected: map[roachpb.StoreID]float64{ - testStoreUSa15: 1.0 / 4.0, - testStoreUSa1: 1.0 / 4.0, - testStoreUSb: 1.0 / 2.0, - testStoreEurope: 1, + stores: map[roachpb.StoreID]struct{}{ + testStoreUSa15: {}, + testStoreUSa1: {}, + testStoreUSb: {}, + testStoreEurope: {}, }, + expected: []roachpb.StoreID{testStoreEurope, testStoreUSb, testStoreUSa15, testStoreUSa1}, }, { name: "four existing replicas with duplicate", - expected: map[roachpb.StoreID]float64{ - testStoreUSa15: 0, - testStoreUSa15Dupe: 0, - testStoreUSb: 1.0 / 2.0, - testStoreEurope: 1, + stores: map[roachpb.StoreID]struct{}{ + testStoreUSa15: {}, + testStoreUSa15Dupe: {}, + testStoreUSb: {}, + testStoreEurope: {}, }, + expected: []roachpb.StoreID{testStoreEurope, testStoreUSb, testStoreUSa15, testStoreUSa15Dupe}, }, { name: "three existing replicas - excluding testStoreUSa15", - expected: map[roachpb.StoreID]float64{ - testStoreUSa1: 1.0 / 2.0, - testStoreUSb: 1.0 / 2.0, - testStoreEurope: 1, + stores: map[roachpb.StoreID]struct{}{ + testStoreUSa1: {}, + testStoreUSb: {}, + testStoreEurope: {}, }, + expected: []roachpb.StoreID{testStoreEurope, testStoreUSa1, testStoreUSb}, }, { name: "three existing replicas - excluding testStoreUSa1", - expected: map[roachpb.StoreID]float64{ - testStoreUSa15: 1.0 / 2.0, - testStoreUSb: 1.0 / 2.0, - testStoreEurope: 1, + stores: map[roachpb.StoreID]struct{}{ + testStoreUSa15: {}, + testStoreUSb: {}, + testStoreEurope: {}, }, + expected: []roachpb.StoreID{testStoreEurope, testStoreUSa15, testStoreUSb}, }, { name: "three existing replicas - excluding testStoreUSb", - expected: map[roachpb.StoreID]float64{ - testStoreUSa15: 1.0 / 4.0, - testStoreUSa1: 1.0 / 4.0, - testStoreEurope: 1.0, + stores: map[roachpb.StoreID]struct{}{ + testStoreUSa15: {}, + testStoreUSa1: {}, + testStoreEurope: {}, }, + expected: []roachpb.StoreID{testStoreEurope, testStoreUSa15, testStoreUSa1}, }, { name: "three existing replicas - excluding testStoreEurope", - expected: map[roachpb.StoreID]float64{ - testStoreUSa15: 1.0 / 4.0, - testStoreUSa1: 1.0 / 4.0, - testStoreUSb: 1.0 / 2.0, + stores: map[roachpb.StoreID]struct{}{ + testStoreUSa15: {}, + testStoreUSa1: {}, + testStoreUSb: {}, }, + expected: []roachpb.StoreID{testStoreUSb, testStoreUSa15, testStoreUSa1}, }, } @@ -755,22 +731,27 @@ func TestDiversityRemovalScore(t *testing.T) { t.Run(tc.name, func(t *testing.T) { existingNodeLocalities := make(map[roachpb.NodeID]roachpb.Locality) for _, s := range testStores { - if _, ok := tc.expected[s.StoreID]; ok { + if _, ok := tc.stores[s.StoreID]; ok { existingNodeLocalities[s.Node.NodeID] = s.Node.Locality } } + var scores storeScores for _, s := range testStores { - if _, ok := tc.expected[s.StoreID]; !ok { + if _, ok := tc.stores[s.StoreID]; !ok { continue } + var score storeScore actualScore := diversityRemovalScore(s.Node.NodeID, existingNodeLocalities) - expectedScore, ok := tc.expected[s.StoreID] - if !ok { - t.Fatalf("no expected score found for storeID %d", s.StoreID) - } - if actualScore != expectedScore { - t.Errorf("store %d expected diversity removal score: %.2f, actual %.2f", s.StoreID, expectedScore, actualScore) + score.storeID = s.StoreID + score.score = actualScore + scores = append(scores, score) + } + sort.Sort(sort.Reverse(scores)) + for i := 0; i < len(scores); { + if scores[i].storeID != tc.expected[i] { + t.Fatalf("expected the result store order to be %v, but got %v", tc.expected, scores) } + i++ } }) } diff --git a/pkg/storage/allocator_test.go b/pkg/storage/allocator_test.go index 48dc900c3c34..4a5657056283 100644 --- a/pkg/storage/allocator_test.go +++ b/pkg/storage/allocator_test.go @@ -180,6 +180,97 @@ var multiDCStores = []*roachpb.StoreDescriptor{ }, } +var multiDiversityDCStores = []*roachpb.StoreDescriptor{ + { + StoreID: 1, + Node: roachpb.NodeDescriptor{ + NodeID: 1, + Locality: roachpb.Locality{ + Tiers: []roachpb.Tier{ + {Key: "datacenter", Value: "a"}, + }, + }, + }, + }, + { + StoreID: 2, + Node: roachpb.NodeDescriptor{ + NodeID: 2, + Locality: roachpb.Locality{ + Tiers: []roachpb.Tier{ + {Key: "datacenter", Value: "a"}, + }, + }, + }, + }, + { + StoreID: 3, + Node: roachpb.NodeDescriptor{ + NodeID: 3, + Locality: roachpb.Locality{ + Tiers: []roachpb.Tier{ + {Key: "datacenter", Value: "b"}, + }, + }, + }, + }, + { + StoreID: 4, + Node: roachpb.NodeDescriptor{ + NodeID: 4, + Locality: roachpb.Locality{ + Tiers: []roachpb.Tier{ + {Key: "datacenter", Value: "b"}, + }, + }, + }, + }, + { + StoreID: 5, + Node: roachpb.NodeDescriptor{ + NodeID: 5, + Locality: roachpb.Locality{ + Tiers: []roachpb.Tier{ + {Key: "datacenter", Value: "c"}, + }, + }, + }, + }, + { + StoreID: 6, + Node: roachpb.NodeDescriptor{ + NodeID: 6, + Locality: roachpb.Locality{ + Tiers: []roachpb.Tier{ + {Key: "datacenter", Value: "c"}, + }, + }, + }, + }, + { + StoreID: 7, + Node: roachpb.NodeDescriptor{ + NodeID: 7, + Locality: roachpb.Locality{ + Tiers: []roachpb.Tier{ + {Key: "datacenter", Value: "d"}, + }, + }, + }, + }, + { + StoreID: 8, + Node: roachpb.NodeDescriptor{ + NodeID: 8, + Locality: roachpb.Locality{ + Tiers: []roachpb.Tier{ + {Key: "datacenter", Value: "d"}, + }, + }, + }, + }, +} + func testRangeInfo(replicas []roachpb.ReplicaDescriptor, rangeID roachpb.RangeID) RangeInfo { return RangeInfo{ Desc: &roachpb.RangeDescriptor{ @@ -1292,6 +1383,284 @@ func TestAllocatorShouldTransferLease(t *testing.T) { } } +func TestAllocatorRemoveTargetLocality(t *testing.T) { + defer leaktest.AfterTest(t)() + + stopper, g, _, a, _ := createTestAllocator( /* deterministic */ false) + defer stopper.Stop(context.Background()) + sg := gossiputil.NewStoreGossiper(g) + sg.GossipStores(multiDiversityDCStores, t) + + // Given a set of existing replicas for a range, pick out the ones that should + // be removed purely on the basis of locality diversity. + testCases := []struct { + existing []roachpb.StoreID + expected []roachpb.StoreID + }{ + { + []roachpb.StoreID{1, 2, 3, 5}, + []roachpb.StoreID{1, 2}, + }, + { + []roachpb.StoreID{1, 2, 3}, + []roachpb.StoreID{1, 2}, + }, + { + []roachpb.StoreID{1, 3, 4, 5}, + []roachpb.StoreID{3, 4}, + }, + { + []roachpb.StoreID{1, 3, 5, 6}, + []roachpb.StoreID{5, 6}, + }, + { + []roachpb.StoreID{1, 3, 5}, + []roachpb.StoreID{1, 3, 5}, + }, + { + []roachpb.StoreID{1, 3, 4, 6, 7, 8}, + []roachpb.StoreID{3, 4, 7, 8}, + }, + } + for _, c := range testCases { + existingRepls := make([]roachpb.ReplicaDescriptor, len(c.existing)) + for i, storeID := range c.existing { + existingRepls[i] = roachpb.ReplicaDescriptor{ + NodeID: roachpb.NodeID(storeID), + StoreID: storeID, + } + } + targetRepl, details, err := a.RemoveTarget( + context.Background(), + config.Constraints{}, + existingRepls, + testRangeInfo(existingRepls, firstRange), + false, + ) + if err != nil { + t.Fatal(err) + } + var found bool + for _, storeID := range c.expected { + if targetRepl.StoreID == storeID { + found = true + break + } + } + if !found { + t.Errorf("expected RemoveTarget(%v) in %v, but got %d; details: %s", c.existing, c.expected, targetRepl.StoreID, details) + } + } +} + +func TestAllocatorAllocateTargetLocality(t *testing.T) { + defer leaktest.AfterTest(t)() + + stopper, g, _, a, _ := createTestAllocator( /* deterministic */ false) + defer stopper.Stop(context.Background()) + sg := gossiputil.NewStoreGossiper(g) + sg.GossipStores(multiDiversityDCStores, t) + + // Given a set of existing replicas for a range, rank which of the remaining + // stores from multiDiversityDCStores would be the best addition to the range + // purely on the basis of locality diversity. + testCases := []struct { + existing []roachpb.StoreID + expected []roachpb.StoreID + }{ + { + []roachpb.StoreID{1, 2, 3}, + []roachpb.StoreID{5, 6, 7, 8}, + }, + { + []roachpb.StoreID{1, 3, 4}, + []roachpb.StoreID{5, 6, 7, 8}, + }, + { + []roachpb.StoreID{3, 4, 5}, + []roachpb.StoreID{1, 2, 7, 8}, + }, + { + []roachpb.StoreID{1, 7, 8}, + []roachpb.StoreID{3, 4, 5, 6}, + }, + { + []roachpb.StoreID{5, 7, 8}, + []roachpb.StoreID{1, 2, 3, 4}, + }, + { + []roachpb.StoreID{1, 3, 5}, + []roachpb.StoreID{7, 8}, + }, + { + []roachpb.StoreID{1, 3, 7}, + []roachpb.StoreID{5, 6}, + }, + { + []roachpb.StoreID{1, 5, 7}, + []roachpb.StoreID{3, 4}, + }, + { + []roachpb.StoreID{3, 5, 7}, + []roachpb.StoreID{1, 2}, + }, + } + + for _, c := range testCases { + existingRepls := make([]roachpb.ReplicaDescriptor, len(c.existing)) + for i, storeID := range c.existing { + existingRepls[i] = roachpb.ReplicaDescriptor{ + NodeID: roachpb.NodeID(storeID), + StoreID: storeID, + } + } + targetStore, details, err := a.AllocateTarget( + context.Background(), + config.Constraints{}, + existingRepls, + testRangeInfo(existingRepls, firstRange), + false, + false, + ) + if err != nil { + t.Fatal(err) + } + var found bool + for _, storeID := range c.expected { + if targetStore.StoreID == storeID { + found = true + break + } + } + if !found { + t.Errorf("expected AllocateTarget(%v) in %v, but got %d; details: %s", c.existing, c.expected, targetStore.StoreID, details) + } + } +} + +func TestAllocatorRebalanceTargetLocality(t *testing.T) { + defer leaktest.AfterTest(t)() + + stopper, g, _, a, _ := createTestAllocator( /* deterministic */ false) + defer stopper.Stop(context.Background()) + + stores := []*roachpb.StoreDescriptor{ + { + StoreID: 1, + Node: multiDiversityDCStores[0].Node, + Capacity: roachpb.StoreCapacity{RangeCount: 10}, + }, + { + StoreID: 2, + Node: multiDiversityDCStores[1].Node, + Capacity: roachpb.StoreCapacity{RangeCount: 20}, + }, + { + StoreID: 3, + Node: multiDiversityDCStores[2].Node, + Capacity: roachpb.StoreCapacity{RangeCount: 10}, + }, + { + StoreID: 4, + Node: multiDiversityDCStores[3].Node, + Capacity: roachpb.StoreCapacity{RangeCount: 20}, + }, + { + StoreID: 5, + Node: multiDiversityDCStores[4].Node, + Capacity: roachpb.StoreCapacity{RangeCount: 10}, + }, + { + StoreID: 6, + Node: multiDiversityDCStores[5].Node, + Capacity: roachpb.StoreCapacity{RangeCount: 20}, + }, + } + sg := gossiputil.NewStoreGossiper(g) + sg.GossipStores(stores, t) + + testCases := []struct { + existing []roachpb.StoreID + expected []roachpb.StoreID + }{ + { + []roachpb.StoreID{1, 2, 3}, + []roachpb.StoreID{5}, + }, + { + []roachpb.StoreID{1, 3, 4}, + []roachpb.StoreID{5}, + }, + { + []roachpb.StoreID{1, 3, 6}, + []roachpb.StoreID{5}, + }, + { + []roachpb.StoreID{1, 2, 5}, + []roachpb.StoreID{3}, + }, + { + []roachpb.StoreID{1, 2, 6}, + []roachpb.StoreID{3}, + }, + { + []roachpb.StoreID{1, 4, 5}, + []roachpb.StoreID{3}, + }, + { + []roachpb.StoreID{1, 4, 6}, + []roachpb.StoreID{3, 5}, + }, + { + []roachpb.StoreID{3, 4, 5}, + []roachpb.StoreID{1}, + }, + { + []roachpb.StoreID{3, 4, 6}, + []roachpb.StoreID{1}, + }, + { + []roachpb.StoreID{4, 5, 6}, + []roachpb.StoreID{1}, + }, + { + []roachpb.StoreID{2, 4, 6}, + []roachpb.StoreID{1, 3, 5}, + }, + } + + for _, c := range testCases { + existingRepls := make([]roachpb.ReplicaDescriptor, len(c.existing)) + for i, storeID := range c.existing { + existingRepls[i] = roachpb.ReplicaDescriptor{ + NodeID: roachpb.NodeID(storeID), + StoreID: storeID, + } + } + targetStore, details := a.RebalanceTarget( + context.Background(), + config.Constraints{}, + nil, + testRangeInfo(existingRepls, firstRange), + storeFilterThrottled, + false, + ) + if targetStore == nil { + t.Fatalf("RebalanceTarget(%v) returned no target store; details: %s", c.existing, details) + } + var found bool + for _, storeID := range c.expected { + if targetStore.StoreID == storeID { + found = true + break + } + } + if !found { + t.Errorf("expected RebalanceTarget(%v) in %v, but got %d; details: %s", c.existing, c.expected, targetStore.StoreID, details) + } + } +} + // Test out the load-based lease transfer algorithm against a variety of // request distributions and inter-node latencies. func TestAllocatorTransferLeaseTargetLoadBased(t *testing.T) {