From 974d79b8a4a762b9a79e657e4e8aeae75878cb53 Mon Sep 17 00:00:00 2001 From: killianmuldoon Date: Fri, 11 Nov 2022 15:23:14 +0000 Subject: [PATCH] Add finalizer reconcile for Topology MachineDeployments and MachineSets Signed-off-by: killianmuldoon --- .../topology/cluster/desired_state.go | 8 -- .../topology/cluster/desired_state_test.go | 3 - .../machinedeployment_controller.go | 28 +++--- .../machinedeployment_controller_test.go | 76 +++++++++++++++- .../machineset/machineset_controller.go | 25 ++++-- .../machineset/machineset_controller_test.go | 88 +++++++++++++++++++ internal/test/builder/builders.go | 28 ++++-- .../test/builder/zz_generated.deepcopy.go | 7 ++ 8 files changed, 225 insertions(+), 38 deletions(-) diff --git a/internal/controllers/topology/cluster/desired_state.go b/internal/controllers/topology/cluster/desired_state.go index 8e2a89cec188..dcc2d6ad6d26 100644 --- a/internal/controllers/topology/cluster/desired_state.go +++ b/internal/controllers/topology/cluster/desired_state.go @@ -28,7 +28,6 @@ import ( "k8s.io/apiserver/pkg/storage/names" "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/controllers/external" @@ -623,13 +622,6 @@ func computeMachineDeployment(_ context.Context, s *scope.Scope, desiredControlP }, } - // If it's a new MachineDeployment, set the finalizer. - // Note: we only add it on creation to avoid race conditions later on when - // the MachineDeployment topology controller removes the finalizer. - if currentMachineDeployment == nil { - controllerutil.AddFinalizer(desiredMachineDeploymentObj, clusterv1.MachineDeploymentTopologyFinalizer) - } - // If an existing MachineDeployment is present, override the MachineDeployment generate name // re-using the existing name (this will help in reconcile). if currentMachineDeployment != nil && currentMachineDeployment.Object != nil { diff --git a/internal/controllers/topology/cluster/desired_state_test.go b/internal/controllers/topology/cluster/desired_state_test.go index 5621285042ff..c321b4c21f88 100644 --- a/internal/controllers/topology/cluster/desired_state_test.go +++ b/internal/controllers/topology/cluster/desired_state_test.go @@ -30,7 +30,6 @@ import ( utilfeature "k8s.io/component-base/featuregate/testing" "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client/fake" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" runtimev1 "sigs.k8s.io/cluster-api/exp/runtime/api/v1alpha1" @@ -1382,7 +1381,6 @@ func TestComputeMachineDeployment(t *testing.T) { g.Expect(actualMd.Labels).To(HaveKeyWithValue(clusterv1.ClusterTopologyMachineDeploymentLabelName, "big-pool-of-machines")) g.Expect(actualMd.Labels).To(HaveKey(clusterv1.ClusterTopologyOwnedLabel)) - g.Expect(controllerutil.ContainsFinalizer(actualMd, clusterv1.MachineDeploymentTopologyFinalizer)).To(BeTrue()) g.Expect(actualMd.Spec.Selector.MatchLabels).To(HaveKey(clusterv1.ClusterTopologyOwnedLabel)) g.Expect(actualMd.Spec.Selector.MatchLabels).To(HaveKeyWithValue(clusterv1.ClusterTopologyMachineDeploymentLabelName, "big-pool-of-machines")) @@ -1436,7 +1434,6 @@ func TestComputeMachineDeployment(t *testing.T) { g.Expect(actualMd.Labels).To(HaveKeyWithValue(clusterv1.ClusterTopologyMachineDeploymentLabelName, "big-pool-of-machines")) g.Expect(actualMd.Labels).To(HaveKey(clusterv1.ClusterTopologyOwnedLabel)) - g.Expect(controllerutil.ContainsFinalizer(actualMd, clusterv1.MachineDeploymentTopologyFinalizer)).To(BeFalse()) g.Expect(actualMd.Spec.Template.ObjectMeta.Labels).To(HaveKeyWithValue("foo", "baz")) g.Expect(actualMd.Spec.Template.ObjectMeta.Labels).To(HaveKeyWithValue("fizz", "buzz")) diff --git a/internal/controllers/topology/machinedeployment/machinedeployment_controller.go b/internal/controllers/topology/machinedeployment/machinedeployment_controller.go index 7de0f63991d7..4d5bbf2d174a 100644 --- a/internal/controllers/topology/machinedeployment/machinedeployment_controller.go +++ b/internal/controllers/topology/machinedeployment/machinedeployment_controller.go @@ -21,6 +21,7 @@ import ( "github.com/pkg/errors" apierrors "k8s.io/apimachinery/pkg/api/errors" + kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -84,7 +85,7 @@ func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opt // // We don't have to set the finalizer, as it's already set during MachineDeployment creation // in the cluster topology controller. -func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Result, reterr error) { log := ctrl.LoggerFrom(ctx) // Fetch the MachineDeployment instance. @@ -112,12 +113,25 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return ctrl.Result{}, nil } + // Create a patch helper to add or remove the finalizer from the MachineDeployment. + patchHelper, err := patch.NewHelper(md, r.Client) + if err != nil { + return ctrl.Result{}, errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: md}) + } + defer func() { + if err := patchHelper.Patch(ctx, md); err != nil { + reterr = kerrors.NewAggregate([]error{reterr, errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: md})}) + } + }() + // Handle deletion reconciliation loop. if !md.ObjectMeta.DeletionTimestamp.IsZero() { return r.reconcileDelete(ctx, md) } - // Nothing to do. + // If the MachineDeployment is not being deleted ensure the finalizer is set. + controllerutil.AddFinalizer(md, clusterv1.MachineDeploymentTopologyFinalizer) + return ctrl.Result{}, nil } @@ -151,16 +165,8 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, md *clusterv1.MachineD return ctrl.Result{}, err } - // Remove the finalizer so the MachineDeployment can be garbage collected by Kubernetes. - patchHelper, err := patch.NewHelper(md, r.Client) - if err != nil { - return ctrl.Result{}, errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: md}) - } - controllerutil.RemoveFinalizer(md, clusterv1.MachineDeploymentTopologyFinalizer) - if err := patchHelper.Patch(ctx, md); err != nil { - return ctrl.Result{}, errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: md}) - } + return ctrl.Result{}, nil } diff --git a/internal/controllers/topology/machinedeployment/machinedeployment_controller_test.go b/internal/controllers/topology/machinedeployment/machinedeployment_controller_test.go index 6298693b70d7..fbe00497a3ee 100644 --- a/internal/controllers/topology/machinedeployment/machinedeployment_controller_test.go +++ b/internal/controllers/topology/machinedeployment/machinedeployment_controller_test.go @@ -27,11 +27,85 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/reconcile" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/internal/test/builder" + "sigs.k8s.io/cluster-api/util" ) +func TestMachineDeploymentTopologyFinalizer(t *testing.T) { + cluster := builder.Cluster(metav1.NamespaceDefault, "fake-cluster").Build() + mdBT := builder.BootstrapTemplate(metav1.NamespaceDefault, "mdBT").Build() + mdIMT := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "mdIMT").Build() + mdBuilder := builder.MachineDeployment(metav1.NamespaceDefault, "md"). + WithClusterName("fake-cluster"). + WithBootstrapTemplate(mdBT). + WithInfrastructureTemplate(mdIMT) + + md := mdBuilder.Build() + mdWithFinalizer := mdBuilder.Build() + mdWithFinalizer.Finalizers = []string{clusterv1.MachineDeploymentTopologyFinalizer} + mdWithDeletionTimestamp := mdBuilder.Build() + deletionTimestamp := metav1.Now() + mdWithDeletionTimestamp.DeletionTimestamp = &deletionTimestamp + + mdWithDeletionTimestampAndFinalizer := mdWithDeletionTimestamp.DeepCopy() + mdWithDeletionTimestampAndFinalizer.Finalizers = []string{clusterv1.MachineDeploymentTopologyFinalizer} + + testCases := []struct { + name string + md *clusterv1.MachineDeployment + expectFinalizer bool + }{ + { + name: "should add ClusterTopology finalizer to a MachineDeployment with no finalizer", + md: md, + expectFinalizer: true, + }, + { + name: "should retain ClusterTopology finalizer on MachineDeployment with finalizer", + md: mdWithFinalizer, + expectFinalizer: true, + }, + { + name: "should not add ClusterTopology finalizer on MachineDeployment with Deletion Timestamp and no finalizer ", + md: mdWithDeletionTimestamp, + expectFinalizer: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + + fakeClient := fake.NewClientBuilder(). + WithScheme(fakeScheme). + WithObjects(tc.md, mdBT, mdIMT, cluster). + Build() + + mdr := &Reconciler{ + Client: fakeClient, + APIReader: fakeClient, + } + + _, err := mdr.Reconcile(ctx, reconcile.Request{ + NamespacedName: util.ObjectKey(tc.md), + }) + g.Expect(err).NotTo(HaveOccurred()) + + key := client.ObjectKey{Namespace: tc.md.Namespace, Name: tc.md.Name} + var actual clusterv1.MachineDeployment + g.Expect(mdr.Client.Get(ctx, key, &actual)).To(Succeed()) + if tc.expectFinalizer { + g.Expect(actual.Finalizers).To(ConsistOf(clusterv1.MachineDeploymentTopologyFinalizer)) + } else { + g.Expect(actual.Finalizers).To(BeEmpty()) + } + }) + } +} + func TestMachineDeploymentReconciler_ReconcileDelete(t *testing.T) { deletionTimeStamp := metav1.Now() @@ -98,7 +172,7 @@ func TestMachineDeploymentReconciler_ReconcileDelete(t *testing.T) { t.Run("Should not delete templates of a MachineDeployment when they are still in use in a MachineSet", func(t *testing.T) { g := NewWithT(t) - ms := builder.MachineSet(md.Namespace, "ms"). + ms := builder.MachineSet(md.Namespace, "md"). WithBootstrapTemplate(mdBT). WithInfrastructureTemplate(mdIMT). WithLabels(map[string]string{ diff --git a/internal/controllers/topology/machineset/machineset_controller.go b/internal/controllers/topology/machineset/machineset_controller.go index 7ecc75af59e6..58c526680107 100644 --- a/internal/controllers/topology/machineset/machineset_controller.go +++ b/internal/controllers/topology/machineset/machineset_controller.go @@ -23,6 +23,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -86,7 +87,7 @@ func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opt // // We don't have to set the finalizer, as it's already set during MachineSet creation // in the MachineSet controller. -func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Result, reterr error) { // Fetch the MachineSet instance. ms := &clusterv1.MachineSet{} if err := r.Client.Get(ctx, req.NamespacedName, ms); err != nil { @@ -119,12 +120,25 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return ctrl.Result{}, nil } + // Create a patch helper to add or remove the finalizer from the MachineSet. + patchHelper, err := patch.NewHelper(ms, r.Client) + if err != nil { + return ctrl.Result{}, errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: ms}) + } + defer func() { + if err := patchHelper.Patch(ctx, ms); err != nil { + reterr = kerrors.NewAggregate([]error{reterr, errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: ms})}) + } + }() + // Handle deletion reconciliation loop. if !ms.ObjectMeta.DeletionTimestamp.IsZero() { return r.reconcileDelete(ctx, ms) } - // Nothing to do. + // If the MachineSet is not being deleted ensure the finalizer is set. + controllerutil.AddFinalizer(ms, clusterv1.MachineSetTopologyFinalizer) + return ctrl.Result{}, nil } @@ -172,14 +186,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, ms *clusterv1.MachineS } // Remove the finalizer so the MachineSet can be garbage collected by Kubernetes. - patchHelper, err := patch.NewHelper(ms, r.Client) - if err != nil { - return ctrl.Result{}, errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: ms}) - } controllerutil.RemoveFinalizer(ms, clusterv1.MachineSetTopologyFinalizer) - if err := patchHelper.Patch(ctx, ms); err != nil { - return ctrl.Result{}, errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: ms}) - } return ctrl.Result{}, nil } diff --git a/internal/controllers/topology/machineset/machineset_controller_test.go b/internal/controllers/topology/machineset/machineset_controller_test.go index 49b9369efa33..166241931121 100644 --- a/internal/controllers/topology/machineset/machineset_controller_test.go +++ b/internal/controllers/topology/machineset/machineset_controller_test.go @@ -27,11 +27,99 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/reconcile" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/internal/test/builder" + "sigs.k8s.io/cluster-api/util" ) +func TestMachineSetTopologyFinalizer(t *testing.T) { + mdName := "md" + + msBT := builder.BootstrapTemplate(metav1.NamespaceDefault, "msBT").Build() + msIMT := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "msIMT").Build() + + cluster := builder.Cluster(metav1.NamespaceDefault, "fake-cluster").Build() + msBuilder := builder.MachineSet(metav1.NamespaceDefault, "ms"). + WithBootstrapTemplate(msBT). + WithInfrastructureTemplate(msIMT). + WithClusterName(cluster.Name). + WithOwnerReferences([]metav1.OwnerReference{ + { + Kind: "MachineDeployment", + APIVersion: clusterv1.GroupVersion.String(), + Name: "md", + }, + }). + WithLabels(map[string]string{ + clusterv1.MachineDeploymentLabelName: mdName, + clusterv1.ClusterTopologyOwnedLabel: "", + }) + + ms := msBuilder.Build() + msWithFinalizer := msBuilder.Build() + msWithFinalizer.Finalizers = []string{clusterv1.MachineSetTopologyFinalizer} + msWithDeletionTimestamp := msBuilder.Build() + deletionTimestamp := metav1.Now() + msWithDeletionTimestamp.DeletionTimestamp = &deletionTimestamp + + msWithDeletionTimestampAndFinalizer := msWithDeletionTimestamp.DeepCopy() + msWithDeletionTimestampAndFinalizer.Finalizers = []string{clusterv1.MachineSetTopologyFinalizer} + + testCases := []struct { + name string + ms *clusterv1.MachineSet + expectFinalizer bool + }{ + { + name: "should add ClusterTopology finalizer to a MachineSet with no finalizer", + ms: ms, + expectFinalizer: true, + }, + { + name: "should retain ClusterTopology finalizer on MachineSet with finalizer", + ms: msWithFinalizer, + expectFinalizer: true, + }, + { + name: "should not add ClusterTopology finalizer on MachineSet with Deletion Timestamp and no finalizer ", + ms: msWithDeletionTimestamp, + expectFinalizer: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + + fakeClient := fake.NewClientBuilder(). + WithScheme(fakeScheme). + WithObjects(tc.ms, msBT, msIMT, cluster). + Build() + + msr := &Reconciler{ + Client: fakeClient, + APIReader: fakeClient, + } + + _, err := msr.Reconcile(ctx, reconcile.Request{ + NamespacedName: util.ObjectKey(tc.ms), + }) + g.Expect(err).NotTo(HaveOccurred()) + + key := client.ObjectKey{Namespace: tc.ms.Namespace, Name: tc.ms.Name} + var actual clusterv1.MachineSet + g.Expect(msr.Client.Get(ctx, key, &actual)).To(Succeed()) + if tc.expectFinalizer { + g.Expect(actual.Finalizers).To(ConsistOf(clusterv1.MachineSetTopologyFinalizer)) + } else { + g.Expect(actual.Finalizers).To(BeEmpty()) + } + }) + } +} + func TestMachineSetReconciler_ReconcileDelete(t *testing.T) { deletionTimeStamp := metav1.Now() diff --git a/internal/test/builder/builders.go b/internal/test/builder/builders.go index 705cc092824d..609b59ac368a 100644 --- a/internal/test/builder/builders.go +++ b/internal/test/builder/builders.go @@ -1151,7 +1151,7 @@ func (m *MachineDeploymentBuilder) Build() *clusterv1.MachineDeployment { return obj } -// MachineSetBuilder holds the variables and objects needed to build a generic MachineSet. +// MachineSetBuilder holds the variables and objects needed to build a MachineSet. type MachineSetBuilder struct { namespace string name string @@ -1159,6 +1159,8 @@ type MachineSetBuilder struct { infrastructureTemplate *unstructured.Unstructured replicas *int32 labels map[string]string + clusterName string + ownerRefs []metav1.OwnerReference } // MachineSet creates a MachineSetBuilder with the given name and namespace. @@ -1175,7 +1177,7 @@ func (m *MachineSetBuilder) WithBootstrapTemplate(ref *unstructured.Unstructured return m } -// WithInfrastructureTemplate adds the passed unstructured object to the MachineSet builder as an infrastructureMachineTemplate. +// WithInfrastructureTemplate adds the passed unstructured object to the MachineSetBuilder as an infrastructureMachineTemplate. func (m *MachineSetBuilder) WithInfrastructureTemplate(ref *unstructured.Unstructured) *MachineSetBuilder { m.infrastructureTemplate = ref return m @@ -1187,12 +1189,24 @@ func (m *MachineSetBuilder) WithLabels(labels map[string]string) *MachineSetBuil return m } -// WithReplicas sets the number of replicas for the MachineSetClassBuilder. +// WithReplicas sets the number of replicas for the MachineSetBuilder. func (m *MachineSetBuilder) WithReplicas(replicas *int32) *MachineSetBuilder { m.replicas = replicas return m } +// WithClusterName sets the number of replicas for the MachineSetBuilder. +func (m *MachineSetBuilder) WithClusterName(name string) *MachineSetBuilder { + m.clusterName = name + return m +} + +// WithOwnerReferences adds ownerReferences for the MachineSetBuilder. +func (m *MachineSetBuilder) WithOwnerReferences(ownerRefs []metav1.OwnerReference) *MachineSetBuilder { + m.ownerRefs = ownerRefs + return m +} + // Build creates a new MachineSet with the variables and objects passed to the MachineSetBuilder. func (m *MachineSetBuilder) Build() *clusterv1.MachineSet { obj := &clusterv1.MachineSet{ @@ -1201,11 +1215,13 @@ func (m *MachineSetBuilder) Build() *clusterv1.MachineSet { APIVersion: clusterv1.GroupVersion.String(), }, ObjectMeta: metav1.ObjectMeta{ - Name: m.name, - Namespace: m.namespace, - Labels: m.labels, + Name: m.name, + Namespace: m.namespace, + Labels: m.labels, + OwnerReferences: m.ownerRefs, }, } + obj.Spec.ClusterName = m.clusterName obj.Spec.Replicas = m.replicas if m.bootstrapTemplate != nil { obj.Spec.Template.Spec.Bootstrap.ConfigRef = objToRef(m.bootstrapTemplate) diff --git a/internal/test/builder/zz_generated.deepcopy.go b/internal/test/builder/zz_generated.deepcopy.go index a616faa9de47..31645fb5899f 100644 --- a/internal/test/builder/zz_generated.deepcopy.go +++ b/internal/test/builder/zz_generated.deepcopy.go @@ -530,6 +530,13 @@ func (in *MachineSetBuilder) DeepCopyInto(out *MachineSetBuilder) { (*out)[key] = val } } + if in.ownerRefs != nil { + in, out := &in.ownerRefs, &out.ownerRefs + *out = make([]v1.OwnerReference, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new MachineSetBuilder.