Introduce DefaultToRetryOnFailure feature gate

Signed-off-by: Matheus Pimenta <[email protected]>
(cherry picked from commit b40c798)

Author: matheuscscp

api/v2/helmrelease_types.go

@@ -489,7 +489,7 @@ type Remediation interface {
 // UpgradeStrategy.
 // +kubebuilder:object:generate=false
 type Strategy interface {
-	GetRetry() Retry
+	GetRetry(defaultToRetryOnFailure bool) Retry
 }
 
 // Retry defines a consistent interface for retry strategies from
@@ -511,7 +511,8 @@ type Install struct {
 	Timeout *metav1.Duration `json:"timeout,omitempty"`
 
 	// Strategy defines the install strategy to use for this HelmRelease.
-	// Defaults to 'RemediateOnFailure'.
+	// Defaults to 'RemediateOnFailure', or 'RetryOnFailure' when the
+	// DefaultToRetryOnFailure feature gate is enabled.
 	// +optional
 	Strategy *InstallStrategy `json:"strategy,omitempty"`
 
@@ -615,8 +616,14 @@ func (in Install) GetRemediation() Remediation {
 
 // GetRetry returns the configured retry strategy for the Helm install
 // action.
-func (in Install) GetRetry() Retry {
-	if in.Strategy == nil || in.Strategy.Name != string(ActionStrategyRetryOnFailure) {
+func (in Install) GetRetry(defaultToRetryOnFailure bool) Retry {
+	if in.Strategy == nil {
+		if defaultToRetryOnFailure {
+			return &InstallStrategy{Name: string(ActionStrategyRetryOnFailure)}
+		}
+		return nil
+	}
+	if in.Strategy.Name != string(ActionStrategyRetryOnFailure) {
 		return nil
 	}
 	return in.Strategy
@@ -764,7 +771,8 @@ type Upgrade struct {
 	Timeout *metav1.Duration `json:"timeout,omitempty"`
 
 	// Strategy defines the upgrade strategy to use for this HelmRelease.
-	// Defaults to 'RemediateOnFailure'.
+	// Defaults to 'RemediateOnFailure', or 'RetryOnFailure' when the
+	// DefaultToRetryOnFailure feature gate is enabled.
 	// +optional
 	Strategy *UpgradeStrategy `json:"strategy,omitempty"`
 
@@ -866,8 +874,14 @@ func (in Upgrade) GetRemediation() Remediation {
 
 // GetRetry returns the configured retry strategy for the Helm upgrade
 // action.
-func (in Upgrade) GetRetry() Retry {
-	if in.Strategy == nil || in.Strategy.Name != string(ActionStrategyRetryOnFailure) {
+func (in Upgrade) GetRetry(defaultToRetryOnFailure bool) Retry {
+	if in.Strategy == nil {
+		if defaultToRetryOnFailure {
+			return &UpgradeStrategy{Name: string(ActionStrategyRetryOnFailure)}
+		}
+		return nil
+	}
+	if in.Strategy.Name != string(ActionStrategyRetryOnFailure) {
 		return nil
 	}
 	return in.Strategy
@@ -1437,13 +1451,14 @@ func (in HelmRelease) GetActiveRemediation() Remediation {
 }
 
 // GetActiveRetry returns the active retry configuration for the
-// HelmRelease.
-func (in HelmRelease) GetActiveRetry() Retry {
+// HelmRelease. When defaultToRetryOnFailure is true and no strategy
+// is explicitly configured, it defaults to RetryOnFailure.
+func (in HelmRelease) GetActiveRetry(defaultToRetryOnFailure bool) Retry {
 	switch in.Status.LastAttemptedReleaseAction {
 	case ReleaseActionInstall:
-		return in.GetInstall().GetRetry()
+		return in.GetInstall().GetRetry(defaultToRetryOnFailure)
 	case ReleaseActionUpgrade:
-		return in.GetUpgrade().GetRetry()
+		return in.GetUpgrade().GetRetry(defaultToRetryOnFailure)
 	default:
 		return nil
 	}

config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml

@@ -492,7 +492,8 @@ spec:
                   strategy:
                     description: |-
                       Strategy defines the install strategy to use for this HelmRelease.
-                      Defaults to 'RemediateOnFailure'.
+                      Defaults to 'RemediateOnFailure', or 'RetryOnFailure' when the
+                      DefaultToRetryOnFailure feature gate is enabled.
                     properties:
                       name:
                         description: Name of the install strategy.
@@ -1013,7 +1014,8 @@ spec:
                   strategy:
                     description: |-
                       Strategy defines the upgrade strategy to use for this HelmRelease.
-                      Defaults to 'RemediateOnFailure'.
+                      Defaults to 'RemediateOnFailure', or 'RetryOnFailure' when the
+                      DefaultToRetryOnFailure feature gate is enabled.
                     properties:
                       name:
                         description: Name of the upgrade strategy.

docs/api/v2/helm.md

@@ -2029,7 +2029,8 @@ InstallStrategy
 <td>
 <em>(Optional)</em>
 <p>Strategy defines the install strategy to use for this HelmRelease.
-Defaults to &lsquo;RemediateOnFailure&rsquo;.</p>
+Defaults to &lsquo;RemediateOnFailure&rsquo;, or &lsquo;RetryOnFailure&rsquo; when the
+DefaultToRetryOnFailure feature gate is enabled.</p>
 </td>
 </tr>
 <tr>
@@ -3154,7 +3155,8 @@ UpgradeStrategy
 <td>
 <em>(Optional)</em>
 <p>Strategy defines the upgrade strategy to use for this HelmRelease.
-Defaults to &lsquo;RemediateOnFailure&rsquo;.</p>
+Defaults to &lsquo;RemediateOnFailure&rsquo;, or &lsquo;RetryOnFailure&rsquo; when the
+DefaultToRetryOnFailure feature gate is enabled.</p>
 </td>
 </tr>
 <tr>

docs/spec/v2/helmreleases.md

@@ -576,7 +576,8 @@ The field offers the following subfields:
   `RemediateOnFailure` or `RetryOnFailure`.
   If the `.spec.install.strategy` field is not specified, the HelmRelease
   reconciliation behaves as if `.spec.install.strategy.name` was set to
-  `RemediateOnFailure`.
+  `RemediateOnFailure`, or `RetryOnFailure` when the
+  `DefaultToRetryOnFailure` feature gate is enabled.
 - `.retryInterval` (Optional): The time to wait between retries of failed
   releases when the install strategy is set to `RetryOnFailure`. Defaults
   to `5m`. Cannot be used with `RemediateOnFailure`.
@@ -654,7 +655,9 @@ The field offers the following subfields:
 - `.name` (Required): The name of the upgrade strategy to use. One of
   `RemediateOnFailure` or `RetryOnFailure`. If the `.spec.upgrade.strategy`
   field is not specified, the HelmRelease reconciliation behaves as if
-  `.spec.upgrade.strategy.name` was set to `RemediateOnFailure`.
+  `.spec.upgrade.strategy.name` was set to `RemediateOnFailure`, or
+  `RetryOnFailure` when the `DefaultToRetryOnFailure` feature gate is
+  enabled.
 - `.retryInterval` (Optional): The time to wait between retries of failed
   releases when the upgrade strategy is set to `RetryOnFailure`. Defaults
   to `5m`. Cannot be used with `RemediateOnFailure`.

internal/controller/helmrelease_controller.go

@@ -109,6 +109,7 @@ type HelmReleaseReconciler struct {
 
 	AdditiveCELDependencyCheck bool
 	AllowExternalArtifact      bool
+	DefaultToRetryOnFailure    bool
 	DirectSourceFetch          bool
 	DisableChartDigestTracking bool
 }
@@ -415,7 +416,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context,
 	}
 
 	// Off we go!
-	if err = intreconcile.NewAtomicRelease(patchHelper, cfg, r.EventRecorder, r.FieldManager, r.DisallowedFieldManagers).Reconcile(ctx, &intreconcile.Request{
+	if err = intreconcile.NewAtomicRelease(patchHelper, cfg, r.EventRecorder, r.FieldManager, r.DisallowedFieldManagers, r.DefaultToRetryOnFailure).Reconcile(ctx, &intreconcile.Request{
 		Object: obj,
 		Chart:  loadedChart,
 		Values: values,
@@ -433,7 +434,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context,
 		}
 		switch {
 		case errors.Is(err, intreconcile.ErrRetryAfterInterval):
-			return jitter.JitteredRequeueInterval(ctrl.Result{RequeueAfter: obj.GetActiveRetry().GetRetryInterval()}), nil
+			return jitter.JitteredRequeueInterval(ctrl.Result{RequeueAfter: obj.GetActiveRetry(r.DefaultToRetryOnFailure).GetRetryInterval()}), nil
 		case errors.Is(err, intreconcile.ErrMustRequeue):
 			return ctrl.Result{Requeue: true}, nil
 		case interrors.IsOneOf(err, intreconcile.ErrExceededMaxRetries, intreconcile.ErrMissingRollbackTarget):

internal/features/features.go

@@ -79,6 +79,14 @@ const (
 	// to allow immediate processing of the new reconciliation request. This can
 	// help avoid getting stuck on failing deployments when fixes are available.
 	CancelHealthCheckOnNewRevision = "CancelHealthCheckOnNewRevision"
+
+	// DefaultToRetryOnFailure changes the default install/upgrade strategy
+	// from RemediateOnFailure to RetryOnFailure when the user has not
+	// explicitly configured a strategy. Unlike RemediateOnFailure, which
+	// has a retry budget, RetryOnFailure retries indefinitely and
+	// auto-clears failures on success, providing better UX especially
+	// when CancelHealthCheckOnNewRevision is enabled.
+	DefaultToRetryOnFailure = "DefaultToRetryOnFailure"
 )
 
 var features = map[string]bool{
@@ -121,6 +129,9 @@ var features = map[string]bool{
 	// CancelHealthCheckOnNewRevision
 	// opt-in from v1.5.0
 	CancelHealthCheckOnNewRevision: false,
+	// DefaultToRetryOnFailure
+	// opt-in from v1.5.2
+	DefaultToRetryOnFailure: false,
 }
 
 func init() {

internal/reconcile/atomic_release.go

@@ -117,18 +117,20 @@ type AtomicRelease struct {
 	strategy                releaseStrategy
 	fieldManager            string
 	disallowedFieldManagers []string
+	defaultToRetryOnFailure bool
 }
 
 // NewAtomicRelease returns a new AtomicRelease reconciler configured with the
 // provided values.
-func NewAtomicRelease(patchHelper *patch.SerialPatcher, cfg *action.ConfigFactory, recorder record.EventRecorder, fieldManager string, disallowedFieldManagers []string) *AtomicRelease {
+func NewAtomicRelease(patchHelper *patch.SerialPatcher, cfg *action.ConfigFactory, recorder record.EventRecorder, fieldManager string, disallowedFieldManagers []string, defaultToRetryOnFailure bool) *AtomicRelease {
 	return &AtomicRelease{
 		patchHelper:             patchHelper,
 		eventRecorder:           recorder,
 		configFactory:           cfg,
 		strategy:                &cleanReleaseStrategy{},
 		fieldManager:            fieldManager,
 		disallowedFieldManagers: disallowedFieldManagers,
+		defaultToRetryOnFailure: defaultToRetryOnFailure,
 	}
 }
 
@@ -230,7 +232,7 @@ func (r *AtomicRelease) Reconcile(ctx context.Context, req *Request) error {
 					fmt.Sprintf("instructed to stop before running %s action reconciler %s", next.Type(), next.Name()),
 				)
 
-				if retry := req.Object.GetActiveRetry(); retry != nil {
+				if retry := req.Object.GetActiveRetry(r.defaultToRetryOnFailure); retry != nil {
 					conditions.MarkReconciling(req.Object, meta.ProgressingWithRetryReason, "retrying after %s", retry.GetRetryInterval().String())
 					return ErrRetryAfterInterval
 				}
@@ -270,7 +272,7 @@ func (r *AtomicRelease) Reconcile(ctx context.Context, req *Request) error {
 					fmt.Sprintf("action reconciler %s of type %s returned error: %s", next.Name(), next.Type(), err),
 				)
 
-				if retry := req.Object.GetActiveRetry(); retry != nil {
+				if retry := req.Object.GetActiveRetry(r.defaultToRetryOnFailure); retry != nil {
 					log.Error(err, fmt.Sprintf("failed to run '%s' action", next.Name()))
 					conditions.MarkReconciling(req.Object, meta.ProgressingWithRetryReason, "retrying after %s", retry.GetRetryInterval().String())
 					return ErrRetryAfterInterval
@@ -288,7 +290,7 @@ func (r *AtomicRelease) Reconcile(ctx context.Context, req *Request) error {
 					"instructed to stop after running %s action reconciler %s", next.Type(), next.Name()),
 				)
 
-				if retry := req.Object.GetActiveRetry(); retry != nil {
+				if retry := req.Object.GetActiveRetry(r.defaultToRetryOnFailure); retry != nil {
 					conditions.MarkReconciling(req.Object, meta.ProgressingWithRetryReason, "retrying after %s", retry.GetRetryInterval().String())
 					return ErrRetryAfterInterval
 				}
@@ -331,7 +333,7 @@ func (r *AtomicRelease) actionForState(ctx context.Context, req *Request, state
 	case ReleaseStatusInSync:
 		log.Info("release in-sync with desired state")
 
-		if retry := req.Object.GetActiveRetry(); retry != nil {
+		if retry := req.Object.GetActiveRetry(r.defaultToRetryOnFailure); retry != nil {
 			req.Object.Status.History.TruncateIgnoringPreviousSnapshots()
 		} else {
 			// Remove all history up to the previous release action.
@@ -347,7 +349,7 @@ func (r *AtomicRelease) actionForState(ctx context.Context, req *Request, state
 
 		if forceRequested {
 			log.Info(msgWithReason("forcing upgrade for in-sync release", "force requested through annotation"))
-			return NewUpgrade(r.configFactory, r.eventRecorder), nil
+			return NewUpgrade(r.configFactory, r.eventRecorder, r.defaultToRetryOnFailure), nil
 		}
 
 		// Since the release is in-sync, remove any remediated condition if
@@ -389,33 +391,33 @@ func (r *AtomicRelease) actionForState(ctx context.Context, req *Request, state
 		if req.Object.GetInstall().GetRemediation().RetriesExhausted(req.Object) {
 			if forceRequested {
 				log.Info(msgWithReason("forcing install while out of retries", "force requested through annotation"))
-				return NewInstall(r.configFactory, r.eventRecorder), nil
+				return NewInstall(r.configFactory, r.eventRecorder, r.defaultToRetryOnFailure), nil
 			}
 
 			return nil, fmt.Errorf("%w: cannot install release", ErrExceededMaxRetries)
 		}
 
-		return NewInstall(r.configFactory, r.eventRecorder), nil
+		return NewInstall(r.configFactory, r.eventRecorder, r.defaultToRetryOnFailure), nil
 	case ReleaseStatusUnmanaged:
 		log.Info(msgWithReason("release not managed by controller", state.Reason))
 
 		// Clear the history as we can no longer rely on it.
 		req.Object.Status.ClearHistory()
 
-		return NewUpgrade(r.configFactory, r.eventRecorder), nil
+		return NewUpgrade(r.configFactory, r.eventRecorder, r.defaultToRetryOnFailure), nil
 	case ReleaseStatusOutOfSync:
 		log.Info(msgWithReason("release out-of-sync with desired state", state.Reason))
 
 		if req.Object.GetUpgrade().GetRemediation().RetriesExhausted(req.Object) {
 			if forceRequested {
 				log.Info(msgWithReason("forcing upgrade while out of retries", "force requested through annotation"))
-				return NewUpgrade(r.configFactory, r.eventRecorder), nil
+				return NewUpgrade(r.configFactory, r.eventRecorder, r.defaultToRetryOnFailure), nil
 			}
 
 			return nil, fmt.Errorf("%w: cannot upgrade release", ErrExceededMaxRetries)
 		}
 
-		return NewUpgrade(r.configFactory, r.eventRecorder), nil
+		return NewUpgrade(r.configFactory, r.eventRecorder, r.defaultToRetryOnFailure), nil
 	case ReleaseStatusDrifted:
 		log.Info(msgWithReason("detected changes in cluster state", diff.SummarizeDiffSetBrief(state.Diff)))
 		for _, change := range state.Diff {
@@ -467,11 +469,11 @@ func (r *AtomicRelease) actionForState(ctx context.Context, req *Request, state
 
 		// If the action strategy is to retry (and not remediate), we behave just like
 		// "flux reconcile hr --force" and .spec.<action>.remediation.retries set to 0.
-		if req.Object.GetActiveRetry() != nil {
+		if req.Object.GetActiveRetry(r.defaultToRetryOnFailure) != nil {
 			req.Object.Status.History.TruncateIgnoringPreviousSnapshots()
 
 			log.V(logger.DebugLevel).Info("retrying upgrade for failed release")
-			return NewUpgrade(r.configFactory, r.eventRecorder), nil
+			return NewUpgrade(r.configFactory, r.eventRecorder, r.defaultToRetryOnFailure), nil
 		}
 
 		remediation := req.Object.GetActiveRemediation()
@@ -480,7 +482,7 @@ func (r *AtomicRelease) actionForState(ctx context.Context, req *Request, state
 		// upgrade the release to see if that fixes the problem.
 		if remediation == nil {
 			log.V(logger.DebugLevel).Info("no active remediation strategy")
-			return NewUpgrade(r.configFactory, r.eventRecorder), nil
+			return NewUpgrade(r.configFactory, r.eventRecorder, r.defaultToRetryOnFailure), nil
 		}
 
 		// If there is no failure count, the conditions under which the failure
@@ -490,14 +492,14 @@ func (r *AtomicRelease) actionForState(ctx context.Context, req *Request, state
 		// attempted again.
 		if remediation.GetFailureCount(req.Object) <= 0 {
 			log.Info("release conditions have changed since last failure")
-			return NewUpgrade(r.configFactory, r.eventRecorder), nil
+			return NewUpgrade(r.configFactory, r.eventRecorder, r.defaultToRetryOnFailure), nil
 		}
 
 		// If the force annotation is set, we can attempt to upgrade the release
 		// without any further checks.
 		if forceRequested {
 			log.Info(msgWithReason("forcing upgrade for failed release", "force requested through annotation"))
-			return NewUpgrade(r.configFactory, r.eventRecorder), nil
+			return NewUpgrade(r.configFactory, r.eventRecorder, r.defaultToRetryOnFailure), nil
 		}
 
 		// We have exhausted the number of retries for the remediation
@@ -526,7 +528,7 @@ func (r *AtomicRelease) actionForState(ctx context.Context, req *Request, state
 					// 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
+					return NewUpgrade(r.configFactory, r.eventRecorder, r.defaultToRetryOnFailure), nil
 				}
 
 				// This may be a temporary error, return it to retry.

internal/reconcile/atomic_release_test.go

@@ -178,7 +178,7 @@ func TestAtomicRelease_Reconcile(t *testing.T) {
 			Chart:  testutil.BuildChart(testutil.ChartWithTestHook()),
 			Values: nil,
 		}
-		g.Expect(NewAtomicRelease(patchHelper, cfg, recorder, testFieldManager, nil).Reconcile(context.TODO(), req)).ToNot(HaveOccurred())
+		g.Expect(NewAtomicRelease(patchHelper, cfg, recorder, testFieldManager, nil, false).Reconcile(context.TODO(), req)).ToNot(HaveOccurred())
 
 		g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{
 			{
@@ -1230,7 +1230,7 @@ func TestAtomicRelease_Reconcile_Scenarios(t *testing.T) {
 				Values: tt.values,
 			}
 
-			err = NewAtomicRelease(patchHelper, cfg, recorder, testFieldManager, nil).Reconcile(context.TODO(), req)
+			err = NewAtomicRelease(patchHelper, cfg, recorder, testFieldManager, nil, false).Reconcile(context.TODO(), req)
 			wantErr := BeNil()
 			if tt.wantErr != nil {
 				wantErr = MatchError(tt.wantErr)
@@ -1462,7 +1462,7 @@ func TestAtomicRelease_Reconcile_PostRenderers_Scenarios(t *testing.T) {
 				Values: tt.values,
 			}
 
-			err = NewAtomicRelease(patchHelper, cfg, recorder, testFieldManager, nil).Reconcile(context.TODO(), req)
+			err = NewAtomicRelease(patchHelper, cfg, recorder, testFieldManager, nil, false).Reconcile(context.TODO(), req)
 			g.Expect(err).ToNot(HaveOccurred())
 
 			g.Expect(obj.Status.ObservedPostRenderersDigest).To(Equal(tt.wantDigest))
@@ -2444,7 +2444,7 @@ func TestAtomicRelease_Reconcile_CommonMetadata_Scenarios(t *testing.T) {
 				Values: tt.values,
 			}
 
-			err = NewAtomicRelease(patchHelper, cfg, recorder, testFieldManager, nil).Reconcile(context.TODO(), req)
+			err = NewAtomicRelease(patchHelper, cfg, recorder, testFieldManager, nil, false).Reconcile(context.TODO(), req)
 			g.Expect(err).ToNot(HaveOccurred())
 
 			g.Expect(obj.Status.ObservedCommonMetadataDigest).To(Equal(tt.wantDigest))