Merge pull request #3057 from JoelSpeed/external-node-ids

CAPI: Do not normalize Node IDs outside of CAPI provider
kubernetes · May 27, 2020 · 0f504d3 · 0f504d3
2 parents 0255c11 + 5e0126a
commit 0f504d3
Show file tree

Hide file tree

Showing 3 changed files with 10 additions and 6 deletions.
diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller_test.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller_test.go
@@ -987,7 +987,7 @@ func TestControllerMachineSetNodeNamesUsingProviderID(t *testing.T) {
 	})
 
 	for i := range testConfig.nodes {
-		if nodeNames[i].Id != string(normalizedProviderString(testConfig.nodes[i].Spec.ProviderID)) {
+		if nodeNames[i].Id != testConfig.nodes[i].Spec.ProviderID {
 			t.Fatalf("expected %q, got %q", testConfig.nodes[i].Spec.ProviderID, nodeNames[i].Id)
 		}
 	}
@@ -1035,7 +1035,7 @@ func TestControllerMachineSetNodeNamesUsingStatusNodeRefName(t *testing.T) {
 	})
 
 	for i := range testConfig.nodes {
-		if nodeNames[i].Id != string(normalizedProviderString(testConfig.nodes[i].Spec.ProviderID)) {
+		if nodeNames[i].Id != testConfig.nodes[i].Spec.ProviderID {
 			t.Fatalf("expected %q, got %q", testConfig.nodes[i].Spec.ProviderID, nodeNames[i].Id)
 		}
 	}

diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup.go
@@ -195,10 +195,14 @@ func (ng *nodegroup) Nodes() ([]cloudprovider.Instance, error) {
 		return nil, err
 	}
 
+	// Nodes do not have normalized IDs, so do not normalize the ID here.
+	// The IDs returned here are used to check if a node is registered or not and
+	// must match the ID on the Node object itself.
+	// https://github.com/kubernetes/autoscaler/blob/a973259f1852303ba38a3a61eeee8489cf4e1b13/cluster-autoscaler/clusterstate/clusterstate.go#L967-L985
 	instances := make([]cloudprovider.Instance, len(nodes))
 	for i := range nodes {
 		instances[i] = cloudprovider.Instance{
-			Id: string(normalizedProviderString(nodes[i])),
+			Id: nodes[i],
 		}
 	}
 

diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go
@@ -659,7 +659,7 @@ func TestNodeGroupDeleteNodes(t *testing.T) {
 		})
 
 		for i := 0; i < len(nodeNames); i++ {
-			if nodeNames[i].Id != string(normalizedProviderString(testConfig.nodes[i].Spec.ProviderID)) {
+			if nodeNames[i].Id != testConfig.nodes[i].Spec.ProviderID {
 				t.Fatalf("expected %q, got %q", testConfig.nodes[i].Spec.ProviderID, nodeNames[i].Id)
 			}
 		}
@@ -844,7 +844,7 @@ func TestNodeGroupDeleteNodesTwice(t *testing.T) {
 		})
 
 		for i := 0; i < len(nodeNames); i++ {
-			if nodeNames[i].Id != string(normalizedProviderString(testConfig.nodes[i].Spec.ProviderID)) {
+			if nodeNames[i].Id != testConfig.nodes[i].Spec.ProviderID {
 				t.Fatalf("expected %q, got %q", testConfig.nodes[i].Spec.ProviderID, nodeNames[i].Id)
 			}
 		}
@@ -998,7 +998,7 @@ func TestNodeGroupWithFailedMachine(t *testing.T) {
 				nodeIndex = i
 			}
 
-			if nodeNames[i].Id != string(normalizedProviderString(testConfig.nodes[nodeIndex].Spec.ProviderID)) {
+			if nodeNames[i].Id != testConfig.nodes[nodeIndex].Spec.ProviderID {
 				t.Fatalf("expected %q, got %q", testConfig.nodes[nodeIndex].Spec.ProviderID, nodeNames[i].Id)
 			}
 		}