Address comments

Author: Sedef

controllers/machine_controller.go

@@ -347,6 +347,16 @@ func (r *MachineReconciler) reconcileDelete(ctx context.Context, cluster *cluste
 	// Return early and don't remove the finalizer if we got an error or
 	// the external reconciliation deletion isn't ready.
 
+	patchHelper, err := patch.NewHelper(m, r.Client)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+	conditions.MarkFalse(m, clusterv1.MachineNodeHealthyCondition, clusterv1.DeletingReason, clusterv1.ConditionSeverityInfo, "")
+	if err := patchMachine(ctx, patchHelper, m); err != nil {
+		conditions.MarkFalse(m, clusterv1.MachineNodeHealthyCondition, clusterv1.DeletionFailedReason, clusterv1.ConditionSeverityInfo, "")
+		return ctrl.Result{}, errors.Wrap(err, "failed to patch Machine")
+	}
+
 	if ok, err := r.reconcileDeleteInfrastructure(ctx, m); !ok || err != nil {
 		return ctrl.Result{}, err
 	}
@@ -358,16 +368,7 @@ func (r *MachineReconciler) reconcileDelete(ctx context.Context, cluster *cluste
 	// We only delete the node after the underlying infrastructure is gone.
 	// https://github.com/kubernetes-sigs/cluster-api/issues/2565
 	if isDeleteNodeAllowed {
-		patchHelper, err := patch.NewHelper(m, r.Client)
-		if err != nil {
-			return ctrl.Result{}, err
-		}
 		logger.Info("Deleting node", "node", m.Status.NodeRef.Name)
-		conditions.MarkFalse(m, clusterv1.MachineNodeHealthyCondition, clusterv1.DeletingReason, clusterv1.ConditionSeverityInfo, "")
-		if err := patchMachine(ctx, patchHelper, m); err != nil {
-			return ctrl.Result{}, errors.Wrap(err, "failed to patch Machine")
-		}
-
 		var deleteNodeErr error
 		waitErr := wait.PollImmediate(2*time.Second, 10*time.Second, func() (bool, error) {
 			if deleteNodeErr = r.deleteNode(ctx, cluster, m.Status.NodeRef.Name); deleteNodeErr != nil && !apierrors.IsNotFound(deleteNodeErr) {

controllers/machine_controller_node.go

@@ -51,8 +51,8 @@ func (r *MachineReconciler) reconcileNode(ctx context.Context, cluster *clusterv
 	// Even if Status.NodeRef exists, continue to do the following checks to make sure Node is healthy
 	node, err := r.getNode(remoteClient, providerID)
 	if err != nil {
-		logger.Error(err, "Failed to assign NodeRef")
-		r.recorder.Event(machine, apicorev1.EventTypeWarning, "FailedSetNodeRef", err.Error())
+		logger.Error(err, "Failed to retrieve Node by ProviderID")
+		r.recorder.Event(machine, apicorev1.EventTypeWarning, "Failed to retrieve Node by ProviderID", err.Error())
 		return reconcile.Result{}, err
 	}
 	if node == nil {
@@ -61,7 +61,7 @@ func (r *MachineReconciler) reconcileNode(ctx context.Context, cluster *clusterv
 		if machine.Status.NodeRef != nil {
 			conditions.MarkFalse(machine, clusterv1.MachineNodeHealthyCondition, clusterv1.NodeNotFoundReason, clusterv1.ConditionSeverityError, "")
 		}
-		return reconcile.Result{}, errors.Wrapf(err, "cannot assign NodeRef to Machine %q in namespace %q, no matching Node", machine.Name, machine.Namespace)
+		return reconcile.Result{}, errors.Wrapf(err, "no matching Node for Machine %q in namespace %q", machine.Name, machine.Namespace)
 	}
 
 	// Set the Machine NodeRef.

controllers/machine_controller_test.go

@@ -624,7 +624,7 @@ func TestMachineConditions(t *testing.T) {
 			Finalizers: []string{clusterv1.MachineFinalizer},
 		},
 		Spec: clusterv1.MachineSpec{
-			ProviderID: pointer.StringPtr("test://id-1"),
+			ProviderID:  pointer.StringPtr("test://id-1"),
 			ClusterName: "test-cluster",
 			InfrastructureRef: corev1.ObjectReference{
 				APIVersion: "infrastructure.cluster.x-k8s.io/v1alpha3",