Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

🌱 refactor: standardize machine filter functions and improve testing #4207

Merged
merged 1 commit into from
Mar 22, 2021
Merged

🌱 refactor: standardize machine filter functions and improve testing #4207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 8 additions & 19 deletions controllers/cluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import (
"strings"
"time"

"sigs.k8s.io/cluster-api/util/collections"

"github.com/pkg/errors"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
Expand Down Expand Up @@ -400,11 +402,13 @@ func (r *ClusterReconciler) listDescendants(ctx context.Context, cluster *cluste
}

// Split machines into control plane and worker machines so we make sure we delete control plane machines last
controlPlaneMachines, workerMachines := splitMachineList(&machines)
descendants.workerMachines = *workerMachines
machineCollection := collections.FromMachineList(&machines)
controlPlaneMachines := machineCollection.Filter(collections.ControlPlaneMachines(cluster.Name))
workerMachines := machineCollection.Difference(controlPlaneMachines)
descendants.workerMachines = collections.ToMachineList(workerMachines)
// Only count control plane machines as descendants if there is no control plane provider.
if cluster.Spec.ControlPlaneRef == nil {
descendants.controlPlaneMachines = *controlPlaneMachines
descendants.controlPlaneMachines = collections.ToMachineList(controlPlaneMachines)

}

Expand Down Expand Up @@ -448,21 +452,6 @@ func (c clusterDescendants) filterOwnedDescendants(cluster *clusterv1.Cluster) (
return ownedDescendants, nil
}

// splitMachineList separates the machines running the control plane from other worker nodes.
func splitMachineList(list *clusterv1.MachineList) (*clusterv1.MachineList, *clusterv1.MachineList) {
nodes := &clusterv1.MachineList{}
controlplanes := &clusterv1.MachineList{}
for i := range list.Items {
machine := &list.Items[i]
if util.IsControlPlaneMachine(machine) {
controlplanes.Items = append(controlplanes.Items, *machine)
} else {
nodes.Items = append(nodes.Items, *machine)
}
}
return controlplanes, nodes
}

