Do not retry updates on conflict (#217)

* Do not retry on conflict
When the reconciliation loops tries to update a CR, and the result is a Conflict, the reconciliation loop should not retry because this error will never succeed. The reconciliation loop needs to be restarted.

Additionally, there was a situation where the resource would be deleted, but the Updater would still try to update the deleted resource, resulting in errors.

Author: ludydoo

pkg/reconciler/internal/updater/updater.go

@@ -21,8 +21,10 @@ import (
 
 	"helm.sh/helm/v3/pkg/release"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/util/retry"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -37,6 +39,7 @@ func New(client client.Client) Updater {
 }
 
 type Updater struct {
+	isCanceled        bool
 	client            client.Client
 	updateFuncs       []UpdateFunc
 	updateStatusFuncs []UpdateStatusFunc
@@ -53,13 +56,38 @@ func (u *Updater) UpdateStatus(fs ...UpdateStatusFunc) {
 	u.updateStatusFuncs = append(u.updateStatusFuncs, fs...)
 }
 
+func (u *Updater) CancelUpdates() {
+	u.isCanceled = true
+}
+
+func isRetryableUpdateError(err error) bool {
+	return !errors.IsConflict(err) && !errors.IsNotFound(err)
+}
+
+// retryOnRetryableUpdateError retries the given function until it succeeds,
+// until the given backoff is exhausted, or until the error is not retryable.
+//
+// In case of a Conflict error, the update cannot be retried because the underlying
+// resource has been modified in the meantime, and the reconciliation loop needs
+// to be restarted anew.
+//
+// A NotFound error means that the object has been deleted, and the reconciliation loop
+// needs to be restarted anew as well.
+func retryOnRetryableUpdateError(backoff wait.Backoff, f func() error) error {
+	return retry.OnError(backoff, isRetryableUpdateError, f)
+}
+
 func (u *Updater) Apply(ctx context.Context, obj *unstructured.Unstructured) error {
+	if u.isCanceled {
+		return nil
+	}
+
 	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.
+	if err := retryOnRetryableUpdateError(backoff, func() error {
 		st := statusFor(obj)
 		needsStatusUpdate := false
 		for _, f := range u.updateStatusFuncs {
@@ -78,7 +106,7 @@ func (u *Updater) Apply(ctx context.Context, obj *unstructured.Unstructured) err
 		return err
 	}
 
-	if err := retry.RetryOnConflict(backoff, func() error {
+	if err := retryOnRetryableUpdateError(backoff, func() error {
 		needsUpdate := false
 		for _, f := range u.updateFuncs {
 			needsUpdate = f(obj) || needsUpdate

pkg/reconciler/reconciler.go

@@ -567,8 +567,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
+		}
+		u.CancelUpdates()
+		return ctrl.Result{}, nil
 	}
 
 	vals, err := r.getValues(ctx, obj)
@@ -660,28 +663,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