Remove stale Ready=False conditions values

When the reconciliation begins, while fulfilling the prerequisites,
Ready=False condition for various reasons are added on the object. On
failure, this reason is persisted on the object. On a subsequent
reconciliation, when the failure is recovered, the Ready=False condition
is not updates until the atomic reconciliation reaches a conclusion.
During this period if the atomic reconciliation enters a retry look due
to constant drift detection and correction, the stale Ready=False
condition with incorrect reason persists on the object. The Ready=False
message is also copied to Reconciling=True condition, resulting in an
incorrect depiction of what's actually happening.
For example, if previously the HelmRelease failed with dependency not
ready error, on a subsequent reconciliation, even after going past the
dependency check and returning from atomic reconciliation due to drift
detection and correction loop scenario, the Ready=False condition
continues to show the stale dependency not ready error.

In order to show more accurate status, the Ready=False conditions added
while fulfilling prerequisites can be removed once those checks have
succeeded, updating Ready=False to Ready=Unknown with "reconciliation in
progress" message. If the atomic reconciliation gets stuck in the drift
detection and correction loop with this, the Ready and Reconciling
conditons would show "reconciliation in progress". This should be a
better indicator of what's going on. The events and logs can be checked
to determine accurately what's causing the reconciliation to be
progressing for ever.

Signed-off-by: Sunny <[email protected]>

internal/controller/helmrelease_controller.go

@@ -247,6 +247,10 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe
 
 		log.Info("all dependencies are ready")
 	}
+	// Remove any stale corresponding Ready=False condition with Unknown.
+	if hasConditionReason(obj, meta.ReadyCondition, v2.DependencyNotReadyReason) {
+		conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress")
+	}
 
 	// Get the HelmChart object for the release.
 	hc, err := r.getHelmChart(ctx, obj)
@@ -266,6 +270,10 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe
 		conditions.MarkFalse(obj, meta.ReadyCondition, v2.ArtifactFailedReason, msg)
 		return ctrl.Result{}, err
 	}
+	// Remove any stale corresponding Ready=False condition with Unknown.
+	if hasConditionReason(obj, meta.ReadyCondition, aclv1.AccessDeniedReason, v2.ArtifactFailedReason) {
+		conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress")
+	}
 
 	// Check if the HelmChart is ready.
 	if ready, reason := isHelmChartReady(hc); !ready {
@@ -276,13 +284,21 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe
 		// the watcher should trigger a reconciliation.
 		return jitter.JitteredRequeueInterval(ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}), errWaitForChart
 	}
+	// Remove any stale corresponding Ready=False condition with Unknown.
+	if hasConditionReason(obj, meta.ReadyCondition, "HelmChartNotReady") {
+		conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress")
+	}
 
 	// Compose values based from the spec and references.
 	values, err := chartutil.ChartValuesFromReferences(ctx, r.Client, obj.Namespace, obj.GetValues(), obj.Spec.ValuesFrom...)
 	if err != nil {
 		conditions.MarkFalse(obj, meta.ReadyCondition, "ValuesError", err.Error())
 		return ctrl.Result{}, err
 	}
+	// Remove any stale corresponding Ready=False condition with Unknown.
+	if hasConditionReason(obj, meta.ReadyCondition, "ValuesError") {
+		conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress")
+	}
 
 	// Load chart from artifact.
 	loadedChart, err := loader.SecureLoadChartFromURL(loader.NewRetryableHTTPClient(ctx, r.artifactFetchRetries), hc.GetArtifact().URL, hc.GetArtifact().Digest)
@@ -297,13 +313,21 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe
 		conditions.MarkFalse(obj, meta.ReadyCondition, v2.ArtifactFailedReason, fmt.Sprintf("Could not load chart: %s", err.Error()))
 		return ctrl.Result{}, err
 	}
+	// Remove any stale corresponding Ready=False condition with Unknown.
+	if hasConditionReason(obj, meta.ReadyCondition, v2.ArtifactFailedReason) {
+		conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress")
+	}
 
 	// Build the REST client getter.
 	getter, err := r.buildRESTClientGetter(ctx, obj)
 	if err != nil {
 		conditions.MarkFalse(obj, meta.ReadyCondition, "RESTClientError", err.Error())
 		return ctrl.Result{}, err
 	}
+	// Remove any stale corresponding Ready=False condition with Unknown.
+	if hasConditionReason(obj, meta.ReadyCondition, "RESETClientError") {
+		conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress")
+	}
 
 	// Attempt to adopt "legacy" v2beta1 release state on a best-effort basis.
 	// If this fails, the controller will fall back to performing an upgrade
@@ -354,6 +378,10 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe
 		conditions.MarkFalse(obj, meta.ReadyCondition, "FactoryError", err.Error())
 		return ctrl.Result{}, err
 	}
+	// Remove any stale corresponding Ready=False condition with Unknown.
+	if hasConditionReason(obj, meta.ReadyCondition, "FactoryError") {
+		conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "reconciliation in progress")
+	}
 
 	// Off we go!
 	if err = intreconcile.NewAtomicRelease(patchHelper, cfg, r.EventRecorder, r.FieldManager).Reconcile(ctx, &intreconcile.Request{
@@ -720,3 +748,14 @@ func isHelmChartReady(obj *sourcev1.HelmChart) (bool, string) {
 		return true, ""
 	}
 }
+
+// hasConditionReason returns if the given Getter's condition type t has any of
+// the given reasons.
+func hasConditionReason(from conditions.Getter, t string, reasons ...string) bool {
+	for _, reason := range reasons {
+		if conditions.GetReason(from, t) == reason {
+			return true
+		}
+	}
+	return false
+}

internal/controller/helmrelease_controller_test.go

@@ -2286,3 +2286,39 @@ func Test_isHelmChartReady(t *testing.T) {
 		})
 	}
 }
+
+func Test_hasConditionReason(t *testing.T) {
+	tests := []struct {
+		name           string
+		existingReason string
+		checkReasons   []string
+		want           bool
+	}{
+		{
+			name:           "no check reason",
+			existingReason: v2.DependencyNotReadyReason,
+			want:           false,
+		},
+		{
+			name:           "no known reason",
+			existingReason: v2.DependencyNotReadyReason,
+			checkReasons:   []string{v2.ArtifactFailedReason, acl.AccessDeniedReason},
+			want:           false,
+		},
+		{
+			name:           "known and unknown reasons",
+			existingReason: v2.ArtifactFailedReason,
+			checkReasons:   []string{acl.AccessDeniedReason, v2.ArtifactFailedReason},
+			want:           true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			obj := &v2.HelmRelease{}
+			conditions.MarkFalse(obj, meta.ReadyCondition, tt.existingReason, "foo")
+			if got := hasConditionReason(obj, meta.ReadyCondition, tt.checkReasons...); got != tt.want {
+				t.Errorf("hasConditionReason() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}