search

technosophos / common-chart

A base Helm chart with shared definitions
https://technosophos.github.io/common-chart/
Other
97 stars 16 forks source link

Label stability between upgrades #3

Open jrnt30 opened 7 years ago

jrnt30 commented 7 years ago

There are a subset of resources that K8 manages where label changes are not allowed, such as StatefulSets. When using the common.labels.standard and similar, one issue that we have run into is the inability to upgrade a version of our Helm chart on an existing deployment.

My suggestion would be that the version should probably be left out of the standard labels & that the chart label strictly refer to the {{.Chart.Name}} and omit the {{.Chart.Version}}

prydonius commented 7 years ago

In https://github.com/prydonius/common-chart/blob/merge-approach/common/templates/_deployment.yaml#L9 I've set the label used in the Deployment PodSpec to just app because of this issue - this is what most of the charts in kubernetes/charts follow.

jrnt30 commented 7 years ago

Understood, however with this approach you are still using the https://github.com/prydonius/common-chart/blob/merge-approach/common/templates/_deployment.yaml#L4 at the deployment level which ultimately will include those attributes. Does the top level metadata block there allow that upgrade properly?

In general, I find it very useful to have the labels that are now absent in your deployment spec still on those pods as a simple way to say kubect get all -l chart=elasticsearch or whatever.

Is there a hard requirement to keeping that version in the label directly instead of the metadata that Helm itself is already storing (and displaying)?

prydonius commented 7 years ago

The Deployment uses the labels defined in it's PodSpec as the selector for ReplicaSets by default, and if these change during upgrade it can't refer back to the old ReplicaSet. The Deployment level labels are not used for this so it's fine to use Chart versions there.

As far as what labels to include in the PodSpec, I think this is a great discussion for kubernetes/charts best practices.

pkrishn6 commented 6 years ago

On this issue,

common.Chartref should not contain the version of the chart as it causes upgrades to fail or rather common.Chartref is not suitable to be used as as selector

Edit: common.ChartRef is not used as a selector by the common-chart repo. I just made it as a general comment as I used it in my implementation and hit the upgrade issue.

pkrishn6 commented 6 years ago

@prydonius : In https://github.com/technosophos/common-chart/blob/master/common/templates/_deployment.yaml#L17

when common.fullname by itself isolates the resource per release, why is there a need to have the release as part of the label and selector?

prydonius commented 6 years ago

common.Chartref should not contain the version of the chart as it causes upgrades to fail or rather common.Chartref is not suitable to be used as as selector

is it currently being used as a selector? that sounds like a bug

when common.fullname by itself isolates the resource per release, why is there a need to have the release as part of the label and selector?

I don't think there's a good reason for this, it's just how charts are doing this today.