From e88c246102542fe67bddcad76df03cc06e57ecd3 Mon Sep 17 00:00:00 2001 From: pingcap-github-bot Date: Fri, 5 Jun 2020 18:24:55 +0800 Subject: [PATCH] rule: prevent misconfiguration of placement rules in some situations (#2481) (#2516) Signed-off-by: disksing --- server/api/rule.go | 2 +- server/schedule/placement/rule_list.go | 19 ++++++++++--- server/schedule/placement/rule_manager.go | 27 +++++++++++++++---- .../schedule/placement/rule_manager_test.go | 25 ++++++++++++++++- tools/pd-ctl/pdctl/command/config_command.go | 5 +++- 5 files changed, 67 insertions(+), 11 deletions(-) diff --git a/server/api/rule.go b/server/api/rule.go index b5491d4295e..cea37148e11 100644 --- a/server/api/rule.go +++ b/server/api/rule.go @@ -195,7 +195,7 @@ func (h *ruleHandler) checkRule(r *placement.Rule) error { if err != nil { return errors.Wrap(err, "end key is not hex format") } - if len(start) > 0 && bytes.Compare(end, start) <= 0 { + if len(end) > 0 && bytes.Compare(end, start) <= 0 { return errors.New("endKey should be greater than startKey") } diff --git a/server/schedule/placement/rule_list.go b/server/schedule/placement/rule_list.go index 8a9d27dce08..a7fcff0aed1 100644 --- a/server/schedule/placement/rule_list.go +++ b/server/schedule/placement/rule_list.go @@ -15,7 +15,11 @@ package placement import ( "bytes" + "encoding/hex" "sort" + "strings" + + "github.com/pkg/errors" ) type splitPointType int @@ -68,9 +72,9 @@ type ruleList struct { ranges []rangeRules // ranges[i] contains rules apply to (ranges[i].startKey, ranges[i+1].startKey). } -func buildRuleList(rules map[[2]string]*Rule) ruleList { +func buildRuleList(rules map[[2]string]*Rule) (ruleList, error) { if len(rules) == 0 { - return ruleList{} + return ruleList{}, errors.New("no rule left") } // collect and sort split points. var points []splitPoint @@ -105,6 +109,15 @@ func buildRuleList(rules map[[2]string]*Rule) ruleList { if i == len(points)-1 || !bytes.Equal(p.key, points[i+1].key) { // next key is different, push sr to rl. rr := sr.rules + if len(rr) == 0 { + var endKey []byte + if i != len(points)-1 { + endKey = points[i+1].key + } + return ruleList{}, errors.Errorf("no rule for range {%s, %s}", + strings.ToUpper(hex.EncodeToString(p.key)), + strings.ToUpper(hex.EncodeToString(endKey))) + } if i != len(points)-1 { rr = append(rr[:0:0], rr...) // clone } @@ -115,7 +128,7 @@ func buildRuleList(rules map[[2]string]*Rule) ruleList { }) } } - return rl + return rl, nil } func (rl ruleList) getSplitKeys(start, end []byte) [][]byte { diff --git a/server/schedule/placement/rule_manager.go b/server/schedule/placement/rule_manager.go index fd398fa4e32..b749ece6c6a 100644 --- a/server/schedule/placement/rule_manager.go +++ b/server/schedule/placement/rule_manager.go @@ -69,7 +69,11 @@ func (m *RuleManager) Initialize(maxReplica int, locationLabels []string) error } m.rules[defaultRule.Key()] = defaultRule } - m.ruleList = buildRuleList(m.rules) + ruleList, err := buildRuleList(m.rules) + if err != nil { + return err + } + m.ruleList = ruleList m.initialized = true return nil } @@ -169,8 +173,13 @@ func (m *RuleManager) SetRule(rule *Rule) error { old := m.rules[rule.Key()] m.rules[rule.Key()] = rule - if err = m.store.SaveRule(rule.StoreKey(), rule); err != nil { - // restore + ruleList, err := buildRuleList(m.rules) + if err == nil { + err = m.store.SaveRule(rule.StoreKey(), rule) + } + + // restore + if err != nil { if old == nil { delete(m.rules, rule.Key()) } else { @@ -179,8 +188,8 @@ func (m *RuleManager) SetRule(rule *Rule) error { return err } + m.ruleList = ruleList log.Info("placement rule updated", zap.Stringer("rule", rule)) - m.ruleList = buildRuleList(m.rules) return nil } @@ -194,13 +203,21 @@ func (m *RuleManager) DeleteRule(group, id string) error { return nil } delete(m.rules, [2]string{group, id}) + + ruleList, err := buildRuleList(m.rules) + if err != nil { + // restore + m.rules[key] = old + return err + } + if err := m.store.DeleteRule(old.StoreKey()); err != nil { // restore m.rules[key] = old return err } + m.ruleList = ruleList log.Info("placement rule removed", zap.Stringer("rule", old)) - m.ruleList = buildRuleList(m.rules) return nil } diff --git a/server/schedule/placement/rule_manager_test.go b/server/schedule/placement/rule_manager_test.go index 011d3141064..e65aad4f2a3 100644 --- a/server/schedule/placement/rule_manager_test.go +++ b/server/schedule/placement/rule_manager_test.go @@ -89,7 +89,6 @@ func (s *testManagerSuite) TestSaveLoad(c *C) { } func (s *testManagerSuite) TestKeys(c *C) { - s.manager.DeleteRule("pd", "default") rules := []*Rule{ {GroupID: "1", ID: "1", Role: "voter", Count: 1, StartKeyHex: "", EndKeyHex: ""}, {GroupID: "2", ID: "2", Role: "voter", Count: 1, StartKeyHex: "11", EndKeyHex: "ff"}, @@ -100,6 +99,7 @@ func (s *testManagerSuite) TestKeys(c *C) { for _, r := range rules { s.manager.SetRule(r) } + s.manager.DeleteRule("pd", "default") splitKeys := [][]string{ {"", "", "11", "22", "44", "dd", "ee", "ff"}, @@ -161,6 +161,29 @@ func (s *testManagerSuite) TestKeys(c *C) { } } +func (s *testManagerSuite) TestRangeGap(c *C) { + // |-- default --| + // cannot delete the last rule + err := s.manager.DeleteRule("pd", "default") + c.Assert(err, NotNil) + + err = s.manager.SetRule(&Rule{GroupID: "pd", ID: "foo", StartKeyHex: "", EndKeyHex: "abcd", Role: "voter", Count: 1}) + c.Assert(err, IsNil) + // |-- default --| + // |-- foo --| + // still cannot delete default since it will cause ("abcd", "") has no rules inside. + err = s.manager.DeleteRule("pd", "default") + c.Assert(err, NotNil) + err = s.manager.SetRule(&Rule{GroupID: "pd", ID: "bar", StartKeyHex: "abcd", EndKeyHex: "", Role: "voter", Count: 1}) + c.Assert(err, IsNil) + // now default can be deleted. + err = s.manager.DeleteRule("pd", "default") + c.Assert(err, IsNil) + // cannot change range since it will cause ("abaa", "abcd") has no rules inside. + err = s.manager.SetRule(&Rule{GroupID: "pd", ID: "foo", StartKeyHex: "", EndKeyHex: "abaa", Role: "voter", Count: 1}) + c.Assert(err, NotNil) +} + func (s *testManagerSuite) dhex(hk string) []byte { k, err := hex.DecodeString(hk) if err != nil { diff --git a/tools/pd-ctl/pdctl/command/config_command.go b/tools/pd-ctl/pdctl/command/config_command.go index 7c846a818bb..6514d344284 100644 --- a/tools/pd-ctl/pdctl/command/config_command.go +++ b/tools/pd-ctl/pdctl/command/config_command.go @@ -520,7 +520,10 @@ func putPlacementRulesFunc(cmd *cobra.Command, args []string) { return } fmt.Printf("saved rule %s/%s\n", r.GroupID, r.ID) - } else { + } + } + for _, r := range rules { + if r.Count == 0 { _, err = doRequest(cmd, path.Join(rulePrefix, r.GroupID, r.ID), http.MethodDelete) if err != nil { fmt.Printf("failed to delete rule %s/%s: %v\n", r.GroupID, r.ID, err)