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

k8s-ci-robot · web-flow · commit 0f504d38c5fa · 2020-05-27T07:28:40.000-07:00
CAPI: Do not normalize Node IDs outside of CAPI provider
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)
 			}
 		}

Original file line number	Diff line number	Diff line change
`@@ -987,7 +987,7 @@ func TestControllerMachineSetNodeNamesUsingProviderID(t *testing.T) {`
`987`	`987`	`})`
`988`	`988`
`989`	`989`	`for i := range testConfig.nodes {`
`990`		`- if nodeNames[i].Id != string(normalizedProviderString(testConfig.nodes[i].Spec.ProviderID)) {`
	`990`	`+ if nodeNames[i].Id != testConfig.nodes[i].Spec.ProviderID {`
`991`	`991`	`t.Fatalf("expected %q, got %q", testConfig.nodes[i].Spec.ProviderID, nodeNames[i].Id)`
`992`	`992`	`}`
`993`	`993`	`}`
`@@ -1035,7 +1035,7 @@ func TestControllerMachineSetNodeNamesUsingStatusNodeRefName(t *testing.T) {`
`1035`	`1035`	`})`
`1036`	`1036`
`1037`	`1037`	`for i := range testConfig.nodes {`
`1038`		`- if nodeNames[i].Id != string(normalizedProviderString(testConfig.nodes[i].Spec.ProviderID)) {`
	`1038`	`+ if nodeNames[i].Id != testConfig.nodes[i].Spec.ProviderID {`
`1039`	`1039`	`t.Fatalf("expected %q, got %q", testConfig.nodes[i].Spec.ProviderID, nodeNames[i].Id)`
`1040`	`1040`	`}`
`1041`	`1041`	`}`
Original file line number	Diff line number	Diff line change
`@@ -195,10 +195,14 @@ func (ng *nodegroup) Nodes() ([]cloudprovider.Instance, error) {`
`195`	`195`	`return nil, err`
`196`	`196`	`}`
`197`	`197`
	`198`	`+ // Nodes do not have normalized IDs, so do not normalize the ID here.`
	`199`	`+ // The IDs returned here are used to check if a node is registered or not and`
	`200`	`+ // must match the ID on the Node object itself.`
	`201`	`+ // https://github.com/kubernetes/autoscaler/blob/a973259f1852303ba38a3a61eeee8489cf4e1b13/cluster-autoscaler/clusterstate/clusterstate.go#L967-L985`
`198`	`202`	`instances := make([]cloudprovider.Instance, len(nodes))`
`199`	`203`	`for i := range nodes {`
`200`	`204`	`instances[i] = cloudprovider.Instance{`
`201`		`- Id: string(normalizedProviderString(nodes[i])),`
	`205`	`+ Id: nodes[i],`
`202`	`206`	`}`
`203`	`207`	`}`
`204`	`208`
Original file line number	Diff line number	Diff line change
`@@ -659,7 +659,7 @@ func TestNodeGroupDeleteNodes(t *testing.T) {`
`659`	`659`	`})`
`660`	`660`
`661`	`661`	`for i := 0; i < len(nodeNames); i++ {`
`662`		`- if nodeNames[i].Id != string(normalizedProviderString(testConfig.nodes[i].Spec.ProviderID)) {`
	`662`	`+ if nodeNames[i].Id != testConfig.nodes[i].Spec.ProviderID {`
`663`	`663`	`t.Fatalf("expected %q, got %q", testConfig.nodes[i].Spec.ProviderID, nodeNames[i].Id)`
`664`	`664`	`}`
`665`	`665`	`}`
`@@ -844,7 +844,7 @@ func TestNodeGroupDeleteNodesTwice(t *testing.T) {`
`844`	`844`	`})`
`845`	`845`
`846`	`846`	`for i := 0; i < len(nodeNames); i++ {`
`847`		`- if nodeNames[i].Id != string(normalizedProviderString(testConfig.nodes[i].Spec.ProviderID)) {`
	`847`	`+ if nodeNames[i].Id != testConfig.nodes[i].Spec.ProviderID {`
`848`	`848`	`t.Fatalf("expected %q, got %q", testConfig.nodes[i].Spec.ProviderID, nodeNames[i].Id)`
`849`	`849`	`}`
`850`	`850`	`}`
`@@ -998,7 +998,7 @@ func TestNodeGroupWithFailedMachine(t *testing.T) {`
`998`	`998`	`nodeIndex = i`
`999`	`999`	`}`
`1000`	`1000`
`1001`		`- if nodeNames[i].Id != string(normalizedProviderString(testConfig.nodes[nodeIndex].Spec.ProviderID)) {`
	`1001`	`+ if nodeNames[i].Id != testConfig.nodes[nodeIndex].Spec.ProviderID {`
`1002`	`1002`	`t.Fatalf("expected %q, got %q", testConfig.nodes[nodeIndex].Spec.ProviderID, nodeNames[i].Id)`
`1003`	`1003`	`}`
`1004`	`1004`	`}`