Merge pull request #8949 from sbueringer/pr-optimize-add-finalizer

k8s-ci-robot · web-flow · commit 730f63ddd766 · 2023-07-03T09:22:51.000-07:00
🐛 all: only set finalizers if deletionTimestamp is not set
diff --git a/controlplane/kubeadm/internal/controllers/controller.go b/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,
diff --git a/exp/addons/internal/controllers/clusterresourceset_controller.go b/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 {
diff --git a/exp/internal/controllers/machinepool_controller.go b/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
diff --git a/internal/controllers/cluster/cluster_controller.go b/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)
 }
diff --git a/internal/controllers/machine/machine_controller.go b/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
diff --git a/internal/controllers/topology/machinedeployment/machinedeployment_controller.go b/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
 }
diff --git a/internal/controllers/topology/machineset/machineset_controller.go b/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
 }
diff --git a/test/infrastructure/docker/exp/internal/controllers/dockermachinepool_controller.go b/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
diff --git a/test/infrastructure/docker/internal/controllers/dockercluster_controller.go b/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)
 }
diff --git a/test/infrastructure/docker/internal/controllers/dockermachine_controller.go b/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
 	}
diff --git a/test/infrastructure/inmemory/internal/controllers/inmemorycluster_controller.go b/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)
 }
diff --git a/test/infrastructure/inmemory/internal/controllers/inmemorymachine_controller.go b/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)
 }

Original file line number	Diff line number	Diff line change
`@@ -129,8 +129,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re`
`129`	`129`	`return r.reconcileDelete(ctx, md)`
`130`	`130`	`}`
`131`	`131`
`132`		`- // If the MachineDeployment is not being deleted ensure the finalizer is set.`
`133`		`- controllerutil.AddFinalizer(md, clusterv1.MachineDeploymentTopologyFinalizer)`
	`132`	`+ // Add finalizer first if not set to avoid the race condition between init and delete.`
	`133`	`+ // Note: Finalizers in general can only be added when the deletionTimestamp is not set.`
	`134`	`+ if !controllerutil.ContainsFinalizer(md, clusterv1.MachineDeploymentTopologyFinalizer) {`
	`135`	`+ controllerutil.AddFinalizer(md, clusterv1.MachineDeploymentTopologyFinalizer)`
	`136`	`+ return ctrl.Result{}, nil`
	`137`	`+ }`
`134`	`138`
`135`	`139`	`return ctrl.Result{}, nil`
`136`	`140`	`}`
Original file line number	Diff line number	Diff line change
`@@ -136,8 +136,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re`
`136`	`136`	`return r.reconcileDelete(ctx, ms)`
`137`	`137`	`}`
`138`	`138`
`139`		`- // If the MachineSet is not being deleted ensure the finalizer is set.`
`140`		`- controllerutil.AddFinalizer(ms, clusterv1.MachineSetTopologyFinalizer)`
	`139`	`+ // Add finalizer first if not set to avoid the race condition between init and delete.`
	`140`	`+ // Note: Finalizers in general can only be added when the deletionTimestamp is not set.`
	`141`	`+ if !controllerutil.ContainsFinalizer(ms, clusterv1.MachineSetTopologyFinalizer) {`
	`142`	`+ controllerutil.AddFinalizer(ms, clusterv1.MachineSetTopologyFinalizer)`
	`143`	`+ return ctrl.Result{}, nil`
	`144`	`+ }`
`141`	`145`
`142`	`146`	`return ctrl.Result{}, nil`
`143`	`147`	`}`
Original file line number	Diff line number	Diff line change
`@@ -143,8 +143,9 @@ func (r *DockerMachineReconciler) Reconcile(ctx context.Context, req ctrl.Reques`
`143`	`143`	`}`
`144`	`144`	`}()`
`145`	`145`
`146`		`- // Add finalizer first if not exist to avoid the race condition between init and delete`
`147`		`- if !controllerutil.ContainsFinalizer(dockerMachine, infrav1.MachineFinalizer) {`
	`146`	`+ // Add finalizer first if not set to avoid the race condition between init and delete.`
	`147`	`+ // Note: Finalizers in general can only be added when the deletionTimestamp is not set.`
	`148`	`+ if dockerMachine.ObjectMeta.DeletionTimestamp.IsZero() && !controllerutil.ContainsFinalizer(dockerMachine, infrav1.MachineFinalizer) {`
`148`	`149`	`controllerutil.AddFinalizer(dockerMachine, infrav1.MachineFinalizer)`
`149`	`150`	`return ctrl.Result{}, nil`
`150`	`151`	`}`