diff --git a/server/schedule/filter/filters.go b/server/schedule/filter/filters.go index e9089f74ae9..d41857fd376 100644 --- a/server/schedule/filter/filters.go +++ b/server/schedule/filter/filters.go @@ -615,7 +615,7 @@ func (f *ruleFitFilter) Source(_ *config.PersistOptions, _ *core.StoreInfo) *pla // the replaced store can match the source rule. // RegionA:[1,2,3], move peer1 --> peer2 will not allow, because it's count not match the rule. // but transfer role peer1 --> peer2, it will support. -func (f *ruleFitFilter) Target(options *config.PersistOptions, store *core.StoreInfo) *plan.Status { +func (f *ruleFitFilter) Target(_ *config.PersistOptions, store *core.StoreInfo) *plan.Status { if f.oldFit.Replace(f.srcStore, store) { return statusOK } diff --git a/server/schedule/placement/fit.go b/server/schedule/placement/fit.go index 3e901f5ba10..82af3c17d11 100644 --- a/server/schedule/placement/fit.go +++ b/server/schedule/placement/fit.go @@ -58,15 +58,27 @@ func (f *RegionFit) IsCached() bool { func (f *RegionFit) Replace(srcStoreID uint64, dstStore *core.StoreInfo) bool { fit := f.getRuleFitByStoreID(srcStoreID) // check the target store is fit all constraints. - if fit == nil || !MatchLabelConstraints(dstStore, fit.Rule.LabelConstraints) { + if fit == nil { return false } - // the members of the rule are same, it shouldn't check the score. + // it is used to transfer role like transfer leader. if fit.contain(dstStore.GetID()) { return true } + // it should be allowed to exchange role if both the source and target's rule role is same. + // e.g. transfer leader from store 1 to store 2, and store-1 and store-2 are both voter. + targetFit := f.getRuleFitByStoreID(dstStore.GetID()) + if targetFit != nil && targetFit.Rule.Role == fit.Rule.Role { + return true + } + + // the target store should be fit all constraints. + if !MatchLabelConstraints(dstStore, fit.Rule.LabelConstraints) { + return false + } + score := isolationStoreScore(srcStoreID, dstStore, fit.stores, fit.Rule.LocationLabels) // restore the source store. return fit.IsolationScore <= score diff --git a/server/schedule/placement/fit_test.go b/server/schedule/placement/fit_test.go index 164720fe9aa..0d6557af9d5 100644 --- a/server/schedule/placement/fit_test.go +++ b/server/schedule/placement/fit_test.go @@ -131,13 +131,18 @@ func TestReplace(t *testing.T) { {"1111,2111,3111,1115_learner", []string{"3/voter//zone", "1/learner/engine=tiflash/host"}, 1115, 2115, true}, // replace failed when the target store is not tiflash {"1111,2111,3111,1115_learner", []string{"3/voter//zone", "1/learner/engine=tiflash/host"}, 1115, 1112, false}, - {"1111_lead,2111,3111", []string{"1/leader/zone=zone1/zone", "2/voter//zone"}, 1111, 1112, true}, + {"1111_leader,2111,3111", []string{"1/leader/zone=zone1/zone", "2/voter//zone"}, 1111, 1112, true}, // replace failed when the leader is not match the leader constraint. {"1111_leader,2111,3111", []string{"1/leader/zone=zone1/zone", "2/voter//zone"}, 1111, 2112, false}, // transfer leader - {"1111_leader,1121,1131", []string{"1/leader/host=host1+host2/host", "3/voter//host"}, 1111, 1121, true}, + {"1111_leader,1121,1131", []string{"1/leader/host=host1+host2/host", "2/voter//host"}, 1111, 1121, true}, // replace failed when the leader is not match the leader constraint. {"1111_leader,1121,1131", []string{"1/leader/host=host1+host2/host", "2/voter//host"}, 1111, 1131, false}, + + // transfer leader success with different rule when the role is the same. + {"1111_leader,1121,1131", []string{"1/voter/host=host1/host", "1/voter/host=host2/host", "1/voter/host=host3/host"}, 1111, 1121, true}, + //// transfer leader failed with different rule, but the role isn't same. + {"1111_leader,1121,1131", []string{"1/leader/host=host1/host", "1/voter/host=host2/host", "1/voter/host=host3/host"}, 1111, 1121, false}, } for _, tc := range testCases { region := makeRegion(tc.region) @@ -146,6 +151,7 @@ func TestReplace(t *testing.T) { rules = append(rules, makeRule(r)) } rf := fitRegion(stores.GetStores(), region, rules, false) + re.True(rf.IsSatisfied()) rf.regionStores = stores.GetStores() re.Equal(rf.Replace(tc.srcStoreID, stores.GetStore(tc.dstStoreID)), tc.ok) } diff --git a/server/schedule/region_scatterer.go b/server/schedule/region_scatterer.go index 8bf58bfd91b..13e4d9215d5 100644 --- a/server/schedule/region_scatterer.go +++ b/server/schedule/region_scatterer.go @@ -394,12 +394,14 @@ func allowLeader(fit *placement.RegionFit, peer *metapb.Peer) bool { if peer.IsWitness { return false } - - rule := fit.GetRuleFit(peer.GetId()).Rule - if rule.IsWitness { + peerFit := fit.GetRuleFit(peer.GetId()) + if peerFit == nil || peerFit.Rule == nil { + return false + } + if peerFit.Rule.IsWitness { return false } - switch rule.Role { + switch peerFit.Rule.Role { case placement.Voter, placement.Leader: return true } diff --git a/server/schedule/region_scatterer_test.go b/server/schedule/region_scatterer_test.go index 432c038a92f..ea8fb80e718 100644 --- a/server/schedule/region_scatterer_test.go +++ b/server/schedule/region_scatterer_test.go @@ -758,6 +758,11 @@ func TestBalanceRegion(t *testing.T) { re.Equal(uint64(150), scatterer.ordinaryEngine.selectedPeer.Get(i, group)) re.Equal(uint64(50), scatterer.ordinaryEngine.selectedLeader.Get(i, group)) } + // Test for unhealthy region + // ref https://github.com/tikv/pd/issues/6099 + region := tc.AddLeaderRegion(1500, 2, 3, 4, 6) + op := scatterer.scatterRegion(region, group) + re.False(isPeerCountChanged(op)) } func isPeerCountChanged(op *operator.Operator) bool {