etcdserver: always remove member directory when bootstrap fails #4087

gyuho · 2015-12-29T05:26:47Z

This removes member directory when bootstrap fails including joining existing
cluster and forming a new cluster. This fixes coreos#3827.

xiang90 · 2015-12-29T05:35:23Z

etcdserver/server.go

@@ -272,6 +272,10 @@ func NewServer(cfg *ServerConfig) (*EtcdServer, error) {
 			var err error
 			str, err = discovery.JoinCluster(cfg.DiscoveryURL, cfg.DiscoveryProxy, m.ID, cfg.InitialPeerURLsMap.String())
 			if err != nil {
+				// It removes member directory when NewServer returns error.
+				// This prevents conflicts with 'proxy' directory when


etcdserver should have no idea about proxy. This is just a necessary cleanup step.

we should also remove member dir in another !haveWAL case.

basically in the two !haveWAL case, we should clean up member dir if bootstrap fails.

gyuho · 2015-12-29T05:52:21Z

@xiang90 I added more checks when deleting. Please take a look again and let me know.
Thanks,

gyuho · 2015-12-29T05:53:41Z

etcdserver/server.go

@@ -241,6 +250,7 @@ func NewServer(cfg *ServerConfig) (*EtcdServer, error) {
 		}
 		existingCluster, err := GetClusterFromRemotePeers(getRemotePeerURLs(cl, cfg.Name), prt)


actually we don't need this I think. What do you mean by another !haveWAL case?

xiang90 · 2015-12-29T05:54:25Z

@gyuho I should be more clear. For every error in the wal not existing case, we should remove member dir. We should only keep member dir if the new member is successfully bootstrapped. We know for sure it is a new member when wal does not perviously exists.

xiang90 · 2015-12-29T05:59:19Z

etcdserver/server.go

@@ -229,6 +229,15 @@ func NewServer(cfg *ServerConfig) (*EtcdServer, error) {
 	if err != nil {
 		return nil, err
 	}
+


I would suggest a simple way:

right after line 225

if !haveWAL { defer func() { if err != nil { // cleans up member directory if bootstrap fails (including forming or joining a new cluster) os.RemoveAll(cfg.MemberDir()) } } }

You also need to change the func signature to

func NewServer(cfg *ServerConfig) (srv *EtcdServer, err error)

xiang90 · 2015-12-29T06:10:16Z

LGTM. Let's test it, at least manually. So we know this does fix the issue reported.

gyuho · 2015-12-29T06:24:15Z

Fixing govet issues. And found that:

func (d *discovery) checkClusterRetry() ([]*client.Node, int, uint64, error) {
    if d.retries < nRetries {
        d.logAndBackoffForRetry("cluster status check")
        return d.checkCluster()
    }
    return nil, 0, 0, ErrTooManyRetries
}

is used for discovering. They just keep retrying and defer never get executed.
Will investigate more.

xiang90 · 2015-12-29T06:32:35Z

@gyuho You are right. Basically you are fixing #3827 now. To totally fix the discovery issue, we have to make sure that we do not create member dir before finishing discovery.

xiang90 · 2015-12-29T06:36:18Z

etcdserver/server.go

@@ -274,8 +284,9 @@ func NewServer(cfg *ServerConfig) (*EtcdServer, error) {
 			if err != nil {
 				return nil, &DiscoveryError{Op: "join", Err: err}
 			}
-			urlsmap, err := types.NewURLsMap(str)
-			if err != nil {
+			urlsmap, e := types.NewURLsMap(str)


gyuho · 2015-12-29T06:46:48Z

@xiang90 I tested manually injecting error to make startEtcd return when !haveWAL and confirmed that the defer statement gets executed and remove member directory.

xiang90 · 2015-12-29T06:48:49Z

etcdserver/server.go

-	prt, err := rafthttp.NewRoundTripper(cfg.PeerTLSInfo, cfg.peerDialTimeout())
-	if err != nil {
-		return nil, err
+	prt, uerr := rafthttp.NewRoundTripper(cfg.PeerTLSInfo, cfg.peerDialTimeout())


we should not call everything uerr. When there is a naming conflict for error, we usually add one prefix. For newRoundTripper, probably just rterr.

xiang90 · 2015-12-29T06:49:01Z

etcdserver/server.go

-		existingCluster, err := GetClusterFromRemotePeers(getRemotePeerURLs(cl, cfg.Name), prt)
-		if err != nil {
-			return nil, fmt.Errorf("cannot fetch cluster info from peer urls: %v", err)
+		existingCluster, uerr := GetClusterFromRemotePeers(getRemotePeerURLs(cl, cfg.Name), prt)


probably gerr

gyuho · 2015-12-29T06:54:57Z

@xiang90 All variable name conflicts and govet shadowing issues are fixed. PTAL.

xiang90 · 2015-12-29T06:55:32Z

@gyuho Can we also update the commit message?

etcdserver: always remove member directory when bootstrap fails (including joining existing cluster and forming a new cluster)

This removes member directory when bootstrap fails including joining existing cluster and forming a new cluster. This fixes etcd-io#3827.

gyuho · 2015-12-29T06:57:06Z

@xiang90 Just did. Thanks!

xiang90 · 2015-12-29T06:59:14Z

LGTM

gyuho · 2015-12-29T06:59:38Z

Thanks, will merge after CI passes.

etcdserver: always remove member directory when bootstrap fails