release: rely on Helm storage for rollbacks

This commit removes our own state keeping around rollbacks except
for the condition we set marking a rollback has happened, but
instead relies on comparison of a dry-run with the latest failed
release. If this results in a diff, a new upgrade will be attempted.

The reason we move to this approach is that it is not possible to
take all variables safely into account and determine if we should
perform a new upgrade attempt based on our own bookkeeping, with
a long list of edge-case bugs as a result.

Relying on the real truth is always better than a reflection of it,
which in the end may not be the actual truth at all.

pkg/apis/helm.fluxcd.io/v1/types.go

@@ -260,20 +260,11 @@ type HelmReleaseStatus struct {
 	// the controller.
 	ObservedGeneration int64 `json:"observedGeneration"`
 
-	// ValuesChecksum holds the SHA256 checksum of the last applied
-	// values.
-	ValuesChecksum string `json:"valuesChecksum"`
-
 	// Revision would define what Git hash or Chart version has currently
 	// been deployed.
 	// +optional
 	Revision string `json:"revision,omitempty"`
 
-	// PrevRevision would define what Git hash or Chart version had previously
-	// been deployed.
-	// +optional
-	PrevRevision string `json:"prevRevision,omitempty"`
-
 	// Conditions contains observations of the resource's state, e.g.,
 	// has the chart which it refers to been fetched.
 	// +optional

pkg/helm/release.go

@@ -44,11 +44,11 @@ type Info struct {
 
 // Chart describes the chart for a release
 type Chart struct {
-	Name         string
-	Version      string
-	AppVersion   string
-	Values       Values
-	Templates    []*File
+	Name       string
+	Version    string
+	AppVersion string
+	Values     Values
+	Templates  []*File
 }
 
 // File represents a file as a name/value pair.

pkg/helm/v2/history.go

@@ -4,24 +4,22 @@ import (
 	"github.com/pkg/errors"
 
 	helmv2 "k8s.io/helm/pkg/helm"
-	"k8s.io/helm/pkg/proto/hapi/release"
 
 	"github.com/fluxcd/helm-operator/pkg/helm"
 )
 
 func (h *HelmV2) History(releaseName string, opts helm.HistoryOptions) ([]*helm.Release, error) {
-	res, err := h.client.ReleaseHistory(releaseName, helmv2.WithMaxHistory(int32(opts.Max)))
+	max := helmv2.WithMaxHistory(256)
+	if opts.Max != 0 {
+		max = helmv2.WithMaxHistory(int32(opts.Max))
+	}
+	res, err := h.client.ReleaseHistory(releaseName, max)
 	if err != nil {
 		return nil, errors.Wrapf(statusMessageErr(err), "failed to retrieve history for [%s]", releaseName)
 	}
-	return getReleaseHistory(res.Releases), nil
-}
-
-func getReleaseHistory(rls []*release.Release) []*helm.Release {
-	history := make([]*helm.Release, len(rls))
-	for i := len(rls) - 1; i >= 0; i-- {
-		r := rls[i]
-		history = append(history, releaseToGenericRelease(r))
+	var rels []*helm.Release
+	for _, r := range res.Releases {
+		rels = append(rels, releaseToGenericRelease(r))
 	}
-	return history
+	return rels, nil
 }

pkg/helm/v2/release.go

@@ -38,11 +38,11 @@ func chartToGenericChart(c *chart.Chart) *helm.Chart {
 	}
 
 	return &helm.Chart{
-		Name:         c.Metadata.Name,
-		Version:      c.Metadata.Version,
-		AppVersion:   c.Metadata.AppVersion,
-		Values:       valuesToGenericValues(c.Values),
-		Templates:    templatesToGenericFiles(c.Templates),
+		Name:       c.Metadata.Name,
+		Version:    c.Metadata.Version,
+		AppVersion: c.Metadata.AppVersion,
+		Values:     valuesToGenericValues(c.Values),
+		Templates:  templatesToGenericFiles(c.Templates),
 	}
 }
 

pkg/helm/v3/history.go

@@ -4,7 +4,6 @@ import (
 	"github.com/pkg/errors"
 
 	"helm.sh/helm/v3/pkg/action"
-	"helm.sh/helm/v3/pkg/release"
 	"helm.sh/helm/v3/pkg/releaseutil"
 
 	"github.com/fluxcd/helm-operator/pkg/helm"
@@ -28,21 +27,11 @@ func (h *HelmV3) History(releaseName string, opts helm.HistoryOptions) ([]*helm.
 
 	releaseutil.Reverse(hist, releaseutil.SortByRevision)
 
-	var rels []*release.Release
+	var rels []*helm.Release
 	for i := 0; i < min(len(hist), client.Max); i++ {
-		rels = append(rels, hist[i])
+		rels = append(rels, releaseToGenericRelease(hist[i]))
 	}
-
-	return getReleaseHistory(hist), nil
-}
-
-func getReleaseHistory(rls []*release.Release) []*helm.Release {
-	history := make([]*helm.Release, len(rls))
-	for i := len(rls) - 1; i >= 0; i-- {
-		r := rls[i]
-		history = append(history, releaseToGenericRelease(r))
-	}
-	return history
+	return rels, nil
 }
 
 func min(x, y int) int {

pkg/helm/v3/release.go

@@ -30,11 +30,11 @@ func releaseToGenericRelease(r *release.Release) *helm.Release {
 // a generic `helm.Chart`
 func chartToGenericChart(c *chart.Chart) *helm.Chart {
 	return &helm.Chart{
-		Name:         c.Name(),
-		Version:      formatVersion(c),
-		AppVersion:   c.AppVersion(),
-		Values:       c.Values,
-		Templates:    filesToGenericFiles(c.Templates),
+		Name:       c.Name(),
+		Version:    formatVersion(c),
+		AppVersion: c.AppVersion(),
+		Values:     c.Values,
+		Templates:  filesToGenericFiles(c.Templates),
 	}
 }
 

pkg/release/release.go

@@ -177,7 +177,6 @@ func (r *Release) Sync(client helm.Client, hr *v1.HelmRelease) (rHr *v1.HelmRele
 		logger.Log("error", ErrComposingValues.Error(), "err", err.Error())
 		return hr, ErrComposingValues
 	}
-	defer status.SetValuesChecksum(r.helmReleaseClient.HelmReleases(hr.Namespace), hr, composedValues.Checksum())
 
 	if ok, err := shouldSync(logger, client, hr, curRel, chartPath, revision, composedValues, r.config.LogDiffs); !ok {
 		if err != nil {
@@ -256,9 +255,12 @@ func (r *Release) Sync(client helm.Client, hr *v1.HelmRelease) (rHr *v1.HelmRele
 	if performRollback {
 		logger.Log("info", "rolling back failed Helm release")
 		rel, err = client.Rollback(hr.GetReleaseName(), helm.RollbackOptions{
-			Namespace: hr.GetTargetNamespace(),
-			Timeout:   hr.GetTimeout(),
-			Force:     hr.Spec.ForceUpgrade,
+			Namespace:    hr.GetTargetNamespace(),
+			Timeout:      hr.Spec.Rollback.GetTimeout(),
+			Wait:         hr.Spec.Rollback.Wait,
+			DisableHooks: hr.Spec.Rollback.DisableHooks,
+			Recreate:     hr.Spec.Rollback.Recreate,
+			Force:        hr.Spec.Rollback.Force,
 		})
 		if err != nil {
 			_ = status.SetCondition(r.helmReleaseClient.HelmReleases(hr.Namespace), hr, status.NewCondition(
@@ -268,7 +270,6 @@ func (r *Release) Sync(client helm.Client, hr *v1.HelmRelease) (rHr *v1.HelmRele
 		}
 		_ = status.SetCondition(r.helmReleaseClient.HelmReleases(hr.Namespace), hr, status.NewCondition(
 			v1.HelmReleaseRolledBack, corev1.ConditionTrue, ReasonSuccess, "Helm rollback succeeded"))
-		status.SetPrevReleaseRevision(r.helmReleaseClient.HelmReleases(hr.Namespace), hr, revision)
 		logger.Log("info", "Helm rollback succeeded")
 
 		// We should still report failure.
@@ -299,82 +300,92 @@ func (r *Release) Uninstall(client helm.Client, hr *v1.HelmRelease) {
 
 // shouldSync determines if the given `v1.HelmRelease` should be synced
 // with Helm. The cheapest checks which do not require a dry-run are
-// consulted first (e.g. is this our first sync, has the release been
-// rolled back, have we already seen this revision of the resource);
-// before running the dry-run release to determine if any undefined
-// mutations have occurred.
+// consulted first (e.g. is this our first sync, have we already seen
+// this revision of the resource); before running the dry-run release to
+// determine if any undefined mutations have occurred.
 func shouldSync(logger log.Logger, client helm.Client, hr *v1.HelmRelease, curRel *helm.Release,
 	chartPath, revision string, values helm.Values, logDiffs bool) (bool, error) {
 
+	// Without valid YAML we will not get anywhere, return early.
+	b, err := values.YAML()
+	if err != nil {
+		return false, ErrComposingValues
+	}
+
+	// If there is no existing release, we should simply sync.
 	if curRel == nil {
 		logger.Log("info", "no existing release", "action", "install")
-		// If there is no existing release, we should simply sync.
 		return true, nil
 	}
 
+	// If the release is not managed by our resource, we skip to avoid conflicts.
 	if ok, resourceID := managedByHelmRelease(curRel, *hr); !ok {
 		logger.Log("warning", "release appears to be managed by "+resourceID, "action", "skip")
 		return false, nil
 	}
 
+	// If the current state of the release does not allow us to safely upgrade, we skip.
 	if s := curRel.Info.Status; !s.AllowsUpgrade() {
 		logger.Log("warning", "unable to sync release with status "+s.String(), "action", "skip")
 		return false, nil
 	}
 
-	if status.HasRolledBack(*hr, revision) {
-		if hr.Status.ValuesChecksum != values.Checksum() {
-			// The release has been rolled back but the values have
-			// changed. We should attempt a new sync to see if the
-			// change resolved the issue that triggered the rollback.
-			logger.Log("info", "values appear to have changed since rollback", "action", "upgrade")
-			return true, nil
-		}
-		logger.Log("warning", "release has been rolled back", "action", "skip")
-		return false, nil
-	}
-
+	// If we have not processed this generation of the release, we should sync.
 	if !status.HasSynced(*hr) {
 		logger.Log("info", "release has not yet been processed", "action", "upgrade")
-
-		// The generation of this `v1.HelmRelease` has not been
-		// processed, we should simply sync.
 		return true, nil
 	}
 
-	b, err := values.YAML()
-	if err != nil {
-		// Without valid YAML values we are unable to sync.
-		return false, ErrComposingValues
-	}
-
+	// Next, we perform a dry-run upgrade and compare the result against the
+	// latest release _or_ the latest failed release in case of a rollback.
+	// If this results in one or more diffs we should sync.
 	logger.Log("info", "performing dry-run upgrade to see if release has diverged")
-
-	// Perform a dry-run upgrade so that we can compare what we ought
-	// to be running matches what is defined in the `v1.HelmRelease`.
 	desRel, err := client.UpgradeFromPath(chartPath, hr.GetReleaseName(), b, helm.UpgradeOptions{DryRun: true})
 	if err != nil {
 		return false, err
 	}
 
-	curValues, desValues := curRel.Values, desRel.Values
-	curChart, desChart := curRel.Chart, desRel.Chart
+	var vDiff, cDiff string
+	switch {
+	case status.HasRolledBack(*hr):
+		logger.Log("info", "release has been rolled back, comparing dry-run output with latest failed release")
+		rels, err := client.History(hr.GetReleaseName(), helm.HistoryOptions{Namespace: hr.GetTargetNamespace()})
+		if err != nil {
+			return false, err
+		}
+		for _, r := range rels {
+			if r.Info.Status == helm.StatusFailed {
+				vDiff, cDiff = compareRelease(r, desRel)
+				break
+			}
+		}
+	default:
+		vDiff, cDiff = compareRelease(curRel, desRel)
+	}
 
-	// Compare values to detect mutations.
-	vDiff := cmp.Diff(curValues, desValues)
 	if vDiff != "" && logDiffs {
 		logger.Log("info", "values have diverged", "diff", vDiff)
 	}
 
-	// Compare chart to detect mutations.
-	cDiff := cmp.Diff(curChart, desChart)
 	if cDiff != "" && logDiffs {
 		logger.Log("info", "chart has diverged", "diff", cDiff)
 	}
 
+	if cDiff != "" || vDiff != "" {
+		logger.Log("info", "dry-run differed", "action", "upgrade")
+	} else {
+		logger.Log("info", "no changes", "action", "skip")
+	}
+
 	return vDiff != "" || cDiff != "", nil
 }
 
+// compareRelease compares the values and charts of the two given
+// releases and returns the diff sets.
+func compareRelease(j *helm.Release, k *helm.Release) (string, string) {
+	return cmp.Diff(j.Values, k.Values), cmp.Diff(j.Chart, k.Chart)
+}
+
 // releaseLogger returns a logger in the context of the given
 // HelmRelease (that being, with metadata included).
 func releaseLogger(logger log.Logger, client helm.Client, hr *v1.HelmRelease) log.Logger {

pkg/status/status.go

@@ -136,7 +136,6 @@ func SetReleaseRevision(client v1client.HelmReleaseInterface, hr *helmfluxv1.Hel
 		}
 
 		cHr := hr.DeepCopy()
-		cHr.Status.PrevRevision = cHr.Status.Revision
 		cHr.Status.Revision = revision
 
 		_, err = client.UpdateStatus(cHr)
@@ -146,63 +145,6 @@ func SetReleaseRevision(client v1client.HelmReleaseInterface, hr *helmfluxv1.Hel
 	return err
 }
 
-// SetReleaseRevision updates the previous revision in the status of the
-// HelmRelease to the given revision, its main purpose is to be able to
-// record the revision of a failed release.
-func SetPrevReleaseRevision(client v1client.HelmReleaseInterface, hr *helmfluxv1.HelmRelease, revision string) error {
-
-	firstTry := true
-	err := retry.RetryOnConflict(retry.DefaultBackoff, func() (err error) {
-		if !firstTry {
-			var getErr error
-			hr, getErr = client.Get(hr.Name, metav1.GetOptions{})
-			if getErr != nil {
-				return getErr
-			}
-		}
-
-		if revision == "" || hr.Status.PrevRevision == revision {
-			return
-		}
-
-		cHr := hr.DeepCopy()
-		cHr.Status.PrevRevision = revision
-
-		_, err = client.UpdateStatus(cHr)
-		firstTry = false
-		return
-	})
-	return err
-}
-
-// SetValuesChecksum updates the values checksum of the HelmRelease to
-// the given checksum.
-func SetValuesChecksum(client v1client.HelmReleaseInterface, hr *helmfluxv1.HelmRelease, valuesChecksum string) error {
-
-	firstTry := true
-	err := retry.RetryOnConflict(retry.DefaultBackoff, func() (err error) {
-		if !firstTry {
-			var getErr error
-			hr, getErr = client.Get(hr.Name, metav1.GetOptions{})
-			if getErr != nil {
-				return getErr
-			}
-		}
-
-		if valuesChecksum == "" || hr.Status.ValuesChecksum == valuesChecksum {
-			return
-		}
-
-		cHr := hr.DeepCopy()
-		cHr.Status.ValuesChecksum = valuesChecksum
-
-		_, err = client.UpdateStatus(cHr)
-		firstTry = false
-		return
-	})
-	return err
-}
-
 // SetObservedGeneration updates the observed generation status of the
 // HelmRelease to the given generation.
 func SetObservedGeneration(client v1client.HelmReleaseInterface, hr *helmfluxv1.HelmRelease, generation int64) error {
@@ -238,7 +180,7 @@ func HasSynced(hr helmfluxv1.HelmRelease) bool {
 
 // HasRolledBack returns if the current generation of the HelmRelease
 // has been rolled back.
-func HasRolledBack(hr helmfluxv1.HelmRelease, revision string) bool {
+func HasRolledBack(hr helmfluxv1.HelmRelease) bool {
 	if !HasSynced(hr) {
 		return false
 	}
@@ -248,16 +190,5 @@ func HasRolledBack(hr helmfluxv1.HelmRelease, revision string) bool {
 		return false
 	}
 
-	chartFetched := GetCondition(hr.Status, helmfluxv1.HelmReleaseChartFetched)
-	if chartFetched != nil {
-		// NB: as two successful state updates can happen right after
-		// each other, on which we both want to act, we _must_ compare
-		// the update timestamps as the transition timestamp will only
-		// change on a status shift.
-		if chartFetched.Status == v1.ConditionTrue && rolledBack.LastUpdateTime.Before(&chartFetched.LastUpdateTime) {
-			return hr.Status.PrevRevision == revision
-		}
-	}
-
 	return rolledBack.Status == v1.ConditionTrue
 }