From 41baa42a1759f50e455d27b92340244feef39794 Mon Sep 17 00:00:00 2001 From: disksing Date: Thu, 2 Jul 2020 00:37:09 +0800 Subject: [PATCH 1/3] placement: fix placement location issue Signed-off-by: disksing --- server/schedule/placement/fit.go | 47 +++++++++++++++----------------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/server/schedule/placement/fit.go b/server/schedule/placement/fit.go index 8f35d30862c..37e56af457d 100644 --- a/server/schedule/placement/fit.go +++ b/server/schedule/placement/fit.go @@ -14,6 +14,7 @@ package placement import ( + "math" "sort" "github.com/pingcap/kvproto/pkg/metapb" @@ -85,9 +86,9 @@ type RuleFit struct { // different Role from configuration (the Role can be migrated to target role // by scheduling). PeersWithDifferentRole []*metapb.Peer - // IsolationLevel indicates at which level of labeling these Peers are - // isolated. A larger value indicates a higher isolation level. - IsolationLevel int + // IsolationScore indicates at which level of labeling these Peers are + // isolated. A larger value is better. + IsolationScore float64 } // IsSatisfied returns if the rule is properly satisfied. @@ -105,9 +106,9 @@ func compareRuleFit(a, b *RuleFit) int { return -1 case len(a.PeersWithDifferentRole) < len(b.PeersWithDifferentRole): return 1 - case a.IsolationLevel < b.IsolationLevel: + case a.IsolationScore < b.IsolationScore: return -1 - case a.IsolationLevel > b.IsolationLevel: + case a.IsolationScore > b.IsolationScore: return 1 default: return 0 @@ -159,7 +160,7 @@ func fitRule(peers []*fitPeer, rule *Rule) *RuleFit { } func newRuleFit(rule *Rule, peers []*fitPeer) *RuleFit { - rf := &RuleFit{Rule: rule, IsolationLevel: isolationLevel(peers, rule.LocationLabels)} + rf := &RuleFit{Rule: rule, IsolationScore: isolationScore(peers, rule.LocationLabels)} for _, p := range peers { rf.Peers = append(rf.Peers, p.Peer) if !p.matchRoleStrict(rule.Role) { @@ -236,28 +237,24 @@ func iterPeersRecr(peers []*fitPeer, index int, out []*fitPeer, f func()) { } } -func isolationLevel(peers []*fitPeer, labels []string) int { - if len(labels) == 0 || len(peers) == 0 { +func isolationScore(peers []*fitPeer, labels []string) float64 { + var score float64 + if len(labels) == 0 || len(peers) <= 1 { return 0 } - if len(peers) == 1 { - return len(labels) - } - if len(peers) == 2 { - for l, label := range labels { - if peers[0].store.GetLabelValue(label) != peers[1].store.GetLabelValue(label) { - return len(labels) - l + // NOTE: following loop is partially duplicated with `core.DistinctScore`. + // The reason not to call it directly is that core.DistinctScore only + // accepts `[]StoreInfo` not `[]*fitPeer` and I don't want alloc slice + // here because it is kind of hot path. + // After Go supports generics, we will be enable to do some refactor and + // reuse `core.DistinctScore`. + const replicaBaseScore = 100 + for i, p1 := range peers { + for _, p2 := range peers[i:] { + if index := p1.store.CompareLocation(p2.store, labels); index != -1 { + score += math.Pow(replicaBaseScore, float64(len(labels)-index-1)) } } - return 0 } - - // TODO: brute force can be improved. - level := len(labels) - iterPeers(peers, 2, func(pair []*fitPeer) { - if l := isolationLevel(pair, labels); l < level { - level = l - } - }) - return level + return score } From 6f811d572da2ee8747ac719808168807ecda5b07 Mon Sep 17 00:00:00 2001 From: disksing Date: Thu, 2 Jul 2020 00:37:22 +0800 Subject: [PATCH 2/3] add tests Signed-off-by: disksing --- server/schedule/checker/rule_checker_test.go | 24 ++++++ server/schedule/placement/fit_test.go | 84 ++++++++++---------- 2 files changed, 65 insertions(+), 43 deletions(-) diff --git a/server/schedule/checker/rule_checker_test.go b/server/schedule/checker/rule_checker_test.go index 3c52da406d7..198048e67d3 100644 --- a/server/schedule/checker/rule_checker_test.go +++ b/server/schedule/checker/rule_checker_test.go @@ -203,6 +203,30 @@ func (s *testRuleCheckerSuite) TestBetterReplacement(c *C) { c.Assert(op, IsNil) } +func (s *testRuleCheckerSuite) TestBetterReplacement2(c *C) { + s.cluster.AddLabelsStore(1, 1, map[string]string{"zone": "z1", "host": "host1"}) + s.cluster.AddLabelsStore(2, 1, map[string]string{"zone": "z1", "host": "host2"}) + s.cluster.AddLabelsStore(3, 1, map[string]string{"zone": "z1", "host": "host3"}) + s.cluster.AddLabelsStore(4, 1, map[string]string{"zone": "z2", "host": "host1"}) + s.cluster.AddLeaderRegionWithRange(1, "", "", 1, 2, 3) + s.ruleManager.SetRule(&placement.Rule{ + GroupID: "pd", + ID: "test", + Index: 100, + Override: true, + Role: placement.Voter, + Count: 3, + LocationLabels: []string{"zone", "host"}, + }) + op := s.rc.Check(s.cluster.GetRegion(1)) + c.Assert(op, NotNil) + c.Assert(op.Desc(), Equals, "move-to-better-location") + c.Assert(op.Step(0).(operator.AddLearner).ToStore, Equals, uint64(4)) + s.cluster.AddLeaderRegionWithRange(1, "", "", 1, 3, 4) + op = s.rc.Check(s.cluster.GetRegion(1)) + c.Assert(op, IsNil) +} + func (s *testRuleCheckerSuite) TestNoBetterReplacement(c *C) { s.cluster.AddLabelsStore(1, 1, map[string]string{"host": "host1"}) s.cluster.AddLabelsStore(2, 1, map[string]string{"host": "host1"}) diff --git a/server/schedule/placement/fit_test.go b/server/schedule/placement/fit_test.go index 32e3ce74af2..1c17d2debf9 100644 --- a/server/schedule/placement/fit_test.go +++ b/server/schedule/placement/fit_test.go @@ -54,8 +54,7 @@ func (s *testFitSuite) TestFitByLocation(c *C) { count int // default: len(peerStoreID) role PeerRoleType // default: Voter // expect result: - expectedPeers []uint64 // default: same as peerStoreID - expectedIsolationLevel int // default: 0 + expectedPeers []uint64 // default: same as peerStoreID } cases := []Case{ @@ -65,59 +64,59 @@ func (s *testFitSuite) TestFitByLocation(c *C) { {peerStoreID: []uint64{1111, 1112, 1113}, count: 3, expectedPeers: []uint64{1111, 1112, 1113}}, {peerStoreID: []uint64{1111, 1112, 1113}, count: 5, expectedPeers: []uint64{1111, 1112, 1113}}, // test isolation level - {peerStoreID: []uint64{1111}, locationLabels: "zone,rack,host", expectedIsolationLevel: 3}, - {peerStoreID: []uint64{1111}, locationLabels: "zone,rack", expectedIsolationLevel: 2}, - {peerStoreID: []uint64{1111}, locationLabels: "zone", expectedIsolationLevel: 1}, - {peerStoreID: []uint64{1111}, locationLabels: "", expectedIsolationLevel: 0}, - {peerStoreID: []uint64{1111, 2111}, locationLabels: "zone,rack,host", expectedIsolationLevel: 3}, - {peerStoreID: []uint64{1111, 2222, 3333}, locationLabels: "zone,rack,host", expectedIsolationLevel: 3}, - {peerStoreID: []uint64{1111, 1211, 3111}, locationLabels: "zone,rack,host", expectedIsolationLevel: 2}, - {peerStoreID: []uint64{1111, 1121, 3111}, locationLabels: "zone,rack,host", expectedIsolationLevel: 1}, - {peerStoreID: []uint64{1111, 1121, 1122}, locationLabels: "zone,rack,host", expectedIsolationLevel: 0}, + {peerStoreID: []uint64{1111}, locationLabels: "zone,rack,host"}, + {peerStoreID: []uint64{1111}, locationLabels: "zone,rack"}, + {peerStoreID: []uint64{1111}, locationLabels: "zone"}, + {peerStoreID: []uint64{1111}, locationLabels: ""}, + {peerStoreID: []uint64{1111, 2111}, locationLabels: "zone,rack,host"}, + {peerStoreID: []uint64{1111, 2222, 3333}, locationLabels: "zone,rack,host"}, + {peerStoreID: []uint64{1111, 1211, 3111}, locationLabels: "zone,rack,host"}, + {peerStoreID: []uint64{1111, 1121, 3111}, locationLabels: "zone,rack,host"}, + {peerStoreID: []uint64{1111, 1121, 1122}, locationLabels: "zone,rack,host"}, // test best location { - peerStoreID: []uint64{1111, 1112, 1113, 2111, 2222, 3222, 3333}, - locationLabels: "zone,rack,host", - count: 3, - expectedPeers: []uint64{1111, 2111, 3222}, - expectedIsolationLevel: 3, + peerStoreID: []uint64{1111, 1112, 1113, 2111, 2222, 3222, 3333}, + locationLabels: "zone,rack,host", + count: 3, + expectedPeers: []uint64{1111, 2111, 3222}, }, { - peerStoreID: []uint64{1111, 1121, 1211, 2111, 2211}, - locationLabels: "zone,rack,host", - count: 3, - expectedPeers: []uint64{1111, 1211, 2111}, - expectedIsolationLevel: 2, + peerStoreID: []uint64{1111, 1121, 1211, 2111, 2211}, + locationLabels: "zone,rack,host", + count: 3, + expectedPeers: []uint64{1111, 1211, 2111}, + }, + { + peerStoreID: []uint64{1111, 1211, 1311, 1411, 2111, 2211, 2311, 3111}, + locationLabels: "zone,rack,host", + count: 5, + expectedPeers: []uint64{1111, 1211, 2111, 2211, 3111}, }, // test role match { - peerStoreID: []uint64{1111, 1112, 1113}, - peerRole: []PeerRoleType{Learner, Follower, Follower}, - count: 1, - expectedPeers: []uint64{1112}, - expectedIsolationLevel: 0, + peerStoreID: []uint64{1111, 1112, 1113}, + peerRole: []PeerRoleType{Learner, Follower, Follower}, + count: 1, + expectedPeers: []uint64{1112}, }, { - peerStoreID: []uint64{1111, 1112, 1113}, - peerRole: []PeerRoleType{Learner, Follower, Follower}, - count: 2, - expectedPeers: []uint64{1112, 1113}, - expectedIsolationLevel: 0, + peerStoreID: []uint64{1111, 1112, 1113}, + peerRole: []PeerRoleType{Learner, Follower, Follower}, + count: 2, + expectedPeers: []uint64{1112, 1113}, }, { - peerStoreID: []uint64{1111, 1112, 1113}, - peerRole: []PeerRoleType{Learner, Follower, Follower}, - count: 3, - expectedPeers: []uint64{1112, 1113, 1111}, - expectedIsolationLevel: 0, + peerStoreID: []uint64{1111, 1112, 1113}, + peerRole: []PeerRoleType{Learner, Follower, Follower}, + count: 3, + expectedPeers: []uint64{1112, 1113, 1111}, }, { - peerStoreID: []uint64{1111, 1112, 1121, 1122, 1131, 1132, 1141, 1142}, - peerRole: []PeerRoleType{Follower, Learner, Learner, Learner, Learner, Follower, Follower, Follower}, - locationLabels: "zone,rack,host", - count: 3, - expectedPeers: []uint64{1111, 1132, 1141}, - expectedIsolationLevel: 1, + peerStoreID: []uint64{1111, 1112, 1121, 1122, 1131, 1132, 1141, 1142}, + peerRole: []PeerRoleType{Follower, Learner, Learner, Learner, Learner, Follower, Follower, Follower}, + locationLabels: "zone,rack,host", + count: 3, + expectedPeers: []uint64{1111, 1132, 1141}, }, } @@ -159,6 +158,5 @@ func (s *testFitSuite) TestFitByLocation(c *C) { } sort.Slice(expectedPeers, func(i, j int) bool { return expectedPeers[i] < expectedPeers[j] }) c.Assert(selectedIDs, DeepEquals, expectedPeers) - c.Assert(ruleFit.IsolationLevel, Equals, cc.expectedIsolationLevel) } } From 201155acea5cfae399e8f6df706c2ef0336081d9 Mon Sep 17 00:00:00 2001 From: disksing Date: Thu, 2 Jul 2020 13:25:54 +0800 Subject: [PATCH 3/3] add isolationScore test Signed-off-by: disksing --- server/schedule/placement/fit.go | 2 +- server/schedule/placement/fit_test.go | 40 ++++++++++++++++++++++++++- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/server/schedule/placement/fit.go b/server/schedule/placement/fit.go index 37e56af457d..69cff7d6947 100644 --- a/server/schedule/placement/fit.go +++ b/server/schedule/placement/fit.go @@ -250,7 +250,7 @@ func isolationScore(peers []*fitPeer, labels []string) float64 { // reuse `core.DistinctScore`. const replicaBaseScore = 100 for i, p1 := range peers { - for _, p2 := range peers[i:] { + for _, p2 := range peers[i+1:] { if index := p1.store.CompareLocation(p2.store, labels); index != -1 { score += math.Pow(replicaBaseScore, float64(len(labels)-index-1)) } diff --git a/server/schedule/placement/fit_test.go b/server/schedule/placement/fit_test.go index 1c17d2debf9..a0474adb78b 100644 --- a/server/schedule/placement/fit_test.go +++ b/server/schedule/placement/fit_test.go @@ -27,7 +27,7 @@ var _ = Suite(&testFitSuite{}) type testFitSuite struct{} -func (s *testFitSuite) TestFitByLocation(c *C) { +func (s *testFitSuite) makeStores() map[uint64]*core.StoreInfo { stores := make(map[uint64]*core.StoreInfo) for zone := 1; zone <= 5; zone++ { for rack := 1; rack <= 5; rack++ { @@ -44,6 +44,11 @@ func (s *testFitSuite) TestFitByLocation(c *C) { } } } + return stores +} + +func (s *testFitSuite) TestFitByLocation(c *C) { + stores := s.makeStores() type Case struct { // peers info @@ -160,3 +165,36 @@ func (s *testFitSuite) TestFitByLocation(c *C) { c.Assert(selectedIDs, DeepEquals, expectedPeers) } } + +func (s *testFitSuite) TestIsolationScore(c *C) { + stores := s.makeStores() + testCases := []struct { + peers1 []uint64 + Checker + peers2 []uint64 + }{ + {[]uint64{1111, 1112}, Less, []uint64{1111, 1121}}, + {[]uint64{1111, 1211}, Less, []uint64{1111, 2111}}, + {[]uint64{1111, 1211, 1311, 2111, 3111}, Less, []uint64{1111, 1211, 2111, 2211, 3111}}, + {[]uint64{1111, 1211, 2111, 2211, 3111}, Equals, []uint64{1111, 2111, 2211, 3111, 3211}}, + {[]uint64{1111, 1211, 2111, 2211, 3111}, Greater, []uint64{1111, 1121, 2111, 2211, 3111}}, + } + + makePeers := func(ids []uint64) []*fitPeer { + var peers []*fitPeer + for _, id := range ids { + peers = append(peers, &fitPeer{ + Peer: &metapb.Peer{StoreId: id}, + store: stores[id], + }) + } + return peers + } + + for _, tc := range testCases { + peers1, peers2 := makePeers(tc.peers1), makePeers(tc.peers2) + score1 := isolationScore(peers1, []string{"zone", "rack", "host"}) + score2 := isolationScore(peers2, []string{"zone", "rack", "host"}) + c.Assert(score1, tc.Checker, score2) + } +}