🐛 MachinePool noderefs should use full provider ID for uniqueness

Author: Cecile Robert-Michon

exp/internal/controllers/machinepool_controller_noderef.go

@@ -156,15 +156,15 @@ func (r *MachinePoolReconciler) deleteRetiredNodes(ctx context.Context, c client
 			continue
 		}
 
-		nodeRefsMap[nodeProviderID.ID()] = node
+		nodeRefsMap[nodeProviderID.String()] = node
 	}
 	for _, providerID := range providerIDList {
 		pid, err := noderefutil.NewProviderID(providerID)
 		if err != nil {
 			log.V(2).Info("Failed to parse ProviderID, skipping", "err", err, "providerID", providerID)
 			continue
 		}
-		delete(nodeRefsMap, pid.ID())
+		delete(nodeRefsMap, pid.String())
 	}
 	for _, node := range nodeRefsMap {
 		if err := c.Delete(ctx, node); err != nil {
@@ -192,7 +192,7 @@ func (r *MachinePoolReconciler) getNodeReferences(ctx context.Context, c client.
 				continue
 			}
 
-			nodeRefsMap[nodeProviderID.ID()] = node
+			nodeRefsMap[nodeProviderID.String()] = node
 		}
 
 		if nodeList.Continue == "" {
@@ -207,7 +207,7 @@ func (r *MachinePoolReconciler) getNodeReferences(ctx context.Context, c client.
 			log.V(2).Info("Failed to parse ProviderID, skipping", "err", err, "providerID", providerID)
 			continue
 		}
-		if node, ok := nodeRefsMap[pid.ID()]; ok {
+		if node, ok := nodeRefsMap[pid.String()]; ok {
 			available++
 			if nodeIsReady(&node) {
 				ready++

exp/internal/controllers/machinepool_controller_noderef_test.go

@@ -66,6 +66,22 @@ func TestMachinePoolGetNodeReference(t *testing.T) {
 				ProviderID: "azure://westus2/id-node-4",
 			},
 		},
+		&corev1.Node{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "azure-nodepool1-0",
+			},
+			Spec: corev1.NodeSpec{
+				ProviderID: "azure://westus2/id-nodepool1/0",
+			},
+		},
+		&corev1.Node{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "azure-nodepool2-0",
+			},
+			Spec: corev1.NodeSpec{
+				ProviderID: "azure://westus2/id-nodepool2/0",
+			},
+		},
 	}
 
 	client := fake.NewClientBuilder().WithObjects(nodeList...).Build()
@@ -137,6 +153,25 @@ func TestMachinePoolGetNodeReference(t *testing.T) {
 				ready:      0,
 			},
 		},
+		{
+			name:           "valid provider id with non-unique instance id, valid azure node",
+			providerIDList: []string{"azure://westus2/id-nodepool1/0"},
+			expected: &getNodeReferencesResult{
+				references: []corev1.ObjectReference{
+					{Name: "azure-nodepool1-0"},
+				},
+			},
+		},
+		{
+			name:           "valid provider ids with same instance ids, valid azure nodes",
+			providerIDList: []string{"azure://westus2/id-nodepool1/0", "azure://westus2/id-nodepool2/0"},
+			expected: &getNodeReferencesResult{
+				references: []corev1.ObjectReference{
+					{Name: "azure-nodepool1-0"},
+					{Name: "azure-nodepool2-0"},
+				},
+			},
+		},
 	}
 
 	for _, test := range testCases {