diff --git a/controllers/machine_controller.go b/controllers/machine_controller.go index 14b9b3becbba..bd4fffb72541 100644 --- a/controllers/machine_controller.go +++ b/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 } } diff --git a/controllers/machine_controller_test.go b/controllers/machine_controller_test.go index ca7bcfd8cbbb..d544305f70e9 100644 --- a/controllers/machine_controller_test.go +++ b/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)) + } + + }) + } +}