Merge pull request #2570 from enxebre/fix-2565

🐛 Ensure infrastructure is destroyed before deleting the node
kubernetes-sigs · Mar 16, 2020 · 8e8c880 · 8e8c880
2 parents 94bd603 + 47e8d16
commit 8e8c880
Show file tree

Hide file tree

Showing 2 changed files with 183 additions and 8 deletions.
diff --git a/controllers/machine_controller.go b/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,12 +311,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
 }

diff --git a/controllers/machine_controller_test.go b/controllers/machine_controller_test.go
@@ -785,3 +785,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))
+			}
+		})
+	}
+}