Fix dataDir not deleted if containing comma (#871)

* typo(cluster): naming

* cluster/spec: countdir should split dir if ',' seplit

* cluster/spec: dirConflict split data_dir

* tests/tiup-cluster: add case of tiflash scale-in/out with multi dir

* cluster/spec: trim dataDir's space before count

Co-authored-by: ti-srebot <66930949+ti-srebot@users.noreply.github.com>

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
 	}
 

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

pkg/cluster/spec/validate.go

@@ -609,18 +609,24 @@ 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 _, dir := range strings.Split(compSpec.Field(j).String(), ",") {
+					dir = strings.TrimSpace(dir)
 					item := usedDir{
 						host: host,
-						dir:  compSpec.Field(j).String(),
+						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
 					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 &&
@@ -652,8 +658,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 +669,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,35 +685,42 @@ 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 "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)
-						}
-					case "LogDir":
-						deployDir := compSpec.FieldByName("DeployDir").String()
-						field := compSpec.FieldByName("LogDir")
-						if field.IsValid() {
-							dir = field.Interface().(string)
-						}
+				j, found := findField(compSpec, dirType)
+				if !found {
+					continue
+				}
 
-						if dir == "" {
-							dir = "log"
-						}
-						if !strings.HasPrefix(dir, "/") {
-							dir = filepath.Join(deployDir, dir)
+				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)
 						}
 					}
-					dir = Abs(s.GlobalOptions.User, dir)
-					dirStats[host+dir]++
+				case "LogDir":
+					field := compSpec.FieldByName("LogDir")
+					if field.IsValid() {
+						dir = field.Interface().(string)
+					}
+
+					if dir == "" {
+						dir = "log"
+					}
+					addHostDir(host, deployDir, strings.TrimSpace(dir))
 				}
 			}
 		}

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,85 @@ 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)
+}
+
+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'")
+}

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
 }

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
 

tests/tiup-cluster/topo/full_scale_in_tiflash.yaml.tpl

@@ -0,0 +1,2 @@
+tiflash_servers:
+  - host: __IPPREFIX__.103