From 63f7a76319e7692855ec9f92899b021614ecc58a Mon Sep 17 00:00:00 2001 From: Sunny Date: Thu, 9 May 2024 10:58:41 +0000 Subject: [PATCH] PostRenderersDigest observation improvements Move the post renderers digest set/update code from summarize() to atomic release reconciler in order to update the observation only at the end of a successful reconciliation. summarize() is for summarizing the status conditions and is also called by all the other action sub-reconcilers, which can update the post renderers digest observation too early. Updating the observed post renderers digest at the very end of a reconciliation introduces an issue where a digest mismatch in DetermineReleaseState() could result in the release to get stuck in a loop as even after running an upgrade due to post renderers value, the new observation isn't reflected immediately in the middle of atomic reconciliation. This can be solved by checking post renderers digest value only for new configurations where the object generation and the ready status condition observed generations don't match, in other words when the generation of a configuration has not be processed. This assumes that an upgrade due to any other reason also takes into account the post renderers value and need not be checked separately for the same config generation. Signed-off-by: Sunny --- internal/reconcile/atomic_release.go | 11 ++ internal/reconcile/atomic_release_test.go | 225 ++++++++++++++++++++++ internal/reconcile/release.go | 11 -- internal/reconcile/release_test.go | 124 ++---------- internal/reconcile/state.go | 25 ++- internal/reconcile/state_test.go | 48 +++++ 6 files changed, 318 insertions(+), 126 deletions(-) diff --git a/internal/reconcile/atomic_release.go b/internal/reconcile/atomic_release.go index d26324a78..7ae18c986 100644 --- a/internal/reconcile/atomic_release.go +++ b/internal/reconcile/atomic_release.go @@ -38,7 +38,9 @@ import ( v2 "github.com/fluxcd/helm-controller/api/v2" "github.com/fluxcd/helm-controller/internal/action" "github.com/fluxcd/helm-controller/internal/diff" + "github.com/fluxcd/helm-controller/internal/digest" interrors "github.com/fluxcd/helm-controller/internal/errors" + "github.com/fluxcd/helm-controller/internal/postrender" ) // OwnedConditions is a list of Condition types owned by the HelmRelease object. @@ -210,6 +212,15 @@ func (r *AtomicRelease) Reconcile(ctx context.Context, req *Request) error { // written to Ready. summarize(req) + // remove stale post-renderers digest on successful reconciliation. + if conditions.IsReady(req.Object) { + req.Object.Status.ObservedPostRenderersDigest = "" + if req.Object.Spec.PostRenderers != nil { + // Update the post-renderers digest if the post-renderers exist. + req.Object.Status.ObservedPostRenderersDigest = postrender.Digest(digest.Canonical, req.Object.Spec.PostRenderers).String() + } + } + return nil } diff --git a/internal/reconcile/atomic_release_test.go b/internal/reconcile/atomic_release_test.go index 108f39154..7081a290d 100644 --- a/internal/reconcile/atomic_release_test.go +++ b/internal/reconcile/atomic_release_test.go @@ -43,7 +43,9 @@ import ( v2 "github.com/fluxcd/helm-controller/api/v2" "github.com/fluxcd/helm-controller/internal/action" + "github.com/fluxcd/helm-controller/internal/digest" "github.com/fluxcd/helm-controller/internal/kube" + "github.com/fluxcd/helm-controller/internal/postrender" "github.com/fluxcd/helm-controller/internal/release" "github.com/fluxcd/helm-controller/internal/testutil" ) @@ -1067,6 +1069,229 @@ func TestAtomicRelease_Reconcile_Scenarios(t *testing.T) { } } +func TestAtomicRelease_Reconcile_PostRenderers_Scenarios(t *testing.T) { + tests := []struct { + name string + releases func(namespace string) []*helmrelease.Release + spec func(spec *v2.HelmReleaseSpec) + values map[string]interface{} + status func(releases []*helmrelease.Release) v2.HelmReleaseStatus + wantDigest string + wantReleaseAction v2.ReleaseAction + }{ + { + name: "addition of post renderers", + releases: func(namespace string) []*helmrelease.Release { + return []*helmrelease.Release{ + testutil.BuildRelease(&helmrelease.MockReleaseOptions{ + Name: mockReleaseName, + Namespace: namespace, + Version: 1, + Status: helmrelease.StatusDeployed, + Chart: testutil.BuildChart(), + }, testutil.ReleaseWithConfig(nil)), + } + }, + spec: func(spec *v2.HelmReleaseSpec) { + spec.PostRenderers = postRenderers + }, + status: func(releases []*helmrelease.Release) v2.HelmReleaseStatus { + return v2.HelmReleaseStatus{ + History: v2.Snapshots{ + release.ObservedToSnapshot(release.ObserveRelease(releases[0])), + }, + Conditions: []metav1.Condition{ + { + Type: meta.ReadyCondition, + Status: metav1.ConditionTrue, + ObservedGeneration: 1, + }, + }, + } + }, + wantDigest: postrender.Digest(digest.Canonical, postRenderers).String(), + wantReleaseAction: v2.ReleaseActionUpgrade, + }, + { + name: "config change and addition of post renderers", + releases: func(namespace string) []*helmrelease.Release { + return []*helmrelease.Release{ + testutil.BuildRelease(&helmrelease.MockReleaseOptions{ + Name: mockReleaseName, + Namespace: namespace, + Version: 1, + Status: helmrelease.StatusDeployed, + Chart: testutil.BuildChart(), + }, testutil.ReleaseWithConfig(nil)), + } + }, + spec: func(spec *v2.HelmReleaseSpec) { + spec.PostRenderers = postRenderers + }, + status: func(releases []*helmrelease.Release) v2.HelmReleaseStatus { + return v2.HelmReleaseStatus{ + History: v2.Snapshots{ + release.ObservedToSnapshot(release.ObserveRelease(releases[0])), + }, + Conditions: []metav1.Condition{ + { + Type: meta.ReadyCondition, + Status: metav1.ConditionTrue, + ObservedGeneration: 1, + }, + }, + } + }, + values: map[string]interface{}{"foo": "baz"}, + wantDigest: postrender.Digest(digest.Canonical, postRenderers).String(), + wantReleaseAction: v2.ReleaseActionUpgrade, + }, + { + name: "existing and new post renderers value", + releases: func(namespace string) []*helmrelease.Release { + return []*helmrelease.Release{ + testutil.BuildRelease(&helmrelease.MockReleaseOptions{ + Name: mockReleaseName, + Namespace: namespace, + Version: 1, + Status: helmrelease.StatusDeployed, + Chart: testutil.BuildChart(), + }, testutil.ReleaseWithConfig(nil)), + } + }, + spec: func(spec *v2.HelmReleaseSpec) { + spec.PostRenderers = postRenderers2 + }, + status: func(releases []*helmrelease.Release) v2.HelmReleaseStatus { + return v2.HelmReleaseStatus{ + History: v2.Snapshots{ + release.ObservedToSnapshot(release.ObserveRelease(releases[0])), + }, + Conditions: []metav1.Condition{ + { + Type: meta.ReadyCondition, + Status: metav1.ConditionTrue, + ObservedGeneration: 1, + }, + }, + ObservedPostRenderersDigest: postrender.Digest(digest.Canonical, postRenderers).String(), + } + }, + wantDigest: postrender.Digest(digest.Canonical, postRenderers2).String(), + wantReleaseAction: v2.ReleaseActionUpgrade, + }, + { + name: "post renderers mismatch remains in sync for processed config", + releases: func(namespace string) []*helmrelease.Release { + return []*helmrelease.Release{ + testutil.BuildRelease(&helmrelease.MockReleaseOptions{ + Name: mockReleaseName, + Namespace: namespace, + Version: 1, + Status: helmrelease.StatusDeployed, + Chart: testutil.BuildChart(), + }, testutil.ReleaseWithConfig(nil)), + } + }, + spec: func(spec *v2.HelmReleaseSpec) { + spec.PostRenderers = postRenderers2 + }, + status: func(releases []*helmrelease.Release) v2.HelmReleaseStatus { + return v2.HelmReleaseStatus{ + History: v2.Snapshots{ + release.ObservedToSnapshot(release.ObserveRelease(releases[0])), + }, + Conditions: []metav1.Condition{ + { + Type: meta.ReadyCondition, + Status: metav1.ConditionTrue, + ObservedGeneration: 2, // This is used to set processed config generation. + }, + }, + ObservedPostRenderersDigest: postrender.Digest(digest.Canonical, postRenderers).String(), + } + }, + wantDigest: postrender.Digest(digest.Canonical, postRenderers2).String(), + wantReleaseAction: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + namedNS, err := testEnv.CreateNamespace(context.TODO(), mockReleaseNamespace) + g.Expect(err).NotTo(HaveOccurred()) + t.Cleanup(func() { + _ = testEnv.Delete(context.TODO(), namedNS) + }) + releaseNamespace := namedNS.Name + + releases := tt.releases(releaseNamespace) + + obj := &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Name: mockReleaseName, + Namespace: releaseNamespace, + // Set a higher generation value to allow setting + // observations in previous generations. + Generation: 2, + }, + Spec: v2.HelmReleaseSpec{ + ReleaseName: mockReleaseName, + TargetNamespace: releaseNamespace, + StorageNamespace: releaseNamespace, + Timeout: &metav1.Duration{Duration: 100 * time.Millisecond}, + }, + } + + if tt.spec != nil { + tt.spec(&obj.Spec) + } + if tt.status != nil { + obj.Status = tt.status(releases) + } + + getter, err := RESTClientGetterFromManager(testEnv.Manager, obj.GetReleaseNamespace()) + g.Expect(err).ToNot(HaveOccurred()) + + cfg, err := action.NewConfigFactory(getter, + action.WithStorage(action.DefaultStorageDriver, obj.GetStorageNamespace()), + ) + g.Expect(err).ToNot(HaveOccurred()) + + store := helmstorage.Init(cfg.Driver) + for _, r := range releases { + g.Expect(store.Create(r)).To(Succeed()) + } + + // We use a fake client here to allow us to work with a minimal release + // object mock. As the fake client does not perform any validation. + // However, for the Helm storage driver to work, we need a real client + // which is therefore initialized separately above. + client := fake.NewClientBuilder(). + WithScheme(testEnv.Scheme()). + WithObjects(obj). + WithStatusSubresource(&v2.HelmRelease{}). + Build() + patchHelper := patch.NewSerialPatcher(obj, client) + recorder := new(record.FakeRecorder) + + req := &Request{ + Object: obj, + Chart: testutil.BuildChart(), + Values: tt.values, + } + + err = NewAtomicRelease(patchHelper, cfg, recorder, testFieldManager).Reconcile(context.TODO(), req) + g.Expect(err).ToNot(HaveOccurred()) + + g.Expect(obj.Status.ObservedPostRenderersDigest).To(Equal(tt.wantDigest)) + g.Expect(obj.Status.LastAttemptedReleaseAction).To(Equal(tt.wantReleaseAction)) + }) + } +} + func TestAtomicRelease_actionForState(t *testing.T) { tests := []struct { name string diff --git a/internal/reconcile/release.go b/internal/reconcile/release.go index 8e8564e66..269b1a2f8 100644 --- a/internal/reconcile/release.go +++ b/internal/reconcile/release.go @@ -28,8 +28,6 @@ import ( v2 "github.com/fluxcd/helm-controller/api/v2" "github.com/fluxcd/helm-controller/internal/action" - "github.com/fluxcd/helm-controller/internal/digest" - "github.com/fluxcd/helm-controller/internal/postrender" "github.com/fluxcd/helm-controller/internal/release" "github.com/fluxcd/helm-controller/internal/storage" ) @@ -190,15 +188,6 @@ func summarize(req *Request) { Message: conds[0].Message, ObservedGeneration: req.Object.Generation, }) - - // remove stale post-renderers digest - if conditions.Get(req.Object, meta.ReadyCondition).Status == metav1.ConditionTrue { - req.Object.Status.ObservedPostRenderersDigest = "" - if req.Object.Spec.PostRenderers != nil { - // Update the post-renderers digest if the post-renderers exist. - req.Object.Status.ObservedPostRenderersDigest = postrender.Digest(digest.Canonical, req.Object.Spec.PostRenderers).String() - } - } } // eventMessageWithLog returns an event message composed out of the given diff --git a/internal/reconcile/release_test.go b/internal/reconcile/release_test.go index 0cd320e78..d2cede74f 100644 --- a/internal/reconcile/release_test.go +++ b/internal/reconcile/release_test.go @@ -31,8 +31,6 @@ import ( v2 "github.com/fluxcd/helm-controller/api/v2" "github.com/fluxcd/helm-controller/internal/action" - "github.com/fluxcd/helm-controller/internal/digest" - "github.com/fluxcd/helm-controller/internal/postrender" ) const ( @@ -50,14 +48,14 @@ var ( Kind: "Deployment", Name: "test", }, - Patch: `|- - apiVersion: apps/v1 - kind: Deployment - metadata: - name: test - spec: - replicas: 2 - `, + Patch: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: test +spec: + replicas: 2 +`, }, }, }, @@ -73,14 +71,14 @@ var ( Kind: "Deployment", Name: "test", }, - Patch: `|- - apiVersion: apps/v1 - kind: Deployment - metadata: - name: test - spec: - replicas: 3 - `, + Patch: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: test +spec: + replicas: 3 +`, }, }, }, @@ -509,96 +507,6 @@ func Test_summarize(t *testing.T) { }, }, }, - { - name: "with postrender", - generation: 1, - status: v2.HelmReleaseStatus{ - Conditions: []metav1.Condition{ - { - Type: v2.ReleasedCondition, - Status: metav1.ConditionTrue, - Reason: v2.InstallSucceededReason, - Message: "Install complete", - ObservedGeneration: 1, - }, - }, - ObservedPostRenderersDigest: postrender.Digest(digest.Canonical, postRenderers).String(), - }, - spec: &v2.HelmReleaseSpec{ - PostRenderers: postRenderers2, - }, - expectedStatus: &v2.HelmReleaseStatus{ - Conditions: []metav1.Condition{ - { - Type: meta.ReadyCondition, - Status: metav1.ConditionTrue, - Reason: v2.InstallSucceededReason, - Message: "Install complete", - ObservedGeneration: 1, - }, - { - Type: v2.ReleasedCondition, - Status: metav1.ConditionTrue, - Reason: v2.InstallSucceededReason, - Message: "Install complete", - ObservedGeneration: 1, - }, - }, - ObservedPostRenderersDigest: postrender.Digest(digest.Canonical, postRenderers2).String(), - }, - }, - { - name: "with PostRenderers and Remediaction success", - generation: 1, - status: v2.HelmReleaseStatus{ - Conditions: []metav1.Condition{ - { - Type: v2.ReleasedCondition, - Status: metav1.ConditionFalse, - Reason: v2.UpgradeFailedReason, - Message: "Upgrade failure", - ObservedGeneration: 1, - }, - { - Type: v2.RemediatedCondition, - Status: metav1.ConditionTrue, - Reason: v2.RollbackSucceededReason, - Message: "Uninstall complete", - ObservedGeneration: 1, - }, - }, - ObservedPostRenderersDigest: postrender.Digest(digest.Canonical, postRenderers).String(), - }, - spec: &v2.HelmReleaseSpec{ - PostRenderers: postRenderers2, - }, - expectedStatus: &v2.HelmReleaseStatus{ - Conditions: []metav1.Condition{ - { - Type: meta.ReadyCondition, - Status: metav1.ConditionFalse, - Reason: v2.RollbackSucceededReason, - Message: "Uninstall complete", - ObservedGeneration: 1, - }, - { - Type: v2.ReleasedCondition, - Status: metav1.ConditionFalse, - Reason: v2.UpgradeFailedReason, - Message: "Upgrade failure", - ObservedGeneration: 1, - }, - { - Type: v2.RemediatedCondition, - Status: metav1.ConditionTrue, - Reason: v2.RollbackSucceededReason, - Message: "Uninstall complete", - ObservedGeneration: 1, - }, - }, - ObservedPostRenderersDigest: postrender.Digest(digest.Canonical, postRenderers).String(), - }, - }, } for _, tt := range tests { diff --git a/internal/reconcile/state.go b/internal/reconcile/state.go index be255d11c..247a752a7 100644 --- a/internal/reconcile/state.go +++ b/internal/reconcile/state.go @@ -21,6 +21,8 @@ import ( "errors" "fmt" + "github.com/fluxcd/pkg/apis/meta" + "github.com/fluxcd/pkg/runtime/conditions" "github.com/fluxcd/pkg/ssa/jsondiff" "helm.sh/helm/v3/pkg/kube" helmrelease "helm.sh/helm/v3/pkg/release" @@ -143,13 +145,22 @@ func DetermineReleaseState(ctx context.Context, cfg *action.ConfigFactory, req * } } - // Verify if postrender digest has changed - var postrenderersDigest string - if req.Object.Spec.PostRenderers != nil { - postrenderersDigest = postrender.Digest(digest.Canonical, req.Object.Spec.PostRenderers).String() - } - if postrenderersDigest != req.Object.Status.ObservedPostRenderersDigest { - return ReleaseState{Status: ReleaseStatusOutOfSync, Reason: "postrender digest has changed"}, nil + // Verify if postrender digest has changed if config has not been + // processed. For the processed or partially processed generation, the + // updated observation will only be reflected at the end of a successful + // reconciliation. Comparing here would result the reconciliation to + // get stuck in this check due to a mismatch forever. The value can't + // change without a new generation. Hence, compare the observed digest + // for new generations only. + ready := conditions.Get(req.Object, meta.ReadyCondition) + if ready != nil && ready.ObservedGeneration != req.Object.Generation { + var postrenderersDigest string + if req.Object.Spec.PostRenderers != nil { + postrenderersDigest = postrender.Digest(digest.Canonical, req.Object.Spec.PostRenderers).String() + } + if postrenderersDigest != req.Object.Status.ObservedPostRenderersDigest { + return ReleaseState{Status: ReleaseStatusOutOfSync, Reason: "postrenderers digest has changed"}, nil + } } // For the further determination of test results, we look at the diff --git a/internal/reconcile/state_test.go b/internal/reconcile/state_test.go index bbd844f94..fb51b8648 100644 --- a/internal/reconcile/state_test.go +++ b/internal/reconcile/state_test.go @@ -27,8 +27,10 @@ import ( helmrelease "helm.sh/helm/v3/pkg/release" helmstorage "helm.sh/helm/v3/pkg/storage" helmdriver "helm.sh/helm/v3/pkg/storage/driver" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "github.com/fluxcd/pkg/apis/meta" "github.com/fluxcd/pkg/ssa/jsondiff" ssanormalize "github.com/fluxcd/pkg/ssa/normalize" ssautil "github.com/fluxcd/pkg/ssa/utils" @@ -474,6 +476,13 @@ func Test_DetermineReleaseState(t *testing.T) { release.ObservedToSnapshot(release.ObserveRelease(releases[0])), }, ObservedPostRenderersDigest: postrender.Digest(digest.Canonical, postRenderers).String(), + Conditions: []metav1.Condition{ + { + Type: meta.ReadyCondition, + Status: metav1.ConditionTrue, + ObservedGeneration: 1, + }, + }, } }, chart: testutil.BuildChart(), @@ -482,6 +491,41 @@ func Test_DetermineReleaseState(t *testing.T) { Status: ReleaseStatusOutOfSync, }, }, + { + name: "postRenderers mismatch ignored for processed generation", + releases: []*helmrelease.Release{ + testutil.BuildRelease(&helmrelease.MockReleaseOptions{ + Name: mockReleaseName, + Namespace: mockReleaseNamespace, + Version: 1, + Status: helmrelease.StatusDeployed, + Chart: testutil.BuildChart(), + }, testutil.ReleaseWithConfig(map[string]interface{}{"foo": "bar"})), + }, + spec: func(spec *v2.HelmReleaseSpec) { + spec.PostRenderers = postRenderers2 + }, + status: func(releases []*helmrelease.Release) v2.HelmReleaseStatus { + return v2.HelmReleaseStatus{ + History: v2.Snapshots{ + release.ObservedToSnapshot(release.ObserveRelease(releases[0])), + }, + ObservedPostRenderersDigest: postrender.Digest(digest.Canonical, postRenderers).String(), + Conditions: []metav1.Condition{ + { + Type: meta.ReadyCondition, + Status: metav1.ConditionTrue, + ObservedGeneration: 2, + }, + }, + } + }, + chart: testutil.BuildChart(), + values: map[string]interface{}{"foo": "bar"}, + want: ReleaseState{ + Status: ReleaseStatusInSync, + }, + }, } for _, tt := range tests { @@ -495,6 +539,10 @@ func Test_DetermineReleaseState(t *testing.T) { StorageNamespace: mockReleaseNamespace, }, } + // Set a non-zero generation so that old observations can be set on + // the object status. + obj.Generation = 2 + if tt.spec != nil { tt.spec(&obj.Spec) }