Refine finalizer handling

- Add finalizer if not existed - Requeue immediately after adding finalizer to avoid the race condition between init and delete Signed-off-by: Gong Zhang <[email protected]>
kubernetes-sigs · Jul 27, 2023 · daa93cd · daa93cd
1 parent 80a870c
commit daa93cd
Show file tree

Hide file tree

Showing 6 changed files with 37 additions and 14 deletions.
diff --git a/controllers/vmware/test/controllers_test.go b/controllers/vmware/test/controllers_test.go
@@ -417,7 +417,9 @@ var _ = Describe("Reconciliation tests", func() {
 			By("Expect a ResourcePolicy to exist")
 			rpKey := client.ObjectKey{Namespace: infraCluster.GetNamespace(), Name: infraCluster.GetName()}
 			resourcePolicy := &vmoprv1.VirtualMachineSetResourcePolicy{}
-			Expect(k8sClient.Get(ctx, rpKey, resourcePolicy)).To(Succeed())
+			Eventually(func() error {
+				return k8sClient.Get(ctx, rpKey, resourcePolicy)
+			}, time.Second*30).Should(Succeed())
 			Expect(len(resourcePolicy.Spec.ClusterModules)).To(BeEquivalentTo(2))
 
 			By("Create the CAPI Machine and wait for it to exist")

diff --git a/controllers/vmware/vspherecluster_reconciler.go b/controllers/vmware/vspherecluster_reconciler.go
@@ -121,6 +121,13 @@ func (r ClusterReconciler) Reconcile(_ goctx.Context, req ctrl.Request) (_ ctrl.
 		return reconcile.Result{}, nil
 	}
 
+	// If the VSphereCluster doesn't have our finalizer, add it.
+	// Requeue immediately after adding finalizer to avoid the race condition between init and delete
+	if !controllerutil.ContainsFinalizer(vsphereCluster, vmwarev1.ClusterFinalizer) {
+		controllerutil.AddFinalizer(vsphereCluster, vmwarev1.ClusterFinalizer)
+		return ctrl.Result{}, nil
+	}
+
 	// Handle non-deleted clusters
 	return ctrl.Result{}, r.reconcileNormal(clusterContext)
 }
@@ -147,9 +154,6 @@ func (r *ClusterReconciler) reconcileDelete(ctx *vmware.ClusterContext) {
 func (r *ClusterReconciler) reconcileNormal(ctx *vmware.ClusterContext) error {
 	ctx.Logger.Info("Reconciling vsphereCluster")
 
-	// If the vsphereCluster doesn't have our finalizer, add it.
-	controllerutil.AddFinalizer(ctx.VSphereCluster, vmwarev1.ClusterFinalizer)
-
 	// Get any failure domains to report back to the CAPI core controller.
 	failureDomains, err := r.getFailureDomains(ctx)
 	if err != nil {

diff --git a/controllers/vspherecluster_reconciler.go b/controllers/vspherecluster_reconciler.go
@@ -128,6 +128,13 @@ func (r clusterReconciler) Reconcile(_ goctx.Context, req ctrl.Request) (_ ctrl.
 		return r.reconcileDelete(clusterContext)
 	}
 
+	// If the VSphereCluster doesn't have our finalizer, add it.
+	// Requeue immediately after adding finalizer to avoid the race condition between init and delete
+	if !ctrlutil.ContainsFinalizer(vsphereCluster, infrav1.ClusterFinalizer) {
+		ctrlutil.AddFinalizer(vsphereCluster, infrav1.ClusterFinalizer)
+		return reconcile.Result{}, nil
+	}
+
 	// Handle non-deleted clusters
 	return r.reconcileNormal(clusterContext)
 }
@@ -218,9 +225,6 @@ func (r clusterReconciler) reconcileDelete(ctx *context.ClusterContext) (reconci
 func (r clusterReconciler) reconcileNormal(ctx *context.ClusterContext) (reconcile.Result, error) {
 	ctx.Logger.Info("Reconciling VSphereCluster")
 
-	// If the VSphereCluster doesn't have our finalizer, add it.
-	ctrlutil.AddFinalizer(ctx.VSphereCluster, infrav1.ClusterFinalizer)
-
 	ok, err := r.reconcileDeploymentZones(ctx)
 	if err != nil {
 		return reconcile.Result{}, err

diff --git a/controllers/vspheredeploymentzone_controller.go b/controllers/vspheredeploymentzone_controller.go
@@ -147,12 +147,17 @@ func (r vsphereDeploymentZoneReconciler) Reconcile(ctx goctx.Context, request re
 		return ctrl.Result{}, r.reconcileDelete(vsphereDeploymentZoneContext)
 	}
 
+	// If the VSphereDeploymentZone doesn't have our finalizer, add it.
+	// Requeue immediately after adding finalizer to avoid the race condition between init and delete
+	if !ctrlutil.ContainsFinalizer(vsphereDeploymentZone, infrav1.DeploymentZoneFinalizer) {
+		ctrlutil.AddFinalizer(vsphereDeploymentZoneContext.VSphereDeploymentZone, infrav1.DeploymentZoneFinalizer)
+		return ctrl.Result{}, nil
+	}
+
 	return ctrl.Result{}, r.reconcileNormal(vsphereDeploymentZoneContext)
 }
 
 func (r vsphereDeploymentZoneReconciler) reconcileNormal(ctx *context.VSphereDeploymentZoneContext) error {
-	ctrlutil.AddFinalizer(ctx.VSphereDeploymentZone, infrav1.DeploymentZoneFinalizer)
-
 	authSession, err := r.getVCenterSession(ctx)
 	if err != nil {
 		ctx.Logger.V(4).Error(err, "unable to create session")

diff --git a/controllers/vspheremachine_controller.go b/controllers/vspheremachine_controller.go
@@ -255,6 +255,12 @@ func (r *machineReconciler) Reconcile(_ goctx.Context, req ctrl.Request) (_ ctrl
 		return reconcile.Result{}, nil
 	}
 
+	// If the VSphereMachine doesn't have our finalizer, add it.
+	// Requeue immediately after adding finalizer to avoid the race condition between init and delete
+	if !ctrlutil.ContainsFinalizer(machineContext.GetVSphereMachine(), infrav1.MachineFinalizer) {
+		ctrlutil.AddFinalizer(machineContext.GetVSphereMachine(), infrav1.MachineFinalizer)
+		return reconcile.Result{}, nil
+	}
 	// Handle non-deleted machines
 	return r.reconcileNormal(machineContext)
 }
@@ -289,9 +295,6 @@ func (r *machineReconciler) reconcileNormal(ctx context.MachineContext) (reconci
 		return reconcile.Result{}, nil
 	}
 
-	// If the VSphereMachine doesn't have our finalizer, add it.
-	ctrlutil.AddFinalizer(ctx.GetVSphereMachine(), infrav1.MachineFinalizer)
-
 	//nolint:gocritic
 	if r.supervisorBased {
 		err := r.setVMModifiers(ctx)

diff --git a/controllers/vspherevm_controller.go b/controllers/vspherevm_controller.go
@@ -279,6 +279,13 @@ func (r vmReconciler) Reconcile(ctx goctx.Context, req ctrl.Request) (_ ctrl.Res
 		}
 	}
 
+	// If the VSphereVM doesn't have our finalizer, add it.
+	// Requeue immediately to avoid the race condition between init and delete
+	if !ctrlutil.ContainsFinalizer(vsphereVM, infrav1.VMFinalizer) {
+		ctrlutil.AddFinalizer(vsphereVM, infrav1.VMFinalizer)
+		return reconcile.Result{}, nil
+	}
+
 	return r.reconcile(vmContext, fetchClusterModuleInput{
 		VSphereCluster: vsphereCluster,
 		Machine:        machine,
@@ -371,8 +378,6 @@ func (r vmReconciler) reconcileNormal(ctx *context.VMContext) (reconcile.Result,
 		r.Logger.Info("VM is failed, won't reconcile", "namespace", ctx.VSphereVM.Namespace, "name", ctx.VSphereVM.Name)
 		return reconcile.Result{}, nil
 	}
-	// If the VSphereVM doesn't have our finalizer, add it.
-	ctrlutil.AddFinalizer(ctx.VSphereVM, infrav1.VMFinalizer)
 
 	if r.isWaitingForStaticIPAllocation(ctx) {
 		conditions.MarkFalse(ctx.VSphereVM, infrav1.VMProvisionedCondition, infrav1.WaitingForStaticIPAllocationReason, clusterv1.ConditionSeverityInfo, "")