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,8 @@ 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 {
+	isDeleteNodeAllowed, err := r.isDeleteNodeAllowed(ctx, m)
+	if err != nil {
 		switch err {
 		case errNilNodeRef:
 			logger.Error(err, "Deleting node is not allowed")
@@ -271,7 +272,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 +284,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,28 +310,22 @@ 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 {
+func (r *MachineReconciler) isDeleteNodeAllowed(ctx context.Context, machine *clusterv1.Machine) (bool, error) {
 	// Cannot delete something that doesn't exist.
 	if machine.Status.NodeRef == nil {
-		return errNilNodeRef
+		return false, errNilNodeRef
 	}
 
 	// Get all of the machines that belong to this cluster.
 	machines, err := getActiveMachinesInCluster(ctx, r.Client, machine.Namespace, machine.Labels[clusterv1.ClusterLabelName])
 	if err != nil {
-		return err
+		return false, err
 	}
 
 	// Whether or not it is okay to delete the NodeRef depends on the
@@ -327,14 +335,14 @@ func (r *MachineReconciler) isDeleteNodeAllowed(ctx context.Context, machine *cl
 	case numControlPlaneMachines == 0:
 		// Do not delete the NodeRef if there are no remaining members of
 		// the control plane.
-		return errNoControlPlaneNodes
+		return false, errNoControlPlaneNodes
 	case numControlPlaneMachines == 1 && util.IsControlPlaneMachine(machine):
 		// Do not delete the NodeRef if this is the last member of the
 		// control plane.
-		return errLastControlPlaneNode
+		return false, errLastControlPlaneNode
 	default:
 		// Otherwise it is okay to delete the NodeRef.
-		return nil
+		return true, nil
 	}
 }
 

controllers/machine_controller_test.go

@@ -784,3 +784,176 @@ 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
+		expected bool
+		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{},
+			},
+			expected: false,
+			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",
+					},
+				},
+			},
+			expected: false,
+			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",
+					},
+				},
+			},
+			expected: false,
+			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",
+					},
+				},
+			},
+			expected: true,
+			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.expected {
+				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,
+			}
+
+			isDeleteNodeAllowed, err := mr.isDeleteNodeAllowed(context.TODO(), tc.machine);
+			g.Expect(isDeleteNodeAllowed).To(Equal(tc.expected))
+			if tc.expectedError == nil {
+				g.Expect(err).To(BeNil())
+			} else {
+				g.Expect(err).To(Equal(tc.expectedError))
+			}
+
+		})
+	}
+}