nodecontroller sync cloud was not setting external id in all code paths

kubernetes · Mar 3, 2015 · 8150649 · 8150649
1 parent 4263f34
commit 8150649
Show file tree

Hide file tree

Showing 3 changed files with 51 additions and 10 deletions.
diff --git a/pkg/cloudprovider/controller/nodecontroller.go b/pkg/cloudprovider/controller/nodecontroller.go
@@ -166,12 +166,17 @@ func (s *NodeController) SyncCloud() error {
 	if err != nil {
 		return err
 	}
+	matches, err = s.PopulateIPs(matches)
+	if err != nil {
+		return err
+	}
 	nodes, err := s.kubeClient.Nodes().List()
 	if err != nil {
 		return err
 	}
 	nodeMap := make(map[string]*api.Node)
-	for _, node := range nodes.Items {
+	for i := range nodes.Items {
+		node := nodes.Items[i]
 		nodeMap[node.Name] = &node
 	}
 

diff --git a/pkg/cloudprovider/controller/nodecontroller_test.go b/pkg/cloudprovider/controller/nodecontroller_test.go
@@ -366,7 +366,8 @@ func TestSyncCloud(t *testing.T) {
 		fakeCloud            *fake_cloud.FakeCloud
 		matchRE              string
 		expectedRequestCount int
-		expectedCreated      []string
+		expectedNameCreated  []string
+		expectedExtIDCreated []string
 		expectedDeleted      []string
 	}{
 		{
@@ -376,10 +377,15 @@ func TestSyncCloud(t *testing.T) {
 			},
 			fakeCloud: &fake_cloud.FakeCloud{
 				Machines: []string{"node0"},
+				ExtID: map[string]string{
+					"node0": "ext-node0",
+					"node1": "ext-node1",
+				},
 			},
 			matchRE:              ".*",
 			expectedRequestCount: 1, // List
-			expectedCreated:      []string{},
+			expectedNameCreated:  []string{},
+			expectedExtIDCreated: []string{},
 			expectedDeleted:      []string{},
 		},
 		{
@@ -389,10 +395,15 @@ func TestSyncCloud(t *testing.T) {
 			},
 			fakeCloud: &fake_cloud.FakeCloud{
 				Machines: []string{"node0", "node1"},
+				ExtID: map[string]string{
+					"node0": "ext-node0",
+					"node1": "ext-node1",
+				},
 			},
 			matchRE:              ".*",
 			expectedRequestCount: 2, // List + Create
-			expectedCreated:      []string{"node1"},
+			expectedNameCreated:  []string{"node1"},
+			expectedExtIDCreated: []string{"ext-node1"},
 			expectedDeleted:      []string{},
 		},
 		{
@@ -402,10 +413,15 @@ func TestSyncCloud(t *testing.T) {
 			},
 			fakeCloud: &fake_cloud.FakeCloud{
 				Machines: []string{"node0"},
+				ExtID: map[string]string{
+					"node0": "ext-node0",
+					"node1": "ext-node1",
+				},
 			},
 			matchRE:              ".*",
 			expectedRequestCount: 2, // List + Delete
-			expectedCreated:      []string{},
+			expectedNameCreated:  []string{},
+			expectedExtIDCreated: []string{},
 			expectedDeleted:      []string{"node1"},
 		},
 		{
@@ -415,10 +431,16 @@ func TestSyncCloud(t *testing.T) {
 			},
 			fakeCloud: &fake_cloud.FakeCloud{
 				Machines: []string{"node0", "node1", "fake"},
+				ExtID: map[string]string{
+					"node0": "ext-node0",
+					"node1": "ext-node1",
+					"fake":  "ext-fake",
+				},
 			},
 			matchRE:              "node[0-9]+",
 			expectedRequestCount: 2, // List + Create
-			expectedCreated:      []string{"node1"},
+			expectedNameCreated:  []string{"node1"},
+			expectedExtIDCreated: []string{"ext-node1"},
 			expectedDeleted:      []string{},
 		},
 	}
@@ -432,8 +454,12 @@ func TestSyncCloud(t *testing.T) {
 			t.Errorf("expected %v call, but got %v.", item.expectedRequestCount, item.fakeNodeHandler.RequestCount)
 		}
 		nodes := sortedNodeNames(item.fakeNodeHandler.CreatedNodes)
-		if !reflect.DeepEqual(item.expectedCreated, nodes) {
-			t.Errorf("expected node list %+v, got %+v", item.expectedCreated, nodes)
+		if !reflect.DeepEqual(item.expectedNameCreated, nodes) {
+			t.Errorf("expected node list %+v, got %+v", item.expectedNameCreated, nodes)
+		}
+		nodeExtIDs := sortedNodeExternalIDs(item.fakeNodeHandler.CreatedNodes)
+		if !reflect.DeepEqual(item.expectedExtIDCreated, nodeExtIDs) {
+			t.Errorf("expected node external id list %+v, got %+v", item.expectedExtIDCreated, nodeExtIDs)
 		}
 		nodes = sortedNodeNames(item.fakeNodeHandler.DeletedNodes)
 		if !reflect.DeepEqual(item.expectedDeleted, nodes) {
@@ -1071,6 +1097,15 @@ func sortedNodeNames(nodes []*api.Node) []string {
 	return nodeNames
 }
 
+func sortedNodeExternalIDs(nodes []*api.Node) []string {
+	nodeExternalIDs := []string{}
+	for _, node := range nodes {
+		nodeExternalIDs = append(nodeExternalIDs, node.Spec.ExternalID)
+	}
+	sort.Strings(nodeExternalIDs)
+	return nodeExternalIDs
+}
+
 func contains(node *api.Node, nodes []*api.Node) bool {
 	for i := 0; i < len(nodes); i++ {
 		if node.Name == nodes[i].Name {

diff --git a/pkg/cloudprovider/fake/fake.go b/pkg/cloudprovider/fake/fake.go
@@ -30,7 +30,7 @@ type FakeCloud struct {
 	Err           error
 	Calls         []string
 	IP            net.IP
-	ExtID         string
+	ExtID         map[string]string
 	Machines      []string
 	NodeResources *api.NodeResources
 	ClusterList   []string
@@ -113,9 +113,10 @@ func (f *FakeCloud) IPAddress(instance string) (net.IP, error) {
 
 // ExternalID is a test-spy implementation of Instances.ExternalID.
 // It adds an entry "external-id" into the internal method call record.
+// It returns an external id to the mapped instance name, if not found, it will return "ext-{instance}"
 func (f *FakeCloud) ExternalID(instance string) (string, error) {
 	f.addCall("external-id")
-	return f.ExtID, f.Err
+	return f.ExtID[instance], f.Err
 }
 
 // List is a test-spy implementation of Instances.List.