From 775291a0ce72728d60e2264f416492619346a899 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/3] 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 1073211e8dd..ec749c818ff 100644 --- a/server/schedule/operator/builder.go +++ b/server/schedule/operator/builder.go @@ -53,8 +53,9 @@ type Builder struct { targetLeaderStoreID uint64 err error - // skip origin check flags + // skip check flags skipOriginJointStateCheck bool + skipPlacementRulesCheck bool // build flags useJointConsensus 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.GetRuleManager().FitRegion(cluster, 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) } @@ -737,7 +748,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 1a42b59921d..3cbfe316a12 100644 --- a/server/schedule/operator/create_operator.go +++ b/server/schedule/operator/create_operator.go @@ -66,8 +66,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) @@ -218,8 +223,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 b365025674b..845a4fffd43 100644 --- a/server/schedule/operator/create_operator_test.go +++ b/server/schedule/operator/create_operator_test.go @@ -16,12 +16,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 bfa21fbd228e74808075e3883a17d462d7b7a88f Mon Sep 17 00:00:00 2001 From: nolouch Date: Tue, 20 Sep 2022 19:01:37 +0800 Subject: [PATCH 2/3] fix test Signed-off-by: nolouch --- go.mod | 1 + server/schedule/operator/builder.go | 19 ++++++++----------- server/schedule/operator/create_operator.go | 14 ++------------ .../schedule/operator/create_operator_test.go | 12 +++--------- 4 files changed, 14 insertions(+), 32 deletions(-) diff --git a/go.mod b/go.mod index bdea6674086..9e8128f4604 100644 --- a/go.mod +++ b/go.mod @@ -40,6 +40,7 @@ require ( github.com/sirupsen/logrus v1.4.2 github.com/spf13/cobra v1.0.0 github.com/spf13/pflag v1.0.5 + github.com/stretchr/testify v1.7.0 github.com/swaggo/http-swagger v0.0.0-20200308142732-58ac5e232fba github.com/swaggo/swag v1.6.6-0.20200529100950-7c765ddd0476 github.com/syndtr/goleveldb v1.0.1-0.20190318030020-c3a204f8e965 diff --git a/server/schedule/operator/builder.go b/server/schedule/operator/builder.go index ec749c818ff..85c892a787e 100644 --- a/server/schedule/operator/builder.go +++ b/server/schedule/operator/builder.go @@ -29,11 +29,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.GetRuleManager().FitRegion(cluster, 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 3cbfe316a12..b1810f593eb 100644 --- a/server/schedule/operator/create_operator.go +++ b/server/schedule/operator/create_operator.go @@ -66,13 +66,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) @@ -223,13 +218,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 845a4fffd43..a74fdefa19f 100644 --- a/server/schedule/operator/create_operator_test.go +++ b/server/schedule/operator/create_operator_test.go @@ -16,27 +16,21 @@ package operator import ( "context" -<<<<<<< HEAD - "strings" -======= "encoding/hex" + "strings" "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" "github.com/tikv/pd/server/schedule/placement" "github.com/tikv/pd/server/versioninfo" + + "github.com/stretchr/testify/require" ) var _ = Suite(&testCreateOperatorSuite{}) From c8393bdd470a96dafa830064735f3766f75b8c8f Mon Sep 17 00:00:00 2001 From: HunDunDM Date: Tue, 27 Sep 2022 14:34:29 +0800 Subject: [PATCH 3/3] fix test Signed-off-by: HunDunDM --- .../schedule/operator/create_operator_test.go | 33 ++++++++++--------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/server/schedule/operator/create_operator_test.go b/server/schedule/operator/create_operator_test.go index a74fdefa19f..27da5b2fa35 100644 --- a/server/schedule/operator/create_operator_test.go +++ b/server/schedule/operator/create_operator_test.go @@ -18,7 +18,6 @@ import ( "context" "encoding/hex" "strings" - "testing" . "github.com/pingcap/check" "github.com/pingcap/errors" @@ -29,8 +28,6 @@ import ( "github.com/tikv/pd/server/core" "github.com/tikv/pd/server/schedule/placement" "github.com/tikv/pd/server/versioninfo" - - "github.com/stretchr/testify/require" ) var _ = Suite(&testCreateOperatorSuite{}) @@ -1078,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", @@ -1111,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) @@ -1123,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)) }