Skip to content

Commit

Permalink
💎 Standardize machine filter functions and improve testing
Browse files Browse the repository at this point in the history
  • Loading branch information
Cecile Robert-Michon committed Feb 19, 2021
1 parent be43e85 commit 1acc194
Show file tree
Hide file tree
Showing 18 changed files with 318 additions and 491 deletions.
25 changes: 6 additions & 19 deletions controllers/cluster_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"fmt"
"path"
"sigs.k8s.io/cluster-api/util/collections"
"strings"
"time"

Expand Down Expand Up @@ -399,11 +400,12 @@ 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
controlPlaneMachines := collections.FromMachineList(&machines).Filter(collections.ControlPlaneMachines(cluster.Name))
workerMachines := collections.FromMachineList(&machines).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 @@ -447,21 +449,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 @@ -474,7 +461,7 @@ func (r *ClusterReconciler) reconcileControlPlaneInitialized(ctx context.Context
return ctrl.Result{}, nil
}

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, "Error getting machines in cluster")
return ctrl.Result{}, err
Expand Down
22 changes: 4 additions & 18 deletions controllers/machine_controller.go
View file
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,16 +457,16 @@ 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)); {
switch numControlPlaneMachines := len(machines.Filter(collections.ControlPlaneMachines(cluster.Name))); {
case numControlPlaneMachines == 0:
// Do not delete the NodeRef if there are no remaining members of
// the control plane.
Expand Down
160 changes: 59 additions & 101 deletions controllers/machine_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1142,88 +1142,6 @@ func TestRemoveMachineFinalizerAfterDeleteReconcile(t *testing.T) {
g.Expect(actual.ObjectMeta.Finalizers).To(BeEmpty())
}

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 @@ -1359,12 +1277,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 @@ -1377,12 +1303,20 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
expectedError: errNilNodeRef,
},
{
name: "no control plane members",
cluster: &clusterv1.Cluster{},
name: "no control plane members",
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 @@ -1399,14 +1333,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 @@ -1426,14 +1365,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 @@ -1454,6 +1398,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 @@ -1463,6 +1409,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 @@ -1477,7 +1427,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 @@ -1497,6 +1447,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 @@ -1511,7 +1465,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 @@ -1531,6 +1485,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 @@ -1545,7 +1503,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 @@ -1607,7 +1565,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 @@ -1627,7 +1585,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

0 comments on commit 1acc194

Please sign in to comment.