Skip to content

Commit

Permalink
reconcile: improve uninstall w/o purging history
Browse files Browse the repository at this point in the history 
This improves the reconciliation of an uninstall when the release has
already been uninstalled while `KeepHistory` has been set, by detecting
the (sadly non-typed) error Helm produces as desired state.

Avoiding certain edge-cases where for example a deleted HelmRelease
would end up in an irrecoverable loop of uninstall attempts, after
being remediated (using an uninstall) before the deletion request.

Signed-off-by: Hidde Beydals <[email protected]>
  • Loading branch information
@hiddeco
hiddeco committed Nov 1, 2023
1 parent 9a33863 commit 3f9ed3b
Show file tree
Hide file tree
Showing 2 changed files with 98 additions and 9 deletions.
19 changes: 18 additions & 1 deletion internal/reconcile/uninstall.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,18 @@ func (r *Uninstall) Reconcile(ctx context.Context, req *Request) error {
return nil
}

// When the release is already uninstalled and the user requested to keep
// the history, we can assume the release is uninstalled while taking note
// that we did not do it.
// This can happen when the release was uninstalled as part of a
// remediation, with a subsequent uninstall request due to the object
// being deleted.
if err != nil && req.Object.GetUninstall().KeepHistory && strings.Contains(err.Error(), "is already deleted") {
conditions.MarkFalse(req.Object, v2.ReleasedCondition, v2.UninstallSucceededReason,
"Release %s was already uninstalled", cur.FullReleaseName())
return nil
}

