From 8f0c1ce7f96723127b1e32833c063d169f3feaa1 Mon Sep 17 00:00:00 2001 From: lhy1024 Date: Wed, 8 Jan 2025 22:04:24 +0800 Subject: [PATCH 1/6] pd-ctl: print warn information when set max-replica to less than 3 Signed-off-by: lhy1024 --- tools/pd-ctl/pdctl/command/config_command.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tools/pd-ctl/pdctl/command/config_command.go b/tools/pd-ctl/pdctl/command/config_command.go index c49b0e3b9e5..6176c64d116 100644 --- a/tools/pd-ctl/pdctl/command/config_command.go +++ b/tools/pd-ctl/pdctl/command/config_command.go @@ -28,6 +28,7 @@ import ( "github.com/spf13/cobra" + sc "github.com/tikv/pd/pkg/schedule/config" "github.com/tikv/pd/pkg/schedule/placement" "github.com/tikv/pd/pkg/utils/apiutil" "github.com/tikv/pd/pkg/utils/reflectutil" @@ -374,6 +375,12 @@ func postConfigDataWithPath(cmd *cobra.Command, key, value, path string) error { val = value } data[key] = val + if key == "max-replicas" { + replica, err := strconv.ParseInt(value, 10, 64) + if err == nil && replica < sc.DefaultMaxReplicas { + cmd.Println("Setting max-replica to less than 3 may be a mistake and carries high risk. Please confirm the setting.") + } + } reqData, err := json.Marshal(data) if err != nil { return err From 0ba586c6e94a17f0f696790415545e2fc1d225ea Mon Sep 17 00:00:00 2001 From: lhy1024 Date: Mon, 13 Jan 2025 11:43:32 +0800 Subject: [PATCH 2/6] add test Signed-off-by: lhy1024 --- tools/pd-ctl/tests/config/config_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/pd-ctl/tests/config/config_test.go b/tools/pd-ctl/tests/config/config_test.go index 3b622beecbf..f0b02620fdd 100644 --- a/tools/pd-ctl/tests/config/config_test.go +++ b/tools/pd-ctl/tests/config/config_test.go @@ -1136,6 +1136,7 @@ func (suite *configTestSuite) checkUpdateDefaultReplicaConfig(cluster *pdTests.T output, err := tests.ExecuteCommand(cmd, "-u", pdAddr, "config", "set", "max-replicas", "2") re.NoError(err) re.Contains(string(output), "Success!") + re.Contains(string(output), "Setting max-replica to less than 3 may be a mistake and carries high risk. Please confirm the setting.") checkMaxReplicas(2) output, err = tests.ExecuteCommand(cmd, "-u", pdAddr, "config", "set", "location-labels", "zone,host") re.NoError(err) @@ -1156,6 +1157,7 @@ func (suite *configTestSuite) checkUpdateDefaultReplicaConfig(cluster *pdTests.T output, err = tests.ExecuteCommand(cmd, "-u", pdAddr, "config", "set", "max-replicas", "3") re.NoError(err) re.Contains(string(output), "Success!") + re.NotContains(string(output), "Setting max-replica to less than 3 may be a mistake and carries high risk. Please confirm the setting.") checkMaxReplicas(3) checkRuleCount(3) From 1a815b9ca55d5bc740c9bba876c9ccc96fc3bf0b Mon Sep 17 00:00:00 2001 From: lhy1024 Date: Mon, 13 Jan 2025 15:38:30 +0800 Subject: [PATCH 3/6] add more check Signed-off-by: lhy1024 --- tools/pd-ctl/pdctl/command/config_command.go | 32 ++++++++++++++++---- tools/pd-ctl/tests/config/config_test.go | 4 +-- 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/tools/pd-ctl/pdctl/command/config_command.go b/tools/pd-ctl/pdctl/command/config_command.go index 6176c64d116..1b4f87436d8 100644 --- a/tools/pd-ctl/pdctl/command/config_command.go +++ b/tools/pd-ctl/pdctl/command/config_command.go @@ -28,7 +28,6 @@ import ( "github.com/spf13/cobra" - sc "github.com/tikv/pd/pkg/schedule/config" "github.com/tikv/pd/pkg/schedule/placement" "github.com/tikv/pd/pkg/utils/apiutil" "github.com/tikv/pd/pkg/utils/reflectutil" @@ -374,13 +373,10 @@ func postConfigDataWithPath(cmd *cobra.Command, key, value, path string) error { if err != nil { val = value } - data[key] = val if key == "max-replicas" { - replica, err := strconv.ParseInt(value, 10, 64) - if err == nil && replica < sc.DefaultMaxReplicas { - cmd.Println("Setting max-replica to less than 3 may be a mistake and carries high risk. Please confirm the setting.") - } + checkMaxReplicas(cmd, val) } + data[key] = val reqData, err := json.Marshal(data) if err != nil { return err @@ -393,6 +389,30 @@ func postConfigDataWithPath(cmd *cobra.Command, key, value, path string) error { return nil } +func checkMaxReplicas(cmd *cobra.Command, value any) { + newReplica, ok := value.(float64) + if !ok { + // If the type is not float64, it will be handled elsewhere + return + } + header := buildHeader(cmd) + r, err := doRequest(cmd, replicatePrefix, http.MethodGet, header) + if err != nil { + cmd.Printf("Failed to get config when checking config: %s\n", err) + return + } + oldConfig := make(map[string]any) + err = json.Unmarshal([]byte(r), &oldConfig) + if err != nil { + cmd.Printf("Failed to unmarshal config when checking config: %s\n", err) + return + } + oldReplica, ok := oldConfig["max-replicas"].(float64) + if ok && newReplica < oldReplica { + cmd.Printf("Setting max-replica to %v which is less than the current replicas (%v). This may pose a risk. Please confirm the setting.\n", newReplica, oldReplica) + } +} + func setConfigCommandFunc(cmd *cobra.Command, args []string) { if len(args) != 2 { cmd.Println(cmd.UsageString()) diff --git a/tools/pd-ctl/tests/config/config_test.go b/tools/pd-ctl/tests/config/config_test.go index f0b02620fdd..b5123b1d785 100644 --- a/tools/pd-ctl/tests/config/config_test.go +++ b/tools/pd-ctl/tests/config/config_test.go @@ -1136,7 +1136,7 @@ func (suite *configTestSuite) checkUpdateDefaultReplicaConfig(cluster *pdTests.T output, err := tests.ExecuteCommand(cmd, "-u", pdAddr, "config", "set", "max-replicas", "2") re.NoError(err) re.Contains(string(output), "Success!") - re.Contains(string(output), "Setting max-replica to less than 3 may be a mistake and carries high risk. Please confirm the setting.") + re.Contains(string(output), "which is less than the current replicas") checkMaxReplicas(2) output, err = tests.ExecuteCommand(cmd, "-u", pdAddr, "config", "set", "location-labels", "zone,host") re.NoError(err) @@ -1157,7 +1157,7 @@ func (suite *configTestSuite) checkUpdateDefaultReplicaConfig(cluster *pdTests.T output, err = tests.ExecuteCommand(cmd, "-u", pdAddr, "config", "set", "max-replicas", "3") re.NoError(err) re.Contains(string(output), "Success!") - re.NotContains(string(output), "Setting max-replica to less than 3 may be a mistake and carries high risk. Please confirm the setting.") + re.NotContains(string(output), "which is less than the current replicas") checkMaxReplicas(3) checkRuleCount(3) From 62113daaf806580b047dca4ffd227d1559c87356 Mon Sep 17 00:00:00 2001 From: lhy1024 Date: Mon, 13 Jan 2025 16:45:47 +0800 Subject: [PATCH 4/6] add more tests Signed-off-by: lhy1024 --- server/api/config.go | 5 +++ tools/pd-ctl/tests/config/config_test.go | 47 +++++++++++++++++++++++- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/server/api/config.go b/server/api/config.go index ff4fe0add82..9e737fbe9bb 100644 --- a/server/api/config.go +++ b/server/api/config.go @@ -28,6 +28,7 @@ import ( "github.com/pingcap/errcode" "github.com/pingcap/errors" + "github.com/pingcap/failpoint" "github.com/pingcap/log" "github.com/tikv/pd/pkg/errs" @@ -411,6 +412,10 @@ func (h *confHandler) SetScheduleConfig(w http.ResponseWriter, r *http.Request) // @Success 200 {object} sc.ReplicationConfig // @Router /config/replicate [get] func (h *confHandler) GetReplicationConfig(w http.ResponseWriter, r *http.Request) { + failpoint.Inject("getReplicationConfigFailed", func(v failpoint.Value) { + code := v.(int) + h.rd.JSON(w, code, "get config failed") + }) if h.svr.IsServiceIndependent(constant.SchedulingServiceName) && r.Header.Get(apiutil.XForbiddenForwardToMicroserviceHeader) != "true" { cfg, err := h.getSchedulingServerConfig() diff --git a/tools/pd-ctl/tests/config/config_test.go b/tools/pd-ctl/tests/config/config_test.go index b5123b1d785..287c8202f3c 100644 --- a/tools/pd-ctl/tests/config/config_test.go +++ b/tools/pd-ctl/tests/config/config_test.go @@ -1136,7 +1136,6 @@ func (suite *configTestSuite) checkUpdateDefaultReplicaConfig(cluster *pdTests.T output, err := tests.ExecuteCommand(cmd, "-u", pdAddr, "config", "set", "max-replicas", "2") re.NoError(err) re.Contains(string(output), "Success!") - re.Contains(string(output), "which is less than the current replicas") checkMaxReplicas(2) output, err = tests.ExecuteCommand(cmd, "-u", pdAddr, "config", "set", "location-labels", "zone,host") re.NoError(err) @@ -1157,7 +1156,6 @@ func (suite *configTestSuite) checkUpdateDefaultReplicaConfig(cluster *pdTests.T output, err = tests.ExecuteCommand(cmd, "-u", pdAddr, "config", "set", "max-replicas", "3") re.NoError(err) re.Contains(string(output), "Success!") - re.NotContains(string(output), "which is less than the current replicas") checkMaxReplicas(3) checkRuleCount(3) @@ -1202,6 +1200,51 @@ func (suite *configTestSuite) checkUpdateDefaultReplicaConfig(cluster *pdTests.T checkRuleIsolationLevel("host") } +func (suite *configTestSuite) TestMaxReplicaChanged() { + suite.env.RunTestInPDMode(suite.checkMaxReplicaChanged) +} + +func (suite *configTestSuite) checkMaxReplicaChanged(cluster *pdTests.TestCluster) { + re := suite.Require() + leaderServer := cluster.GetLeaderServer() + pdAddr := leaderServer.GetAddr() + cmd := ctl.GetRootCmd() + + store := &metapb.Store{ + Id: 1, + State: metapb.StoreState_Up, + } + pdTests.MustPutStore(re, cluster, store) + + // test set max-replicas with invalid value + output, err := tests.ExecuteCommand(cmd, "-u", pdAddr, "config", "set", "max-replicas", "z") + re.NoError(err) + re.NotContains(string(output), "Success!") + // test set max-replicas with less value + output, err = tests.ExecuteCommand(cmd, "-u", pdAddr, "config", "set", "max-replicas", "2") + re.NoError(err) + re.Contains(string(output), "Success!") + re.Contains(string(output), "which is less than the current replicas") + // test set max-replicas with greater value + output, err = tests.ExecuteCommand(cmd, "-u", pdAddr, "config", "set", "max-replicas", "3") + re.NoError(err) + re.Contains(string(output), "Success!") + re.NotContains(string(output), "which is less than the current replicas") + // test meet error when get config failed + failpoint.Enable("github.com/tikv/pd/server/api/getReplicationConfigFailed", `return(200)`) + output, err = tests.ExecuteCommand(cmd, "-u", pdAddr, "config", "set", "max-replicas", "3") + re.NoError(err) + re.Contains(string(output), "Success!") + re.Contains(string(output), "Failed to unmarshal config when checking config") + failpoint.Disable("github.com/tikv/pd/server/api/getReplicationConfigFailed") + failpoint.Enable("github.com/tikv/pd/server/api/getReplicationConfigFailed", `return(500)`) + output, err = tests.ExecuteCommand(cmd, "-u", pdAddr, "config", "set", "max-replicas", "3") + re.NoError(err) + re.Contains(string(output), "Success!") + re.Contains(string(output), "Failed to get config when checking config") + failpoint.Disable("github.com/tikv/pd/server/api/getReplicationConfigFailed") +} + func (suite *configTestSuite) TestPDServerConfig() { suite.env.RunTestBasedOnMode(suite.checkPDServerConfig) } From f7f1af881f1a45dce458524c2331e56758368f8f Mon Sep 17 00:00:00 2001 From: lhy1024 Date: Mon, 13 Jan 2025 16:54:04 +0800 Subject: [PATCH 5/6] fix ci Signed-off-by: lhy1024 --- tools/pd-ctl/tests/config/config_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/pd-ctl/tests/config/config_test.go b/tools/pd-ctl/tests/config/config_test.go index c8f56efda71..ca6b5e02c66 100644 --- a/tools/pd-ctl/tests/config/config_test.go +++ b/tools/pd-ctl/tests/config/config_test.go @@ -1200,7 +1200,7 @@ func (suite *configTestSuite) checkUpdateDefaultReplicaConfig(cluster *pdTests.T } func (suite *configTestSuite) TestMaxReplicaChanged() { - suite.env.RunTestInPDMode(suite.checkMaxReplicaChanged) + suite.env.RunTest(suite.checkMaxReplicaChanged) } func (suite *configTestSuite) checkMaxReplicaChanged(cluster *pdTests.TestCluster) { From 2fb9d2b4bd745b0a21c29cb5c567aaf4393846e7 Mon Sep 17 00:00:00 2001 From: lhy1024 Date: Mon, 13 Jan 2025 17:58:28 +0800 Subject: [PATCH 6/6] address comments Signed-off-by: lhy1024 --- tools/pd-ctl/pdctl/command/config_command.go | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/tools/pd-ctl/pdctl/command/config_command.go b/tools/pd-ctl/pdctl/command/config_command.go index aed4f1a43f2..f0912c07c53 100644 --- a/tools/pd-ctl/pdctl/command/config_command.go +++ b/tools/pd-ctl/pdctl/command/config_command.go @@ -372,9 +372,8 @@ func postConfigDataWithPath(cmd *cobra.Command, key, value, path string) error { val, err := strconv.ParseFloat(value, 64) if err != nil { val = value - } - if key == "max-replicas" { - checkMaxReplicas(cmd, val) + } else if key == "max-replicas" { + checkMaxReplicas(cmd, val.(float64)) } data[key] = val reqData, err := json.Marshal(data) @@ -389,12 +388,7 @@ func postConfigDataWithPath(cmd *cobra.Command, key, value, path string) error { return nil } -func checkMaxReplicas(cmd *cobra.Command, value any) { - newReplica, ok := value.(float64) - if !ok { - // If the type is not float64, it will be handled elsewhere - return - } +func checkMaxReplicas(cmd *cobra.Command, newReplica float64) { header := buildHeader(cmd) r, err := doRequest(cmd, replicatePrefix, http.MethodGet, header) if err != nil {