Backport finalizer fixes to release-v1.40

pkg/controller/apiserver/apiserver_controller.go

@@ -226,7 +226,11 @@ func (r *ReconcileAPIServer) Reconcile(ctx context.Context, request reconcile.Re
 		if errors.IsNotFound(err) {
 			reqLogger.Info("APIServer config not found")
 			r.status.OnCRNotFound()
-			return reconcile.Result{}, r.maintainFinalizer(ctx, nil)
+			f, err := r.maintainFinalizer(ctx, nil)
+			if f {
+				return reconcile.Result{RequeueAfter: utils.FinalizerRemovalRetry}, nil
+			}
+			return reconcile.Result{}, err
 		}
 		r.status.SetDegraded(operatorv1.ResourceReadError, fmt.Sprintf("An error occurred when querying the APIServer resource: %s", msg), err, reqLogger)
 		return reconcile.Result{}, err
@@ -431,7 +435,7 @@ func (r *ReconcileAPIServer) Reconcile(ctx context.Context, request reconcile.Re
 	}
 
 	// API server exists and configuration is valid - maintain a Finalizer on the installation.
-	if err := r.maintainFinalizer(ctx, instance); err != nil {
+	if _, err := r.maintainFinalizer(ctx, instance); err != nil {
 		r.status.SetDegraded(operatorv1.ResourceReadError, "Error setting finalizer on Installation", err, reqLogger)
 		return reconcile.Result{}, err
 	}
@@ -536,7 +540,8 @@ func validateAPIServerResource(instance *operatorv1.APIServer) error {
 // We add a finalizer to the Installation when the API server has been installed, and only remove that finalizer when
 // the API server has been deleted and its pods have stopped running. This allows for a graceful cleanup of API server resources
 // prior to the CNI plugin being removed.
-func (r *ReconcileAPIServer) maintainFinalizer(ctx context.Context, apiserver client.Object) error {
+// The bool return value indicates if the finalizer is set.
+func (r *ReconcileAPIServer) maintainFinalizer(ctx context.Context, apiserver client.Object) (bool, error) {
 	// These objects require graceful termination before the CNI plugin is torn down.
 	apiServerDeployment := v1.Deployment{ObjectMeta: metav1.ObjectMeta{Name: "calico-apiserver", Namespace: render.APIServerNamespace}}
 	return utils.MaintainInstallationFinalizer(ctx, r.client, apiserver, render.APIServerFinalizer, &apiServerDeployment)

pkg/controller/clusterconnection/clusterconnection_controller.go

@@ -218,7 +218,11 @@ func (r *ReconcileConnection) Reconcile(ctx context.Context, request reconcile.R
 		return result, err
 	} else if managementClusterConnection == nil {
 		r.status.OnCRNotFound()
-		return result, r.maintainFinalizer(ctx, nil)
+		f, err := r.maintainFinalizer(ctx, nil)
+		if f {
+			return reconcile.Result{RequeueAfter: utils.FinalizerRemovalRetry}, nil
+		}
+		return result, err
 	}
 	r.status.OnCRFound()
 	// SetMetaData in the TigeraStatus such as observedGenerations.
@@ -270,7 +274,7 @@ func (r *ReconcileConnection) Reconcile(ctx context.Context, request reconcile.R
 		r.status.SetDegraded(operatorv1.ResourceUpdateError, err.Error(), err, reqLogger)
 	}
 
-	if err = r.maintainFinalizer(ctx, managementClusterConnection); err != nil {
+	if _, err = r.maintainFinalizer(ctx, managementClusterConnection); err != nil {
 		r.status.SetDegraded(operatorv1.ResourceReadError, "Error setting finalizer on Installation", err, reqLogger)
 		return reconcile.Result{}, err
 	}
@@ -501,7 +505,8 @@ func (r *ReconcileConnection) Reconcile(ctx context.Context, request reconcile.R
 	return result, nil
 }
 
-func (r *ReconcileConnection) maintainFinalizer(ctx context.Context, managementClusterConnection client.Object) error {
+// The bool return value indicates if the finalizer is set.
+func (r *ReconcileConnection) maintainFinalizer(ctx context.Context, managementClusterConnection client.Object) (bool, error) {
 	// These objects require graceful termination before the CNI plugin is torn down.
 	guardianDeployment := v1.Deployment{ObjectMeta: metav1.ObjectMeta{Name: render.GuardianDeploymentName, Namespace: render.GuardianNamespace}}
 	return utils.MaintainInstallationFinalizer(ctx, r.cli, managementClusterConnection, render.GuardianFinalizer, &guardianDeployment)

pkg/controller/gatewayapi/gatewayapi_controller.go

@@ -165,7 +165,11 @@ func (r *ReconcileGatewayAPI) Reconcile(ctx context.Context, request reconcile.R
 		if errors.IsNotFound(err) {
 			reqLogger.V(2).Info("GatewayAPI object not found")
 			r.status.OnCRNotFound()
-			return reconcile.Result{}, r.maintainFinalizer(ctx, nil)
+			f, err := r.maintainFinalizer(ctx, nil)
+			if f {
+				return reconcile.Result{RequeueAfter: utils.FinalizerRemovalRetry}, nil
+			}
+			return reconcile.Result{}, err
 		}
 		r.status.SetDegraded(operatorv1.ResourceReadError, "Error querying for GatewayAPI CR: "+msg, err, reqLogger)
 		return reconcile.Result{}, err
@@ -405,7 +409,7 @@ func (r *ReconcileGatewayAPI) Reconcile(ctx context.Context, request reconcile.R
 		return reconcile.Result{}, err
 	}
 
-	if err := r.maintainFinalizer(ctx, gatewayAPI); err != nil {
+	if _, err := r.maintainFinalizer(ctx, gatewayAPI); err != nil {
 		r.status.SetDegraded(operatorv1.ResourceReadError, "Error setting finalizer on Installation", err, log)
 		return reconcile.Result{}, err
 	}
@@ -476,7 +480,8 @@ func (r *ReconcileGatewayAPI) patchFelixConfiguration(ctx context.Context) error
 // We add a finalizer to the Installation when the API server has been installed, and only remove that finalizer when
 // the API server has been deleted and its pods have stopped running. This allows for a graceful cleanup of API server resources
 // prior to the CNI plugin being removed.
-func (r *ReconcileGatewayAPI) maintainFinalizer(ctx context.Context, gatewayAPI client.Object) error {
+// The bool return value indicates if the finalizer is set.
+func (r *ReconcileGatewayAPI) maintainFinalizer(ctx context.Context, gatewayAPI client.Object) (bool, error) {
 	// These objects require graceful termination before the CNI plugin is torn down.
 	gatewayAPIDeployment := v1.Deployment{ObjectMeta: metav1.ObjectMeta{Name: "envoy-gateway", Namespace: "tigera-gateway"}}
 	return utils.MaintainInstallationFinalizer(ctx, r.client, gatewayAPI, render.GatewayAPIFinalizer, &gatewayAPIDeployment)

pkg/controller/goldmane/controller.go

@@ -156,7 +156,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 		return reconcile.Result{}, err
 	} else if goldmaneCR == nil {
 		r.status.OnCRNotFound()
-		return reconcile.Result{}, r.maintainFinalizer(ctx, nil)
+		f, err := r.maintainFinalizer(ctx, nil)
+		if f {
+			return reconcile.Result{RequeueAfter: utils.FinalizerRemovalRetry}, nil
+		}
+		return reconcile.Result{}, err
 	}
 	r.status.OnCRFound()
 	// SetMetaData in the TigeraStatus such as observedGenerations.
@@ -218,7 +222,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 		TrustedBundle: trustedBundle,
 	})
 
-	if err := r.maintainFinalizer(ctx, goldmaneCR); err != nil {
+	if _, err := r.maintainFinalizer(ctx, goldmaneCR); err != nil {
 		r.status.SetDegraded(operatorv1.ResourceReadError, "Error setting finalizer on Installation", err, reqLogger)
 		return reconcile.Result{}, err
 	}
@@ -254,7 +258,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 	return reconcile.Result{}, nil
 }
 
-func (r *Reconciler) maintainFinalizer(ctx context.Context, goldmaneCr client.Object) error {
+// The bool return value indicates if the finalizer is set.
+func (r *Reconciler) maintainFinalizer(ctx context.Context, goldmaneCr client.Object) (bool, error) {
 	// These objects require graceful termination before the CNI plugin is torn down.
 	goldmaneDeployment := &v1.Deployment{ObjectMeta: metav1.ObjectMeta{Namespace: common.CalicoNamespace, Name: goldmane.GoldmaneDeploymentName}}
 	return utils.MaintainInstallationFinalizer(ctx, r.cli, goldmaneCr, render.GoldmaneFinalizer, goldmaneDeployment)

pkg/controller/utils/utils.go

@@ -80,6 +80,10 @@ var (
 	// circumstances. Use this as a default when retries are needed.
 	StandardRetry = 30 * time.Second
 
+	// FinalizerRemovalRetry is the amount of time to wait before retrying when
+	// waiting for a finalizer to be removed.
+	FinalizerRemovalRetry = 10 * time.Second
+
 	// AllowedSysctlKeys controls the allowed Sysctl keys can be set in Tuning plugin
 	AllowedSysctlKeys = map[string]bool{
 		"net.ipv4.tcp_keepalive_intvl":  true,
@@ -1034,66 +1038,72 @@ func RemoveInstallationFinalizer(i *operatorv1.Installation, finalizer string) {
 // We add a finalizer to the Installation when the mainResource has been installed, and only remove that finalizer when
 // the resource has been deleted and its secondary resources have stopped running. This allows for a graceful cleanup of any resources
 // prior to the CNI plugin being removed.
+// The bool return value indicates if the finalizer is set.
 func MaintainInstallationFinalizer(
 	ctx context.Context,
 	c client.Client,
 	mainResource client.Object,
 	finalizer string,
 	secondaryResources ...client.Object,
-) error {
+) (bool, error) {
+	finalizerSet := false
+
 	// Get the Installation.
 	installation := &operatorv1.Installation{}
 	if err := c.Get(ctx, DefaultInstanceKey, installation); err != nil {
 		if errors.IsNotFound(err) {
 			log.V(1).Info("Installation config not found")
-			return nil
+			return finalizerSet, nil
 		}
 		log.Error(err, "An error occurred when querying the Installation resource")
-		return err
+		return finalizerSet, err
 	}
-	patchFrom := client.MergeFrom(installation.DeepCopy())
+	patchFrom := client.MergeFromWithOptions(installation.DeepCopy(), client.MergeFromWithOptimisticLock{})
 
 	// Determine the correct finalizers to apply to the Installation.
 	if mainResource != nil {
 		// Add a finalizer indicating that the mainResource is still available.
 		SetInstallationFinalizer(installation, finalizer)
+		finalizerSet = true
 	} else {
 		// Remove the finalizer. We can skip this check if the finalizer is already not present.
 		if !stringsutil.StringInSlice(finalizer, installation.GetFinalizers()) {
 			log.V(2).Info("Finalizer not present, skipping removal", "finalizer", finalizer)
-			return nil
+			return finalizerSet, nil
 		}
+		finalizerSet = true
 
 		// Check if the namespaced secondaryResources are still present.
 		// Keep track of all the secondary resources that the main resource creates.
 		// Only delete the finalizer if all of the secondary resources are deleted and there are no lingering Pods.
 		for _, secondaryResource := range secondaryResources {
 			err := c.Get(ctx, types.NamespacedName{Namespace: secondaryResource.GetNamespace(), Name: secondaryResource.GetName()}, secondaryResource)
 			if err != nil && !errors.IsNotFound(err) {
-				return err
+				return finalizerSet, err
 			} else if errors.IsNotFound(err) {
 				log.Info("Object no longer exists.", "object", secondaryResource)
 			} else {
 				log.Info("Object is still present, waiting for termination", "object", secondaryResource)
-				return nil
+				return finalizerSet, nil
 			}
 
 			// If the secondary resource itself is gone, ensure there are no Pods left over from this resource.
 			terminated, err := AllPodsTerminated(ctx, c, secondaryResource)
 			if err != nil {
-				return err
+				return finalizerSet, err
 			}
 			if !terminated {
 				log.Info("Pods for object are still present, waiting for termination", "object", secondaryResource)
-				return nil
+				return finalizerSet, nil
 			}
 		}
 		log.Info("All objects no longer exist. Removing finalizer", "finalizer", finalizer)
 		RemoveInstallationFinalizer(installation, finalizer)
+		finalizerSet = false
 	}
 
 	// Update the installation with any finalizer changes.
-	return c.Patch(ctx, installation, patchFrom)
+	return finalizerSet, c.Patch(ctx, installation, patchFrom)
 }
 
 func AllPodsTerminated(ctx context.Context, c client.Client, obj client.Object) (bool, error) {

pkg/controller/whisker/controller.go

@@ -159,7 +159,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 		return reconcile.Result{}, err
 	} else if whiskerCR == nil {
 		r.status.OnCRNotFound()
-		return reconcile.Result{}, r.maintainFinalizer(ctx, nil)
+		f, err := r.maintainFinalizer(ctx, nil)
+		if f {
+			return reconcile.Result{RequeueAfter: utils.FinalizerRemovalRetry}, nil
+		}
+		return reconcile.Result{}, err
 	}
 	r.status.OnCRFound()
 	// SetMetaData in the TigeraStatus such as observedGenerations.
@@ -223,7 +227,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 		return reconcile.Result{}, err
 	}
 
-	if err := r.maintainFinalizer(ctx, whiskerCR); err != nil {
+	if _, err := r.maintainFinalizer(ctx, whiskerCR); err != nil {
 		r.status.SetDegraded(operatorv1.ResourceReadError, "Error setting finalizer on Installation", err, reqLogger)
 		return reconcile.Result{}, err
 	}
@@ -284,7 +288,8 @@ func updateWhiskerWithDefaults(instance *operatorv1.Whisker) {
 	}
 }
 
-func (r *Reconciler) maintainFinalizer(ctx context.Context, whiskerCr client.Object) error {
+// The bool return value indicates if the finalizer is set.
+func (r *Reconciler) maintainFinalizer(ctx context.Context, whiskerCr client.Object) (bool, error) {
 	// These objects require graceful termination before the CNI plugin is torn down.
 	whiskerDeployment := &v1.Deployment{ObjectMeta: metav1.ObjectMeta{Namespace: common.CalicoNamespace, Name: whisker.WhiskerDeploymentName}}
 	return utils.MaintainInstallationFinalizer(ctx, r.cli, whiskerCr, render.WhiskerFinalizer, whiskerDeployment)

test/whisker_test.go

@@ -140,9 +140,16 @@ var _ = Describe("Tests for Whisker installation", func() {
 
 		By("Verifying that the whisker and goldmane finalizer is created in the installation CR")
 		install := &operator.Installation{ObjectMeta: metav1.ObjectMeta{Name: "default"}}
-		Expect(GetResource(c, install)).To(BeNil())
-		Expect(install.ObjectMeta.Finalizers).To(ContainElement(render.WhiskerFinalizer))
-		Expect(install.ObjectMeta.Finalizers).To(ContainElement(render.GoldmaneFinalizer))
+		Eventually(func() error {
+			Expect(GetResource(c, install)).To(BeNil())
+			if !slices.Contains(install.ObjectMeta.Finalizers, render.WhiskerFinalizer) {
+				return fmt.Errorf("expected whisker finalizer to be present, but found: %v", install.ObjectMeta.Finalizers)
+			}
+			if !slices.Contains(install.ObjectMeta.Finalizers, render.GoldmaneFinalizer) {
+				return fmt.Errorf("expected goldmane finalizer to be present, but found: %v", install.ObjectMeta.Finalizers)
+			}
+			return nil
+		}, 1*time.Minute, 1*time.Second).Should(BeNil())
 
 		By("Verifying that the whisker finalizer is removed in the installation CR")
 		Expect(c.Delete(context.Background(), whiskerCR)).To(BeNil())