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

@@ -261,7 +261,9 @@ func (r *MachineReconciler) reconcileDelete(ctx context.Context, cluster *cluste
 	logger := r.Log.WithValues("machine", m.Name, "namespace", m.Namespace)
 	logger = logger.WithValues("cluster", cluster.Name)
 
-	if err := r.isDeleteNodeAllowed(ctx, m); err != nil {
+	err := r.isDeleteNodeAllowed(ctx, m)
+	isDeleteNodeAllowed := err == nil
+	if err != nil {
 		switch err {
 		case errNilNodeRef:
 			logger.Error(err, "Deleting node is not allowed")
@@ -271,7 +273,9 @@ func (r *MachineReconciler) reconcileDelete(ctx context.Context, cluster *cluste
 			logger.Error(err, "IsDeleteNodeAllowed check failed")
 			return ctrl.Result{}, err
 		}
-	} else {
+	}
+
+	if isDeleteNodeAllowed {
 		// Drain node before deletion
 		if _, exists := m.ObjectMeta.Annotations[clusterv1.ExcludeNodeDrainingAnnotation]; !exists {
 			logger.Info("Draining node", "node", m.Status.NodeRef.Name)
@@ -281,6 +285,17 @@ 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)
 		}
+	}
+
+	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
+	}
+
+	// We only delete the node after the underlying infrastructure is gone.
+	// https://github.com/kubernetes-sigs/cluster-api/issues/2565
+	if isDeleteNodeAllowed {
 		logger.Info("Deleting node", "node", m.Status.NodeRef.Name)
 
 		var deleteNodeErr error
@@ -296,17 +311,11 @@ 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
 }
 
-// isDeleteNodeAllowed returns nil only if the Machine's NodeRef is not nil
+// isDeleteNodeAllowed returns true 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 *MachineReconciler) isDeleteNodeAllowed(ctx context.Context, machine *clusterv1.Machine) error {
 	// Cannot delete something that doesn't exist.

controllers/machine_controller_test.go

@@ -784,3 +784,169 @@ func Test_clusterToActiveMachines(t *testing.T) {
 		g.Expect(got).To(Equal(tt.want))
 	}
 }
+
+func TestIsDeleteNodeAllowed(t *testing.T) {
+	testCases := []struct {
+		name          string
+		machine       *clusterv1.Machine
+		expectedError error
+	}{
+		{
+			name: "machine without nodeRef",
+			machine: &clusterv1.Machine{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "created",
+					Namespace:  "default",
+					Finalizers: []string{clusterv1.MachineFinalizer, metav1.FinalizerDeleteDependents},
+				},
+				Spec: clusterv1.MachineSpec{
+					ClusterName:       "test-cluster",
+					InfrastructureRef: corev1.ObjectReference{},
+					Bootstrap:         clusterv1.Bootstrap{Data: pointer.StringPtr("data")},
+				},
+				Status: clusterv1.MachineStatus{},
+			},
+			expectedError: errNilNodeRef,
+		},
+		{
+			name: "no control plane members",
+			machine: &clusterv1.Machine{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "created",
+					Namespace:  "default",
+					Finalizers: []string{clusterv1.MachineFinalizer, metav1.FinalizerDeleteDependents},
+				},
+				Spec: clusterv1.MachineSpec{
+					ClusterName:       "test-cluster",
+					InfrastructureRef: corev1.ObjectReference{},
+					Bootstrap:         clusterv1.Bootstrap{Data: pointer.StringPtr("data")},
+				},
+				Status: clusterv1.MachineStatus{
+					NodeRef: &corev1.ObjectReference{
+						Name: "test",
+					},
+				},
+			},
+			expectedError: errNoControlPlaneNodes,
+		},
+		{
+			name: "is last control plane members",
+			machine: &clusterv1.Machine{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "created",
+					Namespace: "default",
+					Labels: map[string]string{
+						clusterv1.ClusterLabelName:             "test",
+						clusterv1.MachineControlPlaneLabelName: "",
+					},
+					Finalizers: []string{clusterv1.MachineFinalizer, metav1.FinalizerDeleteDependents},
+				},
+				Spec: clusterv1.MachineSpec{
+					ClusterName:       "test-cluster",
+					InfrastructureRef: corev1.ObjectReference{},
+					Bootstrap:         clusterv1.Bootstrap{Data: pointer.StringPtr("data")},
+				},
+				Status: clusterv1.MachineStatus{
+					NodeRef: &corev1.ObjectReference{
+						Name: "test",
+					},
+				},
+			},
+			expectedError: errLastControlPlaneNode,
+		},
+		{
+			name: "has nodeRef and control plane is healthy",
+			machine: &clusterv1.Machine{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "created",
+					Namespace: "default",
+					Labels: map[string]string{
+						clusterv1.ClusterLabelName: "test",
+					},
+					Finalizers: []string{clusterv1.MachineFinalizer, metav1.FinalizerDeleteDependents},
+				},
+				Spec: clusterv1.MachineSpec{
+					ClusterName:       "test-cluster",
+					InfrastructureRef: corev1.ObjectReference{},
+					Bootstrap:         clusterv1.Bootstrap{Data: pointer.StringPtr("data")},
+				},
+				Status: clusterv1.MachineStatus{
+					NodeRef: &corev1.ObjectReference{
+						Name: "test",
+					},
+				},
+			},
+			expectedError: nil,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			m1 := &clusterv1.Machine{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "cp1",
+					Namespace: "default",
+					Labels: map[string]string{
+						clusterv1.ClusterLabelName: "test",
+					},
+					Finalizers: []string{clusterv1.MachineFinalizer, metav1.FinalizerDeleteDependents},
+				},
+				Spec: clusterv1.MachineSpec{
+					ClusterName:       "test-cluster",
+					InfrastructureRef: corev1.ObjectReference{},
+					Bootstrap:         clusterv1.Bootstrap{Data: pointer.StringPtr("data")},
+				},
+				Status: clusterv1.MachineStatus{
+					NodeRef: &corev1.ObjectReference{
+						Name: "test1",
+					},
+				},
+			}
+			m2 := &clusterv1.Machine{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "cp2",
+					Namespace: "default",
+					Labels: map[string]string{
+						clusterv1.ClusterLabelName: "test",
+					},
+					Finalizers: []string{clusterv1.MachineFinalizer, metav1.FinalizerDeleteDependents},
+				},
+				Spec: clusterv1.MachineSpec{
+					ClusterName:       "test-cluster",
+					InfrastructureRef: corev1.ObjectReference{},
+					Bootstrap:         clusterv1.Bootstrap{Data: pointer.StringPtr("data")},
+				},
+				Status: clusterv1.MachineStatus{
+					NodeRef: &corev1.ObjectReference{
+						Name: "test2",
+					},
+				},
+			}
+			// For isDeleteNodeAllowed to be true we assume a healthy control plane.
+			if tc.expectedError == nil {
+				m1.Labels[clusterv1.MachineControlPlaneLabelName] = ""
+				m2.Labels[clusterv1.MachineControlPlaneLabelName] = ""
+			}
+
+			mr := &MachineReconciler{
+				Client: fake.NewFakeClientWithScheme(
+					scheme.Scheme,
+					tc.machine,
+					m1,
+					m2,
+				),
+				Log:    log.Log,
+				scheme: scheme.Scheme,
+			}
+
+			err := mr.isDeleteNodeAllowed(context.TODO(), tc.machine)
+			if tc.expectedError == nil {
+				g.Expect(err).To(BeNil())
+			} else {
+				g.Expect(err).To(Equal(tc.expectedError))
+			}
+		})
+	}
+}