From d6ae7c5f8a0c90c30340996c26b3f0187c719eda Mon Sep 17 00:00:00 2001 From: Ludovic Cleroux Date: Fri, 23 Jun 2023 15:55:54 +0200 Subject: [PATCH 1/4] Do not retry on conflict --- pkg/reconciler/internal/updater/updater.go | 14 ++++-- pkg/reconciler/reconciler.go | 54 ++++++++++++---------- 2 files changed, 41 insertions(+), 27 deletions(-) diff --git a/pkg/reconciler/internal/updater/updater.go b/pkg/reconciler/internal/updater/updater.go index f55c79a5..c788cd73 100644 --- a/pkg/reconciler/internal/updater/updater.go +++ b/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 diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 1debb28d..3f7f6f0a 100644 --- a/pkg/reconciler/reconciler.go +++ b/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 From e5bf9c2ab67bc006c95d9dfab0007ef5d81e55bd Mon Sep 17 00:00:00 2001 From: Ludovic Cleroux Date: Thu, 13 Jul 2023 12:28:36 +0200 Subject: [PATCH 2/4] PR Comments --- pkg/reconciler/internal/updater/updater.go | 9 +++++++++ pkg/reconciler/reconciler.go | 8 ++------ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/pkg/reconciler/internal/updater/updater.go b/pkg/reconciler/internal/updater/updater.go index c788cd73..fa9ec462 100644 --- a/pkg/reconciler/internal/updater/updater.go +++ b/pkg/reconciler/internal/updater/updater.go @@ -38,6 +38,7 @@ func New(client client.Client) Updater { } type Updater struct { + isCanceled bool client client.Client updateFuncs []UpdateFunc updateStatusFuncs []UpdateStatusFunc @@ -54,11 +55,19 @@ 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) } 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 diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 3f7f6f0a..41a7d5d8 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -521,12 +521,8 @@ 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 @@ -574,7 +570,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. if err := r.handleDeletion(ctx, actionClient, obj, log); err != nil { return ctrl.Result{}, err } - shouldUpdate = false + u.CancelUpdates() return ctrl.Result{}, nil } @@ -687,7 +683,7 @@ func (r *Reconciler) handleDeletion(ctx context.Context, actionClient helmclient return err } } else { - log.Info("Resource is already terminated, skipping deletion") + log.Info("Resource is already terminated, skipping deletion.") } // Since the client is hitting a cache, waiting for the From 380c00f18db2b1080c85d194d24239e673b8771a Mon Sep 17 00:00:00 2001 From: Ludovic Cleroux Date: Thu, 13 Jul 2023 12:33:22 +0200 Subject: [PATCH 3/4] PR Comments --- pkg/reconciler/internal/updater/updater.go | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/pkg/reconciler/internal/updater/updater.go b/pkg/reconciler/internal/updater/updater.go index fa9ec462..7caf4520 100644 --- a/pkg/reconciler/internal/updater/updater.go +++ b/pkg/reconciler/internal/updater/updater.go @@ -19,6 +19,7 @@ package updater import ( "context" "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/util/wait" "helm.sh/helm/v3/pkg/release" corev1 "k8s.io/api/core/v1" @@ -63,6 +64,19 @@ 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 @@ -73,10 +87,7 @@ func (u *Updater) Apply(ctx context.Context, obj *unstructured.Unstructured) err // 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. - // - // 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 { + if err := retryOnRetryableUpdateError(backoff, func() error { st := statusFor(obj) needsStatusUpdate := false for _, f := range u.updateStatusFuncs { @@ -95,7 +106,7 @@ func (u *Updater) Apply(ctx context.Context, obj *unstructured.Unstructured) err return err } - if err := retry.OnError(backoff, isRetryableUpdateError, func() error { + if err := retryOnRetryableUpdateError(backoff, func() error { needsUpdate := false for _, f := range u.updateFuncs { needsUpdate = f(obj) || needsUpdate From d771a95030597608c6a0d902bbeb5086b2276c54 Mon Sep 17 00:00:00 2001 From: Ludovic Cleroux Date: Thu, 13 Jul 2023 12:33:51 +0200 Subject: [PATCH 4/4] PR Comments --- pkg/reconciler/internal/updater/updater.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/reconciler/internal/updater/updater.go b/pkg/reconciler/internal/updater/updater.go index 7caf4520..482e3fb4 100644 --- a/pkg/reconciler/internal/updater/updater.go +++ b/pkg/reconciler/internal/updater/updater.go @@ -18,13 +18,13 @@ package updater import ( "context" - "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/util/wait" "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"