Skip to content

Commit

Permalink
PostRenderersDigest observation improvements
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@darkowlzz
darkowlzz committed May 9, 2024
1 parent 461d45d commit 63f7a76
Show file tree
Hide file tree
Showing 6 changed files with 318 additions and 126 deletions.
11 changes: 11 additions & 0 deletions internal/reconcile/atomic_release.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
}

Expand Down
225 changes: 225 additions & 0 deletions internal/reconcile/atomic_release_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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
Expand Down
11 changes: 0 additions & 11 deletions internal/reconcile/release.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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
Expand Down
Loading

0 comments on commit 63f7a76

Please sign in to comment.