From 241315cd4ca7473901a9adaff16654f771a7ea7a Mon Sep 17 00:00:00 2001 From: lucklove Date: Thu, 29 Oct 2020 19:33:03 +0800 Subject: [PATCH 1/2] Fix labels warning Fix https://github.com/pingcap/tiup/issues/863 --- pkg/cluster/api/pdapi.go | 6 ++++++ pkg/cluster/spec/validate.go | 18 +++++++++++++----- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/pkg/cluster/api/pdapi.go b/pkg/cluster/api/pdapi.go index 62f01280fe..56808ef285 100644 --- a/pkg/cluster/api/pdapi.go +++ b/pkg/cluster/api/pdapi.go @@ -685,6 +685,9 @@ func (pc *PDClient) StoreList() ([]string, error) { } addrs := []string{} for _, s := range r.Stores { + if s.Store.StateName != "Up" { + continue + } addrs = append(addrs, s.Store.GetAddress()) } return addrs, nil @@ -698,6 +701,9 @@ func (pc *PDClient) GetStoreLabels(address string) (map[string]string, error) { } for _, s := range r.Stores { + if s.Store.StateName != "Up" { + continue + } if address == s.Store.GetAddress() { lbs := s.Store.GetLabels() labels := map[string]string{} diff --git a/pkg/cluster/spec/validate.go b/pkg/cluster/spec/validate.go index 41265df75f..e3cdc737fc 100644 --- a/pkg/cluster/spec/validate.go +++ b/pkg/cluster/spec/validate.go @@ -361,16 +361,24 @@ func CheckTiKVLocationLabels(pdLocLabels []string, slp StoreLabelProvider) error if err != nil { return err } + + storeLabels := map[string]map[string]string{} for _, kv := range kvs { - host := getHostFromAddress(kv) - hosts[host] = hosts[host] + 1 - } - for _, kv := range kvs { - host := getHostFromAddress(kv) ls, err := slp.GetStoreLabels(kv) if err != nil { return err } + // Skip tiflash + if ls["engine"] == "tiflash" { + continue + } + storeLabels[kv] = ls + host := getHostFromAddress(kv) + hosts[host] = hosts[host] + 1 + } + + for kv, ls := range storeLabels { + host := getHostFromAddress(kv) if len(ls) == 0 && hosts[host] > 1 { lerr.TiKVInstances[kv] = append( lerr.TiKVInstances[kv], From 1c26507dfe14539d1d0dc760adb980af9fa5e307 Mon Sep 17 00:00:00 2001 From: lucklove Date: Fri, 30 Oct 2020 15:53:00 +0800 Subject: [PATCH 2/2] Address comment Signed-off-by: lucklove --- pkg/cluster/api/pdapi.go | 39 +++++++++++-------------------- pkg/cluster/manager.go | 6 ++--- pkg/cluster/spec/spec.go | 31 +++++++----------------- pkg/cluster/spec/validate.go | 26 ++++++--------------- pkg/cluster/spec/validate_test.go | 12 +++++----- 5 files changed, 37 insertions(+), 77 deletions(-) diff --git a/pkg/cluster/api/pdapi.go b/pkg/cluster/api/pdapi.go index 56808ef285..e107d5c26e 100644 --- a/pkg/cluster/api/pdapi.go +++ b/pkg/cluster/api/pdapi.go @@ -677,43 +677,30 @@ func (pc *PDClient) GetLocationLabels() ([]string, error) { return rc.LocationLabels, nil } -// StoreList implements StoreLabelProvider -func (pc *PDClient) StoreList() ([]string, error) { - r, err := pc.GetStores() - if err != nil { - return nil, err - } - addrs := []string{} - for _, s := range r.Stores { - if s.Store.StateName != "Up" { - continue - } - addrs = append(addrs, s.Store.GetAddress()) - } - return addrs, nil -} - -// GetStoreLabels implements StoreLabelProvider -func (pc *PDClient) GetStoreLabels(address string) (map[string]string, error) { +// GetTiKVLabels implements TiKVLabelProvider +func (pc *PDClient) GetTiKVLabels() (map[string]map[string]string, error) { r, err := pc.GetStores() if err != nil { return nil, err } + locationLabels := map[string]map[string]string{} for _, s := range r.Stores { if s.Store.StateName != "Up" { continue } - if address == s.Store.GetAddress() { - lbs := s.Store.GetLabels() - labels := map[string]string{} - for _, lb := range lbs { - labels[lb.GetKey()] = lb.GetValue() - } - return labels, nil + lbs := s.Store.GetLabels() + labels := map[string]string{} + for _, lb := range lbs { + labels[lb.GetKey()] = lb.GetValue() + } + // Skip tiflash + if labels["engine"] == "tiflash" { + continue } + locationLabels[s.Store.GetAddress()] = labels } - return nil, errors.Errorf("store %s not found", address) + return locationLabels, nil } // UpdateScheduleConfig updates the PD schedule config diff --git a/pkg/cluster/manager.go b/pkg/cluster/manager.go index 9988f46c45..0066aaa5b0 100644 --- a/pkg/cluster/manager.go +++ b/pkg/cluster/manager.go @@ -604,7 +604,7 @@ func (m *Manager) Display(clusterName string, opt operator.Options) error { 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, pdClient); err != nil { + } else if err := spec.CheckTiKVLabels(lbs, pdClient); err != nil { color.Yellow("\nWARN: there is something wrong with TiKV labels, which may cause data losing:\n%v", err) } @@ -1087,7 +1087,7 @@ func (m *Manager) Deploy( if err != nil { return err } - if err := spec.CheckTiKVLocationLabels(lbs, topo); err != nil { + if err := spec.CheckTiKVLabels(lbs, topo); err != nil { return perrs.Errorf("check TiKV label failed, please fix that before continue:\n%s", err) } } @@ -1526,7 +1526,7 @@ func (m *Manager) ScaleOut( if err != nil { return err } - if err := spec.CheckTiKVLocationLabels(lbs, mergedTopo.(*spec.Specification)); err != nil { + if err := spec.CheckTiKVLabels(lbs, mergedTopo.(*spec.Specification)); 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 44cffd54b0..ec7452d84a 100644 --- a/pkg/cluster/spec/spec.go +++ b/pkg/cluster/spec/spec.go @@ -237,33 +237,18 @@ func (s *Specification) LocationLabels() ([]string, error) { return lbs, nil } -// StoreList implements StoreLabelProvider -func (s *Specification) StoreList() ([]string, error) { +// GetTiKVLabels implements TiKVLabelProvider +func (s *Specification) GetTiKVLabels() (map[string]map[string]string, error) { kvs := s.TiKVServers - addrs := []string{} + locationLabels := map[string]map[string]string{} for _, kv := range kvs { - if kv.IsImported() { - // FIXME: this function implements StoreLabelProvider, which is used to - // detect if the label config is missing. However, we do that - // base on the meta.yaml, whose server.labels field is empty - // for imported TiKV servers, to workaround that, we just skip the - // imported TiKV servers at this time. - continue + address := fmt.Sprintf("%s:%d", kv.Host, kv.GetMainPort()) + var err error + if locationLabels[address], err = kv.Labels(); err != nil { + return nil, err } - addrs = append(addrs, fmt.Sprintf("%s:%d", kv.Host, kv.GetMainPort())) } - return addrs, nil -} - -// GetStoreLabels implements StoreLabelProvider -func (s *Specification) GetStoreLabels(address string) (map[string]string, error) { - kvs := s.TiKVServers - for _, kv := range kvs { - if address == fmt.Sprintf("%s:%d", kv.Host, kv.GetMainPort()) { - return kv.Labels() - } - } - return nil, errors.Errorf("store %s not found", address) + return locationLabels, nil } // AllComponentNames contains the names of all components. diff --git a/pkg/cluster/spec/validate.go b/pkg/cluster/spec/validate.go index e3cdc737fc..20ed32da0c 100644 --- a/pkg/cluster/spec/validate.go +++ b/pkg/cluster/spec/validate.go @@ -339,40 +339,28 @@ func (e *TiKVLabelError) Error() string { return str } -// StoreLabelProvider provide store labels information -type StoreLabelProvider interface { - StoreList() ([]string, error) - GetStoreLabels(address string) (map[string]string, error) +// TiKVLabelProvider provide store labels information +type TiKVLabelProvider interface { + GetTiKVLabels() (map[string]map[string]string, error) } func getHostFromAddress(addr string) string { return strings.Split(addr, ":")[0] } -// CheckTiKVLocationLabels will check if tikv missing label or have wrong label -func CheckTiKVLocationLabels(pdLocLabels []string, slp StoreLabelProvider) error { +// CheckTiKVLabels will check if tikv missing label or have wrong label +func CheckTiKVLabels(pdLocLabels []string, slp TiKVLabelProvider) error { lerr := &TiKVLabelError{ TiKVInstances: make(map[string][]error), } lbs := set.NewStringSet(pdLocLabels...) hosts := make(map[string]int) - kvs, err := slp.StoreList() + storeLabels, err := slp.GetTiKVLabels() if err != nil { return err } - - storeLabels := map[string]map[string]string{} - for _, kv := range kvs { - ls, err := slp.GetStoreLabels(kv) - if err != nil { - return err - } - // Skip tiflash - if ls["engine"] == "tiflash" { - continue - } - storeLabels[kv] = ls + for kv := range storeLabels { host := getHostFromAddress(kv) hosts[host] = hosts[host] + 1 } diff --git a/pkg/cluster/spec/validate_test.go b/pkg/cluster/spec/validate_test.go index 2c5b3f590a..bc1b55445d 100644 --- a/pkg/cluster/spec/validate_test.go +++ b/pkg/cluster/spec/validate_test.go @@ -620,9 +620,9 @@ tikv_servers: status_port: 20180 `), &topo) c.Assert(err, IsNil) - err = CheckTiKVLocationLabels(nil, &topo) + err = CheckTiKVLabels(nil, &topo) c.Assert(err, IsNil) - err = CheckTiKVLocationLabels([]string{}, &topo) + err = CheckTiKVLabels([]string{}, &topo) c.Assert(err, IsNil) // 2 tikv on the same host without label @@ -637,7 +637,7 @@ tikv_servers: status_port: 20181 `), &topo) c.Assert(err, IsNil) - err = CheckTiKVLocationLabels(nil, &topo) + err = CheckTiKVLabels(nil, &topo) c.Assert(err, NotNil) // 2 tikv on the same host with unacquainted label @@ -656,7 +656,7 @@ tikv_servers: server.labels: { zone: "zone1", host: "172.16.5.140" } `), &topo) c.Assert(err, IsNil) - err = CheckTiKVLocationLabels(nil, &topo) + err = CheckTiKVLabels(nil, &topo) c.Assert(err, NotNil) // 2 tikv on the same host with correct label @@ -675,7 +675,7 @@ tikv_servers: server.labels: { zone: "zone1", host: "172.16.5.140" } `), &topo) c.Assert(err, IsNil) - err = CheckTiKVLocationLabels([]string{"zone", "host"}, &topo) + err = CheckTiKVLabels([]string{"zone", "host"}, &topo) c.Assert(err, IsNil) // 2 tikv on the same host with diffrent config style @@ -697,6 +697,6 @@ tikv_servers: host: "172.16.5.140" `), &topo) c.Assert(err, IsNil) - err = CheckTiKVLocationLabels([]string{"zone", "host"}, &topo) + err = CheckTiKVLabels([]string{"zone", "host"}, &topo) c.Assert(err, IsNil) }