release: rely on Helm storage for rollbacks

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
 }