From 9a4f4ec64b8092d2ba3d7493001837df4737e36c Mon Sep 17 00:00:00 2001 From: Cristian Klein Date: Thu, 2 Jan 2020 00:23:10 +0100 Subject: [PATCH 1/3] fix(helm): Avoid corrupting storage via a lock If two `helm upgrade`s are executed at the exact same time, then one of the invocations will fail with "already exists". If one `helm upgrade` is executed and a second one is started while the first is in `pending-upgrade`, then the second invocation will create a new release. Effectively, two helm invocations will simultaneously change the state of Kubernetes resources -- which is scary -- then two releases will be in `deployed` state -- which can cause other issues. This commit fixes the corrupted storage problem, by introducting a poor person's lock. If the last release is in a pending state, then helm will abort. If the last release is in a pending state, due to a previously killed helm, then the user is expected to do `helm rollback`. Closes #7274 Signed-off-by: Cristian Klein --- pkg/action/action.go | 2 ++ pkg/action/upgrade.go | 5 +++++ pkg/release/status.go | 5 +++++ 3 files changed, 12 insertions(+) diff --git a/pkg/action/action.go b/pkg/action/action.go index 698ebd23f..fec86cd42 100644 --- a/pkg/action/action.go +++ b/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. diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go index 921a0eba5..949aebcae 100644 --- a/pkg/action/upgrade.go +++ b/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 diff --git a/pkg/release/status.go b/pkg/release/status.go index 49b0f1544..e0e3ed62a 100644 --- a/pkg/release/status.go +++ b/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 +} From 20fb7bac4e9001751a0b04c71006b4bb506378cf Mon Sep 17 00:00:00 2001 From: Cristian Klein Date: Thu, 9 Jan 2020 00:38:47 +0100 Subject: [PATCH 2/3] fix(helm): Added test for concurrent upgrades Signed-off-by: Cristian Klein --- pkg/action/upgrade_test.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/pkg/action/upgrade_test.go b/pkg/action/upgrade_test.go index f25d115c4..f16de6479 100644 --- a/pkg/action/upgrade_test.go +++ b/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) +} From 0d70c63396083f868963797632715f06c9b90ca9 Mon Sep 17 00:00:00 2001 From: Cristian Klein Date: Mon, 13 Jul 2020 23:41:25 +0200 Subject: [PATCH 3/3] fix(helm): Update test during pending install Signed-off-by: Cristian Klein --- cmd/helm/testdata/output/upgrade-with-pending-install.txt | 1 + cmd/helm/upgrade_test.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 cmd/helm/testdata/output/upgrade-with-pending-install.txt diff --git a/cmd/helm/testdata/output/upgrade-with-pending-install.txt b/cmd/helm/testdata/output/upgrade-with-pending-install.txt new file mode 100644 index 000000000..57a8e7873 --- /dev/null +++ b/cmd/helm/testdata/output/upgrade-with-pending-install.txt @@ -0,0 +1 @@ +Error: UPGRADE FAILED: another operation (install/upgrade/rollback) is in progress diff --git a/cmd/helm/upgrade_test.go b/cmd/helm/upgrade_test.go index 7e88bc0df..64daf2934 100644 --- a/cmd/helm/upgrade_test.go +++ b/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)}, },