Merge pull request #1339 from fluxcd/backport-1338-to-release/v1.4.x

[release/v1.4.x] Fix status reporting for `RetryOnFailure` strategy

Author: stefanprodan

internal/controller/helmrelease_controller_test.go

@@ -692,12 +692,12 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) {
 			ObjectMeta: metav1.ObjectMeta{
 				Name:       "release",
 				Namespace:  "mock",
-				Generation: 2,
+				Generation: 1,
 			},
 			Spec: v2.HelmReleaseSpec{
 				// Trigger a failure by setting an invalid storage namespace,
 				// preventing the release from actually being installed.
-				// This allows us to just test the failure count reset, without
+				// This allows us to just test the RetryOnFailure strategy, without
 				// having to facilitate a full install.
 				StorageNamespace: "not-exist",
 				Install: &v2.Install{
@@ -708,11 +708,8 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) {
 				},
 			},
 			Status: v2.HelmReleaseStatus{
-				HelmChart:               "mock/chart",
-				InstallFailures:         2,
-				UpgradeFailures:         3,
-				Failures:                5,
-				LastAttemptedGeneration: 2,
+				HelmChart:          "mock/chart",
+				ObservedGeneration: 1,
 			},
 		}
 
@@ -734,11 +731,17 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) {
 		g.Expect(res.RequeueAfter).To(BeNumerically("==", time.Minute))
 
 		g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{
+			{
+				Type:    "Reconciling",
+				Status:  "True",
+				Reason:  "ProgressingWithRetry",
+				Message: "retrying after 1m0s",
+			},
 			{
 				Type:    "Ready",
 				Status:  "False",
-				Reason:  "RetryAfterInterval",
-				Message: "Will retry after 1m0s",
+				Reason:  "InstallFailed",
+				Message: "Helm install failed for release mock/release with chart [email protected]: create: failed to create: namespaces \"not-exist\" not found",
 			},
 			{
 				Type:    "Released",

internal/reconcile/atomic_release.go

@@ -195,7 +195,9 @@ func (r *AtomicRelease) Reconcile(ctx context.Context, req *Request) error {
 			}
 
 			// Determine the next action to run based on the current state.
-			log.V(logger.DebugLevel).Info("determining next Helm action based on current state")
+			log.V(logger.DebugLevel).Info(
+				fmt.Sprintf("determining next Helm action based on state: '%s' reason '%s'", state.Status, state.Reason),
+			)
 			if next, err = r.actionForState(ctx, req, state); err != nil {
 				if errors.Is(err, ErrExceededMaxRetries) {
 					conditions.MarkStalled(req.Object, "RetriesExceeded", "Failed to %s after %d attempt(s)",
@@ -211,6 +213,7 @@ func (r *AtomicRelease) Reconcile(ctx context.Context, req *Request) error {
 
 			// If there is no next action, we are done.
 			if next == nil {
+				log.V(logger.DebugLevel).Info("no further action to take, atomic release completed")
 				conditions.Delete(req.Object, meta.ReconcilingCondition)
 
 				// Always summarize; this ensures we restore transient errors
@@ -241,9 +244,7 @@ func (r *AtomicRelease) Reconcile(ctx context.Context, req *Request) error {
 				)
 
 				if retry := req.Object.GetActiveRetry(); retry != nil {
-					conditions.Delete(req.Object, meta.ReconcilingCondition)
-					conditions.MarkFalse(req.Object, meta.ReadyCondition, "RetryAfterInterval",
-						"Will retry after %s", retry.GetRetryInterval().String())
+					conditions.MarkReconciling(req.Object, meta.ProgressingWithRetryReason, "retrying after %s", retry.GetRetryInterval().String())
 					return ErrRetryAfterInterval
 				}
 
@@ -278,11 +279,13 @@ func (r *AtomicRelease) Reconcile(ctx context.Context, req *Request) error {
 			// Run the action sub-reconciler.
 			log.Info(fmt.Sprintf("running '%s' action with timeout of %s", next.Name(), timeoutForAction(next, req.Object).String()))
 			if err = next.Reconcile(ctx, req); err != nil {
+				log.V(logger.DebugLevel).Info(
+					fmt.Sprintf("action reconciler %s of type %s returned error: %s", next.Name(), next.Type(), err),
+				)
+
 				if retry := req.Object.GetActiveRetry(); retry != nil {
 					log.Error(err, fmt.Sprintf("failed to run '%s' action", next.Name()))
-					conditions.Delete(req.Object, meta.ReconcilingCondition)
-					conditions.MarkFalse(req.Object, meta.ReadyCondition, "RetryAfterInterval",
-						"Will retry after %s", retry.GetRetryInterval().String())
+					conditions.MarkReconciling(req.Object, meta.ProgressingWithRetryReason, "retrying after %s", retry.GetRetryInterval().String())
 					return ErrRetryAfterInterval
 				}
 
@@ -299,9 +302,7 @@ func (r *AtomicRelease) Reconcile(ctx context.Context, req *Request) error {
 				)
 
 				if retry := req.Object.GetActiveRetry(); retry != nil {
-					conditions.Delete(req.Object, meta.ReconcilingCondition)
-					conditions.MarkFalse(req.Object, meta.ReadyCondition, "RetryAfterInterval",
-						"Will retry after %s", retry.GetRetryInterval().String())
+					conditions.MarkReconciling(req.Object, meta.ProgressingWithRetryReason, "retrying after %s", retry.GetRetryInterval().String())
 					return ErrRetryAfterInterval
 				}