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

@@ -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)

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
 

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) {

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 {