[incubator/cassandra] Fix chart not being upgradable by removing mutable label from volumeClaimTemplate #12402

Simon3 · 2019-03-20T15:06:49Z

What this PR does / why we need it:

Fix chart not being upgradable by removing mutable label from volumeClaimTemplate

Which issue this PR fixes

fixes Putting chart version in spec.VolumeClaimTemplate.metadata.labels of a StatefulSet prevents ANY future upgrade of existing Release #7803

Special notes for your reviewer:

Note that this fix is a breaking change itself since it creates a modification (removal) of the label, but at least, subsequent chart version bumps will be safe.

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

DCO signed
Chart Version bumped
Variables are documented in the README.md

…value, add support for google credentials as a secret Signed-off-by: Simon Hardy <sha@taktik.com>

Signed-off-by: Simon Hardy <sha@taktik.com>

…ble label from VolumeClaimTemplate Signed-off-by: Simon Hardy <sha@taktik.com>

k8s-ci-robot · 2019-03-20T15:07:04Z

Hi @Simon3. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Simon3 · 2019-03-20T15:08:06Z

/assign @maorfr

maorfr · 2019-03-20T15:17:24Z

/unassign
/assign @maver1ck

thanks @Simon3, allow me to pass this on :)

maver1ck · 2019-03-20T15:29:35Z

/hold
@Simon3
Any reasons of this change ?

Simon3 · 2019-03-20T16:56:20Z

Yes, as explained in #7803, with the current version, if I try to run a helm upgrade on an existing cassandra release after a version bump on the chart, I will get the following error:

Error: UPGRADE FAILED: StatefulSet.apps "cassandra" is invalid: spec: Forbidden: updates to statefulset spec for fields other than 'replicas', 'template', and 'updateStrategy' are forbidden.

In other words, the only way to do an upgrade of a incubator/cassandra release currently is to delete the statefulset and perform the upgrade after, which is annoying and prevents us from doing a rolling upgrade. Note that we will have this problem even if there was no change in the statefulset (apart from the chart version).

