all: only set finalizers if deletionTimestamp is not set

Signed-off-by: Stefan Büringer buringerst@vmware.com

controlplane/kubeadm/internal/controllers/controller.go

@@ -174,8 +174,9 @@ func (r *KubeadmControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.
 		return ctrl.Result{Requeue: true}, nil
 	}
 
-	// Add finalizer first if not exist to avoid the race condition between init and delete
-	if !controllerutil.ContainsFinalizer(kcp, controlplanev1.KubeadmControlPlaneFinalizer) {
+	// 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,

exp/addons/internal/controllers/clusterresourceset_controller.go

@@ -131,17 +131,18 @@ func (r *ClusterResourceSetReconciler) Reconcile(ctx context.Context, req ctrl.R
 		return ctrl.Result{}, err
 	}
 
-	// Add finalizer first if not exist to avoid the race condition between init and delete
-	if !controllerutil.ContainsFinalizer(clusterResourceSet, addonsv1.ClusterResourceSetFinalizer) {
-		controllerutil.AddFinalizer(clusterResourceSet, addonsv1.ClusterResourceSetFinalizer)
-		return ctrl.Result{}, nil
-	}
-
 	// Handle deletion reconciliation loop.
 	if !clusterResourceSet.ObjectMeta.DeletionTimestamp.IsZero() {
 		return 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 {

exp/internal/controllers/machinepool_controller.go

@@ -181,12 +181,6 @@ func (r *MachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	}
 	mp.Labels[clusterv1.ClusterNameLabel] = mp.Spec.ClusterName
 
-	// Add finalizer first if not exist to avoid the race condition between init and delete
-	if !controllerutil.ContainsFinalizer(mp, expv1.MachinePoolFinalizer) {
-		controllerutil.AddFinalizer(mp, expv1.MachinePoolFinalizer)
-		return ctrl.Result{}, nil
-	}
-
 	// Handle deletion reconciliation loop.
 	if !mp.ObjectMeta.DeletionTimestamp.IsZero() {
 		res, err := r.reconcileDelete(ctx, cluster, mp)
@@ -199,6 +193,13 @@ func (r *MachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		return res, 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.
 	res, err := r.reconcile(ctx, cluster, mp)
 	// Requeue if the reconcile failed because the ClusterCacheTracker was locked for

internal/controllers/cluster/cluster_controller.go

@@ -142,17 +142,18 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 		}
 	}()
 
-	// Add finalizer first if not exist to avoid the race condition between init and delete
-	if !controllerutil.ContainsFinalizer(cluster, clusterv1.ClusterFinalizer) {
-		controllerutil.AddFinalizer(cluster, clusterv1.ClusterFinalizer)
-		return ctrl.Result{}, nil
-	}
-
 	// Handle deletion reconciliation loop.
 	if !cluster.ObjectMeta.DeletionTimestamp.IsZero() {
 		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)
 }

internal/controllers/machine/machine_controller.go

@@ -195,12 +195,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 	}
 	m.Labels[clusterv1.ClusterNameLabel] = m.Spec.ClusterName
 
-	// Add finalizer first if not exist to avoid the race condition between init and delete
-	if !controllerutil.ContainsFinalizer(m, clusterv1.MachineFinalizer) {
-		controllerutil.AddFinalizer(m, clusterv1.MachineFinalizer)
-		return ctrl.Result{}, nil
-	}
-
 	// Handle deletion reconciliation loop.
 	if !m.ObjectMeta.DeletionTimestamp.IsZero() {
 		res, err := r.reconcileDelete(ctx, cluster, m)
@@ -213,6 +207,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 		return res, 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(m, clusterv1.MachineFinalizer) {
+		controllerutil.AddFinalizer(m, clusterv1.MachineFinalizer)
+		return ctrl.Result{}, nil
+	}
+
 	// Handle normal reconciliation loop.
 	res, err := r.reconcile(ctx, cluster, m)
 	// Requeue if the reconcile failed because the ClusterCacheTracker was locked for

internal/controllers/topology/machinedeployment/machinedeployment_controller.go

@@ -129,8 +129,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 		return r.reconcileDelete(ctx, md)
 	}
 
-	// If the MachineDeployment is not being deleted ensure the finalizer is set.
-	controllerutil.AddFinalizer(md, clusterv1.MachineDeploymentTopologyFinalizer)
+	// 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
 }

