diff --git a/controllers/machine_controller.go b/controllers/machine_controller.go index 14b9b3becbba..b793ff2a265e 100644 --- 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 index ee1ac101f5d0..561d79b5076d 100644 --- 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)) + } + }) + } +}