diff --git a/internal/controller/helmrelease_controller.go b/internal/controller/helmrelease_controller.go index 660cf30ab..fae653565 100644 --- a/internal/controller/helmrelease_controller.go +++ b/internal/controller/helmrelease_controller.go @@ -375,7 +375,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe if errors.Is(err, intreconcile.ErrMustRequeue) { return ctrl.Result{Requeue: true}, nil } - if errors.Is(err, intreconcile.ErrExceededMaxRetries) { + if interrors.IsOneOf(err, intreconcile.ErrExceededMaxRetries, intreconcile.ErrMissingRollbackTarget) { err = reconcile.TerminalError(err) } return ctrl.Result{}, err diff --git a/internal/reconcile/atomic_release.go b/internal/reconcile/atomic_release.go index 61bba38ae..d83dde129 100644 --- a/internal/reconcile/atomic_release.go +++ b/internal/reconcile/atomic_release.go @@ -59,6 +59,9 @@ var ( // to continue the reconciliation process. ErrMustRequeue = errors.New("must requeue") + // ErrMissingRollbackTarget is returned when the rollback target is missing. + ErrMissingRollbackTarget = errors.New("missing target release for rollback") + // ErrUnknownReleaseStatus is returned when the release status is unknown // and cannot be acted upon. ErrUnknownReleaseStatus = errors.New("unknown release status") @@ -189,6 +192,11 @@ func (r *AtomicRelease) Reconcile(ctx context.Context, req *Request) error { if errors.Is(err, ErrExceededMaxRetries) { conditions.MarkStalled(req.Object, "RetriesExceeded", "Failed to %s after %d attempt(s)", req.Object.Status.LastAttemptedReleaseAction, req.Object.GetActiveRemediation().GetFailureCount(req.Object)) + return err + } + if errors.Is(err, ErrMissingRollbackTarget) { + conditions.MarkStalled(req.Object, "MissingRollbackTarget", "Failed to perform remediation: %s", err.Error()) + return err } return err } @@ -412,10 +420,15 @@ func (r *AtomicRelease) actionForState(ctx context.Context, req *Request, state // before instructing to roll back to it. prev := req.Object.Status.History.Previous(remediation.MustIgnoreTestFailures(req.Object.GetTest().IgnoreFailures)) if _, err := action.VerifySnapshot(r.configFactory.Build(nil), prev); err != nil { - if interrors.IsOneOf(err, action.ErrReleaseNotFound, action.ErrReleaseDisappeared, action.ErrReleaseNotObserved, action.ErrReleaseDigest) { - // If the rollback target is not found or is in any other - // way corrupt, the most correct remediation is to - // reattempt the upgrade. + if errors.Is(err, action.ErrReleaseNotFound) { + // If the rollback target is missing, we cannot roll back + // to it and must fail. + return nil, fmt.Errorf("%w: cannot remediate failed release", ErrMissingRollbackTarget) + } + + if interrors.IsOneOf(err, action.ErrReleaseDisappeared, action.ErrReleaseNotObserved, action.ErrReleaseDigest) { + // If the rollback target is in any way corrupt, + // the most correct remediation is to reattempt the upgrade. log.Info(msgWithReason("unable to verify previous release in storage to roll back to", err.Error())) return NewUpgrade(r.configFactory, r.eventRecorder), nil } diff --git a/internal/reconcile/atomic_release_test.go b/internal/reconcile/atomic_release_test.go index d2050e425..4e7fba159 100644 --- a/internal/reconcile/atomic_release_test.go +++ b/internal/reconcile/atomic_release_test.go @@ -1349,6 +1349,36 @@ func TestAtomicRelease_actionForState(t *testing.T) { }, want: &RollbackRemediation{}, }, + { + name: "failed release with active upgrade remediation and no previous release triggers error", + state: ReleaseState{Status: ReleaseStatusFailed}, + releases: []*helmrelease.Release{ + testutil.BuildRelease(&helmrelease.MockReleaseOptions{ + Name: mockReleaseName, + Namespace: mockReleaseNamespace, + Version: 2, + Status: helmrelease.StatusFailed, + Chart: testutil.BuildChart(), + }), + }, + spec: func(spec *v2.HelmReleaseSpec) { + spec.Upgrade = &v2.Upgrade{ + Remediation: &v2.UpgradeRemediation{ + Retries: 2, + }, + } + }, + status: func(releases []*helmrelease.Release) v2.HelmReleaseStatus { + return v2.HelmReleaseStatus{ + History: v2.Snapshots{ + release.ObservedToSnapshot(release.ObserveRelease(releases[0])), + }, + LastAttemptedReleaseAction: v2.ReleaseActionUpgrade, + UpgradeFailures: 1, + } + }, + wantErr: ErrMissingRollbackTarget, + }, { name: "failed release with active upgrade remediation and unverified previous triggers upgrade", state: ReleaseState{Status: ReleaseStatusFailed}, @@ -1357,7 +1387,7 @@ func TestAtomicRelease_actionForState(t *testing.T) { Name: mockReleaseName, Namespace: mockReleaseNamespace, Version: 2, - Status: helmrelease.StatusSuperseded, + Status: helmrelease.StatusFailed, Chart: testutil.BuildChart(), }), }, @@ -1371,6 +1401,7 @@ func TestAtomicRelease_actionForState(t *testing.T) { status: func(releases []*helmrelease.Release) v2.HelmReleaseStatus { return v2.HelmReleaseStatus{ History: v2.Snapshots{ + release.ObservedToSnapshot(release.ObserveRelease(releases[0])), release.ObservedToSnapshot(release.ObserveRelease( testutil.BuildRelease(&helmrelease.MockReleaseOptions{ Name: mockReleaseName, diff --git a/internal/reconcile/release.go b/internal/reconcile/release.go index 5e89aeab2..249aafbaa 100644 --- a/internal/reconcile/release.go +++ b/internal/reconcile/release.go @@ -36,9 +36,6 @@ var ( // ErrNoLatest is returned when the HelmRelease has no latest release // but this is required by the ActionReconciler. ErrNoLatest = errors.New("no latest release") - // ErrNoPrevious is returned when the HelmRelease has no previous release - // but this is required by the ActionReconciler. - ErrNoPrevious = errors.New("no previous release") // ErrReleaseMismatch is returned when the resulting release after running // an action does not match the expected latest and/or previous release. // This can happen for actions where targeting a release by version is not diff --git a/internal/reconcile/rollback_remediation.go b/internal/reconcile/rollback_remediation.go index 755bf7434..e614f5a30 100644 --- a/internal/reconcile/rollback_remediation.go +++ b/internal/reconcile/rollback_remediation.go @@ -49,8 +49,8 @@ import ( // Remediated=False and a warning event is emitted. // // When the Request.Object does not have a (successful) previous deployed -// release, it returns an error of type ErrNoPrevious. In addition, it -// returns ErrReleaseMismatch if the name and/or namespace of the latest and +// release, it returns an error of type ErrMissingRollbackTarget. In addition, +// it returns ErrReleaseMismatch if the name and/or namespace of the latest and // previous release do not match. Any other returned error indicates the caller // should retry as it did not cause a change to the Helm storage. // @@ -85,7 +85,7 @@ func (r *RollbackRemediation) Reconcile(ctx context.Context, req *Request) error // Previous is required to determine what version to roll back to. prev := req.Object.Status.History.Previous(req.Object.GetUpgrade().GetRemediation().MustIgnoreTestFailures(req.Object.GetTest().IgnoreFailures)) if prev == nil { - return fmt.Errorf("%w: required to rollback", ErrNoPrevious) + return fmt.Errorf("%w: required to rollback", ErrMissingRollbackTarget) } // Confirm previous and current point to the same release. diff --git a/internal/reconcile/rollback_remediation_test.go b/internal/reconcile/rollback_remediation_test.go index cf8e23906..2300aefc3 100644 --- a/internal/reconcile/rollback_remediation_test.go +++ b/internal/reconcile/rollback_remediation_test.go @@ -121,7 +121,7 @@ func TestRollbackRemediation_Reconcile(t *testing.T) { }, }, { - name: "rollback without previous", + name: "rollback without previous target release", releases: func(namespace string) []*helmrelease.Release { return []*helmrelease.Release{ testutil.BuildRelease(&helmrelease.MockReleaseOptions{ @@ -147,7 +147,7 @@ func TestRollbackRemediation_Reconcile(t *testing.T) { }, } }, - wantErr: ErrNoPrevious, + wantErr: ErrMissingRollbackTarget, expectHistory: func(releases []*helmrelease.Release) v2.Snapshots { return v2.Snapshots{ release.ObservedToSnapshot(release.ObserveRelease(releases[1])),