From b7ee8b68c042d81be48f63523455214baee9df36 Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Mon, 14 Oct 2024 11:59:46 +0200 Subject: [PATCH] Handler finalizers early in Reconciles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Stefan Büringer buringerst@vmware.com --- .../internal/controllers/controller.go | 22 +--- .../internal/controllers/controller_test.go | 3 + .../clusterresourceset_controller.go | 13 +-- .../controllers/machinepool_controller.go | 15 ++- .../controllers/cluster/cluster_controller.go | 13 +-- .../controllers/machine/machine_controller.go | 19 ++- .../machinedeployment_controller.go | 13 +-- .../machineset/machineset_controller.go | 19 ++- .../machinedeployment_controller.go | 25 ++-- .../machineset/machineset_controller.go | 31 +++-- .../controllers/dockercluster_controller.go | 13 +-- .../controllers/dockermachine_controller.go | 13 +-- .../controllers/inmemorycluster_controller.go | 13 +-- .../controllers/inmemorymachine_controller.go | 13 +-- util/finalizers/finalizers.go | 55 +++++++++ util/finalizers/finalizers_test.go | 108 ++++++++++++++++++ 16 files changed, 266 insertions(+), 122 deletions(-) create mode 100644 util/finalizers/finalizers.go create mode 100644 util/finalizers/finalizers_test.go diff --git a/controlplane/kubeadm/internal/controllers/controller.go b/controlplane/kubeadm/internal/controllers/controller.go index d19392a4f905..a128a96a7ea9 100644 --- a/controlplane/kubeadm/internal/controllers/controller.go +++ b/controlplane/kubeadm/internal/controllers/controller.go @@ -51,6 +51,7 @@ import ( "sigs.k8s.io/cluster-api/util/annotations" "sigs.k8s.io/cluster-api/util/collections" "sigs.k8s.io/cluster-api/util/conditions" + "sigs.k8s.io/cluster-api/util/finalizers" "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/cluster-api/util/predicates" "sigs.k8s.io/cluster-api/util/secret" @@ -149,6 +150,11 @@ func (r *KubeadmControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, err } + // Add finalizer first if not set to avoid the race condition between init and delete. + if finalizerAdded, err := finalizers.EnsureFinalizer(ctx, r.Client, kcp, controlplanev1.KubeadmControlPlaneFinalizer); err != nil || finalizerAdded { + return ctrl.Result{}, err + } + // Fetch the Cluster. cluster, err := util.GetOwnerCluster(ctx, r.Client, kcp.ObjectMeta) if err != nil { @@ -176,22 +182,6 @@ func (r *KubeadmControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{Requeue: true}, nil } - // Add finalizer first if not set to avoid the race condition between init and delete. - // Note: Finalizers in general can only be added when the deletionTimestamp is not set. - if kcp.ObjectMeta.DeletionTimestamp.IsZero() && !controllerutil.ContainsFinalizer(kcp, controlplanev1.KubeadmControlPlaneFinalizer) { - controllerutil.AddFinalizer(kcp, controlplanev1.KubeadmControlPlaneFinalizer) - - // patch and return right away instead of reusing the main defer, - // because the main defer may take too much time to get cluster status - // Patch ObservedGeneration only if the reconciliation completed successfully - patchOpts := []patch.Option{patch.WithStatusObservedGeneration{}} - if err := patchHelper.Patch(ctx, kcp, patchOpts...); err != nil { - return ctrl.Result{}, errors.Wrapf(err, "failed to add finalizer") - } - - return ctrl.Result{}, nil - } - // Initialize the control plane scope; this includes also checking for orphan machines and // adopt them if necessary. controlPlane, adoptableMachineFound, err := r.initControlPlaneScope(ctx, cluster, kcp) diff --git a/controlplane/kubeadm/internal/controllers/controller_test.go b/controlplane/kubeadm/internal/controllers/controller_test.go index e315fb6d4358..e8a4001e0e58 100644 --- a/controlplane/kubeadm/internal/controllers/controller_test.go +++ b/controlplane/kubeadm/internal/controllers/controller_test.go @@ -317,6 +317,9 @@ func TestReconcileNoCluster(t *testing.T) { Name: "foo", }, }, + Finalizers: []string{ + controlplanev1.KubeadmControlPlaneFinalizer, + }, }, Spec: controlplanev1.KubeadmControlPlaneSpec{ Version: "v1.16.6", diff --git a/exp/addons/internal/controllers/clusterresourceset_controller.go b/exp/addons/internal/controllers/clusterresourceset_controller.go index f4d70c8df48a..6fed7a91affc 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller.go +++ b/exp/addons/internal/controllers/clusterresourceset_controller.go @@ -46,6 +46,7 @@ import ( resourcepredicates "sigs.k8s.io/cluster-api/exp/addons/internal/controllers/predicates" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/conditions" + "sigs.k8s.io/cluster-api/util/finalizers" "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/cluster-api/util/predicates" ) @@ -122,6 +123,11 @@ func (r *ClusterResourceSetReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{}, err } + // Add finalizer first if not set to avoid the race condition between init and delete. + if finalizerAdded, err := finalizers.EnsureFinalizer(ctx, r.Client, clusterResourceSet, addonsv1.ClusterResourceSetFinalizer); err != nil || finalizerAdded { + return ctrl.Result{}, err + } + // Initialize the patch helper. patchHelper, err := patch.NewHelper(clusterResourceSet, r.Client) if err != nil { @@ -152,13 +158,6 @@ func (r *ClusterResourceSetReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{}, r.reconcileDelete(ctx, clusters, clusterResourceSet) } - // Add finalizer first if not set to avoid the race condition between init and delete. - // Note: Finalizers in general can only be added when the deletionTimestamp is not set. - if !controllerutil.ContainsFinalizer(clusterResourceSet, addonsv1.ClusterResourceSetFinalizer) { - controllerutil.AddFinalizer(clusterResourceSet, addonsv1.ClusterResourceSetFinalizer) - return ctrl.Result{}, nil - } - errs := []error{} errClusterLockedOccurred := false for _, cluster := range clusters { diff --git a/exp/internal/controllers/machinepool_controller.go b/exp/internal/controllers/machinepool_controller.go index 9e65c116f20b..f7fc99706daa 100644 --- a/exp/internal/controllers/machinepool_controller.go +++ b/exp/internal/controllers/machinepool_controller.go @@ -46,6 +46,7 @@ import ( "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/annotations" "sigs.k8s.io/cluster-api/util/conditions" + "sigs.k8s.io/cluster-api/util/finalizers" "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/cluster-api/util/predicates" ) @@ -151,9 +152,14 @@ func (r *MachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, err } - log = log.WithValues("Cluster", klog.KRef(mp.ObjectMeta.Namespace, mp.Spec.ClusterName)) + log = log.WithValues("Cluster", klog.KRef(mp.Namespace, mp.Spec.ClusterName)) ctx = ctrl.LoggerInto(ctx, log) + // Add finalizer first if not set to avoid the race condition between init and delete. + if finalizerAdded, err := finalizers.EnsureFinalizer(ctx, r.Client, mp, expv1.MachinePoolFinalizer); err != nil || finalizerAdded { + return ctrl.Result{}, err + } + cluster, err := util.GetClusterByName(ctx, r.Client, mp.ObjectMeta.Namespace, mp.Spec.ClusterName) if err != nil { log.Error(err, "Failed to get Cluster for MachinePool.", "MachinePool", klog.KObj(mp), "Cluster", klog.KRef(mp.ObjectMeta.Namespace, mp.Spec.ClusterName)) @@ -223,13 +229,6 @@ func (r *MachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, err } - // Add finalizer first if not set to avoid the race condition between init and delete. - // Note: Finalizers in general can only be added when the deletionTimestamp is not set. - if !controllerutil.ContainsFinalizer(mp, expv1.MachinePoolFinalizer) { - controllerutil.AddFinalizer(mp, expv1.MachinePoolFinalizer) - return ctrl.Result{}, nil - } - // Handle normal reconciliation loop. scope := &scope{ cluster: cluster, diff --git a/internal/controllers/cluster/cluster_controller.go b/internal/controllers/cluster/cluster_controller.go index 854382965c54..ca1d86e22d0c 100644 --- a/internal/controllers/cluster/cluster_controller.go +++ b/internal/controllers/cluster/cluster_controller.go @@ -48,6 +48,7 @@ import ( "sigs.k8s.io/cluster-api/util/annotations" "sigs.k8s.io/cluster-api/util/collections" "sigs.k8s.io/cluster-api/util/conditions" + "sigs.k8s.io/cluster-api/util/finalizers" "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/cluster-api/util/predicates" ) @@ -120,6 +121,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re return ctrl.Result{}, err } + // Add finalizer first if not set to avoid the race condition between init and delete. + if finalizerAdded, err := finalizers.EnsureFinalizer(ctx, r.Client, cluster, clusterv1.ClusterFinalizer); err != nil || finalizerAdded { + return ctrl.Result{}, err + } + // Return early if the object or Cluster is paused. if annotations.IsPaused(cluster, cluster) { log.Info("Reconciliation is paused for this object") @@ -152,13 +158,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re return r.reconcileDelete(ctx, cluster) } - // Add finalizer first if not set to avoid the race condition between init and delete. - // Note: Finalizers in general can only be added when the deletionTimestamp is not set. - if !controllerutil.ContainsFinalizer(cluster, clusterv1.ClusterFinalizer) { - controllerutil.AddFinalizer(cluster, clusterv1.ClusterFinalizer) - return ctrl.Result{}, nil - } - // Handle normal reconciliation loop. return r.reconcile(ctx, cluster) } diff --git a/internal/controllers/machine/machine_controller.go b/internal/controllers/machine/machine_controller.go index 7f6dc8f30394..4b6189811e46 100644 --- a/internal/controllers/machine/machine_controller.go +++ b/internal/controllers/machine/machine_controller.go @@ -52,6 +52,7 @@ import ( "sigs.k8s.io/cluster-api/util/annotations" "sigs.k8s.io/cluster-api/util/collections" "sigs.k8s.io/cluster-api/util/conditions" + "sigs.k8s.io/cluster-api/util/finalizers" clog "sigs.k8s.io/cluster-api/util/log" "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/cluster-api/util/predicates" @@ -173,6 +174,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re return ctrl.Result{}, err } + log := ctrl.LoggerFrom(ctx).WithValues("Cluster", klog.KRef(m.Namespace, m.Spec.ClusterName)) + ctx = ctrl.LoggerInto(ctx, log) + + // Add finalizer first if not set to avoid the race condition between init and delete. + if finalizerAdded, err := finalizers.EnsureFinalizer(ctx, r.Client, m, clusterv1.MachineFinalizer); err != nil || finalizerAdded { + return ctrl.Result{}, err + } + // AddOwners adds the owners of Machine as k/v pairs to the logger. // Specifically, it will add KubeadmControlPlane, MachineSet and MachineDeployment. ctx, log, err := clog.AddOwners(ctx, r.Client, m) @@ -180,9 +189,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re return ctrl.Result{}, err } - log = log.WithValues("Cluster", klog.KRef(m.ObjectMeta.Namespace, m.Spec.ClusterName)) - ctx = ctrl.LoggerInto(ctx, log) - cluster, err := util.GetClusterByName(ctx, r.Client, m.ObjectMeta.Namespace, m.Spec.ClusterName) if err != nil { return ctrl.Result{}, errors.Wrapf(err, "failed to get cluster %q for machine %q in namespace %q", @@ -220,13 +226,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re } }() - // Add finalizer first if not set to avoid the race condition between init and delete. - // Note: Finalizers in general can only be added when the deletionTimestamp is not set. - if !controllerutil.ContainsFinalizer(m, clusterv1.MachineFinalizer) && m.ObjectMeta.DeletionTimestamp.IsZero() { - controllerutil.AddFinalizer(m, clusterv1.MachineFinalizer) - return ctrl.Result{}, nil - } - alwaysReconcile := []machineReconcileFunc{ r.reconcileMachineOwnerAndLabels, r.reconcileBootstrap, diff --git a/internal/controllers/machinedeployment/machinedeployment_controller.go b/internal/controllers/machinedeployment/machinedeployment_controller.go index d2b996242356..73e73a9e2fe7 100644 --- a/internal/controllers/machinedeployment/machinedeployment_controller.go +++ b/internal/controllers/machinedeployment/machinedeployment_controller.go @@ -43,6 +43,7 @@ import ( "sigs.k8s.io/cluster-api/util/annotations" "sigs.k8s.io/cluster-api/util/conditions" utilconversion "sigs.k8s.io/cluster-api/util/conversion" + "sigs.k8s.io/cluster-api/util/finalizers" clog "sigs.k8s.io/cluster-api/util/log" "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/cluster-api/util/predicates" @@ -131,6 +132,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re log = log.WithValues("Cluster", klog.KRef(deployment.Namespace, deployment.Spec.ClusterName)) ctx = ctrl.LoggerInto(ctx, log) + // Add finalizer first if not set to avoid the race condition between init and delete. + if finalizerAdded, err := finalizers.EnsureFinalizer(ctx, r.Client, deployment, clusterv1.MachineDeploymentFinalizer); err != nil || finalizerAdded { + return ctrl.Result{}, err + } + cluster, err := util.GetClusterByName(ctx, r.Client, deployment.Namespace, deployment.Spec.ClusterName) if err != nil { return ctrl.Result{}, err @@ -165,13 +171,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re return ctrl.Result{}, r.reconcileDelete(ctx, deployment) } - // Add finalizer first if not set to avoid the race condition between init and delete. - // Note: Finalizers in general can only be added when the deletionTimestamp is not set. - if !controllerutil.ContainsFinalizer(deployment, clusterv1.MachineDeploymentFinalizer) { - controllerutil.AddFinalizer(deployment, clusterv1.MachineDeploymentFinalizer) - return ctrl.Result{}, nil - } - err = r.reconcile(ctx, cluster, deployment) if err != nil { r.recorder.Eventf(deployment, corev1.EventTypeWarning, "ReconcileError", "%v", err) diff --git a/internal/controllers/machineset/machineset_controller.go b/internal/controllers/machineset/machineset_controller.go index 3986d469303a..51da8e19440c 100644 --- a/internal/controllers/machineset/machineset_controller.go +++ b/internal/controllers/machineset/machineset_controller.go @@ -56,6 +56,7 @@ import ( "sigs.k8s.io/cluster-api/util/collections" "sigs.k8s.io/cluster-api/util/conditions" utilconversion "sigs.k8s.io/cluster-api/util/conversion" + "sigs.k8s.io/cluster-api/util/finalizers" "sigs.k8s.io/cluster-api/util/labels/format" clog "sigs.k8s.io/cluster-api/util/log" "sigs.k8s.io/cluster-api/util/patch" @@ -149,6 +150,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re return ctrl.Result{}, err } + log := ctrl.LoggerFrom(ctx).WithValues("Cluster", klog.KRef(machineSet.Namespace, machineSet.Spec.ClusterName)) + ctx = ctrl.LoggerInto(ctx, log) + + // Add finalizer first if not set to avoid the race condition between init and delete. + if finalizerAdded, err := finalizers.EnsureFinalizer(ctx, r.Client, machineSet, clusterv1.MachineSetFinalizer); err != nil || finalizerAdded { + return ctrl.Result{}, err + } + // AddOwners adds the owners of MachineSet as k/v pairs to the logger. // Specifically, it will add MachineDeployment. ctx, log, err := clog.AddOwners(ctx, r.Client, machineSet) @@ -156,9 +165,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re return ctrl.Result{}, err } - log = log.WithValues("Cluster", klog.KRef(machineSet.ObjectMeta.Namespace, machineSet.Spec.ClusterName)) - ctx = ctrl.LoggerInto(ctx, log) - cluster, err := util.GetClusterByName(ctx, r.Client, machineSet.ObjectMeta.Namespace, machineSet.Spec.ClusterName) if err != nil { return ctrl.Result{}, err @@ -188,13 +194,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re return ctrl.Result{}, r.reconcileDelete(ctx, machineSet) } - // Add finalizer first if not set to avoid the race condition between init and delete. - // Note: Finalizers in general can only be added when the deletionTimestamp is not set. - if !controllerutil.ContainsFinalizer(machineSet, clusterv1.MachineSetFinalizer) { - controllerutil.AddFinalizer(machineSet, clusterv1.MachineSetFinalizer) - return ctrl.Result{}, nil - } - result, err := r.reconcile(ctx, cluster, machineSet) if err != nil { // Requeue if the reconcile failed because the ClusterCacheTracker was locked for diff --git a/internal/controllers/topology/machinedeployment/machinedeployment_controller.go b/internal/controllers/topology/machinedeployment/machinedeployment_controller.go index fd34df793f03..a44073d1235d 100644 --- a/internal/controllers/topology/machinedeployment/machinedeployment_controller.go +++ b/internal/controllers/topology/machinedeployment/machinedeployment_controller.go @@ -35,6 +35,7 @@ import ( "sigs.k8s.io/cluster-api/internal/controllers/topology/machineset" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/annotations" + "sigs.k8s.io/cluster-api/util/finalizers" "sigs.k8s.io/cluster-api/util/labels" "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/cluster-api/util/predicates" @@ -124,6 +125,17 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re log = log.WithValues("Cluster", klog.KRef(md.Namespace, md.Spec.ClusterName)) ctx = ctrl.LoggerInto(ctx, log) + // Return early if the MachineDeployment is not topology owned. + if !labels.IsTopologyOwned(md) { + log.Info(fmt.Sprintf("Reconciliation is skipped because the MachineDeployment does not have the %q label", clusterv1.ClusterTopologyOwnedLabel)) + return ctrl.Result{}, nil + } + + // Add finalizer first if not set to avoid the race condition between init and delete. + if finalizerAdded, err := finalizers.EnsureFinalizer(ctx, r.Client, md, clusterv1.MachineDeploymentTopologyFinalizer); err != nil || finalizerAdded { + return ctrl.Result{}, err + } + cluster, err := util.GetClusterByName(ctx, r.Client, md.Namespace, md.Spec.ClusterName) if err != nil { return ctrl.Result{}, err @@ -135,12 +147,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re return ctrl.Result{}, nil } - // Return early if the MachineDeployment is not topology owned. - if !labels.IsTopologyOwned(md) { - log.Info(fmt.Sprintf("Reconciliation is skipped because the MachineDeployment does not have the %q label", clusterv1.ClusterTopologyOwnedLabel)) - 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 { @@ -157,13 +163,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re return ctrl.Result{}, r.reconcileDelete(ctx, md) } - // Add finalizer first if not set to avoid the race condition between init and delete. - // Note: Finalizers in general can only be added when the deletionTimestamp is not set. - if !controllerutil.ContainsFinalizer(md, clusterv1.MachineDeploymentTopologyFinalizer) { - controllerutil.AddFinalizer(md, clusterv1.MachineDeploymentTopologyFinalizer) - return ctrl.Result{}, nil - } - return ctrl.Result{}, nil } diff --git a/internal/controllers/topology/machineset/machineset_controller.go b/internal/controllers/topology/machineset/machineset_controller.go index 70b3771cc10a..7cfc8e13173f 100644 --- a/internal/controllers/topology/machineset/machineset_controller.go +++ b/internal/controllers/topology/machineset/machineset_controller.go @@ -36,6 +36,7 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/annotations" + "sigs.k8s.io/cluster-api/util/finalizers" "sigs.k8s.io/cluster-api/util/labels" clog "sigs.k8s.io/cluster-api/util/log" "sigs.k8s.io/cluster-api/util/patch" @@ -121,6 +122,20 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re return ctrl.Result{}, errors.Wrapf(err, "failed to get MachineSet/%s", req.NamespacedName.Name) } + log := ctrl.LoggerFrom(ctx).WithValues("Cluster", klog.KRef(ms.Namespace, ms.Spec.ClusterName)) + ctx = ctrl.LoggerInto(ctx, log) + + // Return early if the MachineSet is not topology owned. + if !labels.IsTopologyOwned(ms) { + log.Info(fmt.Sprintf("Reconciliation is skipped because the MachineSet does not have the %q label", clusterv1.ClusterTopologyOwnedLabel)) + return ctrl.Result{}, nil + } + + // Add finalizer first if not set to avoid the race condition between init and delete. + if finalizerAdded, err := finalizers.EnsureFinalizer(ctx, r.Client, ms, clusterv1.MachineSetTopologyFinalizer); err != nil || finalizerAdded { + return ctrl.Result{}, err + } + // AddOwners adds the owners of MachineSet as k/v pairs to the logger. // Specifically, it will add MachineDeployment. ctx, log, err := clog.AddOwners(ctx, r.Client, ms) @@ -128,9 +143,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re return ctrl.Result{}, err } - log = log.WithValues("Cluster", klog.KRef(ms.Namespace, ms.Spec.ClusterName)) - ctx = ctrl.LoggerInto(ctx, log) - cluster, err := util.GetClusterByName(ctx, r.Client, ms.Namespace, ms.Spec.ClusterName) if err != nil { return ctrl.Result{}, err @@ -142,12 +154,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re return ctrl.Result{}, nil } - // Return early if the MachineSet is not topology owned. - if !labels.IsTopologyOwned(ms) { - log.Info(fmt.Sprintf("Reconciliation is skipped because the MachineSet does not have the %q label", clusterv1.ClusterTopologyOwnedLabel)) - 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 { @@ -164,13 +170,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re return ctrl.Result{}, r.reconcileDelete(ctx, ms) } - // Add finalizer first if not set to avoid the race condition between init and delete. - // Note: Finalizers in general can only be added when the deletionTimestamp is not set. - if !controllerutil.ContainsFinalizer(ms, clusterv1.MachineSetTopologyFinalizer) { - controllerutil.AddFinalizer(ms, clusterv1.MachineSetTopologyFinalizer) - return ctrl.Result{}, nil - } - return ctrl.Result{}, nil } diff --git a/test/infrastructure/docker/internal/controllers/dockercluster_controller.go b/test/infrastructure/docker/internal/controllers/dockercluster_controller.go index 252466fc6b09..ba0ad37c331b 100644 --- a/test/infrastructure/docker/internal/controllers/dockercluster_controller.go +++ b/test/infrastructure/docker/internal/controllers/dockercluster_controller.go @@ -36,6 +36,7 @@ import ( "sigs.k8s.io/cluster-api/test/infrastructure/docker/internal/docker" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/conditions" + "sigs.k8s.io/cluster-api/util/finalizers" "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/cluster-api/util/predicates" ) @@ -67,6 +68,11 @@ func (r *DockerClusterReconciler) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{}, err } + // Add finalizer first if not set to avoid the race condition between init and delete. + if finalizerAdded, err := finalizers.EnsureFinalizer(ctx, r.Client, dockerCluster, infrav1.ClusterFinalizer); err != nil || finalizerAdded { + return ctrl.Result{}, err + } + // Fetch the Cluster. cluster, err := util.GetOwnerCluster(ctx, r.Client, dockerCluster.ObjectMeta) if err != nil { @@ -112,13 +118,6 @@ func (r *DockerClusterReconciler) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{}, r.reconcileDelete(ctx, dockerCluster, externalLoadBalancer) } - // Add finalizer first if not set to avoid the race condition between init and delete. - // Note: Finalizers in general can only be added when the deletionTimestamp is not set. - if !controllerutil.ContainsFinalizer(dockerCluster, infrav1.ClusterFinalizer) { - controllerutil.AddFinalizer(dockerCluster, infrav1.ClusterFinalizer) - return ctrl.Result{}, nil - } - // Handle non-deleted clusters return ctrl.Result{}, r.reconcileNormal(ctx, dockerCluster, externalLoadBalancer) } diff --git a/test/infrastructure/docker/internal/controllers/dockermachine_controller.go b/test/infrastructure/docker/internal/controllers/dockermachine_controller.go index bce85314a3ee..5e72a160afad 100644 --- a/test/infrastructure/docker/internal/controllers/dockermachine_controller.go +++ b/test/infrastructure/docker/internal/controllers/dockermachine_controller.go @@ -46,6 +46,7 @@ import ( "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/annotations" "sigs.k8s.io/cluster-api/util/conditions" + "sigs.k8s.io/cluster-api/util/finalizers" "sigs.k8s.io/cluster-api/util/labels" clog "sigs.k8s.io/cluster-api/util/log" "sigs.k8s.io/cluster-api/util/patch" @@ -82,6 +83,11 @@ func (r *DockerMachineReconciler) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{}, err } + // Add finalizer first if not set to avoid the race condition between init and delete. + if finalizerAdded, err := finalizers.EnsureFinalizer(ctx, r.Client, dockerMachine, infrav1.MachineFinalizer); err != nil || finalizerAdded { + return ctrl.Result{}, err + } + // AddOwners adds the owners of DockerMachine as k/v pairs to the logger. // Specifically, it will add KubeadmControlPlane, MachineSet and MachineDeployment. ctx, log, err := clog.AddOwners(ctx, r.Client, dockerMachine) @@ -153,13 +159,6 @@ func (r *DockerMachineReconciler) Reconcile(ctx context.Context, req ctrl.Reques } }() - // Add finalizer first if not set to avoid the race condition between init and delete. - // Note: Finalizers in general can only be added when the deletionTimestamp is not set. - if dockerMachine.ObjectMeta.DeletionTimestamp.IsZero() && !controllerutil.ContainsFinalizer(dockerMachine, infrav1.MachineFinalizer) { - controllerutil.AddFinalizer(dockerMachine, infrav1.MachineFinalizer) - return ctrl.Result{}, nil - } - // Create a helper for managing the Docker container hosting the machine. // The DockerMachine needs a way to know the name of the Docker container, before MachinePool Machines were implemented, it used the name of the owner Machine. // But since the DockerMachine type is used for both MachineDeployment Machines and MachinePool Machines, we need to accommodate both: diff --git a/test/infrastructure/inmemory/internal/controllers/inmemorycluster_controller.go b/test/infrastructure/inmemory/internal/controllers/inmemorycluster_controller.go index aa2f6033e7a4..32ad7ed8a175 100644 --- a/test/infrastructure/inmemory/internal/controllers/inmemorycluster_controller.go +++ b/test/infrastructure/inmemory/internal/controllers/inmemorycluster_controller.go @@ -37,6 +37,7 @@ import ( inmemoryruntime "sigs.k8s.io/cluster-api/test/infrastructure/inmemory/pkg/runtime" inmemoryserver "sigs.k8s.io/cluster-api/test/infrastructure/inmemory/pkg/server" "sigs.k8s.io/cluster-api/util" + "sigs.k8s.io/cluster-api/util/finalizers" "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/cluster-api/util/predicates" ) @@ -72,6 +73,11 @@ func (r *InMemoryClusterReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, err } + // Add finalizer first if not set to avoid the race condition between init and delete. + if finalizerAdded, err := finalizers.EnsureFinalizer(ctx, r.Client, inMemoryCluster, infrav1.ClusterFinalizer); err != nil || finalizerAdded { + return ctrl.Result{}, err + } + // Fetch the Cluster. cluster, err := util.GetOwnerCluster(ctx, r.Client, inMemoryCluster.ObjectMeta) if err != nil { @@ -109,13 +115,6 @@ func (r *InMemoryClusterReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, r.reconcileDelete(ctx, cluster, inMemoryCluster) } - // Add finalizer first if not set to avoid the race condition between init and delete. - // Note: Finalizers in general can only be added when the deletionTimestamp is not set. - if !controllerutil.ContainsFinalizer(inMemoryCluster, infrav1.ClusterFinalizer) { - controllerutil.AddFinalizer(inMemoryCluster, infrav1.ClusterFinalizer) - return ctrl.Result{}, nil - } - // Handle non-deleted clusters return ctrl.Result{}, r.reconcileNormal(ctx, cluster, inMemoryCluster) } diff --git a/test/infrastructure/inmemory/internal/controllers/inmemorymachine_controller.go b/test/infrastructure/inmemory/internal/controllers/inmemorymachine_controller.go index 0cf3afb58ad6..e45d4aee2fde 100644 --- a/test/infrastructure/inmemory/internal/controllers/inmemorymachine_controller.go +++ b/test/infrastructure/inmemory/internal/controllers/inmemorymachine_controller.go @@ -51,6 +51,7 @@ import ( "sigs.k8s.io/cluster-api/util/annotations" "sigs.k8s.io/cluster-api/util/certs" "sigs.k8s.io/cluster-api/util/conditions" + "sigs.k8s.io/cluster-api/util/finalizers" clog "sigs.k8s.io/cluster-api/util/log" "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/cluster-api/util/predicates" @@ -83,6 +84,11 @@ func (r *InMemoryMachineReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, err } + // Add finalizer first if not set to avoid the race condition between init and delete. + if finalizerAdded, err := finalizers.EnsureFinalizer(ctx, r.Client, inMemoryMachine, infrav1.MachineFinalizer); err != nil || finalizerAdded { + return ctrl.Result{}, err + } + // AddOwners adds the owners of InMemoryMachine as k/v pairs to the logger. // Specifically, it will add KubeadmControlPlane, MachineSet and MachineDeployment. ctx, log, err := clog.AddOwners(ctx, r.Client, inMemoryMachine) @@ -168,13 +174,6 @@ func (r *InMemoryMachineReconciler) Reconcile(ctx context.Context, req ctrl.Requ return r.reconcileDelete(ctx, cluster, machine, inMemoryMachine) } - // Add finalizer first if not set to avoid the race condition between init and delete. - // Note: Finalizers in general can only be added when the deletionTimestamp is not set. - if !controllerutil.ContainsFinalizer(inMemoryMachine, infrav1.MachineFinalizer) { - controllerutil.AddFinalizer(inMemoryMachine, infrav1.MachineFinalizer) - return ctrl.Result{}, nil - } - // Handle non-deleted machines return r.reconcileNormal(ctx, cluster, machine, inMemoryMachine) } diff --git a/util/finalizers/finalizers.go b/util/finalizers/finalizers.go new file mode 100644 index 000000000000..2e351dd8e728 --- /dev/null +++ b/util/finalizers/finalizers.go @@ -0,0 +1,55 @@ +/* +Copyright 2024 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Package finalizers implements finalizer helper functions. +package finalizers + +import ( + "context" + + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + + "sigs.k8s.io/cluster-api/util/patch" +) + +// EnsureFinalizer adds a finalizer if the object doesn't have a deletionTimestamp set +// and if the finalizer is not already set. +// This util is usually used in reconcilers directly after the reconciled object was retrieved +// and before pause is handled or "defer patch" with the patch helper. +func EnsureFinalizer(ctx context.Context, c client.Client, o client.Object, finalizer string) (finalizerAdded bool, err error) { + // Finalizers can only be added when the deletionTimestamp is not set. + if !o.GetDeletionTimestamp().IsZero() { + return false, nil + } + + if controllerutil.ContainsFinalizer(o, finalizer) { + return false, nil + } + + patchHelper, err := patch.NewHelper(o, c) + if err != nil { + return false, err + } + + controllerutil.AddFinalizer(o, finalizer) + + if err := patchHelper.Patch(ctx, o); err != nil { + return false, err + } + + return true, nil +} diff --git a/util/finalizers/finalizers_test.go b/util/finalizers/finalizers_test.go new file mode 100644 index 000000000000..096b696d57eb --- /dev/null +++ b/util/finalizers/finalizers_test.go @@ -0,0 +1,108 @@ +/* +Copyright 2024 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Package finalizers implements finalizer helper functions. +package finalizers + +import ( + "testing" + "time" + + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" +) + +var ( + ctx = ctrl.SetupSignalHandler() +) + +func TestEnsureFinalizer(t *testing.T) { + g := NewWithT(t) + + scheme := runtime.NewScheme() + g.Expect(clusterv1.AddToScheme(scheme)).To(Succeed()) + + testFinalizer := "cluster.x-k8s.io/test-finalizer" + + tests := []struct { + name string + obj client.Object + wantErr bool + wantFinalizersUpdated bool + wantFinalizer bool + }{ + { + name: "should not add finalizer if object has deletionTimestamp", + obj: &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + DeletionTimestamp: &metav1.Time{Time: time.Now()}, + Finalizers: []string{"some-other-finalizer"}, + }, + }, + wantErr: false, + wantFinalizersUpdated: false, + wantFinalizer: false, + }, + { + name: "should not add finalizer if finalizer is already set", + obj: &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + Finalizers: []string{testFinalizer}, + }, + }, + wantErr: false, + wantFinalizersUpdated: false, + wantFinalizer: true, + }, + { + name: "should add finalizer if finalizer is not already set", + obj: &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + Finalizers: []string{"some-other-finalizer"}, + }, + }, + wantErr: false, + wantFinalizersUpdated: true, + wantFinalizer: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tt.obj).Build() + + gotFinalizersUpdated, gotErr := EnsureFinalizer(ctx, c, tt.obj, testFinalizer) + g.Expect(gotErr != nil).To(Equal(tt.wantErr)) + g.Expect(gotFinalizersUpdated).To(Equal(tt.wantFinalizersUpdated)) + + gotObj := tt.obj.DeepCopyObject().(client.Object) + g.Expect(c.Get(ctx, client.ObjectKeyFromObject(gotObj), gotObj)).To(Succeed()) + g.Expect(controllerutil.ContainsFinalizer(gotObj, testFinalizer)).To(Equal(tt.wantFinalizer)) + }) + } +}