internal/controllers/topology/machineset/machineset_controller.go

@@ -136,8 +136,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 		return r.reconcileDelete(ctx, ms)
 	}
 
-	// If the MachineSet is not being deleted ensure the finalizer is set.
-	controllerutil.AddFinalizer(ms, clusterv1.MachineSetTopologyFinalizer)
+	// 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
 }

test/infrastructure/docker/exp/internal/controllers/dockermachinepool_controller.go

@@ -119,17 +119,18 @@ func (r *DockerMachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Re
 		}
 	}()
 
-	// Add finalizer first if not exist to avoid the race condition between init and delete
-	if !controllerutil.ContainsFinalizer(dockerMachinePool, infraexpv1.MachinePoolFinalizer) {
-		controllerutil.AddFinalizer(dockerMachinePool, infraexpv1.MachinePoolFinalizer)
-		return ctrl.Result{}, nil
-	}
-
 	// Handle deleted machines
 	if !dockerMachinePool.ObjectMeta.DeletionTimestamp.IsZero() {
 		return r.reconcileDelete(ctx, cluster, machinePool, dockerMachinePool)
 	}
 
+	// 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(dockerMachinePool, infraexpv1.MachinePoolFinalizer) {
+		controllerutil.AddFinalizer(dockerMachinePool, infraexpv1.MachinePoolFinalizer)
+		return ctrl.Result{}, nil
+	}
+
 	// Handle non-deleted machines
 	res, err = r.reconcileNormal(ctx, cluster, machinePool, dockerMachinePool)
 	// Requeue if the reconcile failed because the ClusterCacheTracker was locked for

test/infrastructure/docker/internal/controllers/dockercluster_controller.go

@@ -106,17 +106,18 @@ func (r *DockerClusterReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 	// In the case of Docker, failure domains don't mean much so we simply copy the Spec into the Status.
 	dockerCluster.Status.FailureDomains = dockerCluster.Spec.FailureDomains
 
-	// Add finalizer first if not exist to avoid the race condition between init and delete
-	if !controllerutil.ContainsFinalizer(dockerCluster, infrav1.ClusterFinalizer) {
-		controllerutil.AddFinalizer(dockerCluster, infrav1.ClusterFinalizer)
-		return ctrl.Result{}, nil
-	}
-
 	// Handle deleted clusters
 	if !dockerCluster.DeletionTimestamp.IsZero() {
 		return 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 r.reconcileNormal(ctx, dockerCluster, externalLoadBalancer)
 }

test/infrastructure/docker/internal/controllers/dockermachine_controller.go

@@ -143,8 +143,9 @@ func (r *DockerMachineReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 		}
 	}()
 
-	// Add finalizer first if not exist to avoid the race condition between init and delete
-	if !controllerutil.ContainsFinalizer(dockerMachine, infrav1.MachineFinalizer) {
+	// 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
 	}

test/infrastructure/inmemory/internal/controllers/inmemorycluster_controller.go

@@ -106,17 +106,18 @@ func (r *InMemoryClusterReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		}
 	}()
 
-	// Add finalizer first if not exist to avoid the race condition between init and delete
-	if !controllerutil.ContainsFinalizer(inMemoryCluster, infrav1.ClusterFinalizer) {
-		controllerutil.AddFinalizer(inMemoryCluster, infrav1.ClusterFinalizer)
-		return ctrl.Result{}, nil
-	}
-
 	// Handle deleted clusters
 	if !inMemoryCluster.DeletionTimestamp.IsZero() {
 		return 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 r.reconcileNormal(ctx, cluster, inMemoryCluster)
 }

test/infrastructure/inmemory/internal/controllers/inmemorymachine_controller.go

@@ -169,17 +169,18 @@ func (r *InMemoryMachineReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		}
 	}()
 
-	// Add finalizer first if not exist to avoid the race condition between init and delete
-	if !controllerutil.ContainsFinalizer(inMemoryMachine, infrav1.MachineFinalizer) {
-		controllerutil.AddFinalizer(inMemoryMachine, infrav1.MachineFinalizer)
-		return ctrl.Result{}, nil
-	}
-
 	// Handle deleted machines
 	if !inMemoryMachine.DeletionTimestamp.IsZero() {
 		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)
 }