Charts: Putting chart version in spec.selector.matchLabels in apps/v1beta2 and later prevents ANY future upgrade of existing Release

Created on 12 Sep 2018  路  11Comments  路  Source: helm/charts

BUG REPORT

tl;dr: any Chart containing Deployment/StatefulSet/DaemonSet apps/v1beta2 or newer using a chart version in a selector or other immutable field can't be upgraded

Edit:
This is the second bug among 3 regarding "I can't upgrade my Release".

Forewords

Immutability

Required selector

This means that for apps/v1beta2 or newer, every change of an existing Deployment, StatefulSet or DaemonSet in spec.selector.mathLabels or other immutable fields will be refused by kubernetes.

The problem

In some charts, we set the chart version (directly or through the chart template) in different parts of the Deployment/StatefulSet/DaemonSet yaml files of the Chart.

This causes helm to attempt to change immutable fields and any upgrade (even if it ONLY change the version in Chart.yaml) will fail.

IN OTHER WORDS, SOME CHARTS OF THIS REPOSITORY ARE DEEPLY BROKEN AND CAN'T BE UPGRADED, only installed once.

Impacted charts

At least, impacted charts are:

  • stable/rabbitmq (fixed in https://github.com/helm/charts/pull/7848)
  • stable/redis (fixed in https://github.com/helm/charts/pull/7686)
  • stable/phpmyadmin (fixed in https://github.com/helm/charts/pull/7830)
  • stable/horovod (fixed in https://github.com/helm/charts/pull/8596)
  • stable/mongodb (partial fix in https://github.com/helm/charts/pull/7441, fixed in https://github.com/helm/charts/pull/10121)
  • stable/nats (fixed in https://github.com/helm/charts/pull/7832)

How to reproduce (using redis as an example)

For each scenario, disable "cluster" option in values.yaml only for simplicity, install the redis chart using git, change the version in chart.yaml (example: 1.2.3 -> 1.2.4) without changing anything else, and try to upgrade. See it fail, and remove the chart:

cd stable/redis
vim values.yaml
helm install --name redis-test .
vim Chart.yaml
helm upgrade redis-test .
helm delete --purge redis-test

1 - Statefulset apps/v1beta2 or newer: spec.selector

Because of templates/redis-master-statefulset.yaml containing:
chart: {{ template "redis.chart" . }}
into spec.selector and `spec.template.metadata.labels

It will cause:

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

(note that here, metadata.labels.Chart does not cause issue because of explicit selector)

(Edit: previous versions of this issue had other examples, since moved to their own issues)

Let's remove this line, uninstall, then reinstall, upgrade: it works.

Conclusion

tl;dr: don't put fields that may change in selector

This is a really blocking problem, even solving this will require to break impacted charts (major version bump).
The only known workaround is to run kubectl delete cascade=false for offending Deployments, StatefulSets or DaemonSets until this is solved, which only kind of works...

A lot of PRs going the wrong way have been merged lately (see this one as example: https://github.com/helm/charts/pull/6909)

And a few attempts without really understanding the problems were tried to solve it: https://github.com/helm/charts/pull/7441

Helm has several related won't-fix issues: https://github.com/helm/helm/issues/2494

cc @mattfarina
cc https://github.com/helm/chart-testing/issues/19 for testing this
cc https://github.com/helm/charts/issues/3011 to document this
cc https://github.com/helm/charts/issues/5657 to document solving (a.k.a breaking) this

picture desaintmartin
👍2

All 11 comments

I've edited the issue because it is even wider than I thought: most of v1beta1 charst are broken as well.

desaintmartin picture desaintmartin on 12 Sep 2018

Thanks for linking this issue to the original k8s PR (kubernetes/kubernetes#50719). In the the charts documentation issue (#7692) I was thinking it may be a good idea to link to the relevant section(s) in the k8s documentation instead, since that鈥檚 more readable.

But as I'm looking through the k8s docs, it seems like the details may be a bit complicated, and your findings so far don't seem to match the documentation. For example:

From https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#label-selector-updates:

Note: In API version apps/v1, a Deployment鈥檚 label selector is immutable after it gets created.

And also in https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#selector

In API version apps/v1, .spec.selector and .metadata.labels do not default to .spec.template.metadata.labels if not set. So they must be set explicitly. Also note that .spec.selector is immutable after creation of the Deployment in apps/v1.

But your findings show that this happened with Deployment even before apps/v1?

And this doesn't seem to be fully documented on all affected resources (I couldn't find any mention of this for StatefulSet, except a note that pod selectors are now required after k8s 1.8: https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/#pod-selector).

Anyway, I'm noting this here rather than the documentation issue #7692 because I'm not sure we have correct notes to link to that are consistent with your findings on this problem - they may also not be comprehensive (of all affected resources). This may be a separate k8s documentation issue if it turns out to be the case.

scottrigby picture scottrigby on 12 Sep 2018

  • Well, the documentation is not clear regarding the impacted version. I can confirm spec.selector.matchLabelsis immutable since apps/v1beta2, in concordance of the linked PR and my own tests. If you see https://v1-8.docs.kubernetes.io/docs/concepts/workloads/controllers/deployment/#label-selector-updates, you'll see that it mentions apps/v1beta2...

  • Let me update the issue with the documentation

  • It is simpler for StatefulSet: everything but a few fields are immutable, at least according to the very explicit error Maybe Kubernetes lack documentation about this. Unfortunately, the API documentation does not state anything about mutability of fields.

  • Finally, for apps/v1beta1 (or extensions/v1beta1) without selector, it is much simpler: helm seems to try to upgrade the spec.template.metadata.labels without updating the (initially generated by kubernetes) spec.selector.matchLabels. We just need to explicitely set spec.selector.matchLabels (in this case, even with chart label) in a minor upgrade. May we separate this into a separate issue?

desaintmartin picture desaintmartin on 13 Sep 2018
👍1

Issue updated to source from documentation. Still no source for StatefulSet.

desaintmartin picture desaintmartin on 13 Sep 2018
👍1

I have updated the whole issue to only focus on the blocking problem regarding apps/v1beta2 and newer.
I created another issue for v1beta1 here: https://github.com/helm/charts/issues/7726

desaintmartin picture desaintmartin on 13 Sep 2018

Edited to move the volumeClaimTemplate bug to its own issue since it is different than the selector problem: https://github.com/helm/charts/issues/7803

desaintmartin picture desaintmartin on 18 Sep 2018

cc @cheyang for horovod.

desaintmartin picture desaintmartin on 25 Sep 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

stale[bot] picture stale[bot] on 20 Nov 2018

AFAIK no more charts have this problem, closing.

desaintmartin picture desaintmartin on 20 Nov 2018
🎉1

Monogodb is still impacted, see their statefulset with .Values.selector.matchLabels.chart.
cc @jkirkham-ratehub @prydonius @tompizmor @sameersbn @carrodher @juan131

desaintmartin picture desaintmartin on 18 Dec 2018

I just created this PR to fix it @desaintmartin
https://github.com/helm/charts/pull/10121

juan131 picture juan131 on 19 Dec 2018
Was this page helpful?
0 / 5 - 0 ratings

Related issues

mluedke picture mluedke  路  3Comments
vsliouniaev picture vsliouniaev  路  3Comments
onyxet picture onyxet  路  3Comments
out-of-band picture out-of-band  路  3Comments
kminehart picture kminehart  路  3Comments