From 5f1a21bc321b326f3fab87b71bdc12bcd7125441 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johnny=20Bergstr=C3=B6m?= Date: Tue, 27 Feb 2018 19:25:40 +0100 Subject: [PATCH] fix(tiller): Supersede multiple deployments (#3539) * add test for rolling back from a FAILED deployment * Update naming of release variables Use same naming as the rest of the file. * Update rollback test - Add logging - Verify other release names not changed * fix(tiller): Supersede multiple deployments There are cases when multiple revisions of a release has been marked with DEPLOYED status. This makes sure any previous deployment will be set to SUPERSEDED when doing rollbacks. Closes #2941 #3513 #3275 --- pkg/storage/storage.go | 24 +++++++++++---- pkg/tiller/release_rollback.go | 46 +++++++++++++++++------------ pkg/tiller/release_rollback_test.go | 31 +++++++++++++++++-- pkg/tiller/release_server.go | 1 + 4 files changed, 76 insertions(+), 26 deletions(-) diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index 4ac5dbf4f..f76eb09f4 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -98,7 +98,7 @@ func (s *Storage) ListDeployed() ([]*rspb.Release, error) { }) } -// ListFilterAll returns the set of releases satisfying satisfying the predicate +// ListFilterAll returns the set of releases satisfying the predicate // (filter0 && filter1 && ... && filterN), i.e. a Release is included in the results // if and only if all filters return true. func (s *Storage) ListFilterAll(fns ...relutil.FilterFunc) ([]*rspb.Release, error) { @@ -108,7 +108,7 @@ func (s *Storage) ListFilterAll(fns ...relutil.FilterFunc) ([]*rspb.Release, err }) } -// ListFilterAny returns the set of releases satisfying satisfying the predicate +// ListFilterAny returns the set of releases satisfying the predicate // (filter0 || filter1 || ... || filterN), i.e. a Release is included in the results // if at least one of the filters returns true. func (s *Storage) ListFilterAny(fns ...relutil.FilterFunc) ([]*rspb.Release, error) { @@ -118,10 +118,24 @@ func (s *Storage) ListFilterAny(fns ...relutil.FilterFunc) ([]*rspb.Release, err }) } -// Deployed returns the deployed release with the provided release name, or +// Deployed returns the last deployed release with the provided release name, or // returns ErrReleaseNotFound if not found. func (s *Storage) Deployed(name string) (*rspb.Release, error) { - s.Log("getting deployed release from %q history", name) + ls, err := s.DeployedAll(name) + if err != nil { + if strings.Contains(err.Error(), "not found") { + return nil, fmt.Errorf("%q has no deployed releases", name) + } + return nil, err + } + + return ls[0], err +} + +// DeployedAll returns all deployed releases with the provided name, or +// returns ErrReleaseNotFound if not found. +func (s *Storage) DeployedAll(name string) ([]*rspb.Release, error) { + s.Log("getting deployed releases from %q history", name) ls, err := s.Driver.Query(map[string]string{ "NAME": name, @@ -129,7 +143,7 @@ func (s *Storage) Deployed(name string) (*rspb.Release, error) { "STATUS": "DEPLOYED", }) if err == nil { - return ls[0], nil + return ls, nil } if strings.Contains(err.Error(), "not found") { return nil, fmt.Errorf("%q has no deployed releases", name) diff --git a/pkg/tiller/release_rollback.go b/pkg/tiller/release_rollback.go index e8b6435b5..fe9a6a032 100644 --- a/pkg/tiller/release_rollback.go +++ b/pkg/tiller/release_rollback.go @@ -69,46 +69,46 @@ func (s *ReleaseServer) prepareRollback(req *services.RollbackReleaseRequest) (* return nil, nil, errInvalidRevision } - crls, err := s.env.Releases.Last(req.Name) + currentRelease, err := s.env.Releases.Last(req.Name) if err != nil { return nil, nil, err } - rbv := req.Version + previousVersion := req.Version if req.Version == 0 { - rbv = crls.Version - 1 + previousVersion = currentRelease.Version - 1 } - s.Log("rolling back %s (current: v%d, target: v%d)", req.Name, crls.Version, rbv) + s.Log("rolling back %s (current: v%d, target: v%d)", req.Name, currentRelease.Version, previousVersion) - prls, err := s.env.Releases.Get(req.Name, rbv) + previousRelease, err := s.env.Releases.Get(req.Name, previousVersion) if err != nil { return nil, nil, err } // Store a new release object with previous release's configuration - target := &release.Release{ + targetRelease := &release.Release{ Name: req.Name, - Namespace: crls.Namespace, - Chart: prls.Chart, - Config: prls.Config, + Namespace: currentRelease.Namespace, + Chart: previousRelease.Chart, + Config: previousRelease.Config, Info: &release.Info{ - FirstDeployed: crls.Info.FirstDeployed, + FirstDeployed: currentRelease.Info.FirstDeployed, LastDeployed: timeconv.Now(), Status: &release.Status{ Code: release.Status_PENDING_ROLLBACK, - Notes: prls.Info.Status.Notes, + Notes: previousRelease.Info.Status.Notes, }, - // Because we lose the reference to rbv elsewhere, we set the + // Because we lose the reference to previous version elsewhere, we set the // message here, and only override it later if we experience failure. - Description: fmt.Sprintf("Rollback to %d", rbv), + Description: fmt.Sprintf("Rollback to %d", previousVersion), }, - Version: crls.Version + 1, - Manifest: prls.Manifest, - Hooks: prls.Hooks, + Version: currentRelease.Version + 1, + Manifest: previousRelease.Manifest, + Hooks: previousRelease.Hooks, } - return crls, target, nil + return currentRelease, targetRelease, nil } func (s *ReleaseServer) performRollback(currentRelease, targetRelease *release.Release, req *services.RollbackReleaseRequest) (*services.RollbackReleaseResponse, error) { @@ -146,8 +146,16 @@ func (s *ReleaseServer) performRollback(currentRelease, targetRelease *release.R } } - currentRelease.Info.Status.Code = release.Status_SUPERSEDED - s.recordRelease(currentRelease, true) + deployed, err := s.env.Releases.DeployedAll(currentRelease.Name) + if err != nil { + return nil, err + } + // Supersede all previous deployments, see issue #2941. + for _, r := range deployed { + s.Log("superseding previous deployment %d", r.Version) + r.Info.Status.Code = release.Status_SUPERSEDED + s.recordRelease(r, true) + } targetRelease.Info.Status.Code = release.Status_DEPLOYED diff --git a/pkg/tiller/release_rollback_test.go b/pkg/tiller/release_rollback_test.go index aa3a764f6..b73501a36 100644 --- a/pkg/tiller/release_rollback_test.go +++ b/pkg/tiller/release_rollback_test.go @@ -139,11 +139,22 @@ func TestRollbackRelease(t *testing.T) { func TestRollbackWithReleaseVersion(t *testing.T) { c := helm.NewContext() rs := rsFixture() + rs.Log = t.Logf + rs.env.Releases.Log = t.Logf + rel2 := releaseStub() + rel2.Name = "other" + rs.env.Releases.Create(rel2) rel := releaseStub() rs.env.Releases.Create(rel) - upgradedRel := upgradeReleaseVersion(rel) + v2 := upgradeReleaseVersion(rel) rs.env.Releases.Update(rel) - rs.env.Releases.Create(upgradedRel) + rs.env.Releases.Create(v2) + v3 := upgradeReleaseVersion(v2) + // retain the original release as DEPLOYED while the update should fail + v2.Info.Status.Code = release.Status_DEPLOYED + v3.Info.Status.Code = release.Status_FAILED + rs.env.Releases.Update(v2) + rs.env.Releases.Create(v3) req := &services.RollbackReleaseRequest{ Name: rel.Name, @@ -155,6 +166,22 @@ func TestRollbackWithReleaseVersion(t *testing.T) { if err != nil { t.Fatalf("Failed rollback: %s", err) } + // check that v2 is now in a SUPERSEDED state + oldRel, err := rs.env.Releases.Get(rel.Name, 2) + if err != nil { + t.Fatalf("Failed to retrieve v1: %s", err) + } + if oldRel.Info.Status.Code != release.Status_SUPERSEDED { + t.Errorf("Expected v2 to be in a SUPERSEDED state, got %q", oldRel.Info.Status.Code) + } + // make sure we didn't update some other deployments. + otherRel, err := rs.env.Releases.Get(rel2.Name, 1) + if err != nil { + t.Fatalf("Failed to retrieve other v1: %s", err) + } + if otherRel.Info.Status.Code != release.Status_DEPLOYED { + t.Errorf("Expected other deployed release to stay untouched, got %q", otherRel.Info.Status.Code) + } } func TestRollbackReleaseNoHooks(t *testing.T) { diff --git a/pkg/tiller/release_server.go b/pkg/tiller/release_server.go index 31a043030..a96c64938 100644 --- a/pkg/tiller/release_server.go +++ b/pkg/tiller/release_server.go @@ -316,6 +316,7 @@ func (s *ReleaseServer) renderResources(ch *chart.Chart, values chartutil.Values return hooks, b, notes, nil } +// recordRelease with an update operation in case reuse has been set. func (s *ReleaseServer) recordRelease(r *release.Release, reuse bool) { if reuse { if err := s.env.Releases.Update(r); err != nil {