*: extract types.Cluster from etcdserver.Cluster

The PR extracts types.Cluster from etcdserver.Cluster. types.Cluster
is used for flag parsing and etcdserver config.

There is no need to expose etcdserver.Cluster public, which contains
lots of etcdserver internal details and methods. This is the first step
for it.

etcdmain/etcd.go

@@ -23,7 +23,6 @@ import (
 	"os"
 	"path"
 	"reflect"
-	"strings"
 	"time"
 
 	"github.com/coreos/etcd/discovery"
@@ -71,6 +70,7 @@ func Main() {
 
 	var stopped <-chan struct{}
 
+	// TODO: check whether fields are set instead of whether fields have default value
 	if cfg.name != defaultName && cfg.initialCluster == initialClusterFromName(defaultName) {
 		cfg.initialCluster = initialClusterFromName(cfg.name)
 	}
@@ -116,7 +116,7 @@ func Main() {
 
 // startEtcd launches the etcd server and HTTP handlers for client/server communication.
 func startEtcd(cfg *config) (<-chan struct{}, error) {
-	cls, err := setupCluster(cfg)
+	urlsmap, token, err := getPeerURLsMapAndToken(cfg)
 	if err != nil {
 		return nil, fmt.Errorf("error setting up initial cluster: %v", err)
 	}
@@ -171,21 +171,22 @@ func startEtcd(cfg *config) (<-chan struct{}, error) {
 	}
 
 	srvcfg := &etcdserver.ServerConfig{
-		Name:            cfg.name,
-		ClientURLs:      cfg.acurls,
-		PeerURLs:        cfg.apurls,
-		DataDir:         cfg.dir,
-		SnapCount:       cfg.snapCount,
-		MaxSnapFiles:    cfg.maxSnapFiles,
-		MaxWALFiles:     cfg.maxWalFiles,
-		Cluster:         cls,
-		DiscoveryURL:    cfg.durl,
-		DiscoveryProxy:  cfg.dproxy,
-		NewCluster:      cfg.isNewCluster(),
-		ForceNewCluster: cfg.forceNewCluster,
-		Transport:       pt,
-		TickMs:          cfg.TickMs,
-		ElectionTicks:   cfg.electionTicks(),
+		Name:                cfg.name,
+		ClientURLs:          cfg.acurls,
+		PeerURLs:            cfg.apurls,
+		DataDir:             cfg.dir,
+		SnapCount:           cfg.snapCount,
+		MaxSnapFiles:        cfg.maxSnapFiles,
+		MaxWALFiles:         cfg.maxWalFiles,
+		InitialPeerURLsMap:  urlsmap,
+		InitialClusterToken: token,
+		DiscoveryURL:        cfg.durl,
+		DiscoveryProxy:      cfg.dproxy,
+		NewCluster:          cfg.isNewCluster(),
+		ForceNewCluster:     cfg.forceNewCluster,
+		Transport:           pt,
+		TickMs:              cfg.TickMs,
+		ElectionTicks:       cfg.electionTicks(),
 	}
 	var s *etcdserver.EtcdServer
 	s, err = etcdserver.NewServer(srvcfg)
@@ -222,7 +223,7 @@ func startEtcd(cfg *config) (<-chan struct{}, error) {
 
 // startProxy launches an HTTP proxy for client communication which proxies to other etcd nodes.
 func startProxy(cfg *config) error {
-	cls, err := setupCluster(cfg)
+	urlsmap, _, err := getPeerURLsMapAndToken(cfg)
 	if err != nil {
 		return fmt.Errorf("error setting up initial cluster: %v", err)
 	}
@@ -232,7 +233,7 @@ func startProxy(cfg *config) error {
 		if err != nil {
 			return err
 		}
-		if cls, err = etcdserver.NewClusterFromString(cfg.durl, s); err != nil {
+		if urlsmap, err = types.NewURLsMap(s); err != nil {
 			return err
 		}
 	}
@@ -267,12 +268,13 @@ func startProxy(cfg *config) error {
 		peerURLs = urls.PeerURLs
 		log.Printf("proxy: using peer urls %v from cluster file ./%s", peerURLs, clusterfile)
 	case os.IsNotExist(err):
-		peerURLs = cls.PeerURLs()
+		peerURLs = urlsmap.URLs()
 		log.Printf("proxy: using peer urls %v ", peerURLs)
 	default:
 		return err
 	}
 
+	clientURLs := []string{}
 	uf := func() []string {
 		gcls, err := etcdserver.GetClusterFromRemotePeers(peerURLs, tr)
 		// TODO: remove the 2nd check when we fix GetClusterFromPeers
@@ -282,33 +284,33 @@ func startProxy(cfg *config) error {
 			return []string{}
 		}
 		if len(gcls.Members()) == 0 {
-			return cls.ClientURLs()
+			return clientURLs
 		}
-		cls = gcls
+		clientURLs = gcls.ClientURLs()
 
-		urls := struct{ PeerURLs []string }{cls.PeerURLs()}
+		urls := struct{ PeerURLs []string }{gcls.PeerURLs()}
 		b, err := json.Marshal(urls)
 		if err != nil {
 			log.Printf("proxy: error on marshal peer urls %s", err)
-			return cls.ClientURLs()
+			return clientURLs
 		}
 
 		err = ioutil.WriteFile(clusterfile+".bak", b, 0600)
 		if err != nil {
 			log.Printf("proxy: error on writing urls %s", err)
-			return cls.ClientURLs()
+			return clientURLs
 		}
 		err = os.Rename(clusterfile+".bak", clusterfile)
 		if err != nil {
 			log.Printf("proxy: error on updating clusterfile %s", err)
-			return cls.ClientURLs()
+			return clientURLs
 		}
-		if !reflect.DeepEqual(cls.PeerURLs(), peerURLs) {
-			log.Printf("proxy: updated peer urls in cluster file from %v to %v", peerURLs, cls.PeerURLs())
+		if !reflect.DeepEqual(gcls.PeerURLs(), peerURLs) {
+			log.Printf("proxy: updated peer urls in cluster file from %v to %v", peerURLs, gcls.PeerURLs())
 		}
-		peerURLs = cls.PeerURLs()
+		peerURLs = gcls.PeerURLs()
 
-		return cls.ClientURLs()
+		return clientURLs
 	}
 	ph := proxy.NewHandler(pt, uf)
 	ph = &cors.CORSHandler{
@@ -335,35 +337,28 @@ func startProxy(cfg *config) error {
 	return nil
 }
 
-// setupCluster sets up an initial cluster definition for bootstrap or discovery.
-func setupCluster(cfg *config) (*etcdserver.Cluster, error) {
-	var cls *etcdserver.Cluster
-	var err error
+// getPeerURLsMapAndToken sets up an initial peer URLsMap and cluster token for bootstrap or discovery.
+func getPeerURLsMapAndToken(cfg *config) (urlsmap types.URLsMap, token string, err error) {
 	switch {
 	case cfg.durl != "":
+		urlsmap = types.URLsMap{}
 		// If using discovery, generate a temporary cluster based on
 		// self's advertised peer URLs
-		clusterStr := genClusterString(cfg.name, cfg.apurls)
-		cls, err = etcdserver.NewClusterFromString(cfg.durl, clusterStr)
+		urlsmap[cfg.name] = cfg.apurls
+		token = cfg.durl
 	case cfg.dnsCluster != "":
-		clusterStr, clusterToken, err := discovery.SRVGetCluster(cfg.name, cfg.dnsCluster, cfg.initialClusterToken, cfg.apurls)
+		var clusterStr string
+		clusterStr, token, err = discovery.SRVGetCluster(cfg.name, cfg.dnsCluster, cfg.initialClusterToken, cfg.apurls)
 		if err != nil {
-			return nil, err
+			return nil, "", err
 		}
-		cls, err = etcdserver.NewClusterFromString(clusterToken, clusterStr)
+		urlsmap, err = types.NewURLsMap(clusterStr)
 	default:
 		// We're statically configured, and cluster has appropriately been set.
-		cls, err = etcdserver.NewClusterFromString(cfg.initialClusterToken, cfg.initialCluster)
-	}
-	return cls, err
-}
-
-func genClusterString(name string, urls types.URLs) string {
-	addrs := make([]string, 0)
-	for _, u := range urls {
-		addrs = append(addrs, fmt.Sprintf("%v=%v", name, u.String()))
+		urlsmap, err = types.NewURLsMap(cfg.initialCluster)
+		token = cfg.initialClusterToken
 	}
-	return strings.Join(addrs, ",")
+	return urlsmap, token, err
 }
 
 // identifyDataDirOrDie returns the type of the data dir.

etcdmain/etcd_test.go

@@ -1,45 +0,0 @@
-// Copyright 2015 CoreOS, Inc.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package etcdmain
-
-import (
-	"testing"
-
-	"github.com/coreos/etcd/pkg/testutil"
-)
-
-func TestGenClusterString(t *testing.T) {
-	tests := []struct {
-		token string
-		urls  []string
-		wstr  string
-	}{
-		{
-			"default", []string{"http://127.0.0.1:2379"},
-			"default=http://127.0.0.1:2379",
-		},
-		{
-			"node1", []string{"http://0.0.0.0:2379", "http://1.1.1.1:2379"},
-			"node1=http://0.0.0.0:2379,node1=http://1.1.1.1:2379",
-		},
-	}
-	for i, tt := range tests {
-		urls := testutil.MustNewURLs(t, tt.urls)
-		str := genClusterString(tt.token, urls)
-		if str != tt.wstr {
-			t.Errorf("#%d: cluster = %s, want %s", i, str, tt.wstr)
-		}
-	}
-}

etcdserver/cluster.go

@@ -15,21 +15,21 @@
 package etcdserver
 
 import (
+	"bytes"
 	"crypto/sha1"
 	"encoding/binary"
 	"encoding/json"
 	"fmt"
 	"log"
-	"net/url"
 	"path"
 	"sort"
 	"strings"
 	"sync"
 
 	"github.com/coreos/etcd/Godeps/_workspace/src/github.com/coreos/go-semver/semver"
-	"github.com/coreos/etcd/pkg/flags"
 	"github.com/coreos/etcd/pkg/netutil"
 	"github.com/coreos/etcd/pkg/types"
+	"github.com/coreos/etcd/raft"
 	"github.com/coreos/etcd/raft/raftpb"
 	"github.com/coreos/etcd/store"
 )
@@ -69,28 +69,15 @@ type Cluster struct {
 	removed map[types.ID]bool
 }
 
-// NewClusterFromString returns a Cluster instantiated from the given cluster token
-// and cluster string, by parsing members from a set of discovery-formatted
-// names-to-IPs, like:
-// mach0=http://1.1.1.1,mach0=http://2.2.2.2,mach1=http://3.3.3.3,mach2=http://4.4.4.4
-func NewClusterFromString(token string, cluster string) (*Cluster, error) {
+func NewCluster(token string, initial types.URLsMap) (*Cluster, error) {
 	c := newCluster(token)
-
-	v, err := url.ParseQuery(strings.Replace(cluster, ",", "&", -1))
-	if err != nil {
-		return nil, err
-	}
-	for name, urls := range v {
-		if len(urls) == 0 || urls[0] == "" {
-			return nil, fmt.Errorf("Empty URL given for %q", name)
-		}
-		purls := &flags.URLsValue{}
-		if err := purls.Set(strings.Join(urls, ",")); err != nil {
-			return nil, err
-		}
-		m := NewMember(name, types.URLs(*purls), c.token, nil)
+	for name, urls := range initial {
+		m := NewMember(name, urls, token, nil)
 		if _, ok := c.members[m.ID]; ok {
-			return nil, fmt.Errorf("Member exists with identical ID %v", m)
+			return nil, fmt.Errorf("member exists with identical ID %v", m)
+		}
+		if uint64(m.ID) == raft.None {
+			return nil, fmt.Errorf("cannot use %x as member id", raft.None)
 		}
 		c.members[m.ID] = m
 	}
@@ -98,14 +85,6 @@ func NewClusterFromString(token string, cluster string) (*Cluster, error) {
 	return c, nil
 }
 
-func NewClusterFromStore(token string, st store.Store) *Cluster {
-	c := newCluster(token)
-	c.store = st
-	c.members, c.removed = membersFromStore(c.store)
-	c.version = clusterVersionFromStore(c.store)
-	return c
-}
-
 func NewClusterFromMembers(token string, id types.ID, membs []*Member) *Cluster {
 	c := newCluster(token)
 	c.id = id
@@ -209,14 +188,19 @@ func (c *Cluster) ClientURLs() []string {
 func (c *Cluster) String() string {
 	c.Lock()
 	defer c.Unlock()
-	sl := []string{}
+	b := &bytes.Buffer{}
+	fmt.Fprintf(b, "{ClusterID:%s ", c.id)
+	var ms []string
 	for _, m := range c.members {
-		for _, u := range m.PeerURLs {
-			sl = append(sl, fmt.Sprintf("%s=%s", m.Name, u))
-		}
+		ms = append(ms, fmt.Sprintf("%+v", m))
 	}
-	sort.Strings(sl)
-	return strings.Join(sl, ",")
+	fmt.Fprintf(b, "Members:[%s] ", strings.Join(ms, " "))
+	var ids []string
+	for id, _ := range c.removed {
+		ids = append(ids, fmt.Sprintf("%s", id))
+	}
+	fmt.Fprintf(b, "RemovedMemberIDs:[%s]}", strings.Join(ids, " "))
+	return b.String()
 }
 
 func (c *Cluster) genID() {
@@ -371,20 +355,6 @@ func (c *Cluster) SetVersion(ver *semver.Version) {
 	c.version = ver
 }
 
-// Validate ensures that there is no identical urls in the cluster peer list
-func (c *Cluster) Validate() error {
-	urlMap := make(map[string]bool)
-	for _, m := range c.Members() {
-		for _, url := range m.PeerURLs {
-			if urlMap[url] {
-				return fmt.Errorf("duplicate url %v in cluster config", url)
-			}
-			urlMap[url] = true
-		}
-	}
-	return nil
-}
-
 func membersFromStore(st store.Store) (map[types.ID]*Member, map[types.ID]bool) {
 	members := make(map[types.ID]*Member)
 	removed := make(map[types.ID]bool)

etcdserver/cluster_test.go

@@ -21,110 +21,12 @@ import (
 	"reflect"
 	"testing"
 
-	"github.com/coreos/etcd/Godeps/_workspace/src/github.com/coreos/go-semver/semver"
 	"github.com/coreos/etcd/pkg/testutil"
 	"github.com/coreos/etcd/pkg/types"
 	"github.com/coreos/etcd/raft/raftpb"
 	"github.com/coreos/etcd/store"
 )
 
-func TestClusterFromString(t *testing.T) {
-	tests := []struct {
-		f    string
-		mems []*Member
-	}{
-		{
-			"mem1=http://10.0.0.1:2379,mem1=http://128.193.4.20:2379,mem2=http://10.0.0.2:2379,default=http://127.0.0.1:2379",
-			[]*Member{
-				newTestMember(3141198903430435750, []string{"http://10.0.0.2:2379"}, "mem2", nil),
-				newTestMember(4322322643958477905, []string{"http://10.0.0.1:2379", "http://128.193.4.20:2379"}, "mem1", nil),
-				newTestMember(12762790032478827328, []string{"http://127.0.0.1:2379"}, "default", nil),
-			},
-		},
-	}
-	for i, tt := range tests {
-		c, err := NewClusterFromString("abc", tt.f)
-		if err != nil {
-			t.Fatalf("#%d: unexpected new error: %v", i, err)
-		}
-		if c.token != "abc" {
-			t.Errorf("#%d: token = %v, want abc", i, c.token)
-		}
-		if !reflect.DeepEqual(c.Members(), tt.mems) {
-			t.Errorf("#%d: members = %+v, want %+v", i, c.Members(), tt.mems)
-		}
-	}
-}
-
-func TestClusterFromStringBad(t *testing.T) {
-	tests := []string{
-		// invalid URL
-		"%^",
-		// no URL defined for member
-		"mem1=,mem2=http://128.193.4.20:2379,mem3=http://10.0.0.2:2379",
-		"mem1,mem2=http://128.193.4.20:2379,mem3=http://10.0.0.2:2379",
-		// bad URL for member
-		"default=http://localhost/",
-		// TODO(philips): anyone know of a 64 bit sha1 hash collision
-		// "06b2f82fd81b2c20=http://128.193.4.20:2379,02c60cb75083ceef=http://128.193.4.20:2379",
-		// the same url for two members
-		"mem1=http://128.193.4.20:2379,mem2=http://128.193.4.20:2379",
-	}
-	for i, tt := range tests {
-		if _, err := NewClusterFromString("abc", tt); err == nil {
-			t.Errorf("#%d: unexpected successful new, want err", i)
-		}
-	}
-}
-
-func TestClusterFromStore(t *testing.T) {
-	tests := []struct {
-		mems []*Member
-		ver  *semver.Version
-	}{
-		{
-			[]*Member{newTestMember(1, nil, "", nil)},
-			semver.Must(semver.NewVersion("2.0.0")),
-		},
-		{
-			nil,
-			nil,
-		},
-		{
-			[]*Member{
-				newTestMember(1, nil, "", nil),
-				newTestMember(2, nil, "", nil),
-			},
-			semver.Must(semver.NewVersion("2.0.0")),
-		},
-	}
-	for i, tt := range tests {
-		st := store.New()
-		hc := newTestCluster(nil)
-		hc.SetStore(st)
-		for _, m := range tt.mems {
-			hc.AddMember(m)
-		}
-		if tt.ver != nil {
-			_, err := st.Set(path.Join(StoreClusterPrefix, "version"), false, tt.ver.String(), store.Permanent)
-			if err != nil {
-				t.Fatal(err)
-			}
-		}
-
-		c := NewClusterFromStore("abc", st)
-		if c.token != "abc" {
-			t.Errorf("#%d: token = %v, want %v", i, c.token, "abc")
-		}
-		if !reflect.DeepEqual(c.Members(), tt.mems) {
-			t.Errorf("#%d: members = %v, want %v", i, c.Members(), tt.mems)
-		}
-		if !reflect.DeepEqual(c.Version(), tt.ver) {
-			t.Errorf("#%d: ver = %v, want %v", i, c.Version(), tt.ver)
-		}
-	}
-}
-
 func TestClusterMember(t *testing.T) {
 	membs := []*Member{
 		newTestMember(1, nil, "node1", nil),
@@ -589,49 +491,6 @@ func TestClusterMembers(t *testing.T) {
 	}
 }
 
-func TestClusterString(t *testing.T) {
-	cls := &Cluster{
-		members: map[types.ID]*Member{
-			1: newTestMember(
-				1,
-				[]string{"http://1.1.1.1:1111", "http://0.0.0.0:0000"},
-				"abc",
-				nil,
-			),
-			2: newTestMember(
-				2,
-				[]string{"http://2.2.2.2:2222"},
-				"def",
-				nil,
-			),
-			3: newTestMember(
-				3,
-				[]string{"http://3.3.3.3:1234", "http://127.0.0.1:2380"},
-				"ghi",
-				nil,
-			),
-			// no PeerURLs = not included
-			4: newTestMember(
-				4,
-				[]string{},
-				"four",
-				nil,
-			),
-			5: newTestMember(
-				5,
-				nil,
-				"five",
-				nil,
-			),
-		},
-	}
-	w := "abc=http://0.0.0.0:0000,abc=http://1.1.1.1:1111,def=http://2.2.2.2:2222,ghi=http://127.0.0.1:2380,ghi=http://3.3.3.3:1234"
-	if g := cls.String(); g != w {
-		t.Fatalf("Cluster.String():\ngot  %#v\nwant %#v", g, w)
-	}
-
-}
-
 func TestClusterRemoveMember(t *testing.T) {
 	st := &storeRecorder{}
 	c := newTestCluster(nil)

etcdserver/config.go

@@ -23,24 +23,24 @@ import (
 
 	"github.com/coreos/etcd/pkg/netutil"
 	"github.com/coreos/etcd/pkg/types"
-	"github.com/coreos/etcd/raft"
 )
 
 // ServerConfig holds the configuration of etcd as taken from the command line or discovery.
 type ServerConfig struct {
-	Name            string
-	DiscoveryURL    string
-	DiscoveryProxy  string
-	ClientURLs      types.URLs
-	PeerURLs        types.URLs
-	DataDir         string
-	SnapCount       uint64
-	MaxSnapFiles    uint
-	MaxWALFiles     uint
-	Cluster         *Cluster
-	NewCluster      bool
-	ForceNewCluster bool
-	Transport       *http.Transport
+	Name                string
+	DiscoveryURL        string
+	DiscoveryProxy      string
+	ClientURLs          types.URLs
+	PeerURLs            types.URLs
+	DataDir             string
+	SnapCount           uint64
+	MaxSnapFiles        uint
+	MaxWALFiles         uint
+	InitialPeerURLsMap  types.URLsMap
+	InitialClusterToken string
+	NewCluster          bool
+	ForceNewCluster     bool
+	Transport           *http.Transport
 
 	TickMs        uint
 	ElectionTicks int
@@ -52,10 +52,10 @@ func (c *ServerConfig) VerifyBootstrap() error {
 	if err := c.verifyLocalMember(true); err != nil {
 		return err
 	}
-	if err := c.Cluster.Validate(); err != nil {
-		return err
+	if checkDuplicateURL(c.InitialPeerURLsMap) {
+		return fmt.Errorf("initial cluster %s has duplicate url", c.InitialPeerURLsMap)
 	}
-	if c.Cluster.String() == "" && c.DiscoveryURL == "" {
+	if c.InitialPeerURLsMap.String() == "" && c.DiscoveryURL == "" {
 		return fmt.Errorf("initial cluster unset and no discovery URL found")
 	}
 	return nil
@@ -70,8 +70,8 @@ func (c *ServerConfig) VerifyJoinExisting() error {
 	if err := c.verifyLocalMember(false); err != nil {
 		return err
 	}
-	if err := c.Cluster.Validate(); err != nil {
-		return err
+	if checkDuplicateURL(c.InitialPeerURLsMap) {
+		return fmt.Errorf("initial cluster %s has duplicate url", c.InitialPeerURLsMap)
 	}
 	if c.DiscoveryURL != "" {
 		return fmt.Errorf("discovery URL should not be set when joining existing initial cluster")
@@ -83,21 +83,19 @@ func (c *ServerConfig) VerifyJoinExisting() error {
 // cluster. If strict is set, it also verifies the configured member
 // has the same peer urls as configured advertised peer urls.
 func (c *ServerConfig) verifyLocalMember(strict bool) error {
-	m := c.Cluster.MemberByName(c.Name)
+	urls := c.InitialPeerURLsMap[c.Name]
 	// Make sure the cluster at least contains the local server.
-	if m == nil {
+	if urls == nil {
 		return fmt.Errorf("couldn't find local name %q in the initial cluster configuration", c.Name)
 	}
-	if uint64(m.ID) == raft.None {
-		return fmt.Errorf("cannot use %x as member id", raft.None)
-	}
 
 	// Advertised peer URLs must match those in the cluster peer list
 	// TODO: Remove URLStringsEqual after improvement of using hostnames #2150 #2123
 	apurls := c.PeerURLs.StringSlice()
 	sort.Strings(apurls)
+	urls.Sort()
 	if strict {
-		if !netutil.URLStringsEqual(apurls, m.PeerURLs) {
+		if !netutil.URLStringsEqual(apurls, urls.StringSlice()) {
 			return fmt.Errorf("%s has different advertised URLs in the cluster and advertised peer URLs list", c.Name)
 		}
 	}
@@ -135,6 +133,20 @@ func (c *ServerConfig) print(initial bool) {
 	log.Printf("etcdserver: advertise client URLs = %s", c.ClientURLs)
 	if initial {
 		log.Printf("etcdserver: initial advertise peer URLs = %s", c.PeerURLs)
-		log.Printf("etcdserver: initial cluster = %s", c.Cluster)
+		log.Printf("etcdserver: initial cluster = %s", c.InitialPeerURLsMap)
+	}
+}
+
+func checkDuplicateURL(urlsmap types.URLsMap) bool {
+	um := make(map[string]bool)
+	for _, urls := range urlsmap {
+		for _, url := range urls {
+			u := url.String()
+			if um[u] {
+				return true
+			}
+			um[u] = true
+		}
 	}
+	return false
 }

etcdserver/config_test.go

@@ -33,14 +33,10 @@ func mustNewURLs(t *testing.T, urls []string) []url.URL {
 }
 
 func TestConfigVerifyBootstrapWithoutClusterAndDiscoveryURLFail(t *testing.T) {
-	cluster, err := NewClusterFromString("", "")
-	if err != nil {
-		t.Fatalf("NewClusterFromString error: %v", err)
-	}
 	c := &ServerConfig{
-		Name:         "node1",
-		DiscoveryURL: "",
-		Cluster:      cluster,
+		Name:               "node1",
+		DiscoveryURL:       "",
+		InitialPeerURLsMap: types.URLsMap{},
 	}
 	if err := c.VerifyBootstrap(); err == nil {
 		t.Errorf("err = nil, want not nil")
@@ -48,16 +44,16 @@ func TestConfigVerifyBootstrapWithoutClusterAndDiscoveryURLFail(t *testing.T) {
 }
 
 func TestConfigVerifyExistingWithDiscoveryURLFail(t *testing.T) {
-	cluster, err := NewClusterFromString("", "node1=http://127.0.0.1:2380")
+	cluster, err := types.NewURLsMap("node1=http://127.0.0.1:2380")
 	if err != nil {
-		t.Fatalf("NewClusterFromString error: %v", err)
+		t.Fatalf("NewCluster error: %v", err)
 	}
 	c := &ServerConfig{
-		Name:         "node1",
-		DiscoveryURL: "http://127.0.0.1:2379/abcdefg",
-		PeerURLs:     mustNewURLs(t, []string{"http://127.0.0.1:2380"}),
-		Cluster:      cluster,
-		NewCluster:   false,
+		Name:               "node1",
+		DiscoveryURL:       "http://127.0.0.1:2379/abcdefg",
+		PeerURLs:           mustNewURLs(t, []string{"http://127.0.0.1:2380"}),
+		InitialPeerURLsMap: cluster,
+		NewCluster:         false,
 	}
 	if err := c.VerifyJoinExisting(); err == nil {
 		t.Errorf("err = nil, want not nil")
@@ -130,20 +126,19 @@ func TestConfigVerifyLocalMember(t *testing.T) {
 	}
 
 	for i, tt := range tests {
-		cluster, err := NewClusterFromString("", tt.clusterSetting)
+		cluster, err := types.NewURLsMap(tt.clusterSetting)
 		if err != nil {
 			t.Fatalf("#%d: Got unexpected error: %v", i, err)
 		}
 		cfg := ServerConfig{
-			Name:    "node1",
-			Cluster: cluster,
+			Name:               "node1",
+			InitialPeerURLsMap: cluster,
 		}
 		if tt.apurls != nil {
 			cfg.PeerURLs = mustNewURLs(t, tt.apurls)
 		}
 		err = cfg.verifyLocalMember(tt.strict)
 		if (err == nil) && tt.shouldError {
-			t.Errorf("%#v", *cluster)
 			t.Errorf("#%d: Got no error where one was expected", i)
 		}
 		if (err != nil) && !tt.shouldError {

etcdserver/raft.go

@@ -192,13 +192,13 @@ func (r *raftNode) resumeSending() {
 	p.Resume()
 }
 
-func startNode(cfg *ServerConfig, ids []types.ID) (id types.ID, n raft.Node, s *raft.MemoryStorage, w *wal.WAL) {
+func startNode(cfg *ServerConfig, cl *Cluster, ids []types.ID) (id types.ID, n raft.Node, s *raft.MemoryStorage, w *wal.WAL) {
 	var err error
-	member := cfg.Cluster.MemberByName(cfg.Name)
+	member := cl.MemberByName(cfg.Name)
 	metadata := pbutil.MustMarshal(
 		&pb.Metadata{
 			NodeID:    uint64(member.ID),
-			ClusterID: uint64(cfg.Cluster.ID()),
+			ClusterID: uint64(cl.ID()),
 		},
 	)
 	if err := os.MkdirAll(cfg.SnapDir(), privateDirMode); err != nil {
@@ -209,14 +209,14 @@ func startNode(cfg *ServerConfig, ids []types.ID) (id types.ID, n raft.Node, s *
 	}
 	peers := make([]raft.Peer, len(ids))
 	for i, id := range ids {
-		ctx, err := json.Marshal((*cfg.Cluster).Member(id))
+		ctx, err := json.Marshal((*cl).Member(id))
 		if err != nil {
 			log.Panicf("marshal member should never fail: %v", err)
 		}
 		peers[i] = raft.Peer{ID: uint64(id), Context: ctx}
 	}
 	id = member.ID
-	log.Printf("etcdserver: start member %s in cluster %s", id, cfg.Cluster.ID())
+	log.Printf("etcdserver: start member %s in cluster %s", id, cl.ID())
 	s = raft.NewMemoryStorage()
 	c := &raft.Config{
 		ID:              uint64(id),
@@ -231,15 +231,16 @@ func startNode(cfg *ServerConfig, ids []types.ID) (id types.ID, n raft.Node, s *
 	return
 }
 
-func restartNode(cfg *ServerConfig, snapshot *raftpb.Snapshot) (types.ID, raft.Node, *raft.MemoryStorage, *wal.WAL) {
+func restartNode(cfg *ServerConfig, snapshot *raftpb.Snapshot) (types.ID, *Cluster, raft.Node, *raft.MemoryStorage, *wal.WAL) {
 	var walsnap walpb.Snapshot
 	if snapshot != nil {
 		walsnap.Index, walsnap.Term = snapshot.Metadata.Index, snapshot.Metadata.Term
 	}
 	w, id, cid, st, ents := readWAL(cfg.WALDir(), walsnap)
-	cfg.Cluster.SetID(cid)
 
-	log.Printf("etcdserver: restart member %s in cluster %s at commit index %d", id, cfg.Cluster.ID(), st.Commit)
+	log.Printf("etcdserver: restart member %s in cluster %s at commit index %d", id, cid, st.Commit)
+	cl := newCluster("")
+	cl.SetID(cid)
 	s := raft.NewMemoryStorage()
 	if snapshot != nil {
 		s.ApplySnapshot(*snapshot)
@@ -256,16 +257,15 @@ func restartNode(cfg *ServerConfig, snapshot *raftpb.Snapshot) (types.ID, raft.N
 	}
 	n := raft.RestartNode(c)
 	raftStatus = n.Status
-	return id, n, s, w
+	return id, cl, n, s, w
 }
 
-func restartAsStandaloneNode(cfg *ServerConfig, snapshot *raftpb.Snapshot) (types.ID, raft.Node, *raft.MemoryStorage, *wal.WAL) {
+func restartAsStandaloneNode(cfg *ServerConfig, snapshot *raftpb.Snapshot) (types.ID, *Cluster, raft.Node, *raft.MemoryStorage, *wal.WAL) {
 	var walsnap walpb.Snapshot
 	if snapshot != nil {
 		walsnap.Index, walsnap.Term = snapshot.Metadata.Index, snapshot.Metadata.Term
 	}
 	w, id, cid, st, ents := readWAL(cfg.WALDir(), walsnap)
-	cfg.Cluster.SetID(cid)
 
 	// discard the previously uncommitted entries
 	for i, ent := range ents {
@@ -289,7 +289,9 @@ func restartAsStandaloneNode(cfg *ServerConfig, snapshot *raftpb.Snapshot) (type
 		st.Commit = ents[len(ents)-1].Index
 	}
 
-	log.Printf("etcdserver: forcing restart of member %s in cluster %s at commit index %d", id, cfg.Cluster.ID(), st.Commit)
+	log.Printf("etcdserver: forcing restart of member %s in cluster %s at commit index %d", id, cid, st.Commit)
+	cl := newCluster("")
+	cl.SetID(cid)
 	s := raft.NewMemoryStorage()
 	if snapshot != nil {
 		s.ApplySnapshot(*snapshot)
@@ -306,7 +308,7 @@ func restartAsStandaloneNode(cfg *ServerConfig, snapshot *raftpb.Snapshot) (type
 	}
 	n := raft.RestartNode(c)
 	raftStatus = n.Status
-	return id, n, s, w
+	return id, cl, n, s, w
 }
 
 // getIDs returns an ordered set of IDs included in the given snapshot and

etcdserver/server.go

@@ -178,6 +178,7 @@ func NewServer(cfg *ServerConfig) (*EtcdServer, error) {
 	var n raft.Node
 	var s *raft.MemoryStorage
 	var id types.ID
+	var cl *Cluster
 
 	// Run the migrations.
 	dataVer, err := version.DetectDataDir(cfg.DataDir)
@@ -197,41 +198,53 @@ func NewServer(cfg *ServerConfig) (*EtcdServer, error) {
 		if err := cfg.VerifyJoinExisting(); err != nil {
 			return nil, err
 		}
-		existingCluster, err := GetClusterFromRemotePeers(getRemotePeerURLs(cfg.Cluster, cfg.Name), cfg.Transport)
+		cl, err = NewCluster(cfg.InitialClusterToken, cfg.InitialPeerURLsMap)
+		if err != nil {
+			return nil, err
+		}
+		existingCluster, err := GetClusterFromRemotePeers(getRemotePeerURLs(cl, cfg.Name), cfg.Transport)
 		if err != nil {
 			return nil, fmt.Errorf("cannot fetch cluster info from peer urls: %v", err)
 		}
-		if err := ValidateClusterAndAssignIDs(cfg.Cluster, existingCluster); err != nil {
+		if err := ValidateClusterAndAssignIDs(cl, existingCluster); err != nil {
 			return nil, fmt.Errorf("error validating peerURLs %s: %v", existingCluster, err)
 		}
 		remotes = existingCluster.Members()
-		cfg.Cluster.SetID(existingCluster.id)
-		cfg.Cluster.SetStore(st)
+		cl.SetID(existingCluster.id)
+		cl.SetStore(st)
 		cfg.Print()
-		id, n, s, w = startNode(cfg, nil)
+		id, n, s, w = startNode(cfg, cl, nil)
 	case !haveWAL && cfg.NewCluster:
 		if err := cfg.VerifyBootstrap(); err != nil {
 			return nil, err
 		}
-		m := cfg.Cluster.MemberByName(cfg.Name)
-		if isMemberBootstrapped(cfg.Cluster, cfg.Name, cfg.Transport) {
+		cl, err = NewCluster(cfg.InitialClusterToken, cfg.InitialPeerURLsMap)
+		if err != nil {
+			return nil, err
+		}
+		m := cl.MemberByName(cfg.Name)
+		if isMemberBootstrapped(cl, cfg.Name, cfg.Transport) {
 			return nil, fmt.Errorf("member %s has already been bootstrapped", m.ID)
 		}
 		if cfg.ShouldDiscover() {
-			str, err := discovery.JoinCluster(cfg.DiscoveryURL, cfg.DiscoveryProxy, m.ID, cfg.Cluster.String())
+			str, err := discovery.JoinCluster(cfg.DiscoveryURL, cfg.DiscoveryProxy, m.ID, cfg.InitialPeerURLsMap.String())
 			if err != nil {
 				return nil, err
 			}
-			if cfg.Cluster, err = NewClusterFromString(cfg.Cluster.token, str); err != nil {
+			urlsmap, err := types.NewURLsMap(str)
+			if err != nil {
 				return nil, err
 			}
-			if err := cfg.Cluster.Validate(); err != nil {
-				return nil, fmt.Errorf("bad discovery cluster: %v", err)
+			if checkDuplicateURL(urlsmap) {
+				return nil, fmt.Errorf("discovery cluster %s has duplicate url", urlsmap)
+			}
+			if cl, err = NewCluster(cfg.InitialClusterToken, urlsmap); err != nil {
+				return nil, err
 			}
 		}
-		cfg.Cluster.SetStore(st)
+		cl.SetStore(st)
 		cfg.PrintWithInitial()
-		id, n, s, w = startNode(cfg, cfg.Cluster.MemberIDs())
+		id, n, s, w = startNode(cfg, cl, cl.MemberIDs())
 	case haveWAL:
 		if err := fileutil.IsDirWriteable(cfg.DataDir); err != nil {
 			return nil, fmt.Errorf("cannot write to data directory: %v", err)
@@ -254,16 +267,17 @@ func NewServer(cfg *ServerConfig) (*EtcdServer, error) {
 			}
 			log.Printf("etcdserver: recovered store from snapshot at index %d", snapshot.Metadata.Index)
 		}
-		cfg.Cluster = NewClusterFromStore(cfg.Cluster.token, st)
 		cfg.Print()
 		if snapshot != nil {
-			log.Printf("etcdserver: loaded cluster information from store: %s", cfg.Cluster)
+			log.Printf("etcdserver: loaded cluster information from store: %s", cl)
 		}
 		if !cfg.ForceNewCluster {
-			id, n, s, w = restartNode(cfg, snapshot)
+			id, cl, n, s, w = restartNode(cfg, snapshot)
 		} else {
-			id, n, s, w = restartAsStandaloneNode(cfg, snapshot)
+			id, cl, n, s, w = restartAsStandaloneNode(cfg, snapshot)
 		}
+		cl.SetStore(st)
+		cl.Recover()
 	default:
 		return nil, fmt.Errorf("unsupported bootstrap config")
 	}
@@ -288,7 +302,7 @@ func NewServer(cfg *ServerConfig) (*EtcdServer, error) {
 		},
 		id:            id,
 		attributes:    Attributes{Name: cfg.Name, ClientURLs: cfg.ClientURLs.StringSlice()},
-		Cluster:       cfg.Cluster,
+		Cluster:       cl,
 		stats:         sstats,
 		lstats:        lstats,
 		SyncTicker:    time.Tick(500 * time.Millisecond),
@@ -297,14 +311,14 @@ func NewServer(cfg *ServerConfig) (*EtcdServer, error) {
 	}
 
 	// TODO: move transport initialization near the definition of remote
-	tr := rafthttp.NewTransporter(cfg.Transport, id, cfg.Cluster.ID(), srv, srv.errorc, sstats, lstats)
+	tr := rafthttp.NewTransporter(cfg.Transport, id, cl.ID(), srv, srv.errorc, sstats, lstats)
 	// add all remotes into transport
 	for _, m := range remotes {
 		if m.ID != id {
 			tr.AddRemote(m.ID, m.PeerURLs)
 		}
 	}
-	for _, m := range cfg.Cluster.Members() {
+	for _, m := range cl.Members() {
 		if m.ID != id {
 			tr.AddPeer(m.ID, m.PeerURLs)
 		}

integration/cluster_test.go

@@ -301,7 +301,7 @@ type cluster struct {
 	Members []*member
 }
 
-func fillClusterForMembers(ms []*member, cName string) error {
+func fillClusterForMembers(ms []*member) error {
 	addrs := make([]string, 0)
 	for _, m := range ms {
 		scheme := "http"
@@ -315,7 +315,7 @@ func fillClusterForMembers(ms []*member, cName string) error {
 	clusterStr := strings.Join(addrs, ",")
 	var err error
 	for _, m := range ms {
-		m.Cluster, err = etcdserver.NewClusterFromString(cName, clusterStr)
+		m.InitialPeerURLsMap, err = types.NewURLsMap(clusterStr)
 		if err != nil {
 			return err
 		}
@@ -330,7 +330,7 @@ func newCluster(t *testing.T, size int, usePeerTLS bool) *cluster {
 		ms[i] = mustNewMember(t, c.name(i), usePeerTLS)
 	}
 	c.Members = ms
-	if err := fillClusterForMembers(c.Members, clusterName); err != nil {
+	if err := fillClusterForMembers(c.Members); err != nil {
 		t.Fatal(err)
 	}
 
@@ -420,7 +420,6 @@ func (c *cluster) HTTPMembers() []client.Member {
 }
 
 func (c *cluster) addMember(t *testing.T, usePeerTLS bool) {
-	clusterStr := c.Members[0].Cluster.String()
 	m := mustNewMember(t, c.name(rand.Int()), usePeerTLS)
 	scheme := "http"
 	if usePeerTLS {
@@ -441,14 +440,11 @@ func (c *cluster) addMember(t *testing.T, usePeerTLS bool) {
 	members := append(c.HTTPMembers(), client.Member{PeerURLs: []string{peerURL}, ClientURLs: []string{}})
 	c.waitMembersMatch(t, members)
 
-	for _, ln := range m.PeerListeners {
-		clusterStr += fmt.Sprintf(",%s=%s://%s", m.Name, scheme, ln.Addr().String())
-	}
-	var err error
-	m.Cluster, err = etcdserver.NewClusterFromString(clusterName, clusterStr)
-	if err != nil {
-		t.Fatal(err)
+	m.InitialPeerURLsMap = types.URLsMap{}
+	for _, mm := range c.Members {
+		m.InitialPeerURLsMap[mm.Name] = mm.PeerURLs
 	}
+	m.InitialPeerURLsMap[m.Name] = m.PeerURLs
 	m.NewCluster = false
 	if err := m.Launch(); err != nil {
 		t.Fatal(err)
@@ -645,10 +641,11 @@ func mustNewMember(t *testing.T, name string, usePeerTLS bool) *member {
 		t.Fatal(err)
 	}
 	clusterStr := fmt.Sprintf("%s=%s://%s", name, peerScheme, pln.Addr().String())
-	m.Cluster, err = etcdserver.NewClusterFromString(clusterName, clusterStr)
+	m.InitialPeerURLsMap, err = types.NewURLsMap(clusterStr)
 	if err != nil {
 		t.Fatal(err)
 	}
+	m.InitialClusterToken = clusterName
 	m.NewCluster = true
 	m.Transport = mustNewTransport(t, m.PeerTLSInfo)
 	m.ElectionTicks = electionTicks
@@ -675,12 +672,13 @@ func (m *member) Clone(t *testing.T) *member {
 		// this should never fail
 		panic(err)
 	}
-	clusterStr := m.Cluster.String()
-	mm.Cluster, err = etcdserver.NewClusterFromString(clusterName, clusterStr)
+	clusterStr := m.InitialPeerURLsMap.String()
+	mm.InitialPeerURLsMap, err = types.NewURLsMap(clusterStr)
 	if err != nil {
 		// this should never fail
 		panic(err)
 	}
+	mm.InitialClusterToken = m.InitialClusterToken
 	mm.Transport = mustNewTransport(t, m.PeerTLSInfo)
 	mm.ElectionTicks = m.ElectionTicks
 	mm.PeerTLSInfo = m.PeerTLSInfo

pkg/types/urlsmap.go

@@ -0,0 +1,71 @@
+// Copyright 2015 CoreOS, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package types
+
+import (
+	"fmt"
+	"net/url"
+	"sort"
+	"strings"
+)
+
+type URLsMap map[string]URLs
+
+// NewURLsMap returns a URLsMap instantiated from the given string,
+// which consists of discovery-formatted names-to-URLs, like:
+// mach0=http://1.1.1.1,mach0=http://2.2.2.2,mach1=http://3.3.3.3,mach2=http://4.4.4.4
+func NewURLsMap(s string) (URLsMap, error) {
+	cl := URLsMap{}
+	v, err := url.ParseQuery(strings.Replace(s, ",", "&", -1))
+	if err != nil {
+		return nil, err
+	}
+	for name, urls := range v {
+		if len(urls) == 0 || urls[0] == "" {
+			return nil, fmt.Errorf("empty URL given for %q", name)
+		}
+		us, err := NewURLs(urls)
+		if err != nil {
+			return nil, err
+		}
+		cl[name] = us
+	}
+	return cl, nil
+}
+
+// String returns NameURLPairs into discovery-formatted name-to-URLs sorted by name.
+func (c URLsMap) String() string {
+	pairs := make([]string, 0)
+	for name, urls := range c {
+		for _, url := range urls {
+			pairs = append(pairs, fmt.Sprintf("%s=%s", name, url.String()))
+		}
+	}
+	sort.Strings(pairs)
+	return strings.Join(pairs, ",")
+}
+
+// URLs returns a list of all URLs.
+// The returned list is sorted in ascending lexicographical order.
+func (c URLsMap) URLs() []string {
+	urls := make([]string, 0)
+	for _, us := range c {
+		for _, u := range us {
+			urls = append(urls, u.String())
+		}
+	}
+	sort.Strings(urls)
+	return urls
+}

pkg/types/urlsmap_test.go

@@ -0,0 +1,69 @@
+// Copyright 2015 CoreOS, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package types
+
+import (
+	"reflect"
+	"testing"
+
+	"github.com/coreos/etcd/pkg/testutil"
+)
+
+func TestParseInitialCluster(t *testing.T) {
+	c, err := NewURLsMap("mem1=http://10.0.0.1:2379,mem1=http://128.193.4.20:2379,mem2=http://10.0.0.2:2379,default=http://127.0.0.1:2379")
+	if err != nil {
+		t.Fatalf("unexpected parse error: %v", err)
+	}
+	wc := URLsMap(map[string]URLs{
+		"mem1":    testutil.MustNewURLs(t, []string{"http://10.0.0.1:2379", "http://128.193.4.20:2379"}),
+		"mem2":    testutil.MustNewURLs(t, []string{"http://10.0.0.2:2379"}),
+		"default": testutil.MustNewURLs(t, []string{"http://127.0.0.1:2379"}),
+	})
+	if !reflect.DeepEqual(c, wc) {
+		t.Errorf("cluster = %+v, want %+v", c, wc)
+	}
+}
+
+func TestParseInitialClusterBad(t *testing.T) {
+	tests := []string{
+		// invalid URL
+		"%^",
+		// no URL defined for member
+		"mem1=,mem2=http://128.193.4.20:2379,mem3=http://10.0.0.2:2379",
+		"mem1,mem2=http://128.193.4.20:2379,mem3=http://10.0.0.2:2379",
+		// bad URL for member
+		"default=http://localhost/",
+	}
+	for i, tt := range tests {
+		if _, err := NewURLsMap(tt); err == nil {
+			t.Errorf("#%d: unexpected successful parse, want err", i)
+		}
+	}
+}
+
+func TestNameURLPairsString(t *testing.T) {
+	cls := URLsMap(map[string]URLs{
+		"abc": testutil.MustNewURLs(t, []string{"http://1.1.1.1:1111", "http://0.0.0.0:0000"}),
+		"def": testutil.MustNewURLs(t, []string{"http://2.2.2.2:2222"}),
+		"ghi": testutil.MustNewURLs(t, []string{"http://3.3.3.3:1234", "http://127.0.0.1:2380"}),
+		// no PeerURLs = not included
+		"four": testutil.MustNewURLs(t, []string{}),
+		"five": testutil.MustNewURLs(t, nil),
+	})
+	w := "abc=http://0.0.0.0:0000,abc=http://1.1.1.1:1111,def=http://2.2.2.2:2222,ghi=http://127.0.0.1:2380,ghi=http://3.3.3.3:1234"
+	if g := cls.String(); g != w {
+		t.Fatalf("NameURLPairs.String():\ngot  %#v\nwant %#v", g, w)
+	}
+}