Merge pull request #884 from fluxcd/update-stale-ready-condition

Remove stale Ready=False conditions value to show more accurate status

go.mod

@@ -21,7 +21,7 @@ require (
 	github.com/fluxcd/pkg/apis/event v0.7.0
 	github.com/fluxcd/pkg/apis/kustomize v1.3.0
 	github.com/fluxcd/pkg/apis/meta v1.3.0
-	github.com/fluxcd/pkg/runtime v0.44.0
+	github.com/fluxcd/pkg/runtime v0.44.1
 	github.com/fluxcd/pkg/ssa v0.36.0
 	github.com/fluxcd/pkg/testserver v0.5.0
 	github.com/fluxcd/source-controller/api v1.2.4

go.sum

@@ -104,8 +104,8 @@ github.com/fluxcd/pkg/apis/kustomize v1.3.0 h1:qvB46CfaOWcL1SyR2RiVWN/j7/035D0Ot
 github.com/fluxcd/pkg/apis/kustomize v1.3.0/go.mod h1:PCXf5kktTzNav0aH2Ns3jsowqwmA9xTcsrEOoPzx/K8=
 github.com/fluxcd/pkg/apis/meta v1.3.0 h1:KxeEc6olmSZvQ5pBONPE4IKxyoWQbqTJF1X6K5nIXpU=
 github.com/fluxcd/pkg/apis/meta v1.3.0/go.mod h1:3Ui8xFkoU4sYehqmscjpq7NjqH2YN1A2iX2okbO3/yA=
-github.com/fluxcd/pkg/runtime v0.44.0 h1:0BEPSpcsYXOiswKG5TWkin8fhCDHb0nDdAtq/5VrCSI=
-github.com/fluxcd/pkg/runtime v0.44.0/go.mod h1:s1AhSOTCEBPaTfz/GdBD/Ws66uOByIuNP4Znrq+is9M=
+github.com/fluxcd/pkg/runtime v0.44.1 h1:XuPTcNIgn/NsoIo/A6qfPZaD9E7cbnJTDbeNw8O1SZQ=
+github.com/fluxcd/pkg/runtime v0.44.1/go.mod h1:s1AhSOTCEBPaTfz/GdBD/Ws66uOByIuNP4Znrq+is9M=
 github.com/fluxcd/pkg/ssa v0.36.0 h1:h9FB6SrrdVlxNQtfG+Fb/Roe1e61EPgtmJ5ORlAxwkU=
 github.com/fluxcd/pkg/ssa v0.36.0/go.mod h1:FJj4xznwBvRM+9h02lGGC0CGYGucPeXO7P6NEPphbys=
 github.com/fluxcd/pkg/testserver v0.5.0 h1:n/Iskk0tXNt2AgIgjz9qeFK/VhEXGfqeazABXZmO2Es=

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 conditions.HasAnyReason(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 conditions.HasAnyReason(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 conditions.HasAnyReason(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 conditions.HasAnyReason(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 conditions.HasAnyReason(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 conditions.HasAnyReason(obj, meta.ReadyCondition, "RESTClientError") {
+		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 conditions.HasAnyReason(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{

internal/controller/helmrelease_controller_test.go

@@ -43,6 +43,7 @@ import (
 	"sigs.k8s.io/yaml"
 
 	"github.com/fluxcd/pkg/apis/acl"
+	aclv1 "github.com/fluxcd/pkg/apis/acl"
 	"github.com/fluxcd/pkg/apis/meta"
 	"github.com/fluxcd/pkg/runtime/conditions"
 	feathelper "github.com/fluxcd/pkg/runtime/features"
@@ -762,6 +763,127 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) {
 		g.Expect(obj.Status.LastAttemptedConfigDigest).To(Equal("sha256:1dabc4e3cbbd6a0818bd460f3a6c9855bfe95d506c74726bc0f2edb0aecb1f4e"))
 		g.Expect(obj.Status.LastAttemptedValuesChecksum).To(BeEmpty())
 	})
+
+	t.Run("error recovery updates ready condition", func(t *testing.T) {
+		g := NewWithT(t)
+
+		// Create a test namespace for storing the Helm release mock.
+		ns, err := testEnv.CreateNamespace(context.TODO(), "error-recovery")
+		g.Expect(err).ToNot(HaveOccurred())
+		t.Cleanup(func() {
+			_ = testEnv.Delete(context.TODO(), ns)
+		})
+
+		// Create HelmChart mock.
+		chartMock := testutil.BuildChart()
+		chartArtifact, err := testutil.SaveChartAsArtifact(chartMock, digest.SHA256, testServer.URL(), testServer.Root())
+		g.Expect(err).ToNot(HaveOccurred())
+
+		chart := &sourcev1b2.HelmChart{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:       "error-recovery",
+				Namespace:  ns.Name,
+				Generation: 1,
+			},
+			Spec: sourcev1b2.HelmChartSpec{
+				Chart:   "testdata/test-helmrepo",
+				Version: "0.1.0",
+				SourceRef: sourcev1b2.LocalHelmChartSourceReference{
+					Kind: sourcev1b2.HelmRepositoryKind,
+					Name: "error-recovery",
+				},
+			},
+			Status: sourcev1b2.HelmChartStatus{
+				ObservedGeneration: 1,
+				Artifact:           chartArtifact,
+				Conditions: []metav1.Condition{
+					{
+						Type:   meta.ReadyCondition,
+						Status: metav1.ConditionTrue,
+					},
+				},
+			},
+		}
+
+		// Create a test Helm release storage mock.
+		rls := testutil.BuildRelease(&helmrelease.MockReleaseOptions{
+			Name:      "error-recovery",
+			Namespace: ns.Name,
+			Version:   1,
+			Chart:     chartMock,
+			Status:    helmrelease.StatusDeployed,
+		}, testutil.ReleaseWithConfig(nil))
+
+		obj := &v2.HelmRelease{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "error-recovery",
+				Namespace: ns.Name,
+			},
+			Spec: v2.HelmReleaseSpec{
+				StorageNamespace: ns.Name,
+			},
+			Status: v2.HelmReleaseStatus{
+				HelmChart: chart.Namespace + "/" + chart.Name,
+				History: v2.Snapshots{
+					release.ObservedToSnapshot(release.ObserveRelease(rls)),
+				},
+			},
+		}
+
+		c := fake.NewClientBuilder().
+			WithScheme(NewTestScheme()).
+			WithStatusSubresource(&v2.HelmRelease{}).
+			WithObjects(chart, obj).
+			Build()
+
+		r := &HelmReleaseReconciler{
+			Client:           c,
+			GetClusterConfig: GetTestClusterConfig,
+			EventRecorder:    record.NewFakeRecorder(32),
+			FieldManager:     "test",
+		}
+
+		// Store the Helm release mock in the test namespace.
+		getter, err := r.buildRESTClientGetter(context.TODO(), obj)
+		g.Expect(err).ToNot(HaveOccurred())
+
+		cfg, err := action.NewConfigFactory(getter, action.WithStorage(helmdriver.SecretsDriverName, obj.GetStorageNamespace()))
+		g.Expect(err).ToNot(HaveOccurred())
+
+		store := helmstorage.Init(cfg.Driver)
+		g.Expect(store.Create(rls)).To(Succeed())
+
+		sp := patch.NewSerialPatcher(obj, r.Client)
+
+		// List of failure reasons to test.
+		prereqFailures := []string{
+			v2.DependencyNotReadyReason,
+			aclv1.AccessDeniedReason,
+			v2.ArtifactFailedReason,
+			"HelmChartNotReady",
+			"ValuesError",
+			"RESTClientError",
+			"FactoryError",
+		}
+
+		// Update ready condition for each failure, reconcile and check if the
+		// stale failure condition gets updated.
+		for _, failReason := range prereqFailures {
+			conditions.MarkFalse(obj, meta.ReadyCondition, failReason, "foo")
+			err := sp.Patch(context.TODO(), obj,
+				patch.WithOwnedConditions{Conditions: intreconcile.OwnedConditions},
+				patch.WithFieldOwner(r.FieldManager),
+			)
+			g.Expect(err).ToNot(HaveOccurred())
+
+			_, err = r.reconcileRelease(context.TODO(), sp, obj)
+			g.Expect(err).ToNot(HaveOccurred())
+
+			ready := conditions.Get(obj, meta.ReadyCondition)
+			g.Expect(ready.Status).To(Equal(metav1.ConditionUnknown))
+			g.Expect(ready.Reason).To(Equal(meta.ProgressingReason))
+		}
+	})
 }
 
 func TestHelmReleaseReconciler_reconcileDelete(t *testing.T) {