From 238b6743ba01af5e58dcb8e3c78f4f99d007028d Mon Sep 17 00:00:00 2001 From: Ludovic Cleroux Date: Fri, 23 Jun 2023 15:55:54 +0200 Subject: [PATCH] Do not retry on conflict --- pkg/reconciler/internal/updater/updater.go | 14 +++++++++++--- pkg/reconciler/reconciler_test.go | 12 ++++++++++++ 2 files changed, 23 insertions(+), 3 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_test.go b/pkg/reconciler/reconciler_test.go index c02899de..6c67233a 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -690,6 +690,12 @@ var _ = Describe("Reconciler", func() { Expect(mgr.GetClient().Delete(ctx, obj)).To(Succeed()) }) + By("expecting a conflict", func() { + res, err := r.Reconcile(ctx, req) + Expect(res).To(Equal(reconcile.Result{})) + Expect(apierrors.IsConflict(err)).To(BeTrue()) + }) + By("successfully reconciling a request", func() { res, err := r.Reconcile(ctx, req) Expect(res).To(Equal(reconcile.Result{})) @@ -1296,6 +1302,12 @@ var _ = Describe("Reconciler", func() { Expect(mgr.GetClient().Delete(ctx, obj)).To(Succeed()) }) + By("expecting a reconciliation conflict", func() { + result, err := r.Reconcile(ctx, req) + Expect(result).To(Equal(reconcile.Result{})) + Expect(apierrors.IsConflict(err)).To(BeTrue()) + }) + By("successfully reconciling a request", func() { res, err := r.Reconcile(ctx, req) Expect(res).To(Equal(reconcile.Result{}))