reconcile: stall without rollback target

This ensures that if there is no target to roll back to due to all of them being in a failed state, the controller stalls instead of ending up in a loop of upgrade attempts. Signed-off-by: Hidde Beydals <[email protected]>
fluxcd · Dec 1, 2023 · c89e13d · c89e13d
1 parent bda0b3a
commit c89e13d
Show file tree

Hide file tree

Showing 6 changed files with 50 additions and 10 deletions.
diff --git 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
@@ -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
 			}
@@ -413,6 +421,10 @@ func (r *AtomicRelease) actionForState(ctx context.Context, req *Request, state
 			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 errors.Is(err, action.ErrReleaseNotFound) {
+						return nil, fmt.Errorf("%w: cannot remediate failed release", ErrMissingRollbackTarget)
+					}
+
 					// If the rollback target is not found or is in any other
 					// way corrupt, the most correct remediation is to
 					// reattempt the upgrade.

diff --git 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
@@ -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
@@ -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
@@ -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])),