Merge pull request #1652 from jonboulle/fix_tests

etcdserver: sort IDs and s/getIDset/getIDs/

etcdserver/force_cluster.go

@@ -18,6 +18,7 @@ package etcdserver
 
 import (
 	"log"
+	"sort"
 
 	"github.com/coreos/etcd/pkg/pbutil"
 	"github.com/coreos/etcd/pkg/types"
@@ -43,7 +44,7 @@ func restartAsStandaloneNode(cfg *ServerConfig, index uint64, snapshot *raftpb.S
 	}
 
 	// force append the configuration change entries
-	toAppEnts := createConfigChangeEnts(getIDset(snapshot, ents), uint64(id), st.Term, st.Commit)
+	toAppEnts := createConfigChangeEnts(getIDs(snapshot, ents), uint64(id), st.Term, st.Commit)
 	ents = append(ents, toAppEnts...)
 
 	// force commit newly appended entries
@@ -62,12 +63,12 @@ func restartAsStandaloneNode(cfg *ServerConfig, index uint64, snapshot *raftpb.S
 	return
 }
 
-// getIDset returns a set of IDs included in the given snapshot and the entries.
-// The given snapshot contians a list of IDs.
-// The given entries might contain two kinds of ID related entry.
-// If the entry type is Add, the contained ID will be added into the set.
-// If the entry type is Remove, the contained ID will be removed from the set.
-func getIDset(snap *raftpb.Snapshot, ents []raftpb.Entry) map[uint64]bool {
+// getIDs returns an ordered set of IDs included in the given snapshot and
+// the entries. The given snapshot/entries can contain two kinds of
+// ID-related entry:
+// - ConfChangeAddNode, in which case the contained ID will be added into the set.
+// - ConfChangeAddRemove, in which case the contained ID will be removed from the set.
+func getIDs(snap *raftpb.Snapshot, ents []raftpb.Entry) []uint64 {
 	ids := make(map[uint64]bool)
 	if snap != nil {
 		for _, id := range snap.Nodes {
@@ -89,13 +90,21 @@ func getIDset(snap *raftpb.Snapshot, ents []raftpb.Entry) map[uint64]bool {
 			log.Panicf("ConfChange Type should be either ConfChangeAddNode or ConfChangeRemoveNode!")
 		}
 	}
-	return ids
+	sids := make(types.Uint64Slice, 0)
+	for id := range ids {
+		sids = append(sids, id)
+	}
+	sort.Sort(sids)
+	return []uint64(sids)
 }
 
-func createConfigChangeEnts(ids map[uint64]bool, self uint64, term, index uint64) []raftpb.Entry {
+// createConfigChangeEnts creates a series of Raft entries (i.e.
+// EntryConfChange) to remove the set of given IDs from the cluster. The ID
+// `self` is _not_ removed, even if present in the set.
+func createConfigChangeEnts(ids []uint64, self uint64, term, index uint64) []raftpb.Entry {
 	ents := make([]raftpb.Entry, 0)
 	next := index + 1
-	for id := range ids {
+	for _, id := range ids {
 		if id == self {
 			continue
 		}

etcdserver/force_cluster_test.go

@@ -24,7 +24,7 @@ import (
 	"github.com/coreos/etcd/raft/raftpb"
 )
 
-func TestGetIDset(t *testing.T) {
+func TestGetIDs(t *testing.T) {
 	addcc := &raftpb.ConfChange{Type: raftpb.ConfChangeAddNode, NodeID: 2}
 	addEntry := raftpb.Entry{Type: raftpb.EntryConfChange, Data: pbutil.MustMarshal(addcc)}
 	removecc := &raftpb.ConfChange{Type: raftpb.ConfChangeRemoveNode, NodeID: 2}
@@ -35,20 +35,20 @@ func TestGetIDset(t *testing.T) {
 		snap *raftpb.Snapshot
 		ents []raftpb.Entry
 
-		widSet map[uint64]bool
+		widSet []uint64
 	}{
-		{nil, []raftpb.Entry{}, map[uint64]bool{}},
-		{&raftpb.Snapshot{Nodes: []uint64{1}}, []raftpb.Entry{}, map[uint64]bool{1: true}},
-		{&raftpb.Snapshot{Nodes: []uint64{1}}, []raftpb.Entry{addEntry}, map[uint64]bool{1: true, 2: true}},
-		{&raftpb.Snapshot{Nodes: []uint64{1}}, []raftpb.Entry{addEntry, removeEntry}, map[uint64]bool{1: true}},
-		{&raftpb.Snapshot{Nodes: []uint64{1}}, []raftpb.Entry{addEntry, normalEntry}, map[uint64]bool{1: true, 2: true}},
-		{&raftpb.Snapshot{Nodes: []uint64{1}}, []raftpb.Entry{addEntry, removeEntry, normalEntry}, map[uint64]bool{1: true}},
+		{nil, []raftpb.Entry{}, []uint64{}},
+		{&raftpb.Snapshot{Nodes: []uint64{1}}, []raftpb.Entry{}, []uint64{1}},
+		{&raftpb.Snapshot{Nodes: []uint64{1}}, []raftpb.Entry{addEntry}, []uint64{1, 2}},
+		{&raftpb.Snapshot{Nodes: []uint64{1}}, []raftpb.Entry{addEntry, removeEntry}, []uint64{1}},
+		{&raftpb.Snapshot{Nodes: []uint64{1}}, []raftpb.Entry{addEntry, normalEntry}, []uint64{1, 2}},
+		{&raftpb.Snapshot{Nodes: []uint64{1}}, []raftpb.Entry{addEntry, removeEntry, normalEntry}, []uint64{1}},
 	}
 
 	for i, tt := range tests {
-		idSet := getIDset(tt.snap, tt.ents)
+		idSet := getIDs(tt.snap, tt.ents)
 		if !reflect.DeepEqual(idSet, tt.widSet) {
-			t.Errorf("#%d: idset = %v, want %v", i, idSet, tt.widSet)
+			t.Errorf("#%d: idset = %#v, want %#v", i, idSet, tt.widSet)
 		}
 	}
 }
@@ -57,35 +57,35 @@ func TestCreateConfigChangeEnts(t *testing.T) {
 	removecc2 := &raftpb.ConfChange{Type: raftpb.ConfChangeRemoveNode, NodeID: 2}
 	removecc3 := &raftpb.ConfChange{Type: raftpb.ConfChangeRemoveNode, NodeID: 3}
 	tests := []struct {
-		ids         map[uint64]bool
+		ids         []uint64
 		self        uint64
 		term, index uint64
 
 		wents []raftpb.Entry
 	}{
 		{
-			map[uint64]bool{1: true},
+			[]uint64{1},
 			1,
 			1, 1,
 
 			[]raftpb.Entry{},
 		},
 		{
-			map[uint64]bool{1: true, 2: true},
+			[]uint64{1, 2},
 			1,
 			1, 1,
 
 			[]raftpb.Entry{{Term: 1, Index: 2, Type: raftpb.EntryConfChange, Data: pbutil.MustMarshal(removecc2)}},
 		},
 		{
-			map[uint64]bool{1: true, 2: true},
+			[]uint64{1, 2},
 			1,
 			2, 2,
 
 			[]raftpb.Entry{{Term: 2, Index: 3, Type: raftpb.EntryConfChange, Data: pbutil.MustMarshal(removecc2)}},
 		},
 		{
-			map[uint64]bool{1: true, 2: true, 3: true},
+			[]uint64{1, 2, 3},
 			1,
 			2, 2,
 
@@ -95,7 +95,7 @@ func TestCreateConfigChangeEnts(t *testing.T) {
 			},
 		},
 		{
-			map[uint64]bool{2: true, 3: true},
+			[]uint64{2, 3},
 			2,
 			2, 2,