From 5847d922111ccb90beba3e6ea072bdc357355fdd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Johnny=20Bergstr=C3=B6m?= <github@joonix.se>
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
(cherry picked from commit 5f1a21bc321b326f3fab87b71bdc12bcd7125441)
---
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 {
--
GitLab