From 03eae77604f2cb23f4c77eaa0e9958a97fb16f78 Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Wed, 15 Mar 2017 13:01:30 -0500 Subject: [PATCH] add cluster.hosts flag had to work around a bunch of viper nonsense with handling string slices changed the validator.Check to use reflect.DeepEqual fixed a bug where env vars need to be reset between tests had to change the structure of pilosa.config to enable the passing of hosts as a slice of strings (it was a slice of structs which each just contained the string host.) so we lose some generality, but we weren't using it anyway. --- cmd/root.go | 22 +++++++++++++++- cmd/server.go | 1 + cmd/server_test.go | 62 +++++++++++++++++++++++++++++++++++++++------- config.go | 17 ++++++------- 4 files changed, 82 insertions(+), 20 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index 1c1e3f2a4..c7a4464a8 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -99,7 +99,27 @@ func setAllConfig(v *viper.Viper, flags *flag.FlagSet, envPrefix string) error { if flagErr != nil { return } - value := v.GetString(f.Name) + var value string + if f.Value.Type() == "stringSlice" { + // special handling is needed for stringSlice as v.GetString will + // always return "" in the case that the value is an actual string + // slice from a config file rather than a comma separated string + // from a flag or env var. + vss := v.GetStringSlice(f.Name) + value = strings.Join(vss, ",") + } else { + value = v.GetString(f.Name) + } + fmt.Printf("Visiting '%v' with value '%v', changed: '%v', new value: '%v'\n", f.Name, f.Value, f.Changed, value) + if f.Changed { + // If f.Changed is true, that means the value has already been set + // by a flag, and we don't need to ask viper for it since the flag + // is the highest priority. This works around a problem with string + // slices where f.Value.Set(csvString) would cause the elements of + // csvString to be appended to the existing value rather than + // replacing it. + return + } flagErr = f.Value.Set(value) }) return flagErr diff --git a/cmd/server.go b/cmd/server.go index ada93b521..b0ed60af3 100644 --- a/cmd/server.go +++ b/cmd/server.go @@ -78,6 +78,7 @@ on the configured port.`, flags.StringVarP(&Serve.Config.DataDir, "data-dir", "d", "~/.pilosa", "Directory to store pilosa data files.") flags.StringVarP(&Serve.Config.Host, "bind", "", ":10101", "Default URI on which pilosa should listen.") flags.IntVarP(&Serve.Config.Cluster.ReplicaN, "cluster.replicas", "", 1, "Number hosts each piece of data should be stored on.") + flags.StringSliceVarP(&Serve.Config.Cluster.Nodes, "cluster.hosts", "", []string{}, "Comma separated list of hosts in cluster.") flags.StringVarP(&Serve.CPUProfile, "cpu-profile", "", "", "Where to store CPU profile.") flags.DurationVarP(&Serve.CPUTime, "cpu-time", "", 30*time.Second, "CPU profile duration.") diff --git a/cmd/server_test.go b/cmd/server_test.go index a5284ebdb..713f7249d 100644 --- a/cmd/server_test.go +++ b/cmd/server_test.go @@ -3,6 +3,7 @@ package cmd_test import ( "fmt" "io/ioutil" + "reflect" "strings" "sync" "testing" @@ -29,7 +30,7 @@ func (v *validator) Check(actual, expected interface{}) { if v.err != nil { return } - if actual != expected { + if !reflect.DeepEqual(actual, expected) { v.err = fmt.Errorf("Actual: '%v' is not equal to '%v'", actual, expected) } } @@ -47,7 +48,7 @@ func TestServerConfig(t *testing.T) { failErr(t, err, "making data dir") tests := []commandTest{ { - args: []string{"server", "--data-dir", actualDataDir}, + args: []string{"server", "--data-dir", actualDataDir, "--cluster.hosts", "example.com:10101,example.com:10110"}, env: map[string]string{"PILOSA_DATA_DIR": "/tmp/myEnvDatadir"}, cfgFileContent: ` data-dir = "/tmp/myFileDatadir" @@ -55,18 +56,54 @@ bind = "localhost:0" [cluster] replicas = 2 + hosts = [ + "localhost:19444", + ] `, validation: func() error { v := validator{} v.Check(cmd.Serve.Config.DataDir, actualDataDir) v.Check(cmd.Serve.Config.Host, "localhost:0") v.Check(cmd.Serve.Config.Cluster.ReplicaN, 2) + v.Check(cmd.Serve.Config.Cluster.Nodes, []string{"example.com:10101", "example.com:10110"}) + return v.Error() + }, + }, + { + args: []string{"server"}, + env: map[string]string{"PILOSA_CLUSTER.HOSTS": "example.com:1110,example.com:1111"}, + cfgFileContent: ` +[cluster] + hosts = [ + "localhost:19444", + ] +`, + validation: func() error { + v := validator{} + v.Check(cmd.Serve.Config.Cluster.Nodes, []string{"example.com:1110", "example.com:1111"}) + return v.Error() + }, + }, + { + args: []string{"server"}, + env: map[string]string{}, + cfgFileContent: ` +[cluster] + hosts = [ + "localhost:19444", + ] + +`, + validation: func() error { + v := validator{} + v.Check(cmd.Serve.Config.Cluster.Nodes, []string{"localhost:19444"}) return v.Error() }, }, } + for i, test := range tests { - com := setupCommand(t, test.args, test.env, test.cfgFileContent) + com := test.setupCommand(t) wait := sync.Mutex{} wait.Lock() var execErr error @@ -84,32 +121,39 @@ bind = "localhost:0" if err := test.validation(); err != nil { t.Fatalf("Failed test %d due to: %v", i, err) } + test.reset() } } -func setupCommand(t *testing.T, args []string, env map[string]string, cfgFileContent string) *cobra.Command { +func (ct commandTest) setupCommand(t *testing.T) *cobra.Command { // make config file cfgFile, err := ioutil.TempFile("", "") failErr(t, err, "making temp file") - _, err = cfgFile.WriteString(cfgFileContent) + _, err = cfgFile.WriteString(ct.cfgFileContent) failErr(t, err, "writing config to temp file") // set up config file args/env - env["PILOSA_CONFIG"] = cfgFile.Name() - args = append(args[:1], append([]string{"--config=" + cfgFile.Name()}, args[1:]...)...) + ct.env["PILOSA_CONFIG"] = cfgFile.Name() + ct.args = append(ct.args[:1], append([]string{"--config=" + cfgFile.Name()}, ct.args[1:]...)...) // set up env - for name, val := range env { + for name, val := range ct.env { err = os.Setenv(name, val) failErr(t, err, fmt.Sprintf("setting environment variable '%s' to '%s'", name, val)) } // make command and set args rc := cmd.NewRootCommand(strings.NewReader(""), ioutil.Discard, ioutil.Discard) - rc.SetArgs(args) + rc.SetArgs(ct.args) err = cfgFile.Close() failErr(t, err, "closing config file") return rc } + +func (ct commandTest) reset() { + for name, _ := range ct.env { + os.Setenv(name, "") + } +} diff --git a/config.go b/config.go index 3b8924c96..182ea03c3 100644 --- a/config.go +++ b/config.go @@ -13,9 +13,9 @@ type Config struct { Host string `toml:"host"` Cluster struct { - ReplicaN int `toml:"replicas"` - Nodes []*ConfigNode `toml:"node"` - PollingInterval Duration `toml:"polling-interval"` + ReplicaN int `toml:"replicas"` + Nodes []string `toml:"hosts"` + PollingInterval Duration `toml:"polling-interval"` } `toml:"cluster"` Plugins struct { @@ -27,10 +27,6 @@ type Config struct { } `toml:"anti-entropy"` } -type ConfigNode struct { - Host string `toml:"host"` -} - // NewConfig returns an instance of Config with default options. func NewConfig() *Config { c := &Config{ @@ -38,6 +34,7 @@ func NewConfig() *Config { } c.Cluster.ReplicaN = DefaultReplicaN c.Cluster.PollingInterval = Duration(DefaultPollingInterval) + c.Cluster.Nodes = []string{} c.AntiEntropy.Interval = Duration(DefaultAntiEntropyInterval) return c } @@ -45,7 +42,7 @@ func NewConfig() *Config { func NewConfigForHosts(hosts []string) *Config { conf := NewConfig() for _, hostport := range hosts { - conf.Cluster.Nodes = append(conf.Cluster.Nodes, &ConfigNode{Host: hostport}) + conf.Cluster.Nodes = append(conf.Cluster.Nodes, hostport) } return conf } @@ -55,8 +52,8 @@ func (c *Config) PilosaCluster() *Cluster { cluster := NewCluster() cluster.ReplicaN = c.Cluster.ReplicaN - for _, n := range c.Cluster.Nodes { - cluster.Nodes = append(cluster.Nodes, &Node{Host: n.Host}) + for _, hostport := range c.Cluster.Nodes { + cluster.Nodes = append(cluster.Nodes, &Node{Host: hostport}) } return cluster