From 120b0887231391d8bf33fdf21b71c20aecd39b55 Mon Sep 17 00:00:00 2001 From: Barak Michener Date: Mon, 6 Oct 2014 19:42:13 -0400 Subject: [PATCH 1/4] Split config into a separate file and add sanity check and test --- etcdserver/config.go | 44 +++++++++++++++++++++++++++++++++++++++ etcdserver/config_test.go | 32 ++++++++++++++++++++++++++++ etcdserver/server.go | 21 ++++--------------- 3 files changed, 80 insertions(+), 17 deletions(-) create mode 100644 etcdserver/config.go create mode 100644 etcdserver/config_test.go diff --git a/etcdserver/config.go b/etcdserver/config.go new file mode 100644 index 000000000..22f489b70 --- /dev/null +++ b/etcdserver/config.go @@ -0,0 +1,44 @@ +package etcdserver + +import ( + "fmt" + "net/http" + + "github.com/coreos/etcd/pkg/types" +) + +// ServerConfig holds the configuration of etcd as taken from the command line or discovery. +type ServerConfig struct { + Name string + DiscoveryURL string + ClientURLs types.URLs + DataDir string + SnapCount int64 + Cluster *Cluster + ClusterState ClusterState + Transport *http.Transport +} + +// Verify sanity-checks the config struct and returns an error for things that +// should never happen. +func (c *ServerConfig) Verify() error { + // Make sure the cluster at least contains the local server. + m := c.Cluster.FindName(c.Name) + if m == nil { + return fmt.Errorf("could not find name %v in cluster!", c.Name) + } + + if c.ClusterState == ClusterStateValueNew { + // No identical IPs in the cluster peer list + urlMap := make(map[string]bool) + for _, m := range *c.Cluster { + for _, url := range m.PeerURLs { + if urlMap[url] { + return fmt.Errorf("duplicate url %v in server config", url) + } + urlMap[url] = true + } + } + } + return nil +} diff --git a/etcdserver/config_test.go b/etcdserver/config_test.go new file mode 100644 index 000000000..2b605cdf1 --- /dev/null +++ b/etcdserver/config_test.go @@ -0,0 +1,32 @@ +package etcdserver + +import ( + "testing" +) + +func TestConfigVerify(t *testing.T) { + cluster := &Cluster{} + cfg := ServerConfig{ + Name: "node1", + Cluster: cluster, + ClusterState: ClusterStateValueNew, + } + + err := cfg.Verify() + if err == nil { + t.Error("Did not get error for lacking self in cluster.") + } + + cluster.Set("node1=http://localhost:7001,node2=http://localhost:7001") + err = cfg.Verify() + if err == nil { + t.Error("Did not get error for double URL in cluster.") + } + + cluster.Set("node1=http://localhost:7001,node2=http://localhost:7002") + err = cfg.Verify() + if err != nil { + t.Errorf("Got unexpected error %v", err) + } + +} diff --git a/etcdserver/server.go b/etcdserver/server.go index 4ad33265a..33c571ae4 100644 --- a/etcdserver/server.go +++ b/etcdserver/server.go @@ -5,7 +5,6 @@ import ( "errors" "log" "math/rand" - "net/http" "os" "path" "sync/atomic" @@ -84,24 +83,12 @@ type RaftTimer interface { Term() int64 } -type ServerConfig struct { - Name string - DiscoveryURL string - ClientURLs types.URLs - DataDir string - SnapCount int64 - Cluster *Cluster - ClusterState ClusterState - Transport *http.Transport -} - // NewServer creates a new EtcdServer from the supplied configuration. The // configuration is considered static for the lifetime of the EtcdServer. func NewServer(cfg *ServerConfig) *EtcdServer { - m := cfg.Cluster.FindName(cfg.Name) - if m == nil { - // Should never happen - log.Fatalf("could not find name %v in cluster!", cfg.Name) + err := cfg.Verify() + if err != nil { + log.Fatalln(err) } snapdir := path.Join(cfg.DataDir, "snap") if err := os.MkdirAll(snapdir, privateDirMode); err != nil { @@ -111,7 +98,7 @@ func NewServer(cfg *ServerConfig) *EtcdServer { st := store.New() var w *wal.WAL var n raft.Node - var err error + m := cfg.Cluster.FindName(cfg.Name) waldir := path.Join(cfg.DataDir, "wal") if !wal.Exist(waldir) { if cfg.DiscoveryURL != "" { From 1a0195e07ebbd08e8bfda4d0d0e951aed941257f Mon Sep 17 00:00:00 2001 From: Barak Michener Date: Mon, 6 Oct 2014 20:05:53 -0400 Subject: [PATCH 2/4] tableize the test --- etcdserver/config_test.go | 44 ++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/etcdserver/config_test.go b/etcdserver/config_test.go index 2b605cdf1..0fc6050d1 100644 --- a/etcdserver/config_test.go +++ b/etcdserver/config_test.go @@ -5,28 +5,30 @@ import ( ) func TestConfigVerify(t *testing.T) { - cluster := &Cluster{} - cfg := ServerConfig{ - Name: "node1", - Cluster: cluster, - ClusterState: ClusterStateValueNew, + + tests := []struct { + clusterSetting string + shouldError bool + }{ + {"", true}, + {"node1=http://localhost:7001,node2=http://localhost:7001", true}, + {"node1=http://localhost:7001,node2=http://localhost:7002", false}, } - err := cfg.Verify() - if err == nil { - t.Error("Did not get error for lacking self in cluster.") + for i, tt := range tests { + cluster := &Cluster{} + cluster.Set(tt.clusterSetting) + cfg := ServerConfig{ + Name: "node1", + Cluster: cluster, + ClusterState: ClusterStateValueNew, + } + err := cfg.Verify() + if (err == nil) && tt.shouldError { + t.Errorf("#%d: Got no error where one was expected", i) + } + if (err != nil) && !tt.shouldError { + t.Errorf("#%d: Got unexpected error: %v", i, err) + } } - - cluster.Set("node1=http://localhost:7001,node2=http://localhost:7001") - err = cfg.Verify() - if err == nil { - t.Error("Did not get error for double URL in cluster.") - } - - cluster.Set("node1=http://localhost:7001,node2=http://localhost:7002") - err = cfg.Verify() - if err != nil { - t.Errorf("Got unexpected error %v", err) - } - } From 8a311e5b76d898ed9ae92414b4d7e571efb8ee4b Mon Sep 17 00:00:00 2001 From: Barak Michener Date: Mon, 6 Oct 2014 20:07:51 -0400 Subject: [PATCH 3/4] remove new cluster check --- etcdserver/config.go | 16 +++++++--------- etcdserver/config_test.go | 5 ++--- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/etcdserver/config.go b/etcdserver/config.go index 22f489b70..c0b00b868 100644 --- a/etcdserver/config.go +++ b/etcdserver/config.go @@ -28,16 +28,14 @@ func (c *ServerConfig) Verify() error { return fmt.Errorf("could not find name %v in cluster!", c.Name) } - if c.ClusterState == ClusterStateValueNew { - // No identical IPs in the cluster peer list - urlMap := make(map[string]bool) - for _, m := range *c.Cluster { - for _, url := range m.PeerURLs { - if urlMap[url] { - return fmt.Errorf("duplicate url %v in server config", url) - } - urlMap[url] = true + // No identical IPs in the cluster peer list + urlMap := make(map[string]bool) + for _, m := range *c.Cluster { + for _, url := range m.PeerURLs { + if urlMap[url] { + return fmt.Errorf("duplicate url %v in server config", url) } + urlMap[url] = true } } return nil diff --git a/etcdserver/config_test.go b/etcdserver/config_test.go index 0fc6050d1..e02d834b5 100644 --- a/etcdserver/config_test.go +++ b/etcdserver/config_test.go @@ -19,9 +19,8 @@ func TestConfigVerify(t *testing.T) { cluster := &Cluster{} cluster.Set(tt.clusterSetting) cfg := ServerConfig{ - Name: "node1", - Cluster: cluster, - ClusterState: ClusterStateValueNew, + Name: "node1", + Cluster: cluster, } err := cfg.Verify() if (err == nil) && tt.shouldError { From d6aea2a795b9c5ba8619c87787d5d98b665f26ec Mon Sep 17 00:00:00 2001 From: Barak Michener Date: Mon, 6 Oct 2014 20:16:25 -0400 Subject: [PATCH 4/4] add golint on the new box and fix appropriate lint --- etcdserver/config.go | 2 +- etcdserver/config_test.go | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/etcdserver/config.go b/etcdserver/config.go index c0b00b868..a9871611c 100644 --- a/etcdserver/config.go +++ b/etcdserver/config.go @@ -25,7 +25,7 @@ func (c *ServerConfig) Verify() error { // Make sure the cluster at least contains the local server. m := c.Cluster.FindName(c.Name) if m == nil { - return fmt.Errorf("could not find name %v in cluster!", c.Name) + return fmt.Errorf("could not find name %v in cluster", c.Name) } // No identical IPs in the cluster peer list diff --git a/etcdserver/config_test.go b/etcdserver/config_test.go index e02d834b5..9d8297d28 100644 --- a/etcdserver/config_test.go +++ b/etcdserver/config_test.go @@ -5,7 +5,6 @@ import ( ) func TestConfigVerify(t *testing.T) { - tests := []struct { clusterSetting string shouldError bool