From 8725e69cf7773f753243a866c842ad0cb63b3553 Mon Sep 17 00:00:00 2001
From: Yicheng Qin <qycqycqycqycqyc@gmail.com>
Date: Mon, 8 Jun 2015 22:23:41 -0700
Subject: [PATCH] etcdserver: allow to update attributes of removed member

There exist the possiblity to update attributes of removed member in
reasonable workflow:
1. start member A
2. leader receives the proposal to remove member A
2. member A sends the proposal of update its attribute to the leader
3. leader commits the two proposals
So etcdserver should allow to update attributes of removed member.
---
 etcdserver/cluster.go       | 11 ++++++++++-
 etcdserver/cluster_test.go  | 36 ++++++++++++++++++++++++++++++++++++
 integration/cluster_test.go | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/etcdserver/cluster.go b/etcdserver/cluster.go
index f8fbfb403..307805165 100644
--- a/etcdserver/cluster.go
+++ b/etcdserver/cluster.go
@@ -318,7 +318,16 @@ func (c *cluster) RemoveMember(id types.ID) {
 func (c *cluster) UpdateAttributes(id types.ID, attr Attributes) {
 	c.Lock()
 	defer c.Unlock()
-	c.members[id].Attributes = attr
+	if m, ok := c.members[id]; ok {
+		m.Attributes = attr
+		return
+	}
+	_, ok := c.removed[id]
+	if ok {
+		plog.Debugf("skipped updating attributes of removed member %s", id)
+	} else {
+		plog.Panicf("error updating attributes of unknown member %s", id)
+	}
 	// TODO: update store in this function
 }
 
diff --git a/etcdserver/cluster_test.go b/etcdserver/cluster_test.go
index 796e54bbd..fe5af2afb 100644
--- a/etcdserver/cluster_test.go
+++ b/etcdserver/cluster_test.go
@@ -506,6 +506,42 @@ func TestClusterRemoveMember(t *testing.T) {
 	}
 }
 
+func TestClusterUpdateAttributes(t *testing.T) {
+	name := "etcd"
+	clientURLs := []string{"http://127.0.0.1:4001"}
+	tests := []struct {
+		mems    []*Member
+		removed map[types.ID]bool
+		wmems   []*Member
+	}{
+		// update attributes of existing member
+		{
+			[]*Member{
+				newTestMember(1, nil, "", nil),
+			},
+			nil,
+			[]*Member{
+				newTestMember(1, nil, name, clientURLs),
+			},
+		},
+		// update attributes of removed member
+		{
+			nil,
+			map[types.ID]bool{types.ID(1): true},
+			nil,
+		},
+	}
+	for i, tt := range tests {
+		c := newTestCluster(tt.mems)
+		c.removed = tt.removed
+
+		c.UpdateAttributes(types.ID(1), Attributes{Name: name, ClientURLs: clientURLs})
+		if g := c.Members(); !reflect.DeepEqual(g, tt.wmems) {
+			t.Errorf("#%d: members = %+v, want %+v", i, g, tt.wmems)
+		}
+	}
+}
+
 func TestNodeToMember(t *testing.T) {
 	n := &store.NodeExtern{Key: "/1234", Nodes: []*store.NodeExtern{
 		{Key: "/1234/attributes", Value: stringp(`{"name":"node1","clientURLs":null}`)},
diff --git a/integration/cluster_test.go b/integration/cluster_test.go
index 6c678158f..b29a2a180 100644
--- a/integration/cluster_test.go
+++ b/integration/cluster_test.go
@@ -270,6 +270,39 @@ func TestIssue2746(t *testing.T) {
 	clusterMustProgress(t, c.Members)
 }
 
+// Ensure etcd will not panic when removing a just started member.
+func TestIssue2904(t *testing.T) {
+	defer afterTest(t)
+	// start 1-member cluster to ensure member 0 is the leader of the cluster.
+	c := NewCluster(t, 1)
+	c.Launch(t)
+	defer c.Terminate(t)
+
+	c.AddMember(t)
+	c.Members[1].Stop(t)
+
+	// send remove member-1 request to the cluster.
+	cc := mustNewHTTPClient(t, c.URLs())
+	ma := client.NewMembersAPI(cc)
+	ctx, cancel := context.WithTimeout(context.Background(), requestTimeout)
+	// the proposal is not committed because member 1 is stopped, but the
+	// proposal is appended to leader's raft log.
+	ma.Remove(ctx, c.Members[1].s.ID().String())
+	cancel()
+
+	// restart member, and expect it to send updateAttr request.
+	// the log in the leader is like this:
+	// [..., remove 1, ..., update attr 1, ...]
+	c.Members[1].Restart(t)
+	// when the member comes back, it ack the proposal to remove itself,
+	// and apply it.
+	<-c.Members[1].s.StopNotify()
+
+	// wait member to be removed.
+	httpmembs := c.HTTPMembers()
+	c.waitMembersMatch(t, httpmembs[:1])
+}
+
 // clusterMustProgress ensures that cluster can make progress. It creates
 // a random key first, and check the new key could be got from all client urls
 // of the cluster.