From 968dd4e97ba791ef81bf15eb9075ad3bf1de2940 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=B7=B7=E6=B2=8CDM?= Date: Wed, 31 Aug 2022 15:20:24 +0800 Subject: [PATCH 1/2] This is an automated cherry-pick of #5458 close tikv/pd#5401 Signed-off-by: ti-chi-bot --- server/schedule/operator/builder.go | 15 +++- server/schedule/operator/create_operator.go | 10 +++ .../schedule/operator/create_operator_test.go | 68 +++++++++++++++++++ 3 files changed, 91 insertions(+), 2 deletions(-) diff --git a/server/schedule/operator/builder.go b/server/schedule/operator/builder.go index e54e6480a1c..97d9e2532d2 100644 --- a/server/schedule/operator/builder.go +++ b/server/schedule/operator/builder.go @@ -52,8 +52,9 @@ type Builder struct { targetLeaderStoreID uint64 err error - // skip origin check flags + // skip check flags skipOriginJointStateCheck bool + skipPlacementRulesCheck bool // build flags allowDemote bool @@ -80,6 +81,11 @@ func SkipOriginJointStateCheck(b *Builder) { b.skipOriginJointStateCheck = true } +// SkipPlacementRulesCheck lets the builder skip the placement rules check for origin and target peers. +func SkipPlacementRulesCheck(b *Builder) { + b.skipPlacementRulesCheck = true +} + // NewBuilder creates a Builder. func NewBuilder(desc string, cluster opt.Cluster, region *core.RegionInfo, opts ...BuilderOption) *Builder { b := &Builder{ @@ -123,8 +129,13 @@ func NewBuilder(desc string, cluster opt.Cluster, region *core.RegionInfo, opts // placement rules var rules []*placement.Rule +<<<<<<< HEAD if err == nil && cluster.GetOpts().IsPlacementRulesEnabled() { fit := cluster.FitRegion(region) +======= + if err == nil && !b.skipPlacementRulesCheck && b.GetOpts().IsPlacementRulesEnabled() { + fit := b.GetRuleManager().FitRegion(b.GetBasicCluster(), region) +>>>>>>> d8620c975 (operator: allows to skip placement rules checks (#5458)) for _, rf := range fit.RuleFits { rules = append(rules, rf.Rule) } @@ -734,7 +745,7 @@ func (b *Builder) allowLeader(peer *metapb.Peer, ignoreClusterLimit bool) bool { } // placement rules - if len(b.rules) == 0 { + if b.skipPlacementRulesCheck || len(b.rules) == 0 { return true } for _, r := range b.rules { diff --git a/server/schedule/operator/create_operator.go b/server/schedule/operator/create_operator.go index 35479392d5f..87e9d6981ec 100644 --- a/server/schedule/operator/create_operator.go +++ b/server/schedule/operator/create_operator.go @@ -58,8 +58,13 @@ func CreateTransferLeaderOperator(desc string, cluster opt.Cluster, region *core } // CreateForceTransferLeaderOperator creates an operator that transfers the leader from a source store to a target store forcible. +<<<<<<< HEAD func CreateForceTransferLeaderOperator(desc string, cluster opt.Cluster, region *core.RegionInfo, sourceStoreID uint64, targetStoreID uint64, kind OpKind) (*Operator, error) { return NewBuilder(desc, cluster, region, SkipOriginJointStateCheck). +======= +func CreateForceTransferLeaderOperator(desc string, ci ClusterInformer, region *core.RegionInfo, sourceStoreID uint64, targetStoreID uint64, kind OpKind) (*Operator, error) { + return NewBuilder(desc, ci, region, SkipOriginJointStateCheck, SkipPlacementRulesCheck). +>>>>>>> d8620c975 (operator: allows to skip placement rules checks (#5458)) SetLeader(targetStoreID). EnableForceTargetLeader(). Build(kind) @@ -210,8 +215,13 @@ func CreateScatterRegionOperator(desc string, cluster opt.Cluster, origin *core. } // CreateLeaveJointStateOperator creates an operator that let region leave joint state. +<<<<<<< HEAD func CreateLeaveJointStateOperator(desc string, cluster opt.Cluster, origin *core.RegionInfo) (*Operator, error) { b := NewBuilder(desc, cluster, origin, SkipOriginJointStateCheck) +======= +func CreateLeaveJointStateOperator(desc string, ci ClusterInformer, origin *core.RegionInfo) (*Operator, error) { + b := NewBuilder(desc, ci, origin, SkipOriginJointStateCheck, SkipPlacementRulesCheck) +>>>>>>> d8620c975 (operator: allows to skip placement rules checks (#5458)) if b.err == nil && !core.IsInJointState(origin.GetPeers()...) { b.err = errors.Errorf("cannot build leave joint state operator for region which is not in joint state") diff --git a/server/schedule/operator/create_operator_test.go b/server/schedule/operator/create_operator_test.go index 97d0db648dc..588092414e8 100644 --- a/server/schedule/operator/create_operator_test.go +++ b/server/schedule/operator/create_operator_test.go @@ -15,12 +15,22 @@ package operator import ( "context" +<<<<<<< HEAD "strings" +======= + "encoding/hex" + "testing" +>>>>>>> d8620c975 (operator: allows to skip placement rules checks (#5458)) . "github.com/pingcap/check" "github.com/pingcap/errors" "github.com/pingcap/kvproto/pkg/metapb" "github.com/pingcap/kvproto/pkg/pdpb" +<<<<<<< HEAD +======= + "github.com/stretchr/testify/require" + "github.com/stretchr/testify/suite" +>>>>>>> d8620c975 (operator: allows to skip placement rules checks (#5458)) "github.com/tikv/pd/pkg/mock/mockcluster" "github.com/tikv/pd/server/config" "github.com/tikv/pd/server/core" @@ -1073,3 +1083,61 @@ func (s *testCreateOperatorSuite) TestMoveRegionWithoutJointConsensus(c *C) { } } } + +// Ref https://github.com/tikv/pd/issues/5401 +func TestCreateLeaveJointStateOperatorWithoutFitRules(t *testing.T) { + re := require.New(t) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + opts := config.NewTestOptions() + cluster := mockcluster.NewCluster(ctx, opts) + re.NoError(cluster.SetRules([]*placement.Rule{ + { + GroupID: "pd", + ID: "default", + StartKeyHex: hex.EncodeToString([]byte("")), + EndKeyHex: hex.EncodeToString([]byte("")), + Role: placement.Voter, + Count: 1, + }, + { + GroupID: "t1", + ID: "t1", + StartKeyHex: hex.EncodeToString([]byte("a")), + EndKeyHex: hex.EncodeToString([]byte("b")), + Role: placement.Voter, + Count: 1, + }, + { + GroupID: "t2", + ID: "t2", + StartKeyHex: hex.EncodeToString([]byte("b")), + EndKeyHex: hex.EncodeToString([]byte("c")), + Role: placement.Voter, + Count: 1, + }, + })) + cluster.AddRegionStore(1, 1) + cluster.AddRegionStore(2, 1) + cluster.AddRegionStore(3, 1) + cluster.AddRegionStore(4, 1) + originPeers := []*metapb.Peer{ + {Id: 3, StoreId: 3, Role: metapb.PeerRole_DemotingVoter}, + {Id: 4, StoreId: 4, Role: metapb.PeerRole_IncomingVoter}, + } + + region := core.NewRegionInfo(&metapb.Region{Id: 1, Peers: originPeers, StartKey: []byte("a"), EndKey: []byte("c")}, originPeers[0]) + op, err := CreateLeaveJointStateOperator("test", cluster, region) + re.NoError(err) + re.Equal(OpLeader, op.Kind()) + re.Len(op.steps, 2) + step0 := op.steps[0].(TransferLeader) + re.Equal(uint64(3), step0.FromStore) + re.Equal(uint64(4), step0.ToStore) + step1 := op.steps[1].(ChangePeerV2Leave) + re.Len(step1.PromoteLearners, 1) + re.Len(step1.DemoteVoters, 1) + re.Equal(uint64(4), step1.PromoteLearners[0].ToStore) + re.Equal(uint64(3), step1.DemoteVoters[0].ToStore) +} From 3fa3e256bcd11e96f97bc9e8b9f086fb76e8d69a Mon Sep 17 00:00:00 2001 From: HunDunDM Date: Tue, 27 Sep 2022 14:52:59 +0800 Subject: [PATCH 2/2] fix Signed-off-by: HunDunDM --- server/schedule/operator/builder.go | 19 ++++----- server/schedule/operator/create_operator.go | 14 +------ .../schedule/operator/create_operator_test.go | 41 ++++++++----------- 3 files changed, 28 insertions(+), 46 deletions(-) diff --git a/server/schedule/operator/builder.go b/server/schedule/operator/builder.go index 97d9e2532d2..00261a47509 100644 --- a/server/schedule/operator/builder.go +++ b/server/schedule/operator/builder.go @@ -28,11 +28,13 @@ import ( ) // Builder is used to create operators. Usage: -// op, err := NewBuilder(desc, cluster, region). -// RemovePeer(store1). -// AddPeer(peer1). -// SetLeader(store2). -// Build(kind) +// +// op, err := NewBuilder(desc, cluster, region). +// RemovePeer(store1). +// AddPeer(peer1). +// SetLeader(store2). +// Build(kind) +// // The generated Operator will choose the most appropriate execution order // according to various constraints. type Builder struct { @@ -129,13 +131,8 @@ func NewBuilder(desc string, cluster opt.Cluster, region *core.RegionInfo, opts // placement rules var rules []*placement.Rule -<<<<<<< HEAD - if err == nil && cluster.GetOpts().IsPlacementRulesEnabled() { + if err == nil && !b.skipPlacementRulesCheck && cluster.GetOpts().IsPlacementRulesEnabled() { fit := cluster.FitRegion(region) -======= - if err == nil && !b.skipPlacementRulesCheck && b.GetOpts().IsPlacementRulesEnabled() { - fit := b.GetRuleManager().FitRegion(b.GetBasicCluster(), region) ->>>>>>> d8620c975 (operator: allows to skip placement rules checks (#5458)) for _, rf := range fit.RuleFits { rules = append(rules, rf.Rule) } diff --git a/server/schedule/operator/create_operator.go b/server/schedule/operator/create_operator.go index 87e9d6981ec..464449da074 100644 --- a/server/schedule/operator/create_operator.go +++ b/server/schedule/operator/create_operator.go @@ -58,13 +58,8 @@ func CreateTransferLeaderOperator(desc string, cluster opt.Cluster, region *core } // CreateForceTransferLeaderOperator creates an operator that transfers the leader from a source store to a target store forcible. -<<<<<<< HEAD func CreateForceTransferLeaderOperator(desc string, cluster opt.Cluster, region *core.RegionInfo, sourceStoreID uint64, targetStoreID uint64, kind OpKind) (*Operator, error) { - return NewBuilder(desc, cluster, region, SkipOriginJointStateCheck). -======= -func CreateForceTransferLeaderOperator(desc string, ci ClusterInformer, region *core.RegionInfo, sourceStoreID uint64, targetStoreID uint64, kind OpKind) (*Operator, error) { - return NewBuilder(desc, ci, region, SkipOriginJointStateCheck, SkipPlacementRulesCheck). ->>>>>>> d8620c975 (operator: allows to skip placement rules checks (#5458)) + return NewBuilder(desc, cluster, region, SkipOriginJointStateCheck, SkipPlacementRulesCheck). SetLeader(targetStoreID). EnableForceTargetLeader(). Build(kind) @@ -215,13 +210,8 @@ func CreateScatterRegionOperator(desc string, cluster opt.Cluster, origin *core. } // CreateLeaveJointStateOperator creates an operator that let region leave joint state. -<<<<<<< HEAD func CreateLeaveJointStateOperator(desc string, cluster opt.Cluster, origin *core.RegionInfo) (*Operator, error) { - b := NewBuilder(desc, cluster, origin, SkipOriginJointStateCheck) -======= -func CreateLeaveJointStateOperator(desc string, ci ClusterInformer, origin *core.RegionInfo) (*Operator, error) { - b := NewBuilder(desc, ci, origin, SkipOriginJointStateCheck, SkipPlacementRulesCheck) ->>>>>>> d8620c975 (operator: allows to skip placement rules checks (#5458)) + b := NewBuilder(desc, cluster, origin, SkipOriginJointStateCheck, SkipPlacementRulesCheck) if b.err == nil && !core.IsInJointState(origin.GetPeers()...) { b.err = errors.Errorf("cannot build leave joint state operator for region which is not in joint state") diff --git a/server/schedule/operator/create_operator_test.go b/server/schedule/operator/create_operator_test.go index 588092414e8..384aee0e763 100644 --- a/server/schedule/operator/create_operator_test.go +++ b/server/schedule/operator/create_operator_test.go @@ -15,22 +15,13 @@ package operator import ( "context" -<<<<<<< HEAD - "strings" -======= "encoding/hex" - "testing" ->>>>>>> d8620c975 (operator: allows to skip placement rules checks (#5458)) + "strings" . "github.com/pingcap/check" "github.com/pingcap/errors" "github.com/pingcap/kvproto/pkg/metapb" "github.com/pingcap/kvproto/pkg/pdpb" -<<<<<<< HEAD -======= - "github.com/stretchr/testify/require" - "github.com/stretchr/testify/suite" ->>>>>>> d8620c975 (operator: allows to skip placement rules checks (#5458)) "github.com/tikv/pd/pkg/mock/mockcluster" "github.com/tikv/pd/server/config" "github.com/tikv/pd/server/core" @@ -1084,15 +1075,18 @@ func (s *testCreateOperatorSuite) TestMoveRegionWithoutJointConsensus(c *C) { } } +var _ = Suite(&testCreateOperatorWithRulesSuite{}) + +type testCreateOperatorWithRulesSuite struct{} + // Ref https://github.com/tikv/pd/issues/5401 -func TestCreateLeaveJointStateOperatorWithoutFitRules(t *testing.T) { - re := require.New(t) +func (s *testCreateOperatorWithRulesSuite) TestCreateLeaveJointStateOperatorWithoutFitRules(c *C) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() opts := config.NewTestOptions() cluster := mockcluster.NewCluster(ctx, opts) - re.NoError(cluster.SetRules([]*placement.Rule{ + err := cluster.SetRules([]*placement.Rule{ { GroupID: "pd", ID: "default", @@ -1117,7 +1111,8 @@ func TestCreateLeaveJointStateOperatorWithoutFitRules(t *testing.T) { Role: placement.Voter, Count: 1, }, - })) + }) + c.Assert(err, IsNil) cluster.AddRegionStore(1, 1) cluster.AddRegionStore(2, 1) cluster.AddRegionStore(3, 1) @@ -1129,15 +1124,15 @@ func TestCreateLeaveJointStateOperatorWithoutFitRules(t *testing.T) { region := core.NewRegionInfo(&metapb.Region{Id: 1, Peers: originPeers, StartKey: []byte("a"), EndKey: []byte("c")}, originPeers[0]) op, err := CreateLeaveJointStateOperator("test", cluster, region) - re.NoError(err) - re.Equal(OpLeader, op.Kind()) - re.Len(op.steps, 2) + c.Assert(err, IsNil) + c.Assert(op.kind, Equals, OpLeader) + c.Assert(op.steps, HasLen, 2) step0 := op.steps[0].(TransferLeader) - re.Equal(uint64(3), step0.FromStore) - re.Equal(uint64(4), step0.ToStore) + c.Assert(step0.FromStore, Equals, uint64(3)) + c.Assert(step0.ToStore, Equals, uint64(4)) step1 := op.steps[1].(ChangePeerV2Leave) - re.Len(step1.PromoteLearners, 1) - re.Len(step1.DemoteVoters, 1) - re.Equal(uint64(4), step1.PromoteLearners[0].ToStore) - re.Equal(uint64(3), step1.DemoteVoters[0].ToStore) + c.Assert(step1.PromoteLearners, HasLen, 1) + c.Assert(step1.DemoteVoters, HasLen, 1) + c.Assert(step1.PromoteLearners[0].ToStore, Equals, uint64(4)) + c.Assert(step1.DemoteVoters[0].ToStore, Equals, uint64(3)) }