diff --git a/internal/controllers/machine/machine_controller.go b/internal/controllers/machine/machine_controller.go index eef941f5ce38..1f5a53ef27f1 100644 --- a/internal/controllers/machine/machine_controller.go +++ b/internal/controllers/machine/machine_controller.go @@ -784,21 +784,24 @@ func (r *Reconciler) isDeleteNodeAllowed(ctx context.Context, cluster *clusterv1 } } - // Get all of the active machines that belong to this cluster. - machines, err := collections.GetFilteredMachinesForCluster(ctx, r.Client, cluster, collections.ActiveMachines) - if err != nil { - return err - } + if util.IsControlPlaneMachine(machine) { + // Get all the machines that belong to this cluster. + machines, err := collections.GetFilteredMachinesForCluster(ctx, r.Client, cluster) + if err != nil { + return err + } - // Whether or not it is okay to delete the NodeRef depends on the - // number of remaining control plane members and whether or not this - // machine is one of them. - numControlPlaneMachines := len(machines.Filter(collections.ControlPlaneMachines(cluster.Name))) - if numControlPlaneMachines == 0 { - // Do not delete the NodeRef if there are no remaining members of - // the control plane. - return errNoControlPlaneNodes + // Whether or not it is okay to delete the NodeRef depends on the + // number of remaining control plane members and whether or not this + // machine is one of them. + numOfControlPlaneMachines := len(machines.Filter(collections.ControlPlaneMachines(cluster.Name))) + if numOfControlPlaneMachines == 1 { + // Do not delete the NodeRef as this is the last remaining member of + // the control plane. + return errLastControlPlaneNode + } } + // Otherwise it is okay to delete the NodeRef. return nil } diff --git a/internal/controllers/machine/machine_controller_test.go b/internal/controllers/machine/machine_controller_test.go index 373a5b0053df..288e02df817f 100644 --- a/internal/controllers/machine/machine_controller_test.go +++ b/internal/controllers/machine/machine_controller_test.go @@ -2529,9 +2529,53 @@ func nodeNameIndex(o client.Object) []string { func TestIsDeleteNodeAllowed(t *testing.T) { deletionts := metav1.Now() + cp1 := &clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cp1", + Namespace: metav1.NamespaceDefault, + Labels: map[string]string{ + clusterv1.ClusterNameLabel: "test-cluster", + clusterv1.MachineControlPlaneLabel: "", + }, + Finalizers: []string{clusterv1.MachineFinalizer}, + }, + Spec: clusterv1.MachineSpec{ + ClusterName: "test-cluster", + InfrastructureRef: corev1.ObjectReference{}, + Bootstrap: clusterv1.Bootstrap{DataSecretName: ptr.To("data")}, + }, + Status: clusterv1.MachineStatus{ + NodeRef: &corev1.ObjectReference{ + Name: "test1", + }, + }, + } + cp2 := &clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cp2", + Namespace: metav1.NamespaceDefault, + Labels: map[string]string{ + clusterv1.ClusterNameLabel: "test-cluster", + clusterv1.MachineControlPlaneLabel: "", + }, + Finalizers: []string{clusterv1.MachineFinalizer}, + }, + Spec: clusterv1.MachineSpec{ + ClusterName: "test-cluster", + InfrastructureRef: corev1.ObjectReference{}, + Bootstrap: clusterv1.Bootstrap{DataSecretName: ptr.To("data")}, + }, + Status: clusterv1.MachineStatus{ + NodeRef: &corev1.ObjectReference{ + Name: "test2", + }, + }, + } + testCases := []struct { name string cluster *clusterv1.Cluster + extraMachines []*clusterv1.Machine machine *clusterv1.Machine expectedError error }{ @@ -2559,10 +2603,13 @@ func TestIsDeleteNodeAllowed(t *testing.T) { }, Status: clusterv1.MachineStatus{}, }, + extraMachines: nil, expectedError: errNilNodeRef, }, { - name: "no control plane members", + // This situation can happen when the control plane is not implemented via Cluster API, + // so for Cluster API the control plane machine members count is zero in this case. + name: "no control plane members, deleting a worker", cluster: &clusterv1.Cluster{ ObjectMeta: metav1.ObjectMeta{ Name: "test-cluster", @@ -2576,7 +2623,8 @@ func TestIsDeleteNodeAllowed(t *testing.T) { Labels: map[string]string{ clusterv1.ClusterNameLabel: "test-cluster", }, - Finalizers: []string{clusterv1.MachineFinalizer}, + Finalizers: []string{clusterv1.MachineFinalizer}, + DeletionTimestamp: &metav1.Time{Time: time.Now().UTC()}, }, Spec: clusterv1.MachineSpec{ ClusterName: "test-cluster", @@ -2589,10 +2637,11 @@ func TestIsDeleteNodeAllowed(t *testing.T) { }, }, }, - expectedError: errNoControlPlaneNodes, + extraMachines: nil, + expectedError: nil, }, { - name: "is last control plane member", + name: "is last control plane member, trying to delete it", cluster: &clusterv1.Cluster{ ObjectMeta: metav1.ObjectMeta{ Name: "test-cluster", @@ -2621,7 +2670,40 @@ func TestIsDeleteNodeAllowed(t *testing.T) { }, }, }, - expectedError: errNoControlPlaneNodes, + extraMachines: nil, + expectedError: errLastControlPlaneNode, + }, + { + name: "only one control plane member, trying to delete a worker", + cluster: &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + Namespace: metav1.NamespaceDefault, + }, + }, + machine: &clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "created", + Namespace: metav1.NamespaceDefault, + Labels: map[string]string{ + clusterv1.ClusterNameLabel: "test-cluster", + }, + Finalizers: []string{clusterv1.MachineFinalizer}, + DeletionTimestamp: &metav1.Time{Time: time.Now().UTC()}, + }, + Spec: clusterv1.MachineSpec{ + ClusterName: "test-cluster", + InfrastructureRef: corev1.ObjectReference{}, + Bootstrap: clusterv1.Bootstrap{DataSecretName: ptr.To("data")}, + }, + Status: clusterv1.MachineStatus{ + NodeRef: &corev1.ObjectReference{ + Name: "test", + }, + }, + }, + extraMachines: nil, + expectedError: nil, }, { name: "has nodeRef and control plane is healthy", @@ -2651,6 +2733,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) { }, }, }, + extraMachines: []*clusterv1.Machine{cp1, cp2}, expectedError: nil, }, { @@ -2664,6 +2747,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) { }, }, machine: &clusterv1.Machine{}, + extraMachines: nil, expectedError: errClusterIsBeingDeleted, }, { @@ -2702,6 +2786,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) { }, }, }, + extraMachines: []*clusterv1.Machine{cp1, cp2}, expectedError: nil, }, { @@ -2740,6 +2825,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) { }, }, }, + extraMachines: nil, expectedError: errControlPlaneIsBeingDeleted, }, { @@ -2778,6 +2864,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) { }, }, }, + extraMachines: nil, expectedError: errControlPlaneIsBeingDeleted, }, } @@ -2822,61 +2909,19 @@ func TestIsDeleteNodeAllowed(t *testing.T) { t.Run(tc.name, func(t *testing.T) { g := NewWithT(t) - m1 := &clusterv1.Machine{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cp1", - Namespace: metav1.NamespaceDefault, - Labels: map[string]string{ - clusterv1.ClusterNameLabel: "test-cluster", - }, - Finalizers: []string{clusterv1.MachineFinalizer}, - }, - Spec: clusterv1.MachineSpec{ - ClusterName: "test-cluster", - InfrastructureRef: corev1.ObjectReference{}, - Bootstrap: clusterv1.Bootstrap{DataSecretName: ptr.To("data")}, - }, - Status: clusterv1.MachineStatus{ - NodeRef: &corev1.ObjectReference{ - Name: "test1", - }, - }, - } - m2 := &clusterv1.Machine{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cp2", - Namespace: metav1.NamespaceDefault, - Labels: map[string]string{ - clusterv1.ClusterNameLabel: "test-cluster", - }, - Finalizers: []string{clusterv1.MachineFinalizer}, - }, - Spec: clusterv1.MachineSpec{ - ClusterName: "test-cluster", - InfrastructureRef: corev1.ObjectReference{}, - Bootstrap: clusterv1.Bootstrap{DataSecretName: ptr.To("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.MachineControlPlaneLabel] = "" - m2.Labels[clusterv1.MachineControlPlaneLabel] = "" - } - - c := fake.NewClientBuilder().WithObjects( + objects := []client.Object{ tc.cluster, tc.machine, - m1, - m2, emp, mcpBeingDeleted, empBeingDeleted, - ).Build() + } + + for _, e := range tc.extraMachines { + objects = append(objects, e) + } + + c := fake.NewClientBuilder().WithObjects(objects...).Build() mr := &Reconciler{ Client: c, } @@ -3197,6 +3242,7 @@ func TestNodeDeletion(t *testing.T) { Namespace: metav1.NamespaceDefault, Labels: map[string]string{ clusterv1.MachineControlPlaneLabel: "", + clusterv1.ClusterNameLabel: "test-cluster", }, Annotations: map[string]string{ "machine.cluster.x-k8s.io/exclude-node-draining": "", diff --git a/util/collections/machine_filters.go b/util/collections/machine_filters.go index be1f0d2d6493..a849cf7c0072 100644 --- a/util/collections/machine_filters.go +++ b/util/collections/machine_filters.go @@ -118,6 +118,7 @@ func ControlPlaneMachines(clusterName string) func(machine *clusterv1.Machine) b if machine == nil { return false } + return selector.Matches(labels.Set(machine.Labels)) } }