diff --git a/pkg/reconciler/internal/updater/updater.go b/pkg/reconciler/internal/updater/updater.go index f55c79a5..482e3fb4 100644 --- a/pkg/reconciler/internal/updater/updater.go +++ b/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 diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 1debb28d..41a7d5d8 100644 --- a/pkg/reconciler/reconciler.go +++ b/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