From 93d2118f71358e61cdcb973dac6b33497b9c15c6 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Thu, 7 Dec 2023 23:17:33 +0100 Subject: [PATCH] controller: enrich "HelmChart not ready" messages This propagates the reason a HelmChart is (likely) not ready to the message of the Ready condition. The goal of this is to make it easier for people to reason about a potential failure that may be happening while retrieving the chart, without having to inspect the HelmChart itself. As at times, they may not have access (due to e.g. not being able to access the namespace, while the controller is allowed to create the object there), or are simply not aware of the fact that this object is created by the controller for them. Signed-off-by: Hidde Beydals --- internal/controller/helmrelease_controller.go | 32 ++++++- .../controller/helmrelease_controller_test.go | 90 ++++++++++++++++++- 2 files changed, 116 insertions(+), 6 deletions(-) diff --git a/internal/controller/helmrelease_controller.go b/internal/controller/helmrelease_controller.go index 0df4d8dd7..82bebd6a1 100644 --- a/internal/controller/helmrelease_controller.go +++ b/internal/controller/helmrelease_controller.go @@ -276,9 +276,9 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe return ctrl.Result{}, err } - // Check chart readiness. - if hc.Generation != hc.Status.ObservedGeneration || !conditions.IsReady(hc) || hc.GetArtifact() == nil { - msg := fmt.Sprintf("HelmChart '%s/%s' is not ready", hc.GetNamespace(), hc.GetName()) + // Check if the HelmChart is ready. + if ready, reason := isHelmChartReady(hc); !ready { + msg := fmt.Sprintf("HelmChart '%s/%s' is not ready: %s", hc.GetNamespace(), hc.GetName(), reason) log.Info(msg) conditions.MarkFalse(obj, meta.ReadyCondition, "HelmChartNotReady", msg) // Do not requeue immediately, when the artifact is created @@ -703,3 +703,29 @@ func (r *HelmReleaseReconciler) requestsForHelmChartChange(ctx context.Context, } return reqs } + +// isHelmChartReady returns true if the given HelmChart is ready, and a reason +// why it is not ready otherwise. +func isHelmChartReady(obj *sourcev1.HelmChart) (bool, string) { + switch { + case obj.Generation != obj.Status.ObservedGeneration: + msg := "latest generation of object has not been reconciled" + + // If the chart is not ready, we can likely provide a more + // concise reason why. + // We do not do this while the Generation matches the Observed + // Generation, as we could then potentially stall on e.g. + // temporary errors which do not have an impact as long as + // there is an Artifact for the current Generation. + if conditions.IsFalse(obj, meta.ReadyCondition) { + msg = conditions.GetMessage(obj, meta.ReadyCondition) + } + return false, msg + case conditions.IsStalled(obj): + return false, conditions.GetMessage(obj, meta.StalledCondition) + case obj.Status.Artifact == nil: + return false, "does not have an artifact" + default: + return true, "" + } +} diff --git a/internal/controller/helmrelease_controller_test.go b/internal/controller/helmrelease_controller_test.go index 1376770c4..75fa2149a 100644 --- a/internal/controller/helmrelease_controller_test.go +++ b/internal/controller/helmrelease_controller_test.go @@ -179,7 +179,7 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) { })) }) - t.Run("waits for HelmChart to be ready", func(t *testing.T) { + t.Run("waits for HelmChart to have an Artifact", func(t *testing.T) { g := NewWithT(t) chart := &sourcev1b2.HelmChart{ @@ -190,11 +190,11 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) { }, Status: sourcev1b2.HelmChartStatus{ ObservedGeneration: 2, - Artifact: &sourcev1.Artifact{}, + Artifact: nil, Conditions: []metav1.Condition{ { Type: meta.ReadyCondition, - Status: metav1.ConditionFalse, + Status: metav1.ConditionTrue, }, }, }, @@ -2206,3 +2206,87 @@ func TestValuesReferenceValidation(t *testing.T) { }) } } + +func Test_isHelmChartReady(t *testing.T) { + mock := &sourcev1b2.HelmChart{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 2, + }, + Status: sourcev1b2.HelmChartStatus{ + ObservedGeneration: 2, + Conditions: []metav1.Condition{ + { + Type: meta.ReadyCondition, + Status: metav1.ConditionTrue, + }, + }, + Artifact: &sourcev1.Artifact{}, + }, + } + + tests := []struct { + name string + obj *sourcev1b2.HelmChart + want bool + wantReason string + }{ + { + name: "chart is ready", + obj: mock.DeepCopy(), + want: true, + }, + { + name: "chart generation differs from observed generation while Ready=True", + obj: func() *sourcev1b2.HelmChart { + m := mock.DeepCopy() + m.Generation = 3 + return m + }(), + want: false, + wantReason: "latest generation of object has not been reconciled", + }, + { + name: "chart generation differs from observed generation while Ready=False", + obj: func() *sourcev1b2.HelmChart { + m := mock.DeepCopy() + m.Generation = 3 + conditions.MarkFalse(m, meta.ReadyCondition, "Reason", "some reason") + return m + }(), + want: false, + wantReason: "some reason", + }, + { + name: "chart has Stalled=True", + obj: func() *sourcev1b2.HelmChart { + m := mock.DeepCopy() + conditions.MarkFalse(m, meta.ReadyCondition, "Reason", "some reason") + conditions.MarkStalled(m, "Reason", "some stalled reason") + return m + }(), + want: false, + wantReason: "some stalled reason", + }, + { + name: "chart does not have an Artifact", + obj: func() *sourcev1b2.HelmChart { + m := mock.DeepCopy() + m.Status.Artifact = nil + return m + }(), + want: false, + wantReason: "does not have an artifact", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, gotReason := isHelmChartReady(tt.obj) + if got != tt.want { + t.Errorf("isHelmChartReady() got = %v, want %v", got, tt.want) + } + if gotReason != tt.wantReason { + t.Errorf("isHelmChartReady() reason = %v, want %v", gotReason, tt.wantReason) + } + }) + } +}