Merge pull request #11985 from daimaxiaxie/early-bind-providerid

🐛 Machine deletion: fallback to InfraMachine providerID if Machine providerID is not set

Author: k8s-ci-robot

internal/controllers/machine/machine_controller.go

@@ -53,6 +53,7 @@ import (
 	"sigs.k8s.io/cluster-api/controllers/external"
 	"sigs.k8s.io/cluster-api/controllers/noderefutil"
 	"sigs.k8s.io/cluster-api/feature"
+	"sigs.k8s.io/cluster-api/internal/contract"
 	"sigs.k8s.io/cluster-api/internal/controllers/machine/drain"
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/annotations"
@@ -436,7 +437,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, s *scope) (ctrl.Result
 	s.deletingReason = clusterv1.MachineDeletingReason
 	s.deletingMessage = "Deletion started"
 
-	err := r.isDeleteNodeAllowed(ctx, cluster, m)
+	err := r.isDeleteNodeAllowed(ctx, cluster, m, s.infraMachine)
 	isDeleteNodeAllowed := err == nil
 	if err != nil {
 		switch err {
@@ -711,14 +712,24 @@ func (r *Reconciler) nodeVolumeDetachTimeoutExceeded(machine *clusterv1.Machine)
 
 // isDeleteNodeAllowed returns nil only if the Machine's NodeRef is not nil
 // and if the Machine is not the last control plane node in the cluster.
-func (r *Reconciler) isDeleteNodeAllowed(ctx context.Context, cluster *clusterv1.Cluster, machine *clusterv1.Machine) error {
+func (r *Reconciler) isDeleteNodeAllowed(ctx context.Context, cluster *clusterv1.Cluster, machine *clusterv1.Machine, infraMachine *unstructured.Unstructured) error {
 	log := ctrl.LoggerFrom(ctx)
 	// Return early if the cluster is being deleted.
 	if !cluster.DeletionTimestamp.IsZero() {
 		return errClusterIsBeingDeleted
 	}
 
-	if machine.Status.NodeRef == nil && machine.Spec.ProviderID != nil {
+	var providerID string
+	if machine.Spec.ProviderID != nil {
+		providerID = *machine.Spec.ProviderID
+	} else if infraMachine != nil {
+		// Fallback to retrieve from infraMachine.
+		if providerIDFromInfraMachine, err := contract.InfrastructureMachine().ProviderID().Get(infraMachine); err == nil {
+			providerID = *providerIDFromInfraMachine
+		}
+	}
+
+	if machine.Status.NodeRef == nil && providerID != "" {
 		// If we don't have a node reference, but a provider id has been set,
 		// try to retrieve the node one more time.
 		//
@@ -729,7 +740,7 @@ func (r *Reconciler) isDeleteNodeAllowed(ctx context.Context, cluster *clusterv1
 		if err != nil {
 			log.Error(err, "Failed to get cluster client while deleting Machine and checking for nodes")
 		} else {
-			node, err := r.getNode(ctx, remoteClient, *machine.Spec.ProviderID)
+			node, err := r.getNode(ctx, remoteClient, providerID)
 			if err != nil && err != ErrNodeNotFound {
 				log.Error(err, "Failed to get node while deleting Machine")
 			} else if err == nil {

internal/controllers/machine/machine_controller_test.go

@@ -2604,6 +2604,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
 		name          string
 		cluster       *clusterv1.Cluster
 		machine       *clusterv1.Machine
+		infraMachine  *unstructured.Unstructured
 		expectedError error
 	}{
 		{
@@ -2630,6 +2631,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
 				},
 				Status: clusterv1.MachineStatus{},
 			},
+			infraMachine:  nil,
 			expectedError: errNilNodeRef,
 		},
 		{
@@ -2660,6 +2662,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
 					},
 				},
 			},
+			infraMachine:  nil,
 			expectedError: errNoControlPlaneNodes,
 		},
 		{
@@ -2692,6 +2695,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
 					},
 				},
 			},
+			infraMachine:  nil,
 			expectedError: errNoControlPlaneNodes,
 		},
 		{
@@ -2722,6 +2726,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
 					},
 				},
 			},
+			infraMachine:  nil,
 			expectedError: nil,
 		},
 		{
@@ -2735,6 +2740,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
 				},
 			},
 			machine:       &clusterv1.Machine{},
+			infraMachine:  nil,
 			expectedError: errClusterIsBeingDeleted,
 		},
 		{
@@ -2773,6 +2779,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
 					},
 				},
 			},
+			infraMachine:  nil,
 			expectedError: nil,
 		},
 		{
@@ -2811,6 +2818,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
 					},
 				},
 			},
+			infraMachine:  nil,
 			expectedError: errControlPlaneIsBeingDeleted,
 		},
 		{
@@ -2849,8 +2857,40 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
 					},
 				},
 			},
+			infraMachine:  nil,
 			expectedError: errControlPlaneIsBeingDeleted,
 		},
+		{
+			name: "no nodeRef, infrastructure machine has providerID",
+			cluster: &clusterv1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-cluster",
+					Namespace: metav1.NamespaceDefault,
+				},
+			},
+			machine: &clusterv1.Machine{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "created",
+					Namespace: metav1.NamespaceDefault,
+					Labels: map[string]string{
+						clusterv1.ClusterNameLabel: "test-cluster",
+					},
+					Finalizers: []string{clusterv1.MachineFinalizer},
+				},
+				Spec: clusterv1.MachineSpec{
+					ClusterName:       "test-cluster",
+					InfrastructureRef: corev1.ObjectReference{},
+					Bootstrap:         clusterv1.Bootstrap{DataSecretName: ptr.To("data")},
+				},
+				Status: clusterv1.MachineStatus{},
+			},
+			infraMachine: &unstructured.Unstructured{Object: map[string]interface{}{
+				"spec": map[string]interface{}{
+					"providerID": "test-node-1",
+				},
+			}},
+			expectedError: nil,
+		},
 	}
 
 	emp := &unstructured.Unstructured{
@@ -2889,6 +2929,16 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
 	empBeingDeleted.SetDeletionTimestamp(&metav1.Time{Time: time.Now()})
 	empBeingDeleted.SetFinalizers([]string{"block-deletion"})
 
+	testNodeA := &corev1.Node{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "node-1",
+		},
+		Spec: corev1.NodeSpec{
+			ProviderID: "test-node-1",
+		},
+	}
+	remoteClient := fake.NewClientBuilder().WithIndex(&corev1.Node{}, "spec.providerID", index.NodeByProviderID).WithObjects(testNodeA).Build()
+
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			g := NewWithT(t)
@@ -2949,10 +2999,11 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
 				empBeingDeleted,
 			).Build()
 			mr := &Reconciler{
-				Client: c,
+				Client:       c,
+				ClusterCache: clustercache.NewFakeClusterCache(remoteClient, client.ObjectKeyFromObject(tc.cluster)),
 			}
 
-			err := mr.isDeleteNodeAllowed(ctx, tc.cluster, tc.machine)
+			err := mr.isDeleteNodeAllowed(ctx, tc.cluster, tc.machine, tc.infraMachine)
 			if tc.expectedError == nil {
 				g.Expect(err).ToNot(HaveOccurred())
 			} else {