func (r *ClusterReconciler) reconcileControlPlaneInitialized(ctx context.Context, cluster *clusterv1.Cluster) (ctrl.Result, error) {
log := ctrl.LoggerFrom(ctx)

Expand All @@ -480,7 +469,7 @@ func (r *ClusterReconciler) reconcileControlPlaneInitialized(ctx context.Context

log.V(4).Info("Checking for control plane initialization")

machines, err := getActiveMachinesInCluster(ctx, r.Client, cluster.Namespace, cluster.Name)
machines, err := collections.GetFilteredMachinesForCluster(ctx, r.Client, cluster, collections.ActiveMachines)
if err != nil {
log.Error(err, "unable to determine ControlPlaneInitialized")
return ctrl.Result{}, err
Expand Down
29 changes: 7 additions & 22 deletions controllers/machine_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package controllers
import (
"context"
"fmt"
"sigs.k8s.io/cluster-api/util/collections"
"time"

"github.com/pkg/errors"
Expand Down Expand Up @@ -126,21 +127,6 @@ func (r *MachineReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manag
return nil
}

func (r *MachineReconciler) clusterToActiveMachines(a client.Object) []reconcile.Request {
requests := []reconcile.Request{}
machines, err := getActiveMachinesInCluster(context.TODO(), r.Client, a.GetNamespace(), a.GetName())
if err != nil {
return requests
}
for _, m := range machines {
r := reconcile.Request{
NamespacedName: util.ObjectKey(m),
}
requests = append(requests, r)
}
return requests
}

func (r *MachineReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Result, reterr error) {
log := ctrl.LoggerFrom(ctx)

Expand Down Expand Up @@ -471,24 +457,23 @@ func (r *MachineReconciler) isDeleteNodeAllowed(ctx context.Context, cluster *cl
}
}

// Get all of the machines that belong to this cluster.
machines, err := getActiveMachinesInCluster(ctx, r.Client, machine.Namespace, machine.Labels[clusterv1.ClusterLabelName])
// 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
}

// 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.
switch numControlPlaneMachines := len(util.GetControlPlaneMachines(machines)); {
case numControlPlaneMachines == 0:
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
default:
// Otherwise it is okay to delete the NodeRef.
return nil
}
// Otherwise it is okay to delete the NodeRef.
return nil
}

func (r *MachineReconciler) drainNode(ctx context.Context, cluster *clusterv1.Cluster, nodeName string) (ctrl.Result, error) {
Expand Down
160 changes: 59 additions & 101 deletions controllers/machine_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1139,88 +1139,6 @@ func TestRemoveMachineFinalizerAfterDeleteReconcile(t *testing.T) {
g.Expect(actual.ObjectMeta.Finalizers).To(Equal([]string{"test"}))
}

func Test_clusterToActiveMachines(t *testing.T) {
testCluster2Machines := &clusterv1.Cluster{
TypeMeta: metav1.TypeMeta{Kind: "Cluster", APIVersion: clusterv1.GroupVersion.String()},
ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "test-cluster-2"},
}
testCluster0Machines := &clusterv1.Cluster{
TypeMeta: metav1.TypeMeta{Kind: "Cluster", APIVersion: clusterv1.GroupVersion.String()},
ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "test-cluster-0"},
}

tests := []struct {
name string
cluster client.Object
want []reconcile.Request
}{
{
name: "cluster with two machines",
cluster: testCluster2Machines,
want: []reconcile.Request{
{
NamespacedName: client.ObjectKey{
Name: "m1",
Namespace: "default",
},
},
{
NamespacedName: client.ObjectKey{
Name: "m2",
Namespace: "default",
},
},
},
},
{
name: "cluster with zero machines",
cluster: testCluster0Machines,
want: []reconcile.Request{},
},
}
for _, tt := range tests {
g := NewWithT(t)

var objs []client.Object
objs = append(objs, testCluster2Machines)
objs = append(objs, testCluster0Machines)

m1 := &clusterv1.Machine{
TypeMeta: metav1.TypeMeta{
Kind: "Machine",
},
ObjectMeta: metav1.ObjectMeta{
Name: "m1",
Namespace: "default",
Labels: map[string]string{
clusterv1.ClusterLabelName: "test-cluster-2",
},
},
}
objs = append(objs, m1)
m2 := &clusterv1.Machine{
TypeMeta: metav1.TypeMeta{
Kind: "Machine",
},
ObjectMeta: metav1.ObjectMeta{
Name: "m2",
Namespace: "default",
Labels: map[string]string{
clusterv1.ClusterLabelName: "test-cluster-2",
},
},
}
objs = append(objs, m2)

r := &MachineReconciler{
Client: helpers.NewFakeClientWithScheme(scheme.Scheme, objs...),
}

got := r.clusterToActiveMachines(tt.cluster)
g.Expect(got).To(Equal(tt.want))
}
}

func TestIsNodeDrainedAllowed(t *testing.T) {
testCluster := &clusterv1.Cluster{
TypeMeta: metav1.TypeMeta{Kind: "Cluster", APIVersion: clusterv1.GroupVersion.String()},
Expand Down Expand Up @@ -1356,12 +1274,20 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
expectedError error
}{
{
name: "machine without nodeRef",
cluster: &clusterv1.Cluster{},
name: "machine without nodeRef",
cluster: &clusterv1.Cluster{
ObjectMeta: metav1.ObjectMeta{
Name: "test-cluster",
Namespace: "default",
},
},
machine: &clusterv1.Machine{
ObjectMeta: metav1.ObjectMeta{
Name: "created",
Namespace: "default",
Name: "created",
Namespace: "default",
Labels: map[string]string{
clusterv1.ClusterLabelName: "test-cluster",
},
Finalizers: []string{clusterv1.MachineFinalizer},
},
Spec: clusterv1.MachineSpec{
Expand All @@ -1374,12 +1300,20 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
expectedError: errNilNodeRef,
},
{
name: "no control plane members",
cluster: &clusterv1.Cluster{},
name: "no control plane members",
Copy link
Contributor Author

@CecileRobertMichon CecileRobertMichon Feb 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test was not fully correct as the machines were being excluded from the filter by not having the right cluster name label and/or namespace. Added all the required object metadata so it's actually checking that the described missing piece (eg. no control plane members) is what causes the error.

cluster: &clusterv1.Cluster{
ObjectMeta: metav1.ObjectMeta{
Name: "test-cluster",
Namespace: "default",
},
},
machine: &clusterv1.Machine{
ObjectMeta: metav1.ObjectMeta{
Name: "created",
Namespace: "default",
Name: "created",
Namespace: "default",
Labels: map[string]string{
clusterv1.ClusterLabelName: "test-cluster",
},
Finalizers: []string{clusterv1.MachineFinalizer},
},
Spec: clusterv1.MachineSpec{
Expand All @@ -1396,14 +1330,19 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
expectedError: errNoControlPlaneNodes,
},
{
name: "is last control plane member",
cluster: &clusterv1.Cluster{},
name: "is last control plane member",
cluster: &clusterv1.Cluster{
ObjectMeta: metav1.ObjectMeta{
Name: "test-cluster",
Namespace: "default",
},
},
machine: &clusterv1.Machine{
ObjectMeta: metav1.ObjectMeta{
Name: "created",
Namespace: "default",
Labels: map[string]string{
clusterv1.ClusterLabelName: "test",
clusterv1.ClusterLabelName: "test-cluster",
clusterv1.MachineControlPlaneLabelName: "",
},
Finalizers: []string{clusterv1.MachineFinalizer},
Expand All @@ -1423,14 +1362,19 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
expectedError: errNoControlPlaneNodes,
},
{
name: "has nodeRef and control plane is healthy",
cluster: &clusterv1.Cluster{},
name: "has nodeRef and control plane is healthy",
cluster: &clusterv1.Cluster{
ObjectMeta: metav1.ObjectMeta{
Name: "test-cluster",
Namespace: "default",
},
},
machine: &clusterv1.Machine{
ObjectMeta: metav1.ObjectMeta{
Name: "created",
Namespace: "default",
Labels: map[string]string{
clusterv1.ClusterLabelName: "test",
clusterv1.ClusterLabelName: "test-cluster",
},
Finalizers: []string{clusterv1.MachineFinalizer},
},
Expand All @@ -1451,6 +1395,8 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
name: "has nodeRef and cluster is being deleted",
cluster: &clusterv1.Cluster{
ObjectMeta: metav1.ObjectMeta{
Name: "test-cluster",
Namespace: "default",
DeletionTimestamp: &deletionts,
},
},
Expand All @@ -1460,6 +1406,10 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
{
name: "has nodeRef and control plane is healthy and externally managed",
cluster: &clusterv1.Cluster{
ObjectMeta: metav1.ObjectMeta{
Name: "test-cluster",
Namespace: "default",
},
Spec: clusterv1.ClusterSpec{
ControlPlaneRef: &corev1.ObjectReference{
APIVersion: "controlplane.cluster.x-k8s.io/v1alpha4",
Expand All @@ -1474,7 +1424,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
Name: "created",
Namespace: "default",
Labels: map[string]string{
clusterv1.ClusterLabelName: "test",
clusterv1.ClusterLabelName: "test-cluster",
},
Finalizers: []string{clusterv1.MachineFinalizer},
},
Expand All @@ -1494,6 +1444,10 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
{
name: "has nodeRef, control plane is being deleted and not externally managed",
cluster: &clusterv1.Cluster{
ObjectMeta: metav1.ObjectMeta{
Name: "test-cluster",
Namespace: "default",
},
Spec: clusterv1.ClusterSpec{
ControlPlaneRef: &corev1.ObjectReference{
APIVersion: "controlplane.cluster.x-k8s.io/v1alpha4",
Expand All @@ -1508,7 +1462,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
Name: "created",
Namespace: "default",
Labels: map[string]string{
clusterv1.ClusterLabelName: "test",
clusterv1.ClusterLabelName: "test-cluster",
},
Finalizers: []string{clusterv1.MachineFinalizer},
},
Expand All @@ -1528,6 +1482,10 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
{
name: "has nodeRef, control plane is being deleted and is externally managed",
cluster: &clusterv1.Cluster{
ObjectMeta: metav1.ObjectMeta{
Name: "test-cluster",
Namespace: "default",
},
Spec: clusterv1.ClusterSpec{
ControlPlaneRef: &corev1.ObjectReference{
APIVersion: "controlplane.cluster.x-k8s.io/v1alpha4",
Expand All @@ -1542,7 +1500,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
Name: "created",
Namespace: "default",
Labels: map[string]string{
clusterv1.ClusterLabelName: "test",
clusterv1.ClusterLabelName: "test-cluster",
},
Finalizers: []string{clusterv1.MachineFinalizer},
},
Expand Down Expand Up @@ -1604,7 +1562,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
Name: "cp1",
Namespace: "default",
Labels: map[string]string{
clusterv1.ClusterLabelName: "test",
clusterv1.ClusterLabelName: "test-cluster",
},
Finalizers: []string{clusterv1.MachineFinalizer},
},
Expand All @@ -1624,7 +1582,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
Name: "cp2",
Namespace: "default",
Labels: map[string]string{
clusterv1.ClusterLabelName: "test",
clusterv1.ClusterLabelName: "test-cluster",
},
Finalizers: []string{clusterv1.MachineFinalizer},
},
Expand Down
Loading