From 8e9ea98495f3ea27d0962beaec83b1022eab973e Mon Sep 17 00:00:00 2001 From: 9547 Date: Fri, 30 Oct 2020 02:33:27 +0800 Subject: [PATCH 1/5] typo(cluster): naming --- components/cluster/command/prune.go | 2 +- pkg/cluster/operation/action.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/components/cluster/command/prune.go b/components/cluster/command/prune.go index c8854fd388..3faca3fe6f 100644 --- a/components/cluster/command/prune.go +++ b/components/cluster/command/prune.go @@ -52,7 +52,7 @@ func newPruneCmd() *cobra.Command { func destroyTombstoneIfNeed(clusterName string, metadata *spec.ClusterMeta, opt operator.Options, skipConfirm bool) error { topo := metadata.Topology - if !operator.NeedCheckTomebsome(topo) { + if !operator.NeedCheckTombstone(topo) { return nil } diff --git a/pkg/cluster/operation/action.go b/pkg/cluster/operation/action.go index 58a521967b..3e8c42c0ad 100644 --- a/pkg/cluster/operation/action.go +++ b/pkg/cluster/operation/action.go @@ -135,8 +135,8 @@ func Stop( return nil } -// NeedCheckTomebsome return true if we need to check and destroy some node. -func NeedCheckTomebsome(topo *spec.Specification) bool { +// NeedCheckTombstone return true if we need to check and destroy some node. +func NeedCheckTombstone(topo *spec.Specification) bool { for _, s := range topo.TiKVServers { if s.Offline { return true From 5e6c2c271f7ba3f7921c041f30d9ffd6a4fbc98b Mon Sep 17 00:00:00 2001 From: 9547 Date: Fri, 30 Oct 2020 03:00:09 +0800 Subject: [PATCH 2/5] cluster/spec: countdir should split dir if ',' seplit --- pkg/cluster/spec/validate.go | 35 ++++++++++++++-------- pkg/cluster/spec/validate_test.go | 48 ++++++++++++++++++++++++++++++- 2 files changed, 70 insertions(+), 13 deletions(-) diff --git a/pkg/cluster/spec/validate.go b/pkg/cluster/spec/validate.go index 20ed32da0c..7bfcf7b288 100644 --- a/pkg/cluster/spec/validate.go +++ b/pkg/cluster/spec/validate.go @@ -652,8 +652,8 @@ func (s *Specification) dirConflictsDetect() error { // prefix, useful to find potential path conflicts func (s *Specification) CountDir(targetHost, dirPrefix string) int { dirTypes := []string{ - "DataDir", "DeployDir", + "DataDir", "LogDir", } @@ -663,6 +663,14 @@ func (s *Specification) CountDir(targetHost, dirPrefix string) int { topoSpec := reflect.ValueOf(s).Elem() dirPrefix = Abs(s.GlobalOptions.User, dirPrefix) + addHostDir := func(host, deployDir, dir string) { + if !strings.HasPrefix(dir, "/") { + dir = filepath.Join(deployDir, dir) + } + dir = Abs(s.GlobalOptions.User, dir) + dirStats[host+dir]++ + } + for i := 0; i < topoSpec.NumField(); i++ { if isSkipField(topoSpec.Field(i)) { continue @@ -671,21 +679,28 @@ func (s *Specification) CountDir(targetHost, dirPrefix string) int { compSpecs := topoSpec.Field(i) for index := 0; index < compSpecs.Len(); index++ { compSpec := compSpecs.Index(index) - // Directory conflicts + deployDir := compSpec.FieldByName("DeployDir").String() + host := compSpec.FieldByName("Host").String() + for _, dirType := range dirTypes { if j, found := findField(compSpec, dirType); found { dir := compSpec.Field(j).String() - host := compSpec.FieldByName("Host").String() switch dirType { // the same as in instance.go for (*instance) + case "DeployDir": + addHostDir(host, deployDir, "") case "DataDir": - deployDir := compSpec.FieldByName("DeployDir").String() // the default data_dir is relative to deploy_dir - if dir != "" && !strings.HasPrefix(dir, "/") { - dir = filepath.Join(deployDir, dir) + if dir == "" { + addHostDir(host, deployDir, dir) + continue + } + for _, dataDir := range strings.Split(dir, ",") { + if dataDir != "" { + addHostDir(host, deployDir, dataDir) + } } case "LogDir": - deployDir := compSpec.FieldByName("DeployDir").String() field := compSpec.FieldByName("LogDir") if field.IsValid() { dir = field.Interface().(string) @@ -694,12 +709,8 @@ func (s *Specification) CountDir(targetHost, dirPrefix string) int { if dir == "" { dir = "log" } - if !strings.HasPrefix(dir, "/") { - dir = filepath.Join(deployDir, dir) - } + addHostDir(host, deployDir, dir) } - dir = Abs(s.GlobalOptions.User, dir) - dirStats[host+dir]++ } } } diff --git a/pkg/cluster/spec/validate_test.go b/pkg/cluster/spec/validate_test.go index bc1b55445d..e6fa8e6f58 100644 --- a/pkg/cluster/spec/validate_test.go +++ b/pkg/cluster/spec/validate_test.go @@ -116,7 +116,7 @@ global: user: "test1" ssh_port: 220 deploy_dir: "test-deploy" - data_dir: "test-data" + data_dir: "test-data" tidb_servers: - host: 172.16.5.138 port: 1234 @@ -700,3 +700,49 @@ tikv_servers: err = CheckTiKVLabels([]string{"zone", "host"}, &topo) c.Assert(err, IsNil) } + +func (s *metaSuiteTopo) TestCountDirMultiPath(c *C) { + topo := Specification{} + + err := yaml.Unmarshal([]byte(` +global: + user: "test1" + ssh_port: 220 + deploy_dir: "test-deploy" +tiflash_servers: + - host: 172.19.0.104 + data_dir: "/home/tidb/birdstorm/data1,/home/tidb/birdstorm/data3" +`), &topo) + c.Assert(err, IsNil) + cnt := topo.CountDir("172.19.0.104", "/home/tidb/birdstorm/data1") + c.Assert(cnt, Equals, 1) + cnt = topo.CountDir("172.19.0.104", "/home/tidb/birdstorm/data2") + c.Assert(cnt, Equals, 0) + cnt = topo.CountDir("172.19.0.104", "/home/tidb/birdstorm/data3") + c.Assert(cnt, Equals, 1) + cnt = topo.CountDir("172.19.0.104", "/home/tidb/birdstorm") + c.Assert(cnt, Equals, 2) + + err = yaml.Unmarshal([]byte(` +global: + user: "test1" + ssh_port: 220 + deploy_dir: "test-deploy" +tiflash_servers: + - host: 172.19.0.104 + data_dir: "birdstorm/data1,/birdstorm/data3" +`), &topo) + c.Assert(err, IsNil) + cnt = topo.CountDir("172.19.0.104", "/home/test1/test-deploy/tiflash-9000/birdstorm/data1") + c.Assert(cnt, Equals, 1) + cnt = topo.CountDir("172.19.0.104", "/birdstorm/data3") + c.Assert(cnt, Equals, 1) + cnt = topo.CountDir("172.19.0.104", "/home/tidb/birdstorm/data3") + c.Assert(cnt, Equals, 0) + cnt = topo.CountDir("172.19.0.104", "/home/test1/test-deploy/tiflash-9000/birdstorm/data3") + c.Assert(cnt, Equals, 0) + cnt = topo.CountDir("172.19.0.104", "/home/tidb/birdstorm") + c.Assert(cnt, Equals, 0) + cnt = topo.CountDir("172.19.0.104", "/birdstorm") + c.Assert(cnt, Equals, 1) +} From 372d919762e85567ed4fdc1b44b9135a5f3ed0ff Mon Sep 17 00:00:00 2001 From: 9547 Date: Mon, 2 Nov 2020 20:43:05 +0800 Subject: [PATCH 3/5] cluster/spec: dirConflict split data_dir --- pkg/cluster/spec/validate.go | 13 +++++++---- pkg/cluster/spec/validate_test.go | 36 +++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/pkg/cluster/spec/validate.go b/pkg/cluster/spec/validate.go index 7bfcf7b288..c2b2cf0339 100644 --- a/pkg/cluster/spec/validate.go +++ b/pkg/cluster/spec/validate.go @@ -609,18 +609,23 @@ func (s *Specification) dirConflictsDetect() error { // Directory conflicts for _, dirType := range dirTypes { - if j, found := findField(compSpec, dirType); found { + j, found := findField(compSpec, dirType) + if !found { + continue + } + + // `yaml:"data_dir,omitempty"` + tp := strings.Split(compSpec.Type().Field(j).Tag.Get("yaml"), ",")[0] + for _, part := range strings.Split(compSpec.Field(j).String(), ",") { item := usedDir{ host: host, - dir: compSpec.Field(j).String(), + dir: part, } // data_dir is relative to deploy_dir by default, so they can be with // same (sub) paths as long as the deploy_dirs are different if item.dir != "" && !strings.HasPrefix(item.dir, "/") { continue } - // `yaml:"data_dir,omitempty"` - tp := strings.Split(compSpec.Type().Field(j).Tag.Get("yaml"), ",")[0] prev, exist := dirStats[item] // not checking between imported nodes if exist && diff --git a/pkg/cluster/spec/validate_test.go b/pkg/cluster/spec/validate_test.go index e6fa8e6f58..97aca02d9a 100644 --- a/pkg/cluster/spec/validate_test.go +++ b/pkg/cluster/spec/validate_test.go @@ -746,3 +746,39 @@ tiflash_servers: cnt = topo.CountDir("172.19.0.104", "/birdstorm") c.Assert(cnt, Equals, 1) } + +func (s *metaSuiteTopo) TestDirectoryConflictsWithMultiDir(c *C) { + topo := Specification{} + + err := yaml.Unmarshal([]byte(` +global: + user: "test1" + ssh_port: 220 + deploy_dir: "test-deploy" + data_dir: "test-data" +tiflash_servers: + - host: 172.16.5.138 + data_dir: "/test-1,/test-2" +pd_servers: + - host: 172.16.5.138 + data_dir: "/test-2" +`), &topo) + c.Assert(err, NotNil) + c.Assert(err.Error(), Equals, "directory conflict for '/test-2' between 'tiflash_servers:172.16.5.138.data_dir' and 'pd_servers:172.16.5.138.data_dir'") + + err = yaml.Unmarshal([]byte(` +global: + user: "test1" + ssh_port: 220 + deploy_dir: "test-deploy" + data_dir: "test-data" +tiflash_servers: + - host: 172.16.5.138 + data_dir: "/test-1,/test-1" +pd_servers: + - host: 172.16.5.138 + data_dir: "/test-2" +`), &topo) + c.Assert(err, NotNil) + c.Assert(err.Error(), Equals, "directory conflict for '/test-1' between 'tiflash_servers:172.16.5.138.data_dir' and 'tiflash_servers:172.16.5.138.data_dir'") +} From 6fa6d4b4074e62c8b5ab4d423f90ca088df35d00 Mon Sep 17 00:00:00 2001 From: 9547 Date: Mon, 2 Nov 2020 21:19:20 +0800 Subject: [PATCH 4/5] tests/tiup-cluster: add case of tiflash scale-in/out with multi dir --- tests/tiup-cluster/script/scale_tools.sh | 20 +++++++++++++++++++ tests/tiup-cluster/topo/full.yaml.tpl | 1 + .../topo/full_scale_in_tiflash.yaml.tpl | 2 ++ 3 files changed, 23 insertions(+) create mode 100644 tests/tiup-cluster/topo/full_scale_in_tiflash.yaml.tpl diff --git a/tests/tiup-cluster/script/scale_tools.sh b/tests/tiup-cluster/script/scale_tools.sh index 4bf60ba359..80080382d5 100755 --- a/tests/tiup-cluster/script/scale_tools.sh +++ b/tests/tiup-cluster/script/scale_tools.sh @@ -79,5 +79,25 @@ function scale_tools() { # make sure grafana dashboards has been set to default (since the full_sale_in_grafana.yaml didn't provide a local dashboards dir) ! tiup-cluster $client exec $name -N $ipprefix.101 --command "grep magic-string-for-test /home/tidb/deploy/grafana-3000/dashboards/tidb.json" + # currently tiflash is not supported in TLS enabled cluster + # and only Tiflash support data-dir in multipath + if [ $test_tls = false ]; then + # ensure tiflash's data dir exists + tiup-cluster $client exec $name -N $ipprefix.103 --command "ls /home/tidb/deploy/tiflash-9000/data1" + tiup-cluster $client exec $name -N $ipprefix.103 --command "ls /data/tiflash-data" + echo "start scale in tiflash" + tiup-cluster $client --yes scale-in $name -N $ipprefix.103:9000 + tiup-cluster $client display $name | grep Tombstone + echo "start prune tiflash" + yes | tiup-cluster $client prune $name + wait_instance_num_reach $name $total_sub_one $native_ssh + ! tiup-cluster $client exec $name -N $ipprefix.103 --command "ls /home/tidb/deploy/tiflash-9000/data1" + ! tiup-cluster $client exec $name -N $ipprefix.103 --command "ls /data/tiflash-data" + echo "start scale out tiflash" + topo=./topo/full_scale_in_tiflash.yaml + sed "s/__IPPREFIX__/$ipprefix/g" $topo.tpl > $topo + tiup-cluster $client --yes scale-out $name $topo + fi + tiup-cluster $client _test $name writable } diff --git a/tests/tiup-cluster/topo/full.yaml.tpl b/tests/tiup-cluster/topo/full.yaml.tpl index 39fd742d52..e6298c1710 100644 --- a/tests/tiup-cluster/topo/full.yaml.tpl +++ b/tests/tiup-cluster/topo/full.yaml.tpl @@ -33,6 +33,7 @@ tikv_servers: # and binary is more than 1G.. tiflash_servers: - host: __IPPREFIX__.103 + data_dir: "data1,/data/tiflash-data" # - host: __IPPREFIX__.104 # - host: __IPPREFIX__.105 diff --git a/tests/tiup-cluster/topo/full_scale_in_tiflash.yaml.tpl b/tests/tiup-cluster/topo/full_scale_in_tiflash.yaml.tpl new file mode 100644 index 0000000000..b4e4efc9c9 --- /dev/null +++ b/tests/tiup-cluster/topo/full_scale_in_tiflash.yaml.tpl @@ -0,0 +1,2 @@ +tiflash_servers: + - host: __IPPREFIX__.103 From 60411161913289ca640370a7dd65b1ea84847cf5 Mon Sep 17 00:00:00 2001 From: 9547 Date: Mon, 2 Nov 2020 21:43:53 +0800 Subject: [PATCH 5/5] cluster/spec: trim dataDir's space before count --- pkg/cluster/spec/validate.go | 59 +++++++++++++++++-------------- pkg/cluster/spec/validate_test.go | 4 +-- 2 files changed, 34 insertions(+), 29 deletions(-) diff --git a/pkg/cluster/spec/validate.go b/pkg/cluster/spec/validate.go index c2b2cf0339..2f423df854 100644 --- a/pkg/cluster/spec/validate.go +++ b/pkg/cluster/spec/validate.go @@ -616,10 +616,11 @@ func (s *Specification) dirConflictsDetect() error { // `yaml:"data_dir,omitempty"` tp := strings.Split(compSpec.Type().Field(j).Tag.Get("yaml"), ",")[0] - for _, part := range strings.Split(compSpec.Field(j).String(), ",") { + for _, dir := range strings.Split(compSpec.Field(j).String(), ",") { + dir = strings.TrimSpace(dir) item := usedDir{ host: host, - dir: part, + dir: dir, } // data_dir is relative to deploy_dir by default, so they can be with // same (sub) paths as long as the deploy_dirs are different @@ -688,34 +689,38 @@ func (s *Specification) CountDir(targetHost, dirPrefix string) int { host := compSpec.FieldByName("Host").String() for _, dirType := range dirTypes { - if j, found := findField(compSpec, dirType); found { - dir := compSpec.Field(j).String() - - switch dirType { // the same as in instance.go for (*instance) - case "DeployDir": - addHostDir(host, deployDir, "") - case "DataDir": - // the default data_dir is relative to deploy_dir - if dir == "" { - addHostDir(host, deployDir, dir) - continue - } - for _, dataDir := range strings.Split(dir, ",") { - if dataDir != "" { - addHostDir(host, deployDir, dataDir) - } - } - case "LogDir": - field := compSpec.FieldByName("LogDir") - if field.IsValid() { - dir = field.Interface().(string) - } + j, found := findField(compSpec, dirType) + if !found { + continue + } - if dir == "" { - dir = "log" - } + dir := compSpec.Field(j).String() + + switch dirType { // the same as in instance.go for (*instance) + case "DeployDir": + addHostDir(host, deployDir, "") + case "DataDir": + // the default data_dir is relative to deploy_dir + if dir == "" { addHostDir(host, deployDir, dir) + continue + } + for _, dataDir := range strings.Split(dir, ",") { + dataDir = strings.TrimSpace(dataDir) + if dataDir != "" { + addHostDir(host, deployDir, dataDir) + } + } + case "LogDir": + field := compSpec.FieldByName("LogDir") + if field.IsValid() { + dir = field.Interface().(string) + } + + if dir == "" { + dir = "log" } + addHostDir(host, deployDir, strings.TrimSpace(dir)) } } } diff --git a/pkg/cluster/spec/validate_test.go b/pkg/cluster/spec/validate_test.go index 97aca02d9a..717d0a5ef9 100644 --- a/pkg/cluster/spec/validate_test.go +++ b/pkg/cluster/spec/validate_test.go @@ -711,7 +711,7 @@ global: deploy_dir: "test-deploy" tiflash_servers: - host: 172.19.0.104 - data_dir: "/home/tidb/birdstorm/data1,/home/tidb/birdstorm/data3" + data_dir: "/home/tidb/birdstorm/data1, /home/tidb/birdstorm/data3" `), &topo) c.Assert(err, IsNil) cnt := topo.CountDir("172.19.0.104", "/home/tidb/birdstorm/data1") @@ -758,7 +758,7 @@ global: data_dir: "test-data" tiflash_servers: - host: 172.16.5.138 - data_dir: "/test-1,/test-2" + data_dir: " /test-1, /test-2" pd_servers: - host: 172.16.5.138 data_dir: "/test-2"