From 1911870958098b774973c6fe56bfdf4441f61596 Mon Sep 17 00:00:00 2001 From: Matthew Morrissette Date: Fri, 17 Apr 2020 10:56:29 -0700 Subject: [PATCH] fix(helm): allow a previously failed release to be upgraded (#7653) Signed-off-by: Matt Morrissette --- ...e-with-bad-or-missing-existing-release.txt | 1 + cmd/helm/upgrade_test.go | 23 +++++++++++++ pkg/action/upgrade.go | 32 ++++++++++++++----- pkg/storage/driver/driver.go | 25 ++++++++++++++- pkg/storage/storage.go | 4 +-- 5 files changed, 74 insertions(+), 11 deletions(-) create mode 100644 cmd/helm/testdata/output/upgrade-with-bad-or-missing-existing-release.txt diff --git a/cmd/helm/testdata/output/upgrade-with-bad-or-missing-existing-release.txt b/cmd/helm/testdata/output/upgrade-with-bad-or-missing-existing-release.txt new file mode 100644 index 000000000..8f24574a6 --- /dev/null +++ b/cmd/helm/testdata/output/upgrade-with-bad-or-missing-existing-release.txt @@ -0,0 +1 @@ +Error: UPGRADE FAILED: "funny-bunny" has no deployed releases diff --git a/cmd/helm/upgrade_test.go b/cmd/helm/upgrade_test.go index 3a6d75adc..6f260ae57 100644 --- a/cmd/helm/upgrade_test.go +++ b/cmd/helm/upgrade_test.go @@ -80,6 +80,10 @@ func TestUpgradeCmd(t *testing.T) { missingDepsPath := "testdata/testcharts/chart-missing-deps" badDepsPath := "testdata/testcharts/chart-bad-requirements" + relWithStatusMock := func(n string, v int, ch *chart.Chart, status release.Status) *release.Release { + return release.Mock(&release.MockReleaseOptions{Name: n, Version: v, Chart: ch, Status: status}) + } + relMock := func(n string, v int, ch *chart.Chart) *release.Release { return release.Mock(&release.MockReleaseOptions{Name: n, Version: v, Chart: ch}) } @@ -139,6 +143,25 @@ func TestUpgradeCmd(t *testing.T) { golden: "output/upgrade-with-bad-dependencies.txt", wantError: true, }, + { + name: "upgrade a non-existent release", + cmd: fmt.Sprintf("upgrade funny-bunny '%s'", chartPath), + golden: "output/upgrade-with-bad-or-missing-existing-release.txt", + wantError: true, + }, + { + name: "upgrade a failed release", + cmd: fmt.Sprintf("upgrade funny-bunny '%s'", chartPath), + golden: "output/upgrade.txt", + rels: []*release.Release{relWithStatusMock("funny-bunny", 2, ch, release.StatusFailed)}, + }, + { + name: "upgrade a pending install release", + cmd: fmt.Sprintf("upgrade funny-bunny '%s'", chartPath), + golden: "output/upgrade-with-bad-or-missing-existing-release.txt", + wantError: true, + rels: []*release.Release{relWithStatusMock("funny-bunny", 2, ch, release.StatusPendingInstall)}, + }, } runTestCmd(t, tests) } diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go index 7565da5a9..67872aa2f 100644 --- a/pkg/action/upgrade.go +++ b/pkg/action/upgrade.go @@ -33,6 +33,7 @@ import ( "helm.sh/helm/v3/pkg/postrender" "helm.sh/helm/v3/pkg/release" "helm.sh/helm/v3/pkg/releaseutil" + "helm.sh/helm/v3/pkg/storage/driver" ) // Upgrade is the action for upgrading releases. @@ -159,12 +160,33 @@ func (u *Upgrade) prepareUpgrade(name string, chart *chart.Chart, vals map[strin return nil, nil, errMissingChart } - // finds the deployed release with the given name - currentRelease, err := u.cfg.Releases.Deployed(name) + // finds the last non-deleted release with the given name + lastRelease, err := u.cfg.Releases.Last(name) if err != nil { + // to keep existing behavior of returning the "%q has no deployed releases" error when an existing release does not exist + if errors.Is(err, driver.ErrReleaseNotFound) { + return nil, nil, driver.NewErrNoDeployedReleases(name) + } return nil, nil, err } + var currentRelease *release.Release + if lastRelease.Info.Status == release.StatusDeployed { + // no need to retrieve the last deployed release from storage as the last release is deployed + currentRelease = lastRelease + } else { + // finds the deployed release with the given name + currentRelease, err = u.cfg.Releases.Deployed(name) + if err != nil { + if errors.Is(err, driver.ErrNoDeployedReleases) && + (lastRelease.Info.Status == release.StatusFailed || lastRelease.Info.Status == release.StatusSuperseded) { + currentRelease = lastRelease + } else { + return nil, nil, err + } + } + } + // determine if values will be reused vals, err = u.reuseValues(chart, currentRelease, vals) if err != nil { @@ -175,12 +197,6 @@ func (u *Upgrade) prepareUpgrade(name string, chart *chart.Chart, vals map[strin return nil, nil, err } - // finds the non-deleted release with the given name - lastRelease, err := u.cfg.Releases.Last(name) - if err != nil { - return nil, nil, err - } - // Increment revision count. This is passed to templates, and also stored on // the release object. revision := lastRelease.Version + 1 diff --git a/pkg/storage/driver/driver.go b/pkg/storage/driver/driver.go index 9a1fbc579..9c01f3766 100644 --- a/pkg/storage/driver/driver.go +++ b/pkg/storage/driver/driver.go @@ -17,6 +17,8 @@ limitations under the License. package driver // import "helm.sh/helm/v3/pkg/storage/driver" import ( + "fmt" + "github.com/pkg/errors" rspb "helm.sh/helm/v3/pkg/release" @@ -28,9 +30,30 @@ var ( // ErrReleaseExists indicates that a release already exists. ErrReleaseExists = errors.New("release: already exists") // ErrInvalidKey indicates that a release key could not be parsed. - ErrInvalidKey = errors.Errorf("release: invalid key") + ErrInvalidKey = errors.New("release: invalid key") + // ErrNoDeployedReleases indicates that there are no releases with the given key in the deployed state + ErrNoDeployedReleases = errors.New("has no deployed releases") ) +// StorageDriverError records an error and the release name that caused it +type StorageDriverError struct { + ReleaseName string + Err error +} + +func (e *StorageDriverError) Error() string { + return fmt.Sprintf("%q %s", e.ReleaseName, e.Err.Error()) +} + +func (e *StorageDriverError) Unwrap() error { return e.Err } + +func NewErrNoDeployedReleases(releaseName string) error { + return &StorageDriverError{ + ReleaseName: releaseName, + Err: ErrNoDeployedReleases, + } +} + // Creator is the interface that wraps the Create method. // // Create stores the release or returns ErrReleaseExists diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index 3e62ae9ee..c195120cd 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -116,7 +116,7 @@ func (s *Storage) Deployed(name string) (*rspb.Release, error) { } if len(ls) == 0 { - return nil, errors.Errorf("%q has no deployed releases", name) + return nil, driver.NewErrNoDeployedReleases(name) } // If executed concurrently, Helm's database gets corrupted @@ -140,7 +140,7 @@ func (s *Storage) DeployedAll(name string) ([]*rspb.Release, error) { return ls, nil } if strings.Contains(err.Error(), "not found") { - return nil, errors.Errorf("%q has no deployed releases", name) + return nil, driver.NewErrNoDeployedReleases(name) } return nil, err }