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

[desaintmartin]

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".

• for the apps/v1beta1 / extensions/v1beta1 problem, see Putting chart version in metadata.labels in a Deployment v1beta1 without selector prevents upgrade #7726.
• For the immutable volumeClaimTemplate in StatefulSet (any version), see Putting chart version in spec.VolumeClaimTemplate.metadata.labels of a StatefulSet prevents ANY future upgrade of existing Release #7803

Forewords

Immutability

• Kubernetes API defines that some fields are immutable. In particular:
  • For a Deployment since apps/v1beta2, spec.selector.matchLabels. Sources:

    • Official documentation: https://v1-8.docs.kubernetes.io/docs/concepts/workloads/controllers/deployment/

      Note: In API version apps/v1beta2, a Deployment’s label selector is immutable after it gets created.

    • PR introducing the feature: Make selector immutable for v1beta2 deployment, replicaset and daemonset prior update kubernetes/kubernetes#50719

  • For a DaemonSet, since apps/v1beta2, spec.selector.matchLabels
  • For a StatefulSet, everything except spec.replicas, spec.template, and spec.updateStrategy (i.e spec.selectorand spec.VolumeClaimTemplate is immutable)

Required selector

• Since Deployments, StatefulSet or DaemonSet apps/v1beta2, spec.selector.matchLabels is mandatory. Sources:

  • Official documentation: https://v1-8.docs.kubernetes.io/docs/concepts/workloads/controllers/deployment/

    In API version apps/v1beta2, .spec.selector and .metadata.labels no longer 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/v1beta2.

• In previous versions it was generated from spec.metadata.labels. Source:

  • Official documentation: https://v1-7.docs.kubernetes.io/docs/concepts/workloads/controllers/deployment/

    If .spec.selector is unspecified, .spec.selector.matchLabels defaults to .spec.template.metadata.labels

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 [stable/rabbitmq] Fix chart not being upgradable #7848)
• stable/redis (fixed in [stable/redis] Major version bump: Fix chart not being upgradable #7686)
• stable/phpmyadmin (fixed in [stable/phpmyadmin] Fix chart not being upgradable #7830)
• stable/horovod (fixed in [stable/horovod] Major version bump: Fix chart not being upgradable. #8596)
• stable/mongodb (partial fix in add selector matchLabels #7441, fixed in [stable/mongodb] Fix chart not being upgradable when replicaset is enabled. #10121)
• stable/nats (fixed in [stable/nats] Fix chart not being upgradable #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: #6909)

And a few attempts without really understanding the problems were tried to solve it: #7441

Helm has several related won't-fix issues: helm/helm#2494

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

[desaintmartin]

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

[scottrigby]

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’s 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’s 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.

[desaintmartin]

• 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]

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

[desaintmartin]

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: #7726

[desaintmartin]

Edited to move the volumeClaimTemplate bug to its own issue since it is different than the selector problem: #7803

[desaintmartin]

cc @cheyang for horovod.

[stale]

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.

[desaintmartin]

AFAIK no more charts have this problem, closing.

[desaintmartin]

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

[juan131]

I just created this PR to fix it @desaintmartin
#10121