From 976cc05af55ff0acdc4331ace5644fbba69e0f98 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Wed, 22 Jan 2020 17:36:01 +0100 Subject: [PATCH] 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 | 9 --- pkg/helm/release.go | 10 ++-- pkg/helm/v2/history.go | 20 +++---- pkg/helm/v2/release.go | 10 ++-- pkg/helm/v3/history.go | 17 +----- pkg/helm/v3/release.go | 10 ++-- pkg/release/release.go | 91 ++++++++++++++++------------- pkg/status/status.go | 71 +--------------------- 8 files changed, 79 insertions(+), 159 deletions(-) diff --git a/pkg/apis/helm.fluxcd.io/v1/types.go b/pkg/apis/helm.fluxcd.io/v1/types.go index 803a49f0f..e3d967e65 100644 --- a/pkg/apis/helm.fluxcd.io/v1/types.go +++ b/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 diff --git a/pkg/helm/release.go b/pkg/helm/release.go index a1211ce5e..7f9471f44 100644 --- a/pkg/helm/release.go +++ b/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. diff --git a/pkg/helm/v2/history.go b/pkg/helm/v2/history.go index ab18ce998..399a2930e 100644 --- a/pkg/helm/v2/history.go +++ b/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 } diff --git a/pkg/helm/v2/release.go b/pkg/helm/v2/release.go index cdf30e0e0..74aa29dde 100644 --- a/pkg/helm/v2/release.go +++ b/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), } } diff --git a/pkg/helm/v3/history.go b/pkg/helm/v3/history.go index 203607041..60536b595 100644 --- a/pkg/helm/v3/history.go +++ b/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 { diff --git a/pkg/helm/v3/release.go b/pkg/helm/v3/release.go index 9e6ea0f2b..276b2807b 100644 --- a/pkg/helm/v3/release.go +++ b/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), } } diff --git a/pkg/release/release.go b/pkg/release/release.go index 765d182bc..36560fa03 100644 --- a/pkg/release/release.go +++ b/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 { diff --git a/pkg/status/status.go b/pkg/status/status.go index f264db714..713082d01 100644 --- a/pkg/status/status.go +++ b/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 }