// The Helm uninstall action does always target the latest release. Before
// accepting results, we need to confirm this is actually the release we
// have recorded as Current.
Expand All @@ -115,7 +127,12 @@ func (r *Uninstall) Reconcile(ctx context.Context, req *Request) error {
// The Helm uninstall action may return without an error while the update
// to the storage failed. Detect this and return an error.
if err == nil && cur.Digest == req.Object.GetCurrent().Digest {
err = fmt.Errorf("uninstall completed with error: %w", ErrNoStorageUpdate)
// An exception is made for the case where the release was already marked
// as uninstalled, which would only result in the release object getting
// removed from the storage.
if s := helmrelease.Status(cur.Status); s != helmrelease.StatusUninstalled {
err = fmt.Errorf("uninstall completed with error: %w", ErrNoStorageUpdate)
}
}

// Handle any error.
Expand Down
88 changes: 80 additions & 8 deletions internal/reconcile/uninstall_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func TestUninstall_Reconcile(t *testing.T) {
expectConditions []metav1.Condition
// expectCurrent is the expected Current release information in the
// HelmRelease after uninstall.
expectCurrent func(releases []*helmrelease.Release) *v2.Snapshot
expectCurrent func(namespace string, releases []*helmrelease.Release) *v2.Snapshot
// expectPrevious returns the expected Previous release information of
// the HelmRelease after uninstall.
expectPrevious func(releases []*helmrelease.Release) *v2.Snapshot
Expand Down Expand Up @@ -110,7 +110,7 @@ func TestUninstall_Reconcile(t *testing.T) {
*conditions.FalseCondition(v2.ReleasedCondition, v2.UninstallSucceededReason,
"succeeded"),
},
expectCurrent: func(releases []*helmrelease.Release) *v2.Snapshot {
expectCurrent: func(namespace string, releases []*helmrelease.Release) *v2.Snapshot {
return release.ObservedToSnapshot(release.ObserveRelease(releases[0]))
},
},
Expand Down Expand Up @@ -145,7 +145,7 @@ func TestUninstall_Reconcile(t *testing.T) {
*conditions.FalseCondition(v2.ReleasedCondition, v2.UninstallFailedReason,
"uninstallation completed with 1 error(s): 1 error occurred:\n\t* timed out waiting for the condition"),
},
expectCurrent: func(releases []*helmrelease.Release) *v2.Snapshot {
expectCurrent: func(namespace string, releases []*helmrelease.Release) *v2.Snapshot {
return release.ObservedToSnapshot(release.ObserveRelease(releases[0]))
},
expectFailures: 1,
Expand Down Expand Up @@ -192,7 +192,7 @@ func TestUninstall_Reconcile(t *testing.T) {
*conditions.FalseCondition(v2.ReleasedCondition, v2.UninstallFailedReason,
ErrNoStorageUpdate.Error()),
},
expectCurrent: func(releases []*helmrelease.Release) *v2.Snapshot {
expectCurrent: func(namespace string, releases []*helmrelease.Release) *v2.Snapshot {
return release.ObservedToSnapshot(release.ObserveRelease(releases[0]))
},
expectFailures: 1,
Expand Down Expand Up @@ -235,7 +235,7 @@ func TestUninstall_Reconcile(t *testing.T) {
*conditions.FalseCondition(v2.ReleasedCondition, v2.UninstallFailedReason,
"delete error"),
},
expectCurrent: func(releases []*helmrelease.Release) *v2.Snapshot {
expectCurrent: func(namespace string, releases []*helmrelease.Release) *v2.Snapshot {
return release.ObservedToSnapshot(release.ObserveRelease(releases[0]))
},
expectFailures: 1,
Expand Down Expand Up @@ -294,7 +294,7 @@ func TestUninstall_Reconcile(t *testing.T) {
*conditions.FalseCondition(v2.ReleasedCondition, v2.UninstallFailedReason,
ErrReleaseMismatch.Error()),
},
expectCurrent: func(releases []*helmrelease.Release) *v2.Snapshot {
expectCurrent: func(namespace string, releases []*helmrelease.Release) *v2.Snapshot {
return release.ObservedToSnapshot(release.ObserveRelease(releases[0]))
},
expectFailures: 1,
Expand Down Expand Up @@ -337,7 +337,79 @@ func TestUninstall_Reconcile(t *testing.T) {
*conditions.FalseCondition(v2.ReleasedCondition, v2.UninstallSucceededReason,
"assuming it is uninstalled"),
},
expectCurrent: func(releases []*helmrelease.Release) *v2.Snapshot {
expectCurrent: func(namespace string, releases []*helmrelease.Release) *v2.Snapshot {
return release.ObservedToSnapshot(release.ObserveRelease(releases[0]))
},
},
{
name: "already uninstalled without keep history",
releases: func(namespace string) []*helmrelease.Release {
return []*helmrelease.Release{
testutil.BuildRelease(&helmrelease.MockReleaseOptions{
Name: mockReleaseName,
Namespace: namespace,
Version: 1,
Chart: testutil.BuildChart(testutil.ChartWithTestHook()),
Status: helmrelease.StatusUninstalled,
}),
}
},
status: func(releases []*helmrelease.Release) v2.HelmReleaseStatus {
return v2.HelmReleaseStatus{
History: v2.ReleaseHistory{
Current: release.ObservedToSnapshot(release.ObserveRelease(releases[0])),
},
}
},
expectConditions: []metav1.Condition{
*conditions.FalseCondition(meta.ReadyCondition, v2.UninstallSucceededReason,
"succeeded"),
*conditions.FalseCondition(v2.ReleasedCondition, v2.UninstallSucceededReason,
"succeeded"),
},
expectCurrent: func(namespace string, releases []*helmrelease.Release) *v2.Snapshot {
rls := testutil.BuildRelease(&helmrelease.MockReleaseOptions{
Name: mockReleaseName,
Namespace: namespace,
Version: 1,
Chart: testutil.BuildChart(testutil.ChartWithTestHook()),
Status: helmrelease.StatusUninstalled,
})
return release.ObservedToSnapshot(release.ObserveRelease(rls))
},
},
{
name: "already uninstalled with keep history",
releases: func(namespace string) []*helmrelease.Release {
return []*helmrelease.Release{
testutil.BuildRelease(&helmrelease.MockReleaseOptions{
Name: mockReleaseName,
Namespace: namespace,
Version: 1,
Chart: testutil.BuildChart(testutil.ChartWithTestHook()),
Status: helmrelease.StatusUninstalled,
}),
}
},
spec: func(spec *v2.HelmReleaseSpec) {
spec.Uninstall = &v2.Uninstall{
KeepHistory: true,
}
},
status: func(releases []*helmrelease.Release) v2.HelmReleaseStatus {
return v2.HelmReleaseStatus{
History: v2.ReleaseHistory{
Current: release.ObservedToSnapshot(release.ObserveRelease(releases[0])),
},
}
},
expectConditions: []metav1.Condition{
*conditions.FalseCondition(meta.ReadyCondition, v2.UninstallSucceededReason,
"was already uninstalled"),
*conditions.FalseCondition(v2.ReleasedCondition, v2.UninstallSucceededReason,
"was already uninstalled"),
},
expectCurrent: func(namespace string, releases []*helmrelease.Release) *v2.Snapshot {
return release.ObservedToSnapshot(release.ObserveRelease(releases[0]))
},
},
Expand Down Expand Up @@ -407,7 +479,7 @@ func TestUninstall_Reconcile(t *testing.T) {
releaseutil.SortByRevision(releases)

if tt.expectCurrent != nil {
g.Expect(obj.GetCurrent()).To(testutil.Equal(tt.expectCurrent(releases)))
g.Expect(obj.GetCurrent()).To(testutil.Equal(tt.expectCurrent(releaseNamespace, releases)))
} else {
g.Expect(obj.GetCurrent()).To(BeNil(), "expected current to be nil")
}
Expand Down

0 comments on commit 3f9ed3b

Please sign in to comment.