From 51455fe41e2850512d94fa64f58305b571d9c003 Mon Sep 17 00:00:00 2001 From: lucklove Date: Mon, 21 Sep 2020 11:54:49 +0800 Subject: [PATCH 01/15] Modify template Signed-off-by: lucklove --- examples/topology.example.yaml | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/examples/topology.example.yaml b/examples/topology.example.yaml index 49bd30f4cf..3ff9be9a97 100644 --- a/examples/topology.example.yaml +++ b/examples/topology.example.yaml @@ -61,6 +61,7 @@ server_configs: schedule.leader-schedule-limit: 4 schedule.region-schedule-limit: 2048 schedule.replica-schedule-limit: 64 + replication.location-labels: ["zone", "host"] tiflash: # path_realtime_mode: false logger.level: "info" @@ -112,11 +113,15 @@ tikv_servers: # log_dir: "/tidb-deploy/tikv-20160/log" # numa_node: "0,1" # # The following configs are used to overwrite the `server_configs.tikv` values. - # config: + config: + server.labels: { zone: "zone1", host: "host1" } # server.grpc-concurrency: 4 - # server.labels: { zone: "zone1", dc: "dc1", host: "host1" } - host: 10.0.1.15 + config: + server.labels: { zone: "zone1", host: "host2" } - host: 10.0.1.16 + config: + server.labels: { zone: "zone2", host: "host3" } tiflash_servers: - host: 10.0.1.14 From d91101664ce7839fa3fdb0e9e29b40e382db886c Mon Sep 17 00:00:00 2001 From: lucklove Date: Mon, 21 Sep 2020 21:01:41 +0800 Subject: [PATCH 02/15] Check tikv labels on display Signed-off-by: lucklove --- examples/topology.example.yaml | 1 + pkg/cluster/api/pdapi.go | 16 +++++++++ pkg/cluster/manager.go | 20 ++++++++--- pkg/cluster/spec/pd.go | 5 +++ pkg/cluster/spec/spec.go | 2 +- pkg/cluster/spec/tikv.go | 5 +++ pkg/cluster/spec/validate.go | 62 ++++++++++++++++++++++++++++++++++ 7 files changed, 106 insertions(+), 5 deletions(-) diff --git a/examples/topology.example.yaml b/examples/topology.example.yaml index 3ff9be9a97..a6f32ed0ae 100644 --- a/examples/topology.example.yaml +++ b/examples/topology.example.yaml @@ -62,6 +62,7 @@ server_configs: schedule.region-schedule-limit: 2048 schedule.replica-schedule-limit: 64 replication.location-labels: ["zone", "host"] + replication.strictly-match-label: true tiflash: # path_realtime_mode: false logger.level: "info" diff --git a/pkg/cluster/api/pdapi.go b/pkg/cluster/api/pdapi.go index 6a572494f5..d20b9951a8 100644 --- a/pkg/cluster/api/pdapi.go +++ b/pkg/cluster/api/pdapi.go @@ -31,6 +31,7 @@ import ( "github.com/pingcap/tiup/pkg/logger/log" "github.com/pingcap/tiup/pkg/utils" pdserverapi "github.com/tikv/pd/server/api" + pdconfig "github.com/tikv/pd/server/config" ) // PDClient is an HTTP client of the PD server @@ -661,6 +662,21 @@ func (pc *PDClient) GetReplicateConfig() ([]byte, error) { }) } +// GetLocationLabels gets the replication.location-labels from config +func (pc *PDClient) GetLocationLabels() ([]string, error) { + config, err := pc.GetReplicateConfig() + if err != nil { + return nil, err + } + + rc := pdconfig.ReplicationConfig{} + if err := json.Unmarshal(config, &rc); err != nil { + return nil, errors.Annotate(err, "unmarshal replication config") + } + + return rc.LocationLabels, nil +} + // UpdateScheduleConfig updates the PD schedule config func (pc *PDClient) UpdateScheduleConfig(body io.Reader) error { return pc.updateConfig(body, pdConfigSchedule) diff --git a/pkg/cluster/manager.go b/pkg/cluster/manager.go index ba4b57b060..c68a1a5b20 100644 --- a/pkg/cluster/manager.go +++ b/pkg/cluster/manager.go @@ -28,11 +28,13 @@ import ( "path/filepath" "sort" "strings" + "time" "github.com/fatih/color" "github.com/joomcode/errorx" perrs "github.com/pingcap/errors" "github.com/pingcap/tiup/pkg/cliutil" + "github.com/pingcap/tiup/pkg/cluster/api" "github.com/pingcap/tiup/pkg/cluster/clusterutil" "github.com/pingcap/tiup/pkg/cluster/executor" operator "github.com/pingcap/tiup/pkg/cluster/operation" @@ -529,6 +531,10 @@ func (m *Manager) Display(clusterName string, opt operator.Options) error { filterRoles := set.NewStringSet(opt.Roles...) filterNodes := set.NewStringSet(opt.Nodes...) pdList := topo.BaseTopo().MasterList + tlsCfg, err := topo.TLSConfig(m.specManager.Path(clusterName, spec.TLSCertKeyDir)) + if err != nil { + return perrs.AddStack(err) + } for _, comp := range topo.ComponentsByStartOrder() { for _, ins := range comp.Instances() { // apply role filter @@ -547,10 +553,6 @@ func (m *Manager) Display(clusterName string, opt operator.Options) error { dataDir = insDirs[1] } - tlsCfg, err := topo.TLSConfig(m.specManager.Path(clusterName, spec.TLSCertKeyDir)) - if err != nil { - return perrs.AddStack(err) - } status := ins.Status(tlsCfg, pdList...) // Query the service status if status == "-" { @@ -595,6 +597,16 @@ func (m *Manager) Display(clusterName string, opt operator.Options) error { cliutil.PrintTable(clusterTable, true) fmt.Printf("Total nodes: %d\n", len(clusterTable)-1) + pdClient := api.NewPDClient(pdList, 10*time.Second, tlsCfg) + lbs, err := pdClient.GetLocationLabels() + if err != nil { + return err + } + kvs := topo.(*spec.Specification).TiKVServers + if err := spec.CheckTiKVLocationLabels(lbs, kvs); err != nil { + fmt.Printf("%v", err) + } + return nil } diff --git a/pkg/cluster/spec/pd.go b/pkg/cluster/spec/pd.go index edb9cf1fe1..41ba7c79ac 100644 --- a/pkg/cluster/spec/pd.go +++ b/pkg/cluster/spec/pd.go @@ -251,6 +251,11 @@ func (i *PDInstance) InitConfig( i.Role()) } + // Enable strictly-match-label by default + if spec.Config["replication.strictly-match-label"] == nil { + spec.Config["replication.strictly-match-label"] = true + } + if err := i.MergeServerConfig(e, globalConfig, spec.Config, paths); err != nil { return err } diff --git a/pkg/cluster/spec/spec.go b/pkg/cluster/spec/spec.go index 5655b37188..f724285cd3 100644 --- a/pkg/cluster/spec/spec.go +++ b/pkg/cluster/spec/spec.go @@ -117,7 +117,7 @@ type BaseTopo struct { MasterList []string } -// Topology represents specification of the cluster. +// Topology represents specification of the cluster. type Topology interface { BaseTopo() *BaseTopo // Validate validates the topology specification and produce error if the diff --git a/pkg/cluster/spec/tikv.go b/pkg/cluster/spec/tikv.go index f79d1b90ce..98eb8ab1e2 100644 --- a/pkg/cluster/spec/tikv.go +++ b/pkg/cluster/spec/tikv.go @@ -120,6 +120,11 @@ func (s TiKVSpec) IsImported() bool { return s.Imported } +// Labels returns the labels of TiKV +func (s TiKVSpec) Labels() (map[string]string, error) { + return nil, nil +} + // TiKVComponent represents TiKV component. type TiKVComponent struct { *Specification diff --git a/pkg/cluster/spec/validate.go b/pkg/cluster/spec/validate.go index 3ccdd98033..ae99e76301 100644 --- a/pkg/cluster/spec/validate.go +++ b/pkg/cluster/spec/validate.go @@ -307,6 +307,68 @@ Please change to use another port or another host. return nil } +// TiKVLabelError indicates that some TiKV servers don't have correct labels +type TiKVLabelError struct { + TiKVInstances map[string][]error +} + +// Error implements error +func (e *TiKVLabelError) Error() string { + str := "" + for id, errs := range e.TiKVInstances { + if len(errs) == 0 { + continue + } + + str += fmt.Sprintf("%s:\n", id) + for _, e := range errs { + str += fmt.Sprintf("\t%s\n", e.Error()) + } + } + return str +} + +// CheckTiKVLocationLabels will check if tikv missing label or have wrong label +func CheckTiKVLocationLabels(pdLocLabels []string, kvs []TiKVSpec) error { + lerr := &TiKVLabelError{ + TiKVInstances: make(map[string][]error), + } + lbs := set.NewStringSet(pdLocLabels...) + hosts := make(map[string]int) + + for _, kv := range kvs { + hosts[kv.Host] = hosts[kv.Host] + 1 + } + + for _, kv := range kvs { + id := fmt.Sprintf("%s:%d", kv.Host, kv.GetMainPort()) + ls, err := kv.Labels() + if err != nil { + return err + } + if len(ls) == 0 && hosts[kv.Host] > 1 { + lerr.TiKVInstances[id] = append( + lerr.TiKVInstances[id], + errors.New("location label missed"), + ) + continue + } + for lname := range ls { + if len(lbs) > 0 && !lbs.Exist(lname) { + lerr.TiKVInstances[id] = append( + lerr.TiKVInstances[id], + fmt.Errorf("label name %s is not specified in pd config (replication.location-labels) %v", lname, pdLocLabels), + ) + } + } + } + + if len(lerr.TiKVInstances) == 0 { + return nil + } + return lerr +} + // platformConflictsDetect checks for conflicts in topology for different OS / Arch // set to the same host / IP func (s *Specification) platformConflictsDetect() error { From 9ccd78c9f390117a64fde7b24e3e151d1923cc39 Mon Sep 17 00:00:00 2001 From: lucklove Date: Mon, 21 Sep 2020 21:41:51 +0800 Subject: [PATCH 03/15] Fix diff config Signed-off-by: lucklove --- pkg/cluster/manager.go | 1 + pkg/cluster/spec/tikv.go | 27 +++++++++++++++++++++++++-- pkg/cluster/spec/validate.go | 7 ++----- 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/pkg/cluster/manager.go b/pkg/cluster/manager.go index c68a1a5b20..e3abf74095 100644 --- a/pkg/cluster/manager.go +++ b/pkg/cluster/manager.go @@ -597,6 +597,7 @@ func (m *Manager) Display(clusterName string, opt operator.Options) error { cliutil.PrintTable(clusterTable, true) fmt.Printf("Total nodes: %d\n", len(clusterTable)-1) + // Check if TiKV's label set correctly pdClient := api.NewPDClient(pdList, 10*time.Second, tlsCfg) lbs, err := pdClient.GetLocationLabels() if err != nil { diff --git a/pkg/cluster/spec/tikv.go b/pkg/cluster/spec/tikv.go index 98eb8ab1e2..df930748c1 100644 --- a/pkg/cluster/spec/tikv.go +++ b/pkg/cluster/spec/tikv.go @@ -121,8 +121,31 @@ func (s TiKVSpec) IsImported() bool { } // Labels returns the labels of TiKV -func (s TiKVSpec) Labels() (map[string]string, error) { - return nil, nil +func (s TiKVSpec) Labels() map[string]string { + lbs := make(map[string]string) + + if s.Config["server"] != nil { + // server: + // labels: + // host: xxx + // zone: yyy + labelsMap := s.Config["server"].(map[interface{}]interface{})["labels"] + if labelsMap == nil { + return lbs + } + for k, v := range labelsMap.(map[interface{}]interface{}) { + lbs[k.(string)] = v.(string) + } + } else if s.Config["server.labels"] != nil { + // server.labels: + // host: xxx + // zone: yyy + for k, v := range s.Config["server.labels"].(map[interface{}]interface{}) { + lbs[k.(string)] = v.(string) + } + } + + return lbs } // TiKVComponent represents TiKV component. diff --git a/pkg/cluster/spec/validate.go b/pkg/cluster/spec/validate.go index ae99e76301..91acd9c111 100644 --- a/pkg/cluster/spec/validate.go +++ b/pkg/cluster/spec/validate.go @@ -342,14 +342,11 @@ func CheckTiKVLocationLabels(pdLocLabels []string, kvs []TiKVSpec) error { for _, kv := range kvs { id := fmt.Sprintf("%s:%d", kv.Host, kv.GetMainPort()) - ls, err := kv.Labels() - if err != nil { - return err - } + ls := kv.Labels() if len(ls) == 0 && hosts[kv.Host] > 1 { lerr.TiKVInstances[id] = append( lerr.TiKVInstances[id], - errors.New("location label missed"), + errors.New("location label missing"), ) continue } From 78517d987f43ba26bd89a0f6a224177a0733121d Mon Sep 17 00:00:00 2001 From: lucklove Date: Tue, 22 Sep 2020 12:14:11 +0800 Subject: [PATCH 04/15] Check TiKV labels on scale-out Signed-off-by: lucklove --- components/cluster/command/scale_out.go | 1 + pkg/cluster/manager.go | 21 ++++++++++++++++++++- pkg/cluster/spec/validate.go | 5 ++--- 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/components/cluster/command/scale_out.go b/components/cluster/command/scale_out.go index 2965085268..813dadbfbc 100644 --- a/components/cluster/command/scale_out.go +++ b/components/cluster/command/scale_out.go @@ -73,6 +73,7 @@ func newScaleOutCmd() *cobra.Command { cmd.Flags().BoolVarP(&opt.SkipCreateUser, "skip-create-user", "", false, "Skip creating the user specified in topology (experimental).") cmd.Flags().StringVarP(&opt.IdentityFile, "identity_file", "i", opt.IdentityFile, "The path of the SSH identity file. If specified, public key authentication will be used.") cmd.Flags().BoolVarP(&opt.UsePassword, "password", "p", false, "Use password of target hosts. If specified, password authentication will be used.") + cmd.Flags().BoolVarP(&opt.NoLabels, "no-labels", "", false, "Don't check TiKV labels") return cmd } diff --git a/pkg/cluster/manager.go b/pkg/cluster/manager.go index e3abf74095..fa589df726 100644 --- a/pkg/cluster/manager.go +++ b/pkg/cluster/manager.go @@ -605,7 +605,7 @@ func (m *Manager) Display(clusterName string, opt operator.Options) error { } kvs := topo.(*spec.Specification).TiKVServers if err := spec.CheckTiKVLocationLabels(lbs, kvs); err != nil { - fmt.Printf("%v", err) + color.Yellow("\nWARN: there is something wrong with TiKV labels, which may cause data losing:\n%v", err) } return nil @@ -1013,6 +1013,7 @@ type ScaleOutOptions struct { SkipCreateUser bool // don't create user IdentityFile string // path to the private key file UsePassword bool // use password instead of identity file for ssh connection + NoLabels bool // don't check labels for TiKV instance } // DeployOptions contains the options for scale out. @@ -1491,6 +1492,24 @@ func (m *Manager) ScaleOut( return err } + if !opt.NoLabels { + // Check if TiKV's label set correctly + pdList := topo.BaseTopo().MasterList + tlsCfg, err := topo.TLSConfig(m.specManager.Path(clusterName, spec.TLSCertKeyDir)) + if err != nil { + return perrs.AddStack(err) + } + pdClient := api.NewPDClient(pdList, 10*time.Second, tlsCfg) + lbs, err := pdClient.GetLocationLabels() + if err != nil { + return err + } + kvs := mergedTopo.(*spec.Specification).TiKVServers + if err := spec.CheckTiKVLocationLabels(lbs, kvs); err != nil { + return perrs.Errorf("check TiKV label failed, please fix that before continue:\n%s", err) + } + } + clusterList, err := m.specManager.GetAllClusters() if err != nil { return err diff --git a/pkg/cluster/spec/validate.go b/pkg/cluster/spec/validate.go index 91acd9c111..2e7d81319a 100644 --- a/pkg/cluster/spec/validate.go +++ b/pkg/cluster/spec/validate.go @@ -339,7 +339,6 @@ func CheckTiKVLocationLabels(pdLocLabels []string, kvs []TiKVSpec) error { for _, kv := range kvs { hosts[kv.Host] = hosts[kv.Host] + 1 } - for _, kv := range kvs { id := fmt.Sprintf("%s:%d", kv.Host, kv.GetMainPort()) ls := kv.Labels() @@ -351,10 +350,10 @@ func CheckTiKVLocationLabels(pdLocLabels []string, kvs []TiKVSpec) error { continue } for lname := range ls { - if len(lbs) > 0 && !lbs.Exist(lname) { + if !lbs.Exist(lname) { lerr.TiKVInstances[id] = append( lerr.TiKVInstances[id], - fmt.Errorf("label name %s is not specified in pd config (replication.location-labels) %v", lname, pdLocLabels), + fmt.Errorf("label name '%s' is not specified in pd config (replication.location-labels: %v)", lname, pdLocLabels), ) } } From 03e1bb135a3199b1f43f67450619ee40d783ee89 Mon Sep 17 00:00:00 2001 From: lucklove Date: Tue, 22 Sep 2020 14:07:43 +0800 Subject: [PATCH 05/15] Check tikv labels on deploy Signed-off-by: lucklove --- components/cluster/command/deploy.go | 1 + pkg/cluster/manager.go | 10 ++++++++++ pkg/cluster/spec/spec.go | 26 ++++++++++++++++++++++++++ pkg/cluster/spec/tikv.go | 16 +++++++--------- 4 files changed, 44 insertions(+), 9 deletions(-) diff --git a/components/cluster/command/deploy.go b/components/cluster/command/deploy.go index 1db8a5df3b..b4d5265b21 100644 --- a/components/cluster/command/deploy.go +++ b/components/cluster/command/deploy.go @@ -97,6 +97,7 @@ func newDeploy() *cobra.Command { cmd.Flags().StringVarP(&opt.IdentityFile, "identity_file", "i", opt.IdentityFile, "The path of the SSH identity file. If specified, public key authentication will be used.") cmd.Flags().BoolVarP(&opt.UsePassword, "password", "p", false, "Use password of target hosts. If specified, password authentication will be used.") cmd.Flags().BoolVarP(&opt.IgnoreConfigCheck, "ignore-config-check", "", opt.IgnoreConfigCheck, "Ignore the config check result") + cmd.Flags().BoolVarP(&opt.NoLabels, "no-labels", "", false, "Don't check TiKV labels") return cmd } diff --git a/pkg/cluster/manager.go b/pkg/cluster/manager.go index fa589df726..be53bc1ab5 100644 --- a/pkg/cluster/manager.go +++ b/pkg/cluster/manager.go @@ -1024,6 +1024,7 @@ type DeployOptions struct { IdentityFile string // path to the private key file UsePassword bool // use password instead of identity file for ssh connection IgnoreConfigCheck bool // ignore config check result + NoLabels bool // don't check labels for TiKV instance } // DeployerInstance is a instance can deploy to a target deploy directory. @@ -1071,6 +1072,15 @@ func (m *Manager) Deploy( base.GlobalOptions.SSHType = sshType } + if !opt.NoLabels { + // Check if TiKV's label set correctly + lbs := topo.(*spec.Specification).LocationLabels() + kvs := topo.(*spec.Specification).TiKVServers + if err := spec.CheckTiKVLocationLabels(lbs, kvs); err != nil { + return perrs.Errorf("check TiKV label failed, please fix that before continue:\n%s", err) + } + } + clusterList, err := m.specManager.GetAllClusters() if err != nil { return err diff --git a/pkg/cluster/spec/spec.go b/pkg/cluster/spec/spec.go index f724285cd3..a2f50e03c6 100644 --- a/pkg/cluster/spec/spec.go +++ b/pkg/cluster/spec/spec.go @@ -210,6 +210,32 @@ func (s *Specification) BaseTopo() *BaseTopo { } } +// LocationLabels returns replication.location-labels in PD config +func (s *Specification) LocationLabels() []string { + lbs := []string{} + + if pdReplica := s.ServerConfigs.PD["replication"]; pdReplica != nil { + // server_configs: + // pd: + // replication: + // location-labels: ["zone", "host"] + if repLbs := pdReplica.(map[interface{}]interface{})["location-labels"]; repLbs != nil { + for _, l := range repLbs.([]interface{}) { + lbs = append(lbs, l.(string)) + } + } + } else if repLbs := s.ServerConfigs.PD["replication.location-labels"]; repLbs != nil { + // server_configs: + // pd.replication: + // location-labels: ["zone", "host"] + for _, l := range repLbs.([]interface{}) { + lbs = append(lbs, l.(string)) + } + } + + return lbs +} + // AllComponentNames contains the names of all components. // should include all components in ComponentsByStartOrder func AllComponentNames() (roles []string) { diff --git a/pkg/cluster/spec/tikv.go b/pkg/cluster/spec/tikv.go index df930748c1..115eabebfd 100644 --- a/pkg/cluster/spec/tikv.go +++ b/pkg/cluster/spec/tikv.go @@ -124,23 +124,21 @@ func (s TiKVSpec) IsImported() bool { func (s TiKVSpec) Labels() map[string]string { lbs := make(map[string]string) - if s.Config["server"] != nil { + if serverCfg := s.Config["server"]; serverCfg != nil { // server: // labels: // host: xxx // zone: yyy - labelsMap := s.Config["server"].(map[interface{}]interface{})["labels"] - if labelsMap == nil { - return lbs - } - for k, v := range labelsMap.(map[interface{}]interface{}) { - lbs[k.(string)] = v.(string) + if labelsMap := serverCfg.(map[interface{}]interface{})["labels"]; labelsMap != nil { + for k, v := range labelsMap.(map[interface{}]interface{}) { + lbs[k.(string)] = v.(string) + } } - } else if s.Config["server.labels"] != nil { + } else if serverLbs := s.Config["server.labels"]; serverLbs != nil { // server.labels: // host: xxx // zone: yyy - for k, v := range s.Config["server.labels"].(map[interface{}]interface{}) { + for k, v := range serverLbs.(map[interface{}]interface{}) { lbs[k.(string)] = v.(string) } } From 3600082fa2a9ee3902d0143bcf705a255d238905 Mon Sep 17 00:00:00 2001 From: lucklove Date: Tue, 22 Sep 2020 16:48:50 +0800 Subject: [PATCH 06/15] Add unit test Signed-off-by: lucklove --- pkg/cluster/spec/spec.go | 4 +- pkg/cluster/spec/spec_test.go | 28 +++++++++ pkg/cluster/spec/validate_test.go | 94 +++++++++++++++++++++++++++++++ 3 files changed, 124 insertions(+), 2 deletions(-) diff --git a/pkg/cluster/spec/spec.go b/pkg/cluster/spec/spec.go index a2f50e03c6..0db8f5e452 100644 --- a/pkg/cluster/spec/spec.go +++ b/pkg/cluster/spec/spec.go @@ -226,8 +226,8 @@ func (s *Specification) LocationLabels() []string { } } else if repLbs := s.ServerConfigs.PD["replication.location-labels"]; repLbs != nil { // server_configs: - // pd.replication: - // location-labels: ["zone", "host"] + // pd: + // replication.location-labels: ["zone", "host"] for _, l := range repLbs.([]interface{}) { lbs = append(lbs, l.(string)) } diff --git a/pkg/cluster/spec/spec_test.go b/pkg/cluster/spec/spec_test.go index edf6f58e64..8f85a34bf0 100644 --- a/pkg/cluster/spec/spec_test.go +++ b/pkg/cluster/spec/spec_test.go @@ -471,3 +471,31 @@ item7 = 700 c.Assert(err, IsNil) c.Assert(string(merge2), DeepEquals, expected) } + +func (s *metaSuiteTopo) TestLocationLabels(c *C) { + spec := Specification{} + + c.Assert(len(spec.LocationLabels()), Equals, 0) + + err := yaml.Unmarshal([]byte(` +server_configs: + pd: + replication.location-labels: ["zone", "host"] +`), &spec) + c.Assert(err, IsNil) + lbs := spec.LocationLabels() + c.Assert(lbs, DeepEquals, []string{"zone", "host"}) + + spec = Specification{} + err = yaml.Unmarshal([]byte(` +server_configs: + pd: + replication: + location-labels: + - zone + - host +`), &spec) + c.Assert(err, IsNil) + lbs = spec.LocationLabels() + c.Assert(lbs, DeepEquals, []string{"zone", "host"}) +} diff --git a/pkg/cluster/spec/validate_test.go b/pkg/cluster/spec/validate_test.go index 0980b1f4d8..f201cb1ccb 100644 --- a/pkg/cluster/spec/validate_test.go +++ b/pkg/cluster/spec/validate_test.go @@ -606,3 +606,97 @@ func (s *metaSuiteTopo) TestRelativePathDetect(c *C) { } } } + +func (s *metaSuiteTopo) TestTiKVLocationLabelsCheck(c *C) { + // 2 tikv on different host + topo := Specification{} + err := yaml.Unmarshal([]byte(` +tikv_servers: + - host: 172.16.5.140 + port: 20160 + status_port: 20180 + - host: 172.16.5.139 + port: 20160 + status_port: 20180 +`), &topo) + c.Assert(err, IsNil) + err = CheckTiKVLocationLabels(nil, topo.TiKVServers) + c.Assert(err, IsNil) + err = CheckTiKVLocationLabels([]string{}, topo.TiKVServers) + c.Assert(err, IsNil) + + // 2 tikv on the same host without label + topo = Specification{} + err = yaml.Unmarshal([]byte(` +tikv_servers: + - host: 172.16.5.140 + port: 20160 + status_port: 20180 + - host: 172.16.5.140 + port: 20161 + status_port: 20181 +`), &topo) + c.Assert(err, IsNil) + err = CheckTiKVLocationLabels(nil, topo.TiKVServers) + c.Assert(err, NotNil) + + // 2 tikv on the same host with unacquainted label + topo = Specification{} + err = yaml.Unmarshal([]byte(` +tikv_servers: + - host: 172.16.5.140 + port: 20160 + status_port: 20180 + config: + server.labels: { zone: "zone1", host: "172.16.5.140" } + - host: 172.16.5.140 + port: 20161 + status_port: 20181 + config: + server.labels: { zone: "zone1", host: "172.16.5.140" } +`), &topo) + c.Assert(err, IsNil) + err = CheckTiKVLocationLabels(nil, topo.TiKVServers) + c.Assert(err, NotNil) + + // 2 tikv on the same host with correct label + topo = Specification{} + err = yaml.Unmarshal([]byte(` +tikv_servers: + - host: 172.16.5.140 + port: 20160 + status_port: 20180 + config: + server.labels: { zone: "zone1", host: "172.16.5.140" } + - host: 172.16.5.140 + port: 20161 + status_port: 20181 + config: + server.labels: { zone: "zone1", host: "172.16.5.140" } +`), &topo) + c.Assert(err, IsNil) + err = CheckTiKVLocationLabels([]string{"zone", "host"}, topo.TiKVServers) + c.Assert(err, IsNil) + + // 2 tikv on the same host with diffrent config style + topo = Specification{} + err = yaml.Unmarshal([]byte(` +tikv_servers: + - host: 172.16.5.140 + port: 20160 + status_port: 20180 + config: + server: + labels: { zone: "zone1", host: "172.16.5.140" } + - host: 172.16.5.140 + port: 20161 + status_port: 20181 + config: + server.labels: + zone: "zone1" + host: "172.16.5.140" +`), &topo) + c.Assert(err, IsNil) + err = CheckTiKVLocationLabels([]string{"zone", "host"}, topo.TiKVServers) + c.Assert(err, IsNil) +} From 499d13d83dda1fded21c2227fcea4954e515d466 Mon Sep 17 00:00:00 2001 From: lucklove Date: Tue, 22 Sep 2020 16:54:38 +0800 Subject: [PATCH 07/15] Adjust comment Signed-off-by: lucklove --- pkg/cluster/api/pdapi.go | 2 +- pkg/cluster/spec/pd.go | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/pkg/cluster/api/pdapi.go b/pkg/cluster/api/pdapi.go index d20b9951a8..15e758aba2 100644 --- a/pkg/cluster/api/pdapi.go +++ b/pkg/cluster/api/pdapi.go @@ -662,7 +662,7 @@ func (pc *PDClient) GetReplicateConfig() ([]byte, error) { }) } -// GetLocationLabels gets the replication.location-labels from config +// GetLocationLabels gets the replication.location-labels config from pd server func (pc *PDClient) GetLocationLabels() ([]string, error) { config, err := pc.GetReplicateConfig() if err != nil { diff --git a/pkg/cluster/spec/pd.go b/pkg/cluster/spec/pd.go index 41ba7c79ac..edb9cf1fe1 100644 --- a/pkg/cluster/spec/pd.go +++ b/pkg/cluster/spec/pd.go @@ -251,11 +251,6 @@ func (i *PDInstance) InitConfig( i.Role()) } - // Enable strictly-match-label by default - if spec.Config["replication.strictly-match-label"] == nil { - spec.Config["replication.strictly-match-label"] = true - } - if err := i.MergeServerConfig(e, globalConfig, spec.Config, paths); err != nil { return err } From 789765e50ac76fffd72cacb41ef8de94523e8b79 Mon Sep 17 00:00:00 2001 From: lucklove Date: Tue, 22 Sep 2020 18:45:14 +0800 Subject: [PATCH 08/15] Adjust output Signed-off-by: lucklove --- pkg/cluster/manager.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/pkg/cluster/manager.go b/pkg/cluster/manager.go index be53bc1ab5..e001efe60c 100644 --- a/pkg/cluster/manager.go +++ b/pkg/cluster/manager.go @@ -598,13 +598,11 @@ func (m *Manager) Display(clusterName string, opt operator.Options) error { fmt.Printf("Total nodes: %d\n", len(clusterTable)-1) // Check if TiKV's label set correctly - pdClient := api.NewPDClient(pdList, 10*time.Second, tlsCfg) - lbs, err := pdClient.GetLocationLabels() - if err != nil { - return err - } kvs := topo.(*spec.Specification).TiKVServers - if err := spec.CheckTiKVLocationLabels(lbs, kvs); err != nil { + pdClient := api.NewPDClient(pdList, 10*time.Second, tlsCfg) + if lbs, err := pdClient.GetLocationLabels(); err != nil { + color.Yellow("\nWARN: get location labels from pd failed: %v", err) + } else if err := spec.CheckTiKVLocationLabels(lbs, kvs); err != nil { color.Yellow("\nWARN: there is something wrong with TiKV labels, which may cause data losing:\n%v", err) } From 81ed482528b33b69ce8eff0778fec7f0e873310f Mon Sep 17 00:00:00 2001 From: lucklove Date: Tue, 22 Sep 2020 19:12:25 +0800 Subject: [PATCH 09/15] Fix type cast Signed-off-by: lucklove --- pkg/cluster/manager.go | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/pkg/cluster/manager.go b/pkg/cluster/manager.go index e001efe60c..eb8600b7c1 100644 --- a/pkg/cluster/manager.go +++ b/pkg/cluster/manager.go @@ -597,13 +597,15 @@ func (m *Manager) Display(clusterName string, opt operator.Options) error { cliutil.PrintTable(clusterTable, true) fmt.Printf("Total nodes: %d\n", len(clusterTable)-1) - // Check if TiKV's label set correctly - kvs := topo.(*spec.Specification).TiKVServers - pdClient := api.NewPDClient(pdList, 10*time.Second, tlsCfg) - if lbs, err := pdClient.GetLocationLabels(); err != nil { - color.Yellow("\nWARN: get location labels from pd failed: %v", err) - } else if err := spec.CheckTiKVLocationLabels(lbs, kvs); err != nil { - color.Yellow("\nWARN: there is something wrong with TiKV labels, which may cause data losing:\n%v", err) + if topo, ok := topo.(*spec.Specification); ok { + // Check if TiKV's label set correctly + kvs := topo.TiKVServers + pdClient := api.NewPDClient(pdList, 10*time.Second, tlsCfg) + if lbs, err := pdClient.GetLocationLabels(); err != nil { + color.Yellow("\nWARN: get location labels from pd failed: %v", err) + } else if err := spec.CheckTiKVLocationLabels(lbs, kvs); err != nil { + color.Yellow("\nWARN: there is something wrong with TiKV labels, which may cause data losing:\n%v", err) + } } return nil @@ -1070,10 +1072,10 @@ func (m *Manager) Deploy( base.GlobalOptions.SSHType = sshType } - if !opt.NoLabels { + if topo, ok := topo.(*spec.Specification); ok && !opt.NoLabels { // Check if TiKV's label set correctly - lbs := topo.(*spec.Specification).LocationLabels() - kvs := topo.(*spec.Specification).TiKVServers + lbs := topo.LocationLabels() + kvs := topo.TiKVServers if err := spec.CheckTiKVLocationLabels(lbs, kvs); err != nil { return perrs.Errorf("check TiKV label failed, please fix that before continue:\n%s", err) } From 90525eb2ca6d72c4af4beca7e84e2a46074f5b95 Mon Sep 17 00:00:00 2001 From: lucklove Date: Tue, 22 Sep 2020 22:07:25 +0800 Subject: [PATCH 10/15] Use GetValueFromPath Signed-off-by: lucklove --- pkg/cluster/manager.go | 22 +++++------ pkg/cluster/spec/server_config.go | 53 +++++++++++++++++++++----- pkg/cluster/spec/server_config_test.go | 27 +++++++++++++ pkg/cluster/spec/spec.go | 15 +------- pkg/cluster/spec/spec_test.go | 9 ++--- pkg/cluster/spec/tikv.go | 15 +------- 6 files changed, 87 insertions(+), 54 deletions(-) diff --git a/pkg/cluster/manager.go b/pkg/cluster/manager.go index eb8600b7c1..f3a7f4a390 100644 --- a/pkg/cluster/manager.go +++ b/pkg/cluster/manager.go @@ -186,7 +186,7 @@ func (m *Manager) StopCluster(clusterName string, options operator.Options) erro tlsCfg, err := topo.TLSConfig(m.specManager.Path(clusterName, spec.TLSCertKeyDir)) if err != nil { - return perrs.AddStack(err) + return err } t := task.NewBuilder(). @@ -223,7 +223,7 @@ func (m *Manager) RestartCluster(clusterName string, options operator.Options) e tlsCfg, err := topo.TLSConfig(m.specManager.Path(clusterName, spec.TLSCertKeyDir)) if err != nil { - return perrs.AddStack(err) + return err } t := task.NewBuilder(). @@ -293,7 +293,7 @@ func (m *Manager) CleanCluster(clusterName string, gOpt operator.Options, cleanO tlsCfg, err := topo.TLSConfig(m.specManager.Path(clusterName, spec.TLSCertKeyDir)) if err != nil { - return perrs.AddStack(err) + return err } if !skipConfirm { @@ -356,7 +356,7 @@ func (m *Manager) DestroyCluster(clusterName string, gOpt operator.Options, dest tlsCfg, err := topo.TLSConfig(m.specManager.Path(clusterName, spec.TLSCertKeyDir)) if err != nil { - return perrs.AddStack(err) + return err } if !skipConfirm { @@ -533,7 +533,7 @@ func (m *Manager) Display(clusterName string, opt operator.Options) error { pdList := topo.BaseTopo().MasterList tlsCfg, err := topo.TLSConfig(m.specManager.Path(clusterName, spec.TLSCertKeyDir)) if err != nil { - return perrs.AddStack(err) + return err } for _, comp := range topo.ComponentsByStartOrder() { for _, ins := range comp.Instances() { @@ -762,7 +762,7 @@ func (m *Manager) Reload(clusterName string, opt operator.Options, skipRestart b tlsCfg, err := topo.TLSConfig(m.specManager.Path(clusterName, spec.TLSCertKeyDir)) if err != nil { - return perrs.AddStack(err) + return err } if !skipRestart { tb = tb.Func("UpgradeCluster", func(ctx *task.Context) error { @@ -907,7 +907,7 @@ func (m *Manager) Upgrade(clusterName string, clusterVersion string, opt operato tlsCfg, err := topo.TLSConfig(m.specManager.Path(clusterName, spec.TLSCertKeyDir)) if err != nil { - return perrs.AddStack(err) + return err } t := task.NewBuilder(). SSHKeySet( @@ -977,7 +977,7 @@ func (m *Manager) Patch(clusterName string, packagePath string, opt operator.Opt tlsCfg, err := topo.TLSConfig(m.specManager.Path(clusterName, spec.TLSCertKeyDir)) if err != nil { - return perrs.AddStack(err) + return err } t := task.NewBuilder(). SSHKeySet( @@ -1428,7 +1428,7 @@ func (m *Manager) ScaleIn( tlsCfg, err := topo.TLSConfig(m.specManager.Path(clusterName, spec.TLSCertKeyDir)) if err != nil { - return perrs.AddStack(err) + return err } b := task.NewBuilder(). @@ -1507,7 +1507,7 @@ func (m *Manager) ScaleOut( pdList := topo.BaseTopo().MasterList tlsCfg, err := topo.TLSConfig(m.specManager.Path(clusterName, spec.TLSCertKeyDir)) if err != nil { - return perrs.AddStack(err) + return err } pdClient := api.NewPDClient(pdList, 10*time.Second, tlsCfg) lbs, err := pdClient.GetLocationLabels() @@ -1914,7 +1914,7 @@ func buildScaleOutTask( tlsCfg, err := topo.TLSConfig(m.specManager.Path(clusterName, spec.TLSCertKeyDir)) if err != nil { - return nil, perrs.AddStack(err) + return nil, err } // Initialize the environments diff --git a/pkg/cluster/spec/server_config.go b/pkg/cluster/spec/server_config.go index 37a30b87c6..0d1de10da3 100644 --- a/pkg/cluster/spec/server_config.go +++ b/pkg/cluster/spec/server_config.go @@ -99,25 +99,19 @@ func patch(origin map[string]interface{}, key string, val interface{}) { } } -func flattenMap(ms map[string]interface{}) (map[string]interface{}, error) { +func flattenMap(ms map[string]interface{}) map[string]interface{} { result := map[string]interface{}{} for k, v := range ms { key, val := flattenKey(k, v) patch(result, key, val) } - return result, nil + return result } func merge(orig map[string]interface{}, overwrites ...map[string]interface{}) (map[string]interface{}, error) { - lhs, err := flattenMap(orig) - if err != nil { - return nil, err - } + lhs := flattenMap(orig) for _, overwrite := range overwrites { - rhs, err := flattenMap(overwrite) - if err != nil { - return nil, err - } + rhs := flattenMap(overwrite) for k, v := range rhs { patch(lhs, k, v) } @@ -125,6 +119,45 @@ func merge(orig map[string]interface{}, overwrites ...map[string]interface{}) (m return lhs, nil } +// GetValueFromPath try to find the value by path recursively +func GetValueFromPath(m map[string]interface{}, p string) interface{} { + ss := strings.Split(p, ".") + + searchMap := make(map[interface{}]interface{}) + for k, v := range m { + searchMap[k] = v + } + + return searchValue(searchMap, ss) +} + +func searchValue(m map[interface{}]interface{}, ss []string) interface{} { + l := len(ss) + switch l { + case 0: + return m + case 1: + return m[ss[0]] + } + + if m[strings.Join(ss, ".")] != nil { + return m[strings.Join(ss, ".")] + } + + for i := l - 1; i > 0; i-- { + key := strings.Join(ss[:i], ".") + if m[key] == nil { + continue + } + if pm, ok := m[key].(map[interface{}]interface{}); ok { + return searchValue(pm, ss[i:]) + } + return nil + } + + return nil +} + // Merge2Toml merge the config of global. func Merge2Toml(comp string, global, overwrite map[string]interface{}) ([]byte, error) { return merge2Toml(comp, global, overwrite) diff --git a/pkg/cluster/spec/server_config_test.go b/pkg/cluster/spec/server_config_test.go index 73e66a3a06..ab390934ea 100644 --- a/pkg/cluster/spec/server_config_test.go +++ b/pkg/cluster/spec/server_config_test.go @@ -35,3 +35,30 @@ server_configs: decimal = bytes.Contains(get, []byte("0.0")) c.Assert(decimal, check.IsTrue) } + +func (s *configSuite) TestGetValueFromPath(c *check.C) { + yamlData := []byte(` +server_configs: + tidb: + a.b.c.d: 1 + a: + b: + c: + d: 2 + a.b: + c.e: 3 + a.b.c: + f: 4 + h.i.j.k: [1, 2, 3] +`) + + topo := new(Specification) + + err := yaml.Unmarshal(yamlData, topo) + c.Assert(err, check.IsNil) + + c.Assert(GetValueFromPath(topo.ServerConfigs.TiDB, "a.b.c.d"), check.Equals, 1) + c.Assert(GetValueFromPath(topo.ServerConfigs.TiDB, "a.b.c.e"), check.Equals, nil) + c.Assert(GetValueFromPath(topo.ServerConfigs.TiDB, "a.b.c.f"), check.Equals, 4) + c.Assert(GetValueFromPath(topo.ServerConfigs.TiDB, "h.i.j.k"), check.DeepEquals, []interface{}{1, 2, 3}) +} diff --git a/pkg/cluster/spec/spec.go b/pkg/cluster/spec/spec.go index 0db8f5e452..ca7e7312d6 100644 --- a/pkg/cluster/spec/spec.go +++ b/pkg/cluster/spec/spec.go @@ -214,20 +214,7 @@ func (s *Specification) BaseTopo() *BaseTopo { func (s *Specification) LocationLabels() []string { lbs := []string{} - if pdReplica := s.ServerConfigs.PD["replication"]; pdReplica != nil { - // server_configs: - // pd: - // replication: - // location-labels: ["zone", "host"] - if repLbs := pdReplica.(map[interface{}]interface{})["location-labels"]; repLbs != nil { - for _, l := range repLbs.([]interface{}) { - lbs = append(lbs, l.(string)) - } - } - } else if repLbs := s.ServerConfigs.PD["replication.location-labels"]; repLbs != nil { - // server_configs: - // pd: - // replication.location-labels: ["zone", "host"] + if repLbs := GetValueFromPath(s.ServerConfigs.PD, "replication.location-labels"); repLbs != nil { for _, l := range repLbs.([]interface{}) { lbs = append(lbs, l.(string)) } diff --git a/pkg/cluster/spec/spec_test.go b/pkg/cluster/spec/spec_test.go index 8f85a34bf0..2b6877309c 100644 --- a/pkg/cluster/spec/spec_test.go +++ b/pkg/cluster/spec/spec_test.go @@ -171,8 +171,7 @@ tidb_servers: }, }, } - got, err := flattenMap(topo.ServerConfigs.TiDB) - c.Assert(err, IsNil) + got := flattenMap(topo.ServerConfigs.TiDB) c.Assert(got, DeepEquals, expected) buf := &bytes.Buffer{} err = toml.NewEncoder(buf).Encode(expected) @@ -200,7 +199,7 @@ tidb_servers: }, }, } - got, err = flattenMap(topo.TiDBServers[0].Config) + got = flattenMap(topo.TiDBServers[0].Config) c.Assert(err, IsNil) c.Assert(got, DeepEquals, expected) @@ -214,7 +213,7 @@ tidb_servers: }, }, } - got, err = flattenMap(topo.TiDBServers[1].Config) + got = flattenMap(topo.TiDBServers[1].Config) c.Assert(err, IsNil) c.Assert(got, DeepEquals, expected) } @@ -244,7 +243,7 @@ tikv_servers: }, }, } - got, err := flattenMap(topo.TiKVServers[0].Config) + got := flattenMap(topo.TiKVServers[0].Config) c.Assert(err, IsNil) c.Assert(got, DeepEquals, expected) } diff --git a/pkg/cluster/spec/tikv.go b/pkg/cluster/spec/tikv.go index 115eabebfd..044acc5805 100644 --- a/pkg/cluster/spec/tikv.go +++ b/pkg/cluster/spec/tikv.go @@ -124,20 +124,7 @@ func (s TiKVSpec) IsImported() bool { func (s TiKVSpec) Labels() map[string]string { lbs := make(map[string]string) - if serverCfg := s.Config["server"]; serverCfg != nil { - // server: - // labels: - // host: xxx - // zone: yyy - if labelsMap := serverCfg.(map[interface{}]interface{})["labels"]; labelsMap != nil { - for k, v := range labelsMap.(map[interface{}]interface{}) { - lbs[k.(string)] = v.(string) - } - } - } else if serverLbs := s.Config["server.labels"]; serverLbs != nil { - // server.labels: - // host: xxx - // zone: yyy + if serverLbs := GetValueFromPath(s.Config, "server.labels"); serverLbs != nil { for k, v := range serverLbs.(map[interface{}]interface{}) { lbs[k.(string)] = v.(string) } From 1fd0df3b6a603e5c932c04a176c404bf8da320b7 Mon Sep 17 00:00:00 2001 From: lucklove Date: Wed, 23 Sep 2020 10:59:15 +0800 Subject: [PATCH 11/15] Refuse instance replication.location-labels Signed-off-by: lucklove --- pkg/cluster/manager.go | 5 ++++- pkg/cluster/spec/spec.go | 15 +++++++++++++-- pkg/cluster/spec/spec_test.go | 26 ++++++++++++++++++++++---- 3 files changed, 39 insertions(+), 7 deletions(-) diff --git a/pkg/cluster/manager.go b/pkg/cluster/manager.go index f3a7f4a390..e43e230f69 100644 --- a/pkg/cluster/manager.go +++ b/pkg/cluster/manager.go @@ -1074,7 +1074,10 @@ func (m *Manager) Deploy( if topo, ok := topo.(*spec.Specification); ok && !opt.NoLabels { // Check if TiKV's label set correctly - lbs := topo.LocationLabels() + lbs, err := topo.LocationLabels() + if err != nil { + return err + } kvs := topo.TiKVServers if err := spec.CheckTiKVLocationLabels(lbs, kvs); err != nil { return perrs.Errorf("check TiKV label failed, please fix that before continue:\n%s", err) diff --git a/pkg/cluster/spec/spec.go b/pkg/cluster/spec/spec.go index ca7e7312d6..98e0cc390f 100644 --- a/pkg/cluster/spec/spec.go +++ b/pkg/cluster/spec/spec.go @@ -211,16 +211,27 @@ func (s *Specification) BaseTopo() *BaseTopo { } // LocationLabels returns replication.location-labels in PD config -func (s *Specification) LocationLabels() []string { +func (s *Specification) LocationLabels() ([]string, error) { lbs := []string{} + // We don't allow user define location-labels in instance config + for _, pd := range s.PDServers { + if GetValueFromPath(pd.Config, "replication.location-labels") != nil { + return nil, errors.Errorf( + "replication.location-labels can't be defined in instance %s:%d, please move it to the global server_configs field", + pd.Host, + pd.GetMainPort(), + ) + } + } + if repLbs := GetValueFromPath(s.ServerConfigs.PD, "replication.location-labels"); repLbs != nil { for _, l := range repLbs.([]interface{}) { lbs = append(lbs, l.(string)) } } - return lbs + return lbs, nil } // AllComponentNames contains the names of all components. diff --git a/pkg/cluster/spec/spec_test.go b/pkg/cluster/spec/spec_test.go index 2b6877309c..e40f9c2f67 100644 --- a/pkg/cluster/spec/spec_test.go +++ b/pkg/cluster/spec/spec_test.go @@ -474,15 +474,18 @@ item7 = 700 func (s *metaSuiteTopo) TestLocationLabels(c *C) { spec := Specification{} - c.Assert(len(spec.LocationLabels()), Equals, 0) + lbs, err := spec.LocationLabels() + c.Assert(err, IsNil) + c.Assert(len(lbs), Equals, 0) - err := yaml.Unmarshal([]byte(` + err = yaml.Unmarshal([]byte(` server_configs: pd: replication.location-labels: ["zone", "host"] `), &spec) c.Assert(err, IsNil) - lbs := spec.LocationLabels() + lbs, err = spec.LocationLabels() + c.Assert(err, IsNil) c.Assert(lbs, DeepEquals, []string{"zone", "host"}) spec = Specification{} @@ -495,6 +498,21 @@ server_configs: - host `), &spec) c.Assert(err, IsNil) - lbs = spec.LocationLabels() + lbs, err = spec.LocationLabels() + c.Assert(err, IsNil) c.Assert(lbs, DeepEquals, []string{"zone", "host"}) + + spec = Specification{} + err = yaml.Unmarshal([]byte(` +pd_servers: + - host: 172.16.5.140 + config: + replication: + location-labels: + - zone + - host +`), &spec) + c.Assert(err, IsNil) + _, err = spec.LocationLabels() + c.Assert(err, NotNil) } From f52f5a955281a48c2f9cbcb3e550fdd4008e802e Mon Sep 17 00:00:00 2001 From: lucklove Date: Wed, 23 Sep 2020 12:02:06 +0800 Subject: [PATCH 12/15] Fix dm Signed-off-by: lucklove --- pkg/cluster/manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cluster/manager.go b/pkg/cluster/manager.go index e43e230f69..f7492b7773 100644 --- a/pkg/cluster/manager.go +++ b/pkg/cluster/manager.go @@ -1505,7 +1505,7 @@ func (m *Manager) ScaleOut( return err } - if !opt.NoLabels { + if topo, ok := topo.(*spec.Specification); ok && !opt.NoLabels { // Check if TiKV's label set correctly pdList := topo.BaseTopo().MasterList tlsCfg, err := topo.TLSConfig(m.specManager.Path(clusterName, spec.TLSCertKeyDir)) From 8d7e5f7aa80b97c98e6d6163142287d53613ae7d Mon Sep 17 00:00:00 2001 From: SIGSEGV Date: Wed, 23 Sep 2020 16:39:38 +0800 Subject: [PATCH 13/15] Update pkg/cluster/spec/validate.go Co-authored-by: Lonng --- pkg/cluster/spec/validate.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cluster/spec/validate.go b/pkg/cluster/spec/validate.go index 2e7d81319a..5a2bf75e98 100644 --- a/pkg/cluster/spec/validate.go +++ b/pkg/cluster/spec/validate.go @@ -345,7 +345,7 @@ func CheckTiKVLocationLabels(pdLocLabels []string, kvs []TiKVSpec) error { if len(ls) == 0 && hosts[kv.Host] > 1 { lerr.TiKVInstances[id] = append( lerr.TiKVInstances[id], - errors.New("location label missing"), + errors.New("multiple TiKV instances are deployed at the same host but location label missing"), ) continue } From 65251feb5bc1bb3e7f6d4e1b90419479f878dd72 Mon Sep 17 00:00:00 2001 From: lucklove Date: Thu, 24 Sep 2020 17:28:26 +0800 Subject: [PATCH 14/15] Address comment Signed-off-by: lucklove --- pkg/cluster/api/pdapi.go | 2 +- pkg/cluster/spec/spec.go | 6 +++++- pkg/cluster/spec/tikv.go | 15 ++++++++++++--- pkg/cluster/spec/validate.go | 23 +++++++++++++++++++---- 4 files changed, 37 insertions(+), 9 deletions(-) diff --git a/pkg/cluster/api/pdapi.go b/pkg/cluster/api/pdapi.go index 15e758aba2..828f1df89f 100644 --- a/pkg/cluster/api/pdapi.go +++ b/pkg/cluster/api/pdapi.go @@ -671,7 +671,7 @@ func (pc *PDClient) GetLocationLabels() ([]string, error) { rc := pdconfig.ReplicationConfig{} if err := json.Unmarshal(config, &rc); err != nil { - return nil, errors.Annotate(err, "unmarshal replication config") + return nil, errors.Annotatef(err, "unmarshal replication config: %s", string(config)) } return rc.LocationLabels, nil diff --git a/pkg/cluster/spec/spec.go b/pkg/cluster/spec/spec.go index 98e0cc390f..b3b6b37208 100644 --- a/pkg/cluster/spec/spec.go +++ b/pkg/cluster/spec/spec.go @@ -227,7 +227,11 @@ func (s *Specification) LocationLabels() ([]string, error) { if repLbs := GetValueFromPath(s.ServerConfigs.PD, "replication.location-labels"); repLbs != nil { for _, l := range repLbs.([]interface{}) { - lbs = append(lbs, l.(string)) + lb, ok := l.(string) + if !ok { + return nil, errors.Errorf("replication.location-labels contains non-string label: %v", l) + } + lbs = append(lbs, lb) } } diff --git a/pkg/cluster/spec/tikv.go b/pkg/cluster/spec/tikv.go index 044acc5805..3dba2dc242 100644 --- a/pkg/cluster/spec/tikv.go +++ b/pkg/cluster/spec/tikv.go @@ -121,16 +121,25 @@ func (s TiKVSpec) IsImported() bool { } // Labels returns the labels of TiKV -func (s TiKVSpec) Labels() map[string]string { +func (s TiKVSpec) Labels() (map[string]string, error) { lbs := make(map[string]string) if serverLbs := GetValueFromPath(s.Config, "server.labels"); serverLbs != nil { for k, v := range serverLbs.(map[interface{}]interface{}) { - lbs[k.(string)] = v.(string) + key, ok := k.(string) + if !ok { + return nil, errors.Errorf("TiKV label name %v is not a string", k) + } + value, ok := v.(string) + if !ok { + return nil, errors.Errorf("TiKV label value %v is not a string", v) + } + + lbs[key] = value } } - return lbs + return lbs, nil } // TiKVComponent represents TiKV component. diff --git a/pkg/cluster/spec/validate.go b/pkg/cluster/spec/validate.go index 2e7d81319a..90320ef6b2 100644 --- a/pkg/cluster/spec/validate.go +++ b/pkg/cluster/spec/validate.go @@ -17,6 +17,7 @@ import ( "fmt" "path/filepath" "reflect" + "sort" "strconv" "strings" @@ -314,15 +315,26 @@ type TiKVLabelError struct { // Error implements error func (e *TiKVLabelError) Error() string { + ids := []string{} + for id := range e.TiKVInstances { + ids = append(ids, id) + } + sort.Strings(ids) + str := "" - for id, errs := range e.TiKVInstances { - if len(errs) == 0 { + for _, id := range ids { + if len(e.TiKVInstances[id]) == 0 { continue } + errs := []string{} + for _, e := range e.TiKVInstances[id] { + errs = append(errs, e.Error()) + } + sort.Strings(errs) str += fmt.Sprintf("%s:\n", id) for _, e := range errs { - str += fmt.Sprintf("\t%s\n", e.Error()) + str += fmt.Sprintf("\t%s\n", e) } } return str @@ -341,7 +353,10 @@ func CheckTiKVLocationLabels(pdLocLabels []string, kvs []TiKVSpec) error { } for _, kv := range kvs { id := fmt.Sprintf("%s:%d", kv.Host, kv.GetMainPort()) - ls := kv.Labels() + ls, err := kv.Labels() + if err != nil { + return err + } if len(ls) == 0 && hosts[kv.Host] > 1 { lerr.TiKVInstances[id] = append( lerr.TiKVInstances[id], From 01f1124a6fbef3dd96948787f405bb6cf64bbc76 Mon Sep 17 00:00:00 2001 From: lucklove Date: Thu, 24 Sep 2020 19:15:35 +0800 Subject: [PATCH 15/15] Address comment Signed-off-by: lucklove --- pkg/cluster/spec/tikv.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cluster/spec/tikv.go b/pkg/cluster/spec/tikv.go index 3dba2dc242..71c3b676db 100644 --- a/pkg/cluster/spec/tikv.go +++ b/pkg/cluster/spec/tikv.go @@ -128,11 +128,11 @@ func (s TiKVSpec) Labels() (map[string]string, error) { for k, v := range serverLbs.(map[interface{}]interface{}) { key, ok := k.(string) if !ok { - return nil, errors.Errorf("TiKV label name %v is not a string", k) + return nil, errors.Errorf("TiKV label name %v is not a string, check the instance: %s:%d", k, s.Host, s.GetMainPort()) } value, ok := v.(string) if !ok { - return nil, errors.Errorf("TiKV label value %v is not a string", v) + return nil, errors.Errorf("TiKV label value %v is not a string, check the instance: %s:%d", v, s.Host, s.GetMainPort()) } lbs[key] = value