From 1f0cbacd392f218840480bae8e6185acc095b38d Mon Sep 17 00:00:00 2001 From: JaySon Date: Wed, 25 Nov 2020 03:44:30 -0600 Subject: [PATCH] Support deploy TiFlash on multi-disks with "storage" configurations since v4.0.9 (#931) --- examples/topology.example.yaml | 57 ++--- pkg/cluster/spec/parse_topology_test.go | 144 +++++++++++++ pkg/cluster/spec/spec.go | 14 ++ pkg/cluster/spec/spec_test.go | 126 +++++++++++ pkg/cluster/spec/tiflash.go | 273 +++++++++++++++++++++--- pkg/cluster/spec/validate.go | 14 ++ pkg/cluster/spec/validate_test.go | 81 +++++++ pkg/utils/diff.go | 42 +++- pkg/utils/diff_test.go | 44 ++++ 9 files changed, 731 insertions(+), 64 deletions(-) diff --git a/examples/topology.example.yaml b/examples/topology.example.yaml index 6d40d87ef7..e2aab6540f 100644 --- a/examples/topology.example.yaml +++ b/examples/topology.example.yaml @@ -418,8 +418,10 @@ server_configs: # - key: "dc" # value: "sha" tiflash: - # path_realtime_mode: false logger.level: "info" + ## Deprecated multi-disks storage path setting style since v4.0.9, + ## check storage.* configurations in host level for new style. + # path_realtime_mode: false tiflash-learner: log-level: "info" # raftstore.apply-pool-size: 4 @@ -488,37 +490,40 @@ tiflash_servers: # flash_proxy_status_port: 20292 # metrics_port: 8234 # deploy_dir: /tidb-deploy/tiflash-9000 + ## With cluster version >= v4.0.9 and you want to deploy a multi-disk TiFlash node, it is recommended to + ## check config.storage.* for details. The data_dir will be ignored if you defined those configurations. + ## Setting data_dir to a ','-joined string is still supported but deprecated. + ## Check https://docs.pingcap.com/tidb/stable/tiflash-configuration#multi-disk-deployment for more details. # data_dir: /tidb-data/tiflash-9000 # log_dir: /tidb-deploy/tiflash-9000/log # numa_node: "0,1" - # # - # # `path_realtime_mode` only works if `data_dir` is specified with multiple paths. - # # - # # path_realtime_mode: - # # "true" means only other paths instead of first path can be used to store older data. - # # "false" means all paths can be used to store older data. - # # - # # TiFlash only uses the first path to store the latest data (i.e. "delta"). And others for the older data (i.e. "stable". which is the majority of data), - # # - # # E.g, if you intend to use an fast and smaller NVMe SSD (256GB) to speed up data ingestion speed in TiFlash, - # # and 4 extra normal SSDs (512GB each) for main storage. Then your configurations should be look like: - # # - # # data_dir: /nvme_ssd_256/data,/ssd1_512/data,/ssd2_512/data,/ssd3_512/data,/ssd4_512/data - # # config: - # # path_realtime_mode: true - # # - # # - # # And if your first disk is big enough, to fully use the capacity of it, use configurations look like: - # # - # # data_dir: /nvme_ssd_512/data,/ssd1_512/data,/ssd2_512/data,/ssd3_512/data,/ssd4_512/data - # # config: - # # path_realtime_mode: false - # # - # # # # The following configs are used to overwrite the `server_configs.tiflash` values. # config: - # path_realtime_mode: false # logger.level: "info" + # ## Deprecated multi-disks storage path setting. Deprecated since v4.0.9, + # ## check following storage.* configurations for new style. + # # path_realtime_mode: false + # # + # ## Multi-disks storage paths settings. These will overwrite the `data_dir`. + # ## If there are multiple SSD disks on the machine, + # ## specify the path list on `storage.main.dir` can improve TiFlash performance. + # # storage.main.dir: [ "/nvme_ssd0_512/tiflash", "/nvme_ssd1_512/tiflash" ] + # ## Store capacity of each path, i.e. max data size allowed. + # ## If it is not set, or is set to 0s, the actual disk capacity is used. + # # storage.main.capacity = [ 536870912000, 536870912000 ] + # # + # ## If there are multiple disks with different IO metrics (e.g. one SSD and some HDDs) + # ## on the machine, + # ## set `storage.latest.dir` to store the latest data on SSD (disks with higher IOPS metrics) + # ## set `storage.main.dir` to store the main data on HDD (disks with lower IOPS metrics) + # ## can improve TiFlash performance. + # # storage.main.dir: [ "/hdd0_512/tiflash", "/hdd1_512/tiflash", "/hdd2_512/tiflash" ] + # # storage.latest.dir: [ "/nvme_ssd0_100/tiflash" ] + # ## Store capacity of each path, i.e. max data size allowed. + # ## If it is not set, or is set to 0s, the actual disk capacity is used. + # # storage.main.capacity = [ 536870912000, 536870912000 ] + # # storage.latest.capacity = [ 107374182400 ] + # # # The following configs are used to overwrite the `server_configs.tiflash-learner` values. # learner_config: # log-level: "info" diff --git a/pkg/cluster/spec/parse_topology_test.go b/pkg/cluster/spec/parse_topology_test.go index a28e02a2ce..268f167bb2 100644 --- a/pkg/cluster/spec/parse_topology_test.go +++ b/pkg/cluster/spec/parse_topology_test.go @@ -268,6 +268,150 @@ tikv_servers: }) } +func (s *topoSuite) TestTiFlashStorage(c *check.C) { + // test tiflash storage section, 'storage.main.dir' should not be defined in server_configs + withTempFile(` +server_configs: + tiflash: + storage.main.dir: [/data1/tiflash] +tiflash_servers: + - host: 172.16.5.140 + data_dir: /ssd0/tiflash,/ssd1/tiflash,/ssd2/tiflash + config: + storage.main.dir: [/ssd0/tiflash, /ssd1/tiflash, /ssd2/tiflash] + storage.latest.dir: [/ssd0/tiflash, /ssd1/tiflash, /ssd2/tiflash] +`, func(file string) { + topo := Specification{} + err := ParseTopologyYaml(file, &topo) + c.Assert(err, check.NotNil) + }) + + // test tiflash storage section, 'storage.latest.dir' should not be defined in server_configs + withTempFile(` +server_configs: + tiflash: + storage.latest.dir: [/data1/tiflash] +tiflash_servers: + - host: 172.16.5.140 + data_dir: /ssd0/tiflash,/ssd1/tiflash,/ssd2/tiflash + config: + storage.main.dir: [/ssd0/tiflash, /ssd1/tiflash, /ssd2/tiflash] + storage.latest.dir: [/ssd0/tiflash, /ssd1/tiflash, /ssd2/tiflash] +`, func(file string) { + topo := Specification{} + err := ParseTopologyYaml(file, &topo) + c.Assert(err, check.NotNil) + }) + + // test tiflash storage section defined data dir + // test for depreacated setting, for backward compatibility + withTempFile(` +tiflash_servers: + - host: 172.16.5.140 + data_dir: /ssd0/tiflash + config: +`, func(file string) { + topo := Specification{} + err := ParseTopologyYaml(file, &topo) + c.Assert(err, check.IsNil) + ExpandRelativeDir(&topo) + + c.Assert(topo.TiFlashServers[0].DeployDir, check.Equals, "/home/tidb/deploy/tiflash-9000") + c.Assert(topo.TiFlashServers[0].DataDir, check.Equals, "/ssd0/tiflash") + c.Assert(topo.TiFlashServers[0].LogDir, check.Equals, "") + }) + + // test tiflash storage section defined data dir + withTempFile(` +tiflash_servers: + - host: 172.16.5.140 + data_dir: /ssd0/tiflash,/ssd1/tiflash,/ssd2/tiflash + config: + storage.main.dir: [/ssd0/tiflash, /ssd1/tiflash, /ssd2/tiflash] + storage.latest.dir: [/ssd0/tiflash, /ssd1/tiflash, /ssd2/tiflash] +`, func(file string) { + topo := Specification{} + err := ParseTopologyYaml(file, &topo) + c.Assert(err, check.IsNil) + ExpandRelativeDir(&topo) + + c.Assert(topo.TiFlashServers[0].DeployDir, check.Equals, "/home/tidb/deploy/tiflash-9000") + c.Assert(topo.TiFlashServers[0].DataDir, check.Equals, "/ssd0/tiflash,/ssd1/tiflash,/ssd2/tiflash") + c.Assert(topo.TiFlashServers[0].LogDir, check.Equals, "") + }) + + // test tiflash storage section defined data dir, "data_dir" will be ignored + withTempFile(` +tiflash_servers: + - host: 172.16.5.140 + # if storage.main.dir is defined, data_dir will be ignored + data_dir: /hdd0/tiflash + config: + storage.main.dir: [/ssd0/tiflash, /ssd1/tiflash, /ssd2/tiflash] +`, func(file string) { + topo := Specification{} + err := ParseTopologyYaml(file, &topo) + c.Assert(err, check.IsNil) + ExpandRelativeDir(&topo) + + c.Assert(topo.TiFlashServers[0].DeployDir, check.Equals, "/home/tidb/deploy/tiflash-9000") + c.Assert(topo.TiFlashServers[0].DataDir, check.Equals, "/ssd0/tiflash,/ssd1/tiflash,/ssd2/tiflash") + c.Assert(topo.TiFlashServers[0].LogDir, check.Equals, "") + }) + + // test tiflash storage section defined data dir + // if storage.latest.dir is not empty, the first path in + // storage.latest.dir will be the first path in 'DataDir' + // DataDir is the union set of storage.latest.dir and storage.main.dir + withTempFile(` +tiflash_servers: + - host: 172.16.5.140 + data_dir: /ssd0/tiflash + config: + storage.main.dir: [/hdd0/tiflash, /hdd1/tiflash, /hdd2/tiflash] + storage.latest.dir: [/ssd0/tiflash, /ssd1/tiflash, /ssd2/tiflash, /hdd0/tiflash] +`, func(file string) { + topo := Specification{} + err := ParseTopologyYaml(file, &topo) + c.Assert(err, check.IsNil) + ExpandRelativeDir(&topo) + + c.Assert(topo.TiFlashServers[0].DeployDir, check.Equals, "/home/tidb/deploy/tiflash-9000") + c.Assert(topo.TiFlashServers[0].DataDir, check.Equals, "/ssd0/tiflash,/hdd0/tiflash,/hdd1/tiflash,/hdd2/tiflash,/ssd1/tiflash,/ssd2/tiflash") + c.Assert(topo.TiFlashServers[0].LogDir, check.Equals, "") + }) + + // test tiflash storage section defined data dir + // should always define storage.main.dir if any 'storage.*' is defined + withTempFile(` +tiflash_servers: + - host: 172.16.5.140 + data_dir: /ssd0/tiflash + config: + #storage.main.dir: [/hdd0/tiflash, /hdd1/tiflash, /hdd2/tiflash] + storage.latest.dir: [/ssd0/tiflash, /ssd1/tiflash, /ssd2/tiflash, /hdd0/tiflash] +`, func(file string) { + topo := Specification{} + err := ParseTopologyYaml(file, &topo) + c.Assert(err, check.NotNil) + }) + + // test tiflash storage section defined data dir + // storage.main.dir should always use absolute path + withTempFile(` +tiflash_servers: + - host: 172.16.5.140 + data_dir: /ssd0/tiflash + config: + storage.main.dir: [tiflash/data, ] + storage.latest.dir: [/ssd0/tiflash, /ssd1/tiflash, /ssd2/tiflash, /hdd0/tiflash] +`, func(file string) { + topo := Specification{} + err := ParseTopologyYaml(file, &topo) + c.Assert(err, check.NotNil) + }) +} + func merge4test(base, scale string) (*Specification, error) { baseTopo := Specification{} if err := ParseTopologyYaml(base, &baseTopo); err != nil { diff --git a/pkg/cluster/spec/spec.go b/pkg/cluster/spec/spec.go index d0a700b33c..c67537a6bd 100644 --- a/pkg/cluster/spec/spec.go +++ b/pkg/cluster/spec/spec.go @@ -25,6 +25,7 @@ import ( "github.com/pingcap/errors" "github.com/pingcap/tiup/pkg/cluster/executor" "github.com/pingcap/tiup/pkg/cluster/template/scripts" + "github.com/pingcap/tiup/pkg/logger/log" "github.com/pingcap/tiup/pkg/meta" "go.etcd.io/etcd/clientv3" ) @@ -309,6 +310,19 @@ func (s *Specification) UnmarshalYAML(unmarshal func(interface{}) error) error { return err } + // Rewrite TiFlashSpec.DataDir since we may override it with configurations. + // Should do it before validatation because we need to detect dir conflicts. + for i := 0; i < len(s.TiFlashServers); i++ { + dataDir, err := s.TiFlashServers[i].GetOverrideDataDir() + if err != nil { + return err + } + if s.TiFlashServers[i].DataDir != dataDir { + log.Infof("'tiflash_server:%s.data_dir' is overwritten by its storage configuration. Now the data_dir is %s", s.TiFlashServers[i].Host, dataDir) + s.TiFlashServers[i].DataDir = dataDir + } + } + return s.Validate() } diff --git a/pkg/cluster/spec/spec_test.go b/pkg/cluster/spec/spec_test.go index eb91fbb7a2..4b6aed7bcb 100644 --- a/pkg/cluster/spec/spec_test.go +++ b/pkg/cluster/spec/spec_test.go @@ -19,6 +19,7 @@ import ( "github.com/BurntSushi/toml" . "github.com/pingcap/check" + "github.com/pingcap/tiup/pkg/cluster/template/scripts" "gopkg.in/yaml.v2" ) @@ -557,3 +558,128 @@ pd_servers: _, err = spec.LocationLabels() c.Assert(err, NotNil) } + +func (s *metaSuiteTopo) TestTiFlashStorageSection(c *C) { + spec := &Specification{} + err := yaml.Unmarshal([]byte(` +tiflash_servers: + - host: 172.16.5.138 + data_dir: /hdd0/tiflash,/hdd1/tiflash + config: + storage.main.dir: [/ssd0/tiflash, /ssd1/tiflash] + storage.latest.dir: [/ssd0/tiflash] +`), spec) + c.Assert(err, IsNil) + + flashComp := FindComponent(spec, ComponentTiFlash) + instances := flashComp.Instances() + c.Assert(len(instances), Equals, 1) + // parse using clusterVersion<"v4.0.9" + { + ins := instances[0] + // This should be the same with tiflash_server instance's "data_dir" + dataDir := "/hdd0/tiflash,/hdd1/tiflash" + cfg := scripts.NewTiFlashScript(ins.GetHost(), "", dataDir, "", "", "") + conf, err := ins.(*TiFlashInstance).initTiFlashConfig(cfg, "v4.0.8", spec.ServerConfigs.TiFlash) + c.Assert(err, IsNil) + + path, ok := conf["path"] + c.Assert(ok, IsTrue) + c.Assert(path, Equals, dataDir) + } + // parse using clusterVersion>="v4.0.9" + checkWithVersion := func(ver string) { + ins := instances[0].(*TiFlashInstance) + dataDir := "/ssd0/tiflash" + cfg := scripts.NewTiFlashScript(ins.GetHost(), "", dataDir, "", "", "") + conf, err := ins.initTiFlashConfig(cfg, ver, spec.ServerConfigs.TiFlash) + c.Assert(err, IsNil) + + _, ok := conf["path"] + c.Assert(ok, IsTrue) + + // After merging instance configurations with "storgae", the "path" property should be removed. + conf, err = ins.mergeTiFlashInstanceConfig(ver, conf, ins.InstanceSpec.(TiFlashSpec).Config) + c.Assert(err, IsNil) + _, ok = conf["path"] + c.Assert(ok, IsFalse) + + if storageSection, ok := conf["storage"]; ok { + if mainSection, ok := storageSection.(map[string]interface{})["main"]; ok { + if mainDirsSection, ok := mainSection.(map[string]interface{})["dir"]; ok { + var mainDirs []interface{} = mainDirsSection.([]interface{}) + c.Assert(len(mainDirs), Equals, 2) + c.Assert(mainDirs[0].(string), Equals, "/ssd0/tiflash") + c.Assert(mainDirs[1].(string), Equals, "/ssd1/tiflash") + } else { + c.Error("Can not get storage.main.dir section") + } + } else { + c.Error("Can not get storage.main section") + } + if latestSection, ok := storageSection.(map[string]interface{})["latest"]; ok { + if latestDirsSection, ok := latestSection.(map[string]interface{})["dir"]; ok { + var latestDirs []interface{} = latestDirsSection.([]interface{}) + c.Assert(len(latestDirs), Equals, 1) + c.Assert(latestDirs[0].(string), Equals, "/ssd0/tiflash") + } else { + c.Error("Can not get storage.main.dir section") + } + + } else { + c.Error("Can not get storage.main section") + } + } else { + c.Error("Can not get storage section") + } + } + checkWithVersion("v4.0.9") + checkWithVersion("nightly") +} + +func (s *metaSuiteTopo) TestTiFlashInvalidStorageSection(c *C) { + spec := &Specification{} + + testCases := [][]byte{ + []byte(` +tiflash_servers: + - host: 172.16.5.138 + data_dir: /hdd0/tiflash,/hdd1/tiflash + config: + # storage.main.dir is not defined + storage.latest.dir: ["/ssd0/tiflash"] +`), + []byte(` +tiflash_servers: + - host: 172.16.5.138 + data_dir: /hdd0/tiflash,/hdd1/tiflash + config: + # storage.main.dir is empty string array + storage.main.dir: [] + storage.latest.dir: ["/ssd0/tiflash"] +`), + []byte(` +tiflash_servers: + - host: 172.16.5.138 + data_dir: /hdd0/tiflash,/hdd1/tiflash + config: + # storage.main.dir is not a string array + storage.main.dir: /hdd0/tiflash,/hdd1/tiflash + storage.latest.dir: ["/ssd0/tiflash"] +`), + []byte(` +tiflash_servers: + - host: 172.16.5.138 + data_dir: /hdd0/tiflash,/hdd1/tiflash + config: + # storage.main.dir is not a string array + storage.main.dir: [0, 1] + storage.latest.dir: ["/ssd0/tiflash"] +`), + } + + for _, testCase := range testCases { + err := yaml.Unmarshal([]byte(testCase), spec) + c.Check(err, NotNil) + } +} diff --git a/pkg/cluster/spec/tiflash.go b/pkg/cluster/spec/tiflash.go index da271826cf..c4f59e80a6 100644 --- a/pkg/cluster/spec/tiflash.go +++ b/pkg/cluster/spec/tiflash.go @@ -20,15 +20,15 @@ import ( "fmt" "io/ioutil" "path/filepath" - "reflect" + "sort" "strings" "time" "github.com/pingcap/tiup/pkg/cluster/api" "github.com/pingcap/tiup/pkg/cluster/executor" "github.com/pingcap/tiup/pkg/cluster/template/scripts" - "github.com/pingcap/tiup/pkg/logger/log" "github.com/pingcap/tiup/pkg/meta" + "github.com/pingcap/tiup/pkg/set" "golang.org/x/mod/semver" "gopkg.in/yaml.v2" ) @@ -45,7 +45,7 @@ type TiFlashSpec struct { FlashProxyStatusPort int `yaml:"flash_proxy_status_port" default:"20292"` StatusPort int `yaml:"metrics_port" default:"8234"` DeployDir string `yaml:"deploy_dir,omitempty"` - DataDir string `yaml:"data_dir,omitempty"` + DataDir string `yaml:"data_dir,omitempty" validate:"data_dir:expandable"` LogDir string `yaml:"log_dir,omitempty"` TmpDir string `yaml:"tmp_path,omitempty"` Offline bool `yaml:"offline,omitempty"` @@ -87,6 +87,99 @@ func (s TiFlashSpec) IsImported() bool { return s.Imported } +// key names for storage config +const ( + TiFlashStorageKeyMainDirs string = "storage.main.dir" + TiFlashStorageKeyLatestDirs string = "storage.latest.dir" + TiFlashStorageKeyRaftDirs string = "storage.raft.dir" +) + +// GetOverrideDataDir returns the data dir. +// If users have defined TiFlashStorageKeyMainDirs, then override "DataDir" with +// the directories defined in TiFlashStorageKeyMainDirs and TiFlashStorageKeyLatestDirs +func (s TiFlashSpec) GetOverrideDataDir() (string, error) { + getStrings := func(key string) []string { + var strs []string + if dirsVal, ok := s.Config[key]; ok { + if dirs, ok := dirsVal.([]interface{}); ok && len(dirs) > 0 { + for _, elem := range dirs { + if elemStr, ok := elem.(string); ok { + elemStr := strings.TrimSuffix(strings.TrimSpace(elemStr), "/") + strs = append(strs, elemStr) + } + } + } + } + return strs + } + mainDirs := getStrings(TiFlashStorageKeyMainDirs) + latestDirs := getStrings(TiFlashStorageKeyLatestDirs) + if len(mainDirs) == 0 && len(latestDirs) == 0 { + return s.DataDir, nil + } + + // If storage is defined, the path defined in "data_dir" will be ignored + // check whether the directories is uniq in the same configuration item + // and make the dirSet uniq + checkAbsolute := func(d, host, key string) error { + if !strings.HasPrefix(d, "/") { + return fmt.Errorf("directory '%s' should be an absolute path in 'tiflash_servers:%s.config.%s'", d, s.Host, key) + } + return nil + } + + dirSet := set.NewStringSet() + for _, d := range latestDirs { + if err := checkAbsolute(d, s.Host, TiFlashStorageKeyLatestDirs); err != nil { + return "", err + } + if dirSet.Exist(d) { + return "", &meta.ValidateErr{ + Type: meta.TypeConflict, + Target: "directory", + LHS: fmt.Sprintf("tiflash_servers:%s.config.%s", s.Host, TiFlashStorageKeyLatestDirs), + RHS: fmt.Sprintf("tiflash_servers:%s.config.%s", s.Host, TiFlashStorageKeyLatestDirs), + Value: d, + } + } + dirSet.Insert(d) + } + mainDirSet := set.NewStringSet() + for _, d := range mainDirs { + if err := checkAbsolute(d, s.Host, TiFlashStorageKeyMainDirs); err != nil { + return "", err + } + if mainDirSet.Exist(d) { + return "", &meta.ValidateErr{ + Type: meta.TypeConflict, + Target: "directory", + LHS: fmt.Sprintf("tiflash_servers:%s.config.%s", s.Host, TiFlashStorageKeyMainDirs), + RHS: fmt.Sprintf("tiflash_servers:%s.config.%s", s.Host, TiFlashStorageKeyMainDirs), + Value: d, + } + } + mainDirSet.Insert(d) + dirSet.Insert(d) + } + // keep the firstPath + var firstPath string + if len(latestDirs) != 0 { + firstPath = latestDirs[0] + } else { + firstPath = mainDirs[0] + } + dirSet.Remove(firstPath) + // join (stable sorted) paths with "," + keys := make([]string, len(dirSet)) + i := 0 + for k := range dirSet { + keys[i] = k + i++ + } + sort.Strings(keys) + return firstPath + "," + strings.Join(keys, ","), nil +} + // TiFlashComponent represents TiFlash component. type TiFlashComponent struct{ Topology *Specification } @@ -140,9 +233,9 @@ func (i *TiFlashInstance) GetServicePort() int { return i.InstanceSpec.(TiFlashSpec).FlashServicePort } -// checkIncorrectDataDir checks TiFlash's key should not be set in config +// checkIncorrectKey checks TiFlash's key should not be set in config func (i *TiFlashInstance) checkIncorrectKey(key string) error { - errMsg := "NOTE: TiFlash `%s` is should NOT be set in topo's \"%s\" config, its value will be ignored, you should set `data_dir` in each host instead, please check your topology" + errMsg := "NOTE: TiFlash `%s` should NOT be set in topo's \"%s\" config, its value will be ignored, you should set `data_dir` in each host instead, please check your topology" if dir, ok := i.InstanceSpec.(TiFlashSpec).Config[key].(string); ok && dir != "" { return fmt.Errorf(errMsg, key, "host") } @@ -152,42 +245,125 @@ func (i *TiFlashInstance) checkIncorrectKey(key string) error { return nil } -// checkIncorrectDataDir checks incorrect data_dir settings -func (i *TiFlashInstance) checkIncorrectDataDir() error { - if err := i.checkIncorrectKey("data_dir"); err != nil { - return err +// checkIncorrectServerConfigs checks TiFlash's key should not be set in server_config +func (i *TiFlashInstance) checkIncorrectServerConfigs(key string) error { + errMsg := "NOTE: TiFlash `%[1]s` should NOT be set in topo's \"%[2]s\" config, you should set `%[1]s` in each host instead, please check your topology" + if _, ok := i.topo.(*Specification).ServerConfigs.TiFlash[key]; ok { + return fmt.Errorf(errMsg, key, "server_configs") } - return i.checkIncorrectKey("path") + return nil } -// DataDir represents TiFlash's DataDir -func (i *TiFlashInstance) DataDir() string { - if err := i.checkIncorrectDataDir(); err != nil { - log.Errorf(err.Error()) +// isValidStringArray detect the key in `config` is valid or not. +// The configuration is valid only the key-value is defined, and the +// value is a non-empty string array. +// Return (key is defined or not, the value is valid or not) +func isValidStringArray(key string, config map[string]interface{}, couldEmpty bool) (bool, error) { + var ( + dirsVal interface{} + isKeyDefined bool + isAllElemsString bool = true + ) + if dirsVal, isKeyDefined = config[key]; !isKeyDefined { + return isKeyDefined, nil } - dataDir := reflect.ValueOf(i.InstanceSpec).FieldByName("DataDir") - if !dataDir.IsValid() { - return "" + if dirs, ok := dirsVal.([]interface{}); ok && (couldEmpty || len(dirs) > 0) { + // ensure dirs is non-empty string array + for _, elem := range dirs { + if _, ok := elem.(string); !ok { + isAllElemsString = false + break + } + } + if isAllElemsString { + return isKeyDefined, nil + } + } + return isKeyDefined, fmt.Errorf("'%s' should be a non-empty string array, please check the tiflash configuration in your yaml file", TiFlashStorageKeyMainDirs) +} + +// checkTiFlashStorageConfig detect the "storage" section in `config` +// is valid or not. +func checkTiFlashStorageConfig(config map[string]interface{}) (bool, error) { + var ( + isStorageDirsDefined bool + err error + ) + if isStorageDirsDefined, err = isValidStringArray(TiFlashStorageKeyMainDirs, config, false); err != nil { + return isStorageDirsDefined, err } - var dirs []string - for _, dir := range strings.Split(dataDir.String(), ",") { - if dir == "" { - continue + if !isStorageDirsDefined { + containsStorageSectionKey := func(config map[string]interface{}) (string, bool) { + for k := range config { + if strings.HasPrefix(k, "storage.") { + return k, true + } + } + return "", false } - if !strings.HasPrefix(dir, "/") { - dirs = append(dirs, filepath.Join(i.DeployDir(), dir)) - } else { - dirs = append(dirs, dir) + if key, contains := containsStorageSectionKey(config); contains { + return isStorageDirsDefined, fmt.Errorf("You must set '%s' before setting '%s', please check the tiflash configuration in your yaml file", TiFlashStorageKeyMainDirs, key) } } - return strings.Join(dirs, ",") + return isStorageDirsDefined, nil +} + +// CheckIncorrectConfigs checks incorrect settings +func (i *TiFlashInstance) CheckIncorrectConfigs() error { + // data_dir / path should not be set in config + if err := i.checkIncorrectKey("data_dir"); err != nil { + return err + } + if err := i.checkIncorrectKey("path"); err != nil { + return err + } + // storage.main/latest/raft.dir should not be set in server_config + if err := i.checkIncorrectServerConfigs(TiFlashStorageKeyMainDirs); err != nil { + return err + } + if err := i.checkIncorrectServerConfigs(TiFlashStorageKeyLatestDirs); err != nil { + return err + } + if err := i.checkIncorrectServerConfigs(TiFlashStorageKeyRaftDirs); err != nil { + return err + } + // storage.* in instance level + if _, err := checkTiFlashStorageConfig(i.InstanceSpec.(TiFlashSpec).Config); err != nil { + return err + } + // no matter storgae.latest.dir is defined or not, return err + _, err := isValidStringArray(TiFlashStorageKeyLatestDirs, i.InstanceSpec.(TiFlashSpec).Config, true) + return err +} + +// need to check the configuration after clusterVersion >= v4.0.9. +func checkTiFlashStorageConfigWithVersion(clusterVersion string, config map[string]interface{}) (bool, error) { + if semver.Compare(clusterVersion, "v4.0.9") >= 0 || clusterVersion == "nightly" { + return checkTiFlashStorageConfig(config) + } + return false, nil } -// InitTiFlashConfig initializes TiFlash config file -func (i *TiFlashInstance) InitTiFlashConfig(cfg *scripts.TiFlashScript, src map[string]interface{}) (map[string]interface{}, error) { +// InitTiFlashConfig initializes TiFlash config file with the configurations in server_configs +func (i *TiFlashInstance) initTiFlashConfig(cfg *scripts.TiFlashScript, clusterVersion string, src map[string]interface{}) (map[string]interface{}, error) { + var ( + pathConfig string + isStorageDirsDefined bool + err error + ) + if isStorageDirsDefined, err = checkTiFlashStorageConfigWithVersion(clusterVersion, src); err != nil { + return nil, err + } + // For backward compatibility, we need to rollback to set 'path' + if isStorageDirsDefined { + pathConfig = "#" + } else { + pathConfig = fmt.Sprintf(`path: "%s"`, cfg.DataDir) + } + topo := Specification{} - err := yaml.Unmarshal([]byte(fmt.Sprintf(` + err = yaml.Unmarshal([]byte(fmt.Sprintf(` server_configs: tiflash: default_profile: "default" @@ -195,7 +371,7 @@ server_configs: listen_host: "0.0.0.0" mark_cache_size: 5368709120 tmp_path: "%[11]s" - path: "%[1]s" + %[1]s tcp_port: %[3]d http_port: %[4]d flash.tidb_status_addr: "%[5]s" @@ -232,7 +408,7 @@ server_configs: profiles.default.max_memory_usage: 0 profiles.default.use_uncompressed_cache: 0 profiles.readonly.readonly: 1 -`, cfg.DataDir, cfg.LogDir, cfg.TCPPort, cfg.HTTPPort, cfg.TiDBStatusAddrs, cfg.IP, cfg.FlashServicePort, +`, pathConfig, cfg.LogDir, cfg.TCPPort, cfg.HTTPPort, cfg.TiDBStatusAddrs, cfg.IP, cfg.FlashServicePort, cfg.StatusPort, cfg.PDAddrs, cfg.DeployDir, cfg.TmpDir)), &topo) if err != nil { @@ -247,6 +423,26 @@ server_configs: return conf, nil } +func (i *TiFlashInstance) mergeTiFlashInstanceConfig(clusterVersion string, globalConf, instanceConf map[string]interface{}) (map[string]interface{}, error) { + var ( + isStorageDirsDefined bool + err error + conf map[string]interface{} + ) + if isStorageDirsDefined, err = checkTiFlashStorageConfigWithVersion(clusterVersion, instanceConf); err != nil { + return nil, err + } + if isStorageDirsDefined { + delete(globalConf, "path") + } + + conf, err = merge(globalConf, instanceConf) + if err != nil { + return nil, err + } + return conf, nil +} + // InitTiFlashLearnerConfig initializes TiFlash learner config file func (i *TiFlashInstance) InitTiFlashLearnerConfig(cfg *scripts.TiFlashScript, clusterVersion string, src map[string]interface{}) (map[string]interface{}, error) { topo := Specification{} @@ -376,8 +572,8 @@ func (i *TiFlashInstance) InitConfig( return err } - conf, err = i.InitTiFlashConfig(cfg, topo.ServerConfigs.TiFlash) - if err != nil { + // Init the configuration using cfg and server_configs + if conf, err = i.initTiFlashConfig(cfg, clusterVersion, topo.ServerConfigs.TiFlash); err != nil { return err } @@ -397,13 +593,22 @@ func (i *TiFlashInstance) InitConfig( if err != nil { return err } + // TODO: maybe we also need to check the imported config? + // if _, err = checkTiFlashStorageConfigWithVersion(clusterVersion, importConfig); err != nil { + // return err + // } conf, err = mergeImported(importConfig, conf) if err != nil { return err } } - return i.MergeServerConfig(e, conf, spec.Config, paths) + // Check the configuration of instance level + if conf, err = i.mergeTiFlashInstanceConfig(clusterVersion, conf, spec.Config); err != nil { + return err + } + + return i.MergeServerConfig(e, conf, nil, paths) } // ScaleConfig deploy temporary config on scaling diff --git a/pkg/cluster/spec/validate.go b/pkg/cluster/spec/validate.go index 476db90bbc..2ca0169136 100644 --- a/pkg/cluster/spec/validate.go +++ b/pkg/cluster/spec/validate.go @@ -810,6 +810,16 @@ func (s *Specification) validatePDNames() error { return nil } +func (s *Specification) validateTiFlashConfigs() error { + c := FindComponent(s, ComponentTiFlash) + for _, ins := range c.Instances() { + if err := ins.(*TiFlashInstance).CheckIncorrectConfigs(); err != nil { + return err + } + } + return nil +} + // Validate validates the topology specification and produce error if the // specification invalid (e.g: port conflicts or directory conflicts) func (s *Specification) Validate() error { @@ -837,6 +847,10 @@ func (s *Specification) Validate() error { return err } + if err := s.validateTiFlashConfigs(); err != nil { + return err + } + return RelativePathDetect(s, isSkipField) } diff --git a/pkg/cluster/spec/validate_test.go b/pkg/cluster/spec/validate_test.go index ee867ce8b1..5aeb7dd735 100644 --- a/pkg/cluster/spec/validate_test.go +++ b/pkg/cluster/spec/validate_test.go @@ -745,6 +745,28 @@ tiflash_servers: c.Assert(cnt, Equals, 0) cnt = topo.CountDir("172.19.0.104", "/birdstorm") c.Assert(cnt, Equals, 1) + + err = yaml.Unmarshal([]byte(` +global: + user: "test1" + ssh_port: 220 + deploy_dir: "test-deploy" +tiflash_servers: + - host: 172.19.0.104 + data_dir: /data1 # this is ignored + config: + # test with these paths + storage.main.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) } func (s *metaSuiteTopo) TestDirectoryConflictsWithMultiDir(c *C) { @@ -783,6 +805,65 @@ pd_servers: 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'") } +func (s *metaSuiteTopo) TestDirectoryConflictsWithTiFlashMultiDir2(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" # this will be overwrite by storage.main.dir + config: + storage.main.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 + # this will be overwrite by storage.main.dir + data_dir: "/test-1" + config: + storage.main.dir: [ /test-2, /test-2 ] # conflict inside +pd_servers: + - host: 172.16.5.138 + data_dir: "/test-1" +`), &topo) + c.Assert(err, NotNil) + c.Assert(err.Error(), Equals, "directory conflict for '/test-2' between 'tiflash_servers:172.16.5.138.config.storage.main.dir' and 'tiflash_servers:172.16.5.138.config.storage.main.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" # this will be overwrite by storage.main.dir + config: + # no conflict between main and latest + storage.main.dir: [ /test-1, /test-2] + storage.latest.dir: [ /test-1, /test-2] +pd_servers: + - host: 172.16.5.138 + data_dir: "/test-3" +`), &topo) + c.Assert(err, IsNil) +} + func (s *metaSuiteTopo) TestPdServerWithSameName(c *C) { topo := Specification{} err := yaml.Unmarshal([]byte(` diff --git a/pkg/utils/diff.go b/pkg/utils/diff.go index 2ea867d754..a71d905a16 100644 --- a/pkg/utils/diff.go +++ b/pkg/utils/diff.go @@ -19,14 +19,16 @@ import ( "strconv" "strings" + "github.com/pingcap/tiup/pkg/set" "github.com/r3labs/diff" "github.com/sergi/go-diff/diffmatchpatch" ) const ( - validateTagName = "validate" - validateTagEditable = "editable" - validateTagIgnore = "ignore" + validateTagName = "validate" + validateTagEditable = "editable" + validateTagIgnore = "ignore" + validateTagExpandable = "expandable" // r3labs/diff drops everything after the first ',' in the tag value, so we use a different // separator for the tag value and its options validateTagSeperator = ":" @@ -42,6 +44,34 @@ func ShowDiff(t1 string, t2 string, w io.Writer) { fmt.Fprint(w, dmp.DiffPrettyText(diffs)) } +func validateExpandable(fromField, toField interface{}) bool { + fromStr, ok := fromField.(string) + if !ok { + return false + } + toStr, ok := toField.(string) + if !ok { + return false + } + tidyPaths := func(arr []string) []string { + for i := 0; i < len(arr); i++ { + arr[i] = strings.TrimSuffix(strings.TrimSpace(arr[i]), "/") + } + return arr + } + fromPaths := tidyPaths(strings.Split(fromStr, ",")) + toPaths := tidyPaths(strings.Split(toStr, ",")) + // The first path must be the same + if len(fromPaths) > 0 && len(toPaths) > 0 && fromPaths[0] != toPaths[0] { + return false + } + // The intersection size must be the same with from size + fromSet := set.NewStringSet(fromPaths...) + toSet := set.NewStringSet(toPaths...) + inter := fromSet.Intersection(toSet) + return len(inter) == len(fromSet) +} + // ValidateSpecDiff checks and validates the new spec to see if the modified // keys are all marked as editable func ValidateSpecDiff(s1, s2 interface{}) error { @@ -73,6 +103,7 @@ func ValidateSpecDiff(s1, s2 interface{}) error { pathEditable := true pathIgnore := false + pathExpandable := false for _, p := range c.Path { key, ctl := parseValidateTagValue(p) if _, err := strconv.Atoi(key); err == nil { @@ -83,12 +114,15 @@ func ValidateSpecDiff(s1, s2 interface{}) error { pathIgnore = true continue } + if ctl == validateTagExpandable { + pathExpandable = validateExpandable(c.From, c.To) + } if ctl != validateTagEditable { pathEditable = false } } // if the path has any ignorable item, just ignore it - if pathIgnore || pathEditable && (c.Type == diff.CREATE || c.Type == diff.DELETE) { + if pathIgnore || (pathEditable && (c.Type == diff.CREATE || c.Type == diff.DELETE)) || pathExpandable { // If *every* parent elements on the path are all marked as editable, // AND the field itself is marked as editable, it is allowed to add or delete continue diff --git a/pkg/utils/diff_test.go b/pkg/utils/diff_test.go index 7323f22f6b..40ca83028a 100644 --- a/pkg/utils/diff_test.go +++ b/pkg/utils/diff_test.go @@ -25,6 +25,7 @@ type sampleDataMeta struct { StrSlice []string `yaml:"strs,omitempty" validate:"strs:editable"` MapSlice []map[string]interface{} `yaml:"maps,omitempty" validate:"maps:ignore"` StrElem string `yaml:"stre" validate:"editable"` + StrElem2 string `yaml:"str2,omitempty" validate:"str2:expandable"` StructSlice1 []sampleDataElem `yaml:"slice1" validate:"slice1:editable"` StructSlice2 []sampleDataElem `yaml:"slice2,omitempty"` StructSlice3 []sampleDataEditable `yaml:"slice3,omitempty" validate:"slice3:editable"` @@ -473,3 +474,46 @@ maps: err = ValidateSpecDiff(d1, d2) c.Assert(err, IsNil) } + +func (d *diffSuite) TestValidateSpecDiffExpandable(c *C) { + var d1 sampleDataMeta + var d2 sampleDataMeta + var err error + + err = yaml.Unmarshal([]byte(` +str2: "/ssd0/tiflash,/ssd1/tiflash" +`), &d1) + c.Assert(err, IsNil) + + // Expand path + err = yaml.Unmarshal([]byte(` +str2: "/ssd0/tiflash,/ssd1/tiflash,/ssd2/tiflash" +`), &d2) + c.Assert(err, IsNil) + err = ValidateSpecDiff(d1, d2) + c.Assert(err, IsNil) + + // Expand path with non-sorted paths + err = yaml.Unmarshal([]byte(` +str2: "/ssd0/tiflash,/ssd2/tiflash,/ssd1/tiflash" +`), &d2) + c.Assert(err, IsNil) + err = ValidateSpecDiff(d1, d2) + c.Assert(err, IsNil) + + // Expand path with non-sorted paths. Changing the first path is not allowed. + err = yaml.Unmarshal([]byte(` +str2: "/ssd1/tiflash,/ssd0/tiflash,/ssd2/tiflash" +`), &d2) + c.Assert(err, IsNil) + err = ValidateSpecDiff(d1, d2) + c.Assert(err, NotNil) + + // Shirnking paths is not allowed + err = yaml.Unmarshal([]byte(` +str2: "/ssd0/tiflash" +`), &d2) + c.Assert(err, IsNil) + err = ValidateSpecDiff(d1, d2) + c.Assert(err, NotNil) +}