Merge pull request #7322 from cristiklein/fix/less-corruption-with-concurrent-executions

fix(helm): Avoid corrupting storage via a lock

cmd/helm/testdata/output/upgrade-with-pending-install.txt

@@ -0,0 +1 @@
+Error: UPGRADE FAILED: another operation (install/upgrade/rollback) is in progress

cmd/helm/upgrade_test.go

@@ -158,7 +158,7 @@ func TestUpgradeCmd(t *testing.T) {
 		{
 			name:      "upgrade a pending install release",
 			cmd:       fmt.Sprintf("upgrade funny-bunny '%s'", chartPath),
-			golden:    "output/upgrade-with-bad-or-missing-existing-release.txt",
+			golden:    "output/upgrade-with-pending-install.txt",
 			wantError: true,
 			rels:      []*release.Release{relWithStatusMock("funny-bunny", 2, ch, release.StatusPendingInstall)},
 		},

pkg/action/action.go

@@ -60,6 +60,8 @@ var (
 	errInvalidRevision = errors.New("invalid release revision")
 	// errInvalidName indicates that an invalid release name was provided
 	errInvalidName = errors.New("invalid release name, must match regex ^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])+$ and the length must not longer than 53")
+	// errPending indicates that another instance of Helm is already applying an operation on a release.
+	errPending = errors.New("another operation (install/upgrade/rollback) is in progress")
 )
 
 // ValidName is a regular expression for resource names.

pkg/action/upgrade.go

@@ -170,6 +170,11 @@ func (u *Upgrade) prepareUpgrade(name string, chart *chart.Chart, vals map[strin
 		return nil, nil, err
 	}
 
+	// Concurrent `helm upgrade`s will either fail here with `errPending` or when creating the release with "already exists". This should act as a pessimistic lock.
+	if lastRelease.Info.Status.IsPending() {
+		return nil, nil, errPending
+	}
+
 	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

pkg/action/upgrade_test.go

@@ -253,3 +253,23 @@ func TestUpgradeRelease_ReuseValues(t *testing.T) {
 		is.Equal(expectedValues, updatedRes.Config)
 	})
 }
+
+func TestUpgradeRelease_Pending(t *testing.T) {
+	req := require.New(t)
+
+	upAction := upgradeAction(t)
+	rel := releaseStub()
+	rel.Name = "come-fail-away"
+	rel.Info.Status = release.StatusDeployed
+	upAction.cfg.Releases.Create(rel)
+	rel2 := releaseStub()
+	rel2.Name = "come-fail-away"
+	rel2.Info.Status = release.StatusPendingUpgrade
+	rel2.Version = 2
+	upAction.cfg.Releases.Create(rel2)
+
+	vals := map[string]interface{}{}
+
+	_, err := upAction.Run(rel.Name, buildChart(), vals)
+	req.Contains(err.Error(), "progress", err)
+}

pkg/release/status.go

@@ -42,3 +42,8 @@ const (
 )
 
 func (x Status) String() string { return string(x) }
+
+// IsPending determines if this status is a state or a transition.
+func (x Status) IsPending() bool {
+	return x == StatusPendingInstall || x == StatusPendingUpgrade || x == StatusPendingRollback
+}