Merge pull request #2939 from yichengq/fix-update-attr

etcdserver: allow to update attributes of removed member

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
 }
 

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}`)},

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.