Do not retry on conflict

pkg/reconciler/internal/updater/updater.go

@@ -18,6 +18,7 @@ package updater
 
 import (
 	"context"
+	"k8s.io/apimachinery/pkg/api/errors"
 
 	"helm.sh/helm/v3/pkg/release"
 	corev1 "k8s.io/api/core/v1"
@@ -53,13 +54,20 @@ func (u *Updater) UpdateStatus(fs ...UpdateStatusFunc) {
 	u.updateStatusFuncs = append(u.updateStatusFuncs, fs...)
 }
 
+func isRetryableUpdateError(err error) bool {
+	return !errors.IsConflict(err) && !errors.IsNotFound(err)
+}
+
 func (u *Updater) Apply(ctx context.Context, obj *unstructured.Unstructured) error {
 	backoff := retry.DefaultRetry
 
 	// Always update the status first. During uninstall, if
 	// we remove the finalizer, updating the status will fail
-	// because the object and its status will be garbage-collected
-	if err := retry.RetryOnConflict(backoff, func() error {
+	// because the object and its status will be garbage-collected.
+	//
+	// In case of a Conflict error, the update cannot be retried,
+	// and the reconciliation loop needs to be restarted anew.
+	if err := retry.OnError(backoff, isRetryableUpdateError, func() error {
 		st := statusFor(obj)
 		needsStatusUpdate := false
 		for _, f := range u.updateStatusFuncs {
@@ -78,7 +86,7 @@ func (u *Updater) Apply(ctx context.Context, obj *unstructured.Unstructured) err
 		return err
 	}
 
-	if err := retry.RetryOnConflict(backoff, func() error {
+	if err := retry.OnError(backoff, isRetryableUpdateError, func() error {
 		needsUpdate := false
 		for _, f := range u.updateFuncs {
 			needsUpdate = f(obj) || needsUpdate

pkg/reconciler/reconciler.go

@@ -521,8 +521,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.
 		return ctrl.Result{}, err
 	}
 
+	shouldUpdate := true
 	u := updater.New(r.client)
 	defer func() {
+		if !shouldUpdate {
+			return
+		}
 		applyErr := u.Apply(ctx, obj)
 		if err == nil && !apierrors.IsNotFound(applyErr) {
 			err = applyErr
@@ -567,8 +571,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.
 	u.UpdateStatus(updater.EnsureCondition(conditions.Initialized(corev1.ConditionTrue, "", "")))
 
 	if obj.GetDeletionTimestamp() != nil {
-		err := r.handleDeletion(ctx, actionClient, obj, log)
-		return ctrl.Result{}, err
+		if err := r.handleDeletion(ctx, actionClient, obj, log); err != nil {
+			return ctrl.Result{}, err
+		}
+		shouldUpdate = false
+		return ctrl.Result{}, nil
 	}
 
 	vals, err := r.getValues(ctx, obj)
@@ -660,28 +667,27 @@ const (
 )
 
 func (r *Reconciler) handleDeletion(ctx context.Context, actionClient helmclient.ActionInterface, obj *unstructured.Unstructured, log logr.Logger) error {
-	if !controllerutil.ContainsFinalizer(obj, uninstallFinalizer) {
-		log.Info("Resource is terminated, skipping reconciliation")
-		return nil
-	}
-
-	// Use defer in a closure so that it executes before we wait for
-	// the deletion of the CR. This might seem unnecessary since we're
-	// applying changes to the CR after is has a deletion timestamp.
-	// However, if uninstall fails, the finalizer will not be removed
-	// and we need to be able to update the conditions on the CR to
-	// indicate that the uninstall failed.
-	if err := func() (err error) {
-		uninstallUpdater := updater.New(r.client)
-		defer func() {
-			applyErr := uninstallUpdater.Apply(ctx, obj)
-			if err == nil {
-				err = applyErr
-			}
-		}()
-		return r.doUninstall(actionClient, &uninstallUpdater, obj, log)
-	}(); err != nil {
-		return err
+	if controllerutil.ContainsFinalizer(obj, uninstallFinalizer) {
+		// Use defer in a closure so that it executes before we wait for
+		// the deletion of the CR. This might seem unnecessary since we're
+		// applying changes to the CR after is has a deletion timestamp.
+		// However, if uninstall fails, the finalizer will not be removed
+		// and we need to be able to update the conditions on the CR to
+		// indicate that the uninstall failed.
+		if err := func() (err error) {
+			uninstallUpdater := updater.New(r.client)
+			defer func() {
+				applyErr := uninstallUpdater.Apply(ctx, obj)
+				if err == nil {
+					err = applyErr
+				}
+			}()
+			return r.doUninstall(actionClient, &uninstallUpdater, obj, log)
+		}(); err != nil {
+			return err
+		}
+	} else {
+		log.Info("Resource is already terminated, skipping deletion")
 	}
 
 	// Since the client is hitting a cache, waiting for the