It seems like this behaviour is normal, and the standard way to fix this problem is to fix this is to remove the chart version from the labels of the volumeClaimTemplates (since it can't be updated by definition of statefulset).

maver1ck · 2019-03-20T17:35:09Z

Which Chart version do you have ?
Those labels was changed in 0.3.0 year ago.
fba95c3

I don't think that idea of changing it right know is good because this will break compatibility for other users.

Simon3 · 2019-03-20T17:43:21Z

I'm using the latest version of the chart.

The problem is that chart: {{ template "cassandra.chart" . }} actually calls the following:

{{- define "cassandra.chart" -}}
{{- printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 | trimSuffix "-" -}}
{{- end -}}

Which depends on the chart version, which changes on every commit here. So it actually breaks compatibility for other users each time someone commits something here. Of course my change will do the same, but at least, subsequent commits and version bumps won't impact the statefulset, so the problem will be solved.

Simon3 · 2019-03-27T09:47:31Z

Any news?

desaintmartin · 2019-04-01T10:49:40Z

Hello,
As as suggestion, you can write in the readme how to fix the problem, by taking inspiration from https://github.com/helm/charts/pull/7686/files#diff-b290a06961a11374d72f584e5925762cR63

This PR is mandatory, otherwise nobody can upgrade this chart.

Signed-off-by: Simon Hardy <sha@taktik.com>

desaintmartin · 2019-04-01T11:50:08Z

incubator/cassandra/README.md

+
+This version fixes https://github.com/helm/charts/issues/7803 by removing mutable labels in `spec.VolumeClaimTemplate.metadata.labels` so that it is upgradable.
+
+Before 0.12.0, in order to upgrade, you have to delete the Cassandra StatefulSet before upgrading: 


"Until this version"?

Signed-off-by: Simon Hardy <sha@taktik.com>

desaintmartin · 2019-04-01T12:22:37Z

incubator/cassandra/templates/statefulset.yaml

@@ -181,7 +181,6 @@ spec:
      name: data
      labels:
        app: {{ template "cassandra.name" . }}
-        chart: {{ template "cassandra.chart" . }}
        release: {{ .Release.Name }}
        heritage: {{ .Release.Service }}


One last thing: I think you can delete this one as well: https://github.com/helm/charts/blob/master/REVIEW_GUIDELINES.md#persistentvolumeclaim

Keeping it won't cause issue, but it may be better to delete.

Which one? heritage? Or the entire labels?

Signed-off-by: Simon Hardy <sha@taktik.com>

Simon3 · 2019-04-02T09:25:51Z

sha:~/Documents/taktik/charts helm upgrade cassandra incubator/cassandra -f cassandra-internal-values.yaml UPGRADE FAILED ROLLING BACK Error: StatefulSet.apps "cassandra" is invalid: spec: Forbidden: updates to statefulset spec for fields other than 'replicas', 'template', and 'updateStrategy' are forbidden. Error: UPGRADE FAILED: StatefulSet.apps "cassandra" is invalid: spec: Forbidden: updates to statefulset spec for fields other than 'replicas', 'template', and 'updateStrategy' are forbidden.

So again, I can't schedule my backups without restarting all my Cassandra pods... And this will be the case each time we bump this chart version until someone finally decides to merge this PR. @maver1ck could you do something please? it's been 2 weeks

desaintmartin · 2019-04-02T09:41:59Z

/approve
I can't merge but this follows good practices defined in charts.git.

maorfr · 2019-04-02T09:53:19Z

/ok-to-test

maorfr · 2019-04-22T09:13:52Z

/approve

@maver1ck, is this good to merge from your side?

maver1ck · 2019-04-22T16:03:26Z

/lgtm

k8s-ci-robot · 2019-04-22T16:03:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: desaintmartin, maorfr, maver1ck, Simon3

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~incubator/cassandra/OWNERS~~ [maorfr,maver1ck]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Simon3 · 2019-04-25T12:41:00Z

Are we good?

maver1ck · 2019-04-25T13:33:52Z

/hold cancel

maver1ck · 2019-04-25T13:34:10Z

Forgot about hold label :(

…able label from volumeClaimTemplate (helm#12402) * [incubator/cassandra] rename backup.image.repository, update default value, add support for google credentials as a secret Signed-off-by: Simon Hardy <sha@taktik.com> * [incubator/cassandra] cleanup Signed-off-by: Simon Hardy <sha@taktik.com> * [incubator/cassandra] comment Signed-off-by: Simon Hardy <sha@taktik.com> * [incubator/cassandra] Fix chart not being upgradable by removing mutable label from VolumeClaimTemplate Signed-off-by: Simon Hardy <sha@taktik.com> * [incubator/cassandra] Update readme regarding chart not being upgradable Signed-off-by: Simon Hardy <sha@taktik.com> * [incubator/cassandra] readme small update Signed-off-by: Simon Hardy <sha@taktik.com> * [incubator/cassandra] remove heritage from volumeClaimTemplates labels Signed-off-by: Simon Hardy <sha@taktik.com>

Simon3 added 5 commits March 18, 2019 17:18

[incubator/cassandra] rename backup.image.repository, update default …

965b625

…value, add support for google credentials as a secret Signed-off-by: Simon Hardy <sha@taktik.com>

[incubator/cassandra] cleanup

159c832

Signed-off-by: Simon Hardy <sha@taktik.com>

[incubator/cassandra] comment

c5ffcfb

Signed-off-by: Simon Hardy <sha@taktik.com>

[incubator/cassandra] Fix chart not being upgradable by removing muta…

17773a5

…ble label from VolumeClaimTemplate Signed-off-by: Simon Hardy <sha@taktik.com>

Merge branch 'master' of https://github.com/helm/charts

1e7dc66

helm-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Mar 20, 2019

k8s-ci-robot requested a review from maorfr March 20, 2019 15:06

helm-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 20, 2019

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 20, 2019

k8s-ci-robot assigned maorfr Mar 20, 2019

k8s-ci-robot assigned maver1ck and unassigned maorfr Mar 20, 2019

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 20, 2019

Simon3 mentioned this pull request Apr 1, 2019

Putting chart version in spec.VolumeClaimTemplate.metadata.labels of a StatefulSet prevents ANY future upgrade of existing Release #7803

Closed

Merge branch 'master' of https://github.com/helm/charts

bf0174d

[incubator/cassandra] Update readme regarding chart not being upgradable

b58b58d

Signed-off-by: Simon Hardy <sha@taktik.com>

helm-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 1, 2019

desaintmartin reviewed Apr 1, 2019

View reviewed changes

[incubator/cassandra] readme small update

80943d9

Signed-off-by: Simon Hardy <sha@taktik.com>

desaintmartin reviewed Apr 1, 2019

View reviewed changes

[incubator/cassandra] remove heritage from volumeClaimTemplates labels

7ab4771

Signed-off-by: Simon Hardy <sha@taktik.com>

k8s-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 2, 2019

Merge branch 'master' into master

139276b

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2019

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 25, 2019

k8s-ci-robot merged commit ab30648 into helm:master Apr 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[incubator/cassandra] Fix chart not being upgradable by removing mutable label from volumeClaimTemplate #12402

[incubator/cassandra] Fix chart not being upgradable by removing mutable label from volumeClaimTemplate #12402

Simon3 commented Mar 20, 2019

k8s-ci-robot commented Mar 20, 2019

Simon3 commented Mar 20, 2019

maorfr commented Mar 20, 2019

maver1ck commented Mar 20, 2019

Simon3 commented Mar 20, 2019

maver1ck commented Mar 20, 2019

Simon3 commented Mar 20, 2019

Simon3 commented Mar 27, 2019

desaintmartin commented Apr 1, 2019

desaintmartin Apr 1, 2019

Simon3 Apr 1, 2019

desaintmartin Apr 1, 2019

Simon3 Apr 1, 2019

desaintmartin Apr 1, 2019

Simon3 commented Apr 2, 2019

desaintmartin commented Apr 2, 2019

maorfr commented Apr 2, 2019

maorfr commented Apr 22, 2019

maver1ck commented Apr 22, 2019

k8s-ci-robot commented Apr 22, 2019

Simon3 commented Apr 25, 2019

maver1ck commented Apr 25, 2019

maver1ck commented Apr 25, 2019


		This version fixes https://github.com/helm/charts/issues/7803 by removing mutable labels in `spec.VolumeClaimTemplate.metadata.labels` so that it is upgradable.

		Before 0.12.0, in order to upgrade, you have to delete the Cassandra StatefulSet before upgrading:

[incubator/cassandra] Fix chart not being upgradable by removing mutable label from volumeClaimTemplate #12402

[incubator/cassandra] Fix chart not being upgradable by removing mutable label from volumeClaimTemplate #12402

Conversation

Simon3 commented Mar 20, 2019

What this PR does / why we need it:

Which issue this PR fixes

Special notes for your reviewer:

Checklist

k8s-ci-robot commented Mar 20, 2019

Simon3 commented Mar 20, 2019

maorfr commented Mar 20, 2019

maver1ck commented Mar 20, 2019

Simon3 commented Mar 20, 2019

maver1ck commented Mar 20, 2019

Simon3 commented Mar 20, 2019

Simon3 commented Mar 27, 2019

desaintmartin commented Apr 1, 2019

desaintmartin Apr 1, 2019

Choose a reason for hiding this comment

Simon3 Apr 1, 2019

Choose a reason for hiding this comment

desaintmartin Apr 1, 2019

Choose a reason for hiding this comment

Simon3 Apr 1, 2019

Choose a reason for hiding this comment

desaintmartin Apr 1, 2019

Choose a reason for hiding this comment

Simon3 commented Apr 2, 2019

desaintmartin commented Apr 2, 2019

maorfr commented Apr 2, 2019

maorfr commented Apr 22, 2019

maver1ck commented Apr 22, 2019

k8s-ci-robot commented Apr 22, 2019

Simon3 commented Apr 25, 2019

maver1ck commented Apr 25, 2019

maver1ck commented Apr 25, 2019