diff --git a/ddl/placement/BUILD.bazel b/ddl/placement/BUILD.bazel index 53e6cb534d33e..5c2bb6833317b 100644 --- a/ddl/placement/BUILD.bazel +++ b/ddl/placement/BUILD.bazel @@ -35,7 +35,7 @@ go_test( embed = [":placement"], flaky = True, race = "on", - shard_count = 24, + shard_count = 25, deps = [ "//kv", "//meta", diff --git a/ddl/placement/bundle.go b/ddl/placement/bundle.go index 3aebb19bd969e..28b3a0999dfd9 100644 --- a/ddl/placement/bundle.go +++ b/ddl/placement/bundle.go @@ -288,27 +288,13 @@ func (b *Bundle) String() string { // Tidy will post optimize Rules, trying to generate rules that suits PD. func (b *Bundle) Tidy() error { - extraCnt := map[PeerRoleType]int{} - newRules := b.Rules[:0] - - // One Bundle is from one PlacementSettings, rule share same location labels, so we can use the first rule's location labels. - var locationLabels []string + tempRules := b.Rules[:0] + id := 0 for _, rule := range b.Rules { - if len(rule.LocationLabels) > 0 { - locationLabels = rule.LocationLabels - break - } - } - for i, rule := range b.Rules { // useless Rule if rule.Count <= 0 { continue } - // merge all empty constraints - if len(rule.Constraints) == 0 { - extraCnt[rule.Role] += rule.Count - continue - } // refer to tidb#22065. // add -engine=tiflash to every rule to avoid schedules to tiflash instances. // placement rules in SQL is not compatible with `set tiflash replica` yet @@ -320,36 +306,139 @@ func (b *Bundle) Tidy() error { if err != nil { return err } - // Constraints.Add() will automatically avoid duplication - // if -engine=tiflash is added and there is only one constraint - // then it must be -engine=tiflash - // it is seen as an empty constraint, so merge it - if len(rule.Constraints) == 1 { - extraCnt[rule.Role] += rule.Count + rule.ID = strconv.Itoa(id) + tempRules = append(tempRules, rule) + id++ + } + + groups := make(map[string]*constraintsGroup) + finalRules := tempRules[:0] + for _, rule := range tempRules { + key := rule.Constraints.FingerPrint() + existing, ok := groups[key] + if !ok { + groups[key] = &constraintsGroup{rules: []*Rule{rule}} continue } - rule.ID = strconv.Itoa(i) - newRules = append(newRules, rule) - } - for role, cnt := range extraCnt { - // add -engine=tiflash, refer to tidb#22065. - newRules = append(newRules, &Rule{ - ID: string(role), - Role: role, - Count: cnt, - Constraints: []Constraint{{ - Op: NotIn, - Key: EngineLabelKey, - Values: []string{EngineLabelTiFlash}, - }}, - // the merged rule should have the same location labels with the original rules. - LocationLabels: locationLabels, - }) + existing.rules = append(existing.rules, rule) + } + for _, group := range groups { + group.MergeRulesByRole() + } + if err := transformableLeaderConstraint(groups); err != nil { + return err + } + for _, group := range groups { + finalRules = append(finalRules, group.rules...) + } + // sort by id + sort.SliceStable(finalRules, func(i, j int) bool { + return finalRules[i].ID < finalRules[j].ID + }) + b.Rules = finalRules + return nil +} + +// constraintsGroup is a group of rules with the same constraints. +type constraintsGroup struct { + rules []*Rule + // canBecameLeader means the group has leader/voter role, + // it's valid if it has leader. + canBecameLeader bool + // isLeaderGroup means it has specified leader role in this group. + isLeaderGroup bool +} + +func transformableLeaderConstraint(groups map[string]*constraintsGroup) error { + var leaderGroup *constraintsGroup + canBecameLeaderNum := 0 + for _, group := range groups { + if group.isLeaderGroup { + if leaderGroup != nil { + return ErrInvalidPlacementOptions + } + leaderGroup = group + } + if group.canBecameLeader { + canBecameLeaderNum++ + } + } + // If there is a specified group should have leader, and only this group can be a leader, that means + // the leader's priority is certain, so we can merge the transformable rules into one. + // eg: + // - [ group1 (L F), group2 (F) ], after merging is [group1 (2*V), group2 (F)], we still know the leader prefers group1. + // - [ group1 (L F), group2 (V) ], after merging is [group1 (2*V), group2 (V)], we can't know leader priority after merge. + if leaderGroup != nil && canBecameLeaderNum == 1 { + leaderGroup.MergeTransformableRoles() } - b.Rules = newRules return nil } +// MergeRulesByRole merges the rules with the same role. +func (c *constraintsGroup) MergeRulesByRole() { + // Create a map to store rules by role + rulesByRole := make(map[PeerRoleType][]*Rule) + + // Iterate through each rule + for _, rule := range c.rules { + // Add the rule to the map based on its role + rulesByRole[rule.Role] = append(rulesByRole[rule.Role], rule) + if rule.Role == Leader || rule.Role == Voter { + c.canBecameLeader = true + } + if rule.Role == Leader { + c.isLeaderGroup = true + } + } + + // Clear existing rules + c.rules = nil + + // Iterate through each role and merge the rules + for _, rules := range rulesByRole { + mergedRule := rules[0] + for i, rule := range rules { + if i == 0 { + continue + } + mergedRule.Count += rule.Count + if mergedRule.ID > rule.ID { + mergedRule.ID = rule.ID + } + } + c.rules = append(c.rules, mergedRule) + } +} + +// MergeTransformableRoles merges all the rules to one that can be transformed to other roles. +func (c *constraintsGroup) MergeTransformableRoles() { + if len(c.rules) == 0 || len(c.rules) == 1 { + return + } + var mergedRule *Rule + newRules := make([]*Rule, 0, len(c.rules)) + for _, rule := range c.rules { + // Learner is not transformable, it should be promote by PD. + if rule.Role == Learner { + newRules = append(newRules, rule) + continue + } + if mergedRule == nil { + mergedRule = rule + continue + } + mergedRule.Count += rule.Count + if mergedRule.ID > rule.ID { + mergedRule.ID = rule.ID + } + } + if mergedRule != nil { + mergedRule.Role = Voter + newRules = append(newRules, mergedRule) + } + c.rules = newRules +} + // Reset resets the bundle ID and keyrange of all rules. func (b *Bundle) Reset(ruleIndex int, newIDs []int64) *Bundle { // eliminate the redundant rules. diff --git a/ddl/placement/bundle_test.go b/ddl/placement/bundle_test.go index 0dc9032da8562..918c2cf18daac 100644 --- a/ddl/placement/bundle_test.go +++ b/ddl/placement/bundle_test.go @@ -17,6 +17,7 @@ package placement import ( "encoding/hex" "fmt" + "reflect" "testing" "github.com/pingcap/failpoint" @@ -861,17 +862,17 @@ func TestTidy(t *testing.T) { bundle.Rules = append(bundle.Rules, rules1...) bundle.Rules = append(bundle.Rules, rules2...) + require.Len(t, bundle.Rules, 3) err = bundle.Tidy() require.NoError(t, err) - require.Len(t, bundle.Rules, 2) - require.Equal(t, "1", bundle.Rules[0].ID) + require.Len(t, bundle.Rules, 1) + require.Equal(t, "0", bundle.Rules[0].ID) require.Len(t, bundle.Rules[0].Constraints, 3) require.Equal(t, Constraint{ Op: NotIn, Key: EngineLabelKey, Values: []string{EngineLabelTiFlash}, }, bundle.Rules[0].Constraints[2]) - require.Equal(t, "2", bundle.Rules[1].ID) // merge rules3, err := NewRules(Follower, 4, "") @@ -892,19 +893,18 @@ func TestTidy(t *testing.T) { } chkfunc := func() { require.NoError(t, err) - require.Len(t, bundle.Rules, 3) + require.Len(t, bundle.Rules, 2) require.Equal(t, "0", bundle.Rules[0].ID) require.Equal(t, "1", bundle.Rules[1].ID) - require.Equal(t, "follower", bundle.Rules[2].ID) - require.Equal(t, 9, bundle.Rules[2].Count) + require.Equal(t, 9, bundle.Rules[1].Count) require.Equal(t, Constraints{ { Op: NotIn, Key: EngineLabelKey, Values: []string{EngineLabelTiFlash}, }, - }, bundle.Rules[2].Constraints) - require.Equal(t, []string{"zone", "host"}, bundle.Rules[2].LocationLabels) + }, bundle.Rules[1].Constraints) + require.Equal(t, []string{"zone", "host"}, bundle.Rules[1].LocationLabels) } err = bundle.Tidy() chkfunc() @@ -921,10 +921,474 @@ func TestTidy(t *testing.T) { require.NoError(t, err) require.Equal(t, bundle, bundle2) - bundle.Rules[2].Constraints = append(bundle.Rules[2].Constraints, Constraint{ + bundle.Rules[1].Constraints = append(bundle.Rules[1].Constraints, Constraint{ Op: In, Key: EngineLabelKey, Values: []string{EngineLabelTiFlash}, }) require.ErrorIs(t, bundle.Tidy(), ErrConflictingConstraints) } + +func TestTidy2(t *testing.T) { + tests := []struct { + name string + bundle Bundle + expected Bundle + }{ + { + name: "Empty bundle", + bundle: Bundle{ + Rules: []*Rule{}, + }, + expected: Bundle{ + Rules: []*Rule{}, + }, + }, + { + name: "Rules with empty constraints are merged", + bundle: Bundle{ + Rules: []*Rule{ + { + ID: "1", + Role: Leader, + Count: 1, + Constraints: Constraints{}, + LocationLabels: []string{"region"}, + }, + { + ID: "2", + Role: Voter, + Count: 2, + Constraints: Constraints{}, + LocationLabels: []string{"region"}, + }, + }, + }, + expected: Bundle{ + Rules: []*Rule{ + { + ID: "0", + Role: Voter, + Count: 3, + Constraints: Constraints{}, + LocationLabels: []string{"region"}, + }, + }, + }, + }, + { + name: "Rules with same constraints are merged, Leader + Follower", + bundle: Bundle{ + Rules: []*Rule{ + { + ID: "1", + Role: Leader, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "2", + Role: Follower, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 2, + LocationLabels: []string{"region"}, + }, + }, + }, + expected: Bundle{ + Rules: []*Rule{ + { + ID: "0", + Role: Voter, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 3, + LocationLabels: []string{"region"}, + }, + }, + }, + }, + { + name: "Rules with same constraints are merged, Leader + Voter", + bundle: Bundle{ + Rules: []*Rule{ + { + ID: "1", + Role: Leader, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "2", + Role: Voter, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 2, + LocationLabels: []string{"region"}, + }, + }, + }, + expected: Bundle{ + Rules: []*Rule{ + { + ID: "0", + Role: Voter, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 3, + LocationLabels: []string{"region"}, + }, + }, + }, + }, + { + name: "Rules with same constraints and role are merged, Leader + Follower + Voter", + bundle: Bundle{ + Rules: []*Rule{ + { + ID: "1", + Role: Leader, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "2", + Role: Follower, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "3", + Role: Voter, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + }, + }, + expected: Bundle{ + Rules: []*Rule{ + { + ID: "0", + Role: Voter, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 3, + LocationLabels: []string{"region"}, + }, + }, + }, + }, + { + name: "Rules with same constraints and role are merged, Leader + Follower + Voter + Learner", + bundle: Bundle{ + Rules: []*Rule{ + { + ID: "1", + Role: Leader, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "2", + Role: Follower, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "3", + Role: Voter, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "4", + Role: Learner, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 2, + LocationLabels: []string{"region"}, + }, + }, + }, + expected: Bundle{ + Rules: []*Rule{ + { + ID: "0", + Role: Voter, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 3, + LocationLabels: []string{"region"}, + }, + { + ID: "3", + Role: Learner, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 2, + LocationLabels: []string{"region"}, + }, + }, + }, + }, + { + name: "Rules with same constraints and role are merged, Leader + Follower + Learner | Follower", + bundle: Bundle{ + Rules: []*Rule{ + { + ID: "1", + Role: Leader, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "2", + Role: Follower, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "3", + Role: Learner, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "4", + Role: Follower, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"2"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + }, + }, + expected: Bundle{ + Rules: []*Rule{ + { + ID: "0", + Role: Voter, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 2, + LocationLabels: []string{"region"}, + }, + { + ID: "2", + Role: Learner, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "3", + Role: Follower, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"2"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + }, + }, + }, + { + name: "Rules with same constraints and role are merged, Leader + Follower + Learner | Voter", + bundle: Bundle{ + Rules: []*Rule{ + { + ID: "1", + Role: Leader, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "2", + Role: Follower, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "3", + Role: Learner, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "4", + Role: Voter, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"2"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + }, + }, + expected: Bundle{ + Rules: []*Rule{ + { + ID: "0", + Role: Leader, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "1", + Role: Follower, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "2", + Role: Learner, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "3", + Role: Voter, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"2"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + }, + }, + }, + { + name: "Rules with different constraints are kept separate", + bundle: Bundle{ + Rules: []*Rule{ + { + ID: "1", + Role: Leader, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "2", + Role: Follower, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"2"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + }, + }, + expected: Bundle{ + Rules: []*Rule{ + { + ID: "0", + Role: Leader, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "1", + Role: Follower, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"2"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.bundle.Tidy() + require.NoError(t, err) + + require.Equal(t, len(tt.expected.Rules), len(tt.bundle.Rules)) + + for i, rule := range tt.bundle.Rules { + expectedRule := tt.expected.Rules[i] + // Tiflash is always excluded from the constraints. + expectedRule.Constraints.Add(Constraint{ + Op: NotIn, + Key: EngineLabelKey, + Values: []string{EngineLabelTiFlash}, + }) + if !reflect.DeepEqual(rule, expectedRule) { + t.Errorf("unexpected rule at index %d:\nactual=%#v,\nexpected=%#v\n", i, rule, expectedRule) + } + } + }) + } +} diff --git a/ddl/placement/constraints.go b/ddl/placement/constraints.go index 38b678857eee3..99467a5fc7f36 100644 --- a/ddl/placement/constraints.go +++ b/ddl/placement/constraints.go @@ -15,7 +15,12 @@ package placement import ( + "cmp" + "crypto/sha256" + "encoding/base64" "fmt" + "slices" + "sort" "strings" "gopkg.in/yaml.v2" @@ -110,3 +115,34 @@ func (constraints *Constraints) Add(label Constraint) error { } return nil } + +// FingerPrint returns a unique string for the constraints. +func (constraints *Constraints) FingerPrint() string { + copied := make(Constraints, len(*constraints)) + copy(copied, *constraints) + slices.SortStableFunc(copied, func(i, j Constraint) int { + a, b := constraintToString(&i), constraintToString(&j) + return cmp.Compare(a, b) + }) + var combinedConstraints string + for _, constraint := range copied { + combinedConstraints += constraintToString(&constraint) + } + + // Calculate the SHA256 hash of the concatenated constraints + hash := sha256.Sum256([]byte(combinedConstraints)) + + // Encode the hash as a base64 string + hashStr := base64.StdEncoding.EncodeToString(hash[:]) + + return hashStr +} + +func constraintToString(c *Constraint) string { + // Sort the values in the constraint + sortedValues := make([]string, len(c.Values)) + copy(sortedValues, c.Values) + sort.Strings(sortedValues) + sortedValuesStr := strings.Join(sortedValues, ",") + return c.Key + "|" + string(c.Op) + "|" + sortedValuesStr +}