Ensure infrastructure is destroyed before deleting the node

If there were an edge case where we deleted an unreachable node and therefore removing any pod from etcd, but an stateful process were still running on the underlying host, we'll be immediately freeing up the pod name from the apiserver. "This would let the StatefulSet controller create a replacement Pod with that same identity; this can lead to the duplication of a still-running Pod, and if said Pod can still communicate with the other members of the StatefulSet, will violate the at most one semantics that StatefulSet is designed to guarantee."

To mitigate this it'd be safer to delete the node only after the owned infraMachine is gone. This would give stronger guarantees that the underlying host is destroyed and so there's no chance of a stateful "leaked" process still running.

kubernetes-sigs#2565

Author: enxebre

controllers/machine_controller.go

@@ -281,8 +281,16 @@ func (r *MachineReconciler) reconcileDelete(ctx context.Context, cluster *cluste
 			}
 			r.recorder.Eventf(m, corev1.EventTypeNormal, "SuccessfulDrainNode", "success draining Machine's node %q", m.Status.NodeRef.Name)
 		}
-		logger.Info("Deleting node", "node", m.Status.NodeRef.Name)
 
+		// Verify infrastructure is destroyed before attempting to delete the node.
+		// https://github.com/kubernetes-sigs/cluster-api/issues/2565
+		if ok, err := r.reconcileDeleteExternal(ctx, m); !ok || err != nil {
+			// Return early and don't remove the finalizer if we got an error or
+			// the external reconciliation deletion isn't ready.
+			return ctrl.Result{}, err
+		}
+
+		logger.Info("Deleting node", "node", m.Status.NodeRef.Name)
 		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) {
@@ -296,12 +304,6 @@ func (r *MachineReconciler) reconcileDelete(ctx context.Context, cluster *cluste
 		}
 	}
 
-	if ok, err := r.reconcileDeleteExternal(ctx, m); !ok || err != nil {
-		// Return early and don't remove the finalizer if we got an error or
-		// the external reconciliation deletion isn't ready.
-		return ctrl.Result{}, err
-	}
-
 	controllerutil.RemoveFinalizer(m, clusterv1.MachineFinalizer)
 	return ctrl.Result{}, nil
 }