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 Mar 15, 2021
1 parent 0bc5750 commit b1c8248
Show file tree
Hide file tree
Showing 20 changed files with 361 additions and 378 deletions.
27 changes: 8 additions & 19 deletions controllers/cluster_controller.go
View file
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
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
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",
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

0 comments on commit b1c8248

Please sign in to comment.