Skip to content

Commit

Permalink
Do not retry on conflict
Browse files Browse the repository at this point in the history
  • Loading branch information
ludydoo committed Jun 26, 2023
1 parent c16a400 commit 238b674
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 3 deletions.
14 changes: 11 additions & 3 deletions pkg/reconciler/internal/updater/updater.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand Down
12 changes: 12 additions & 0 deletions pkg/reconciler/reconciler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}))
Expand Down Expand Up @@ -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{}))
Expand Down

0 comments on commit 238b674

Please sign in to comment.