support kubernetes recommended labels

[dxma]

Background:

When using dask helm as subchart in my setup, I found it use different set of labels. This will confuse some kubernetes services such as istio and kiali.

Changes:

This PR enables possibility to use kubernetes recommended label set.

• helm classic labels when Values.label.style == helm (default)
• kubernetes recommended when Values.label.style == kubernetes

Output:

all labels:
helm.sh/chart: dask-0.0.1-set.by.chartpress
app.kubernetes.io/name: dask-jupyter
app.kubernetes.io/instance: dask-jupyter
app.kubernetes.io/component: jupyter
app.kubernetes.io/version: "2022.6.1"
app.kubernetes.io/part-of: dask
app.kubernetes.io/managed-by: Helm
app.kubernetes.io/created-by: dask-0.0.1-set.by.chartpress

selector labels:
app.kubernetes.io/name: dask-jupyter
app.kubernetes.io/instance: dask-jupyter
app.kubernetes.io/component: jupyter

I haven't applied this option to daskhub yet.

Cheers,
Dongxu

[jacobtomlinson]

This is great thanks for taking the time here. One small comment.

[jacobtomlinson]

I'm curious why these component options are configurable? What use cases do you see for changing them?

[dxma]

Hi, mainly because the value 'scheduler/worker/jupyter' is too generic :-) Think of making dask a part of large system, there might be existing deployments using one of these component value already. In that case, people might prefer to change it into something like 'dask-scheduler' instead.
my $0.02

[jacobtomlinson]

Fair enough. I'm just on guard for config creep, especially for things that wont get used much. I haven't seen a need for this.

If you think this will come up for folks and isn't just a "what if" then I'm happy for it to go in.

[jacobtomlinson]

I like this.

[consideRatio]

When changing the selector labels of a deployment/statefulset resource, upgrades will fail - the selector labela of a deployment/statefulset resource are immutable.

This can be done still by deleting the deployment/statefulset resource ahead of the helm upgrade manually.

When deleting the deployment, one could declare that the resources created for it, the replicasets that in turn create pods, should still be around but orphaned instead of deleted as is the default. That makes sense if the new deployments selector labels would select the old labels, otherwise not.

Since there is no changelog etc for the charts, this is problematic in mind as it is a breaking change troublesome to document, especially since we use calver and cant indicate a breaking change via the version number either.

A non-breaking change would be to add the new labels next to the old in pod definitions, without changing the selector labels. In jupyterhub helm chart, i proposed this once but we ended up not making a change like this. The recommended labels from helm/k8s community has changed twice since jupyterhub helm chart was created, probably settled down now. A few charts has made these breaking changes, a few havent.

I think if this is to be merged, documentation about it being a breaking change should be established someplace. For example in a CHANGELOG.md file associated with the helm chart in question.

[dxma]

When changing the selector labels of a deployment/statefulset resource, upgrades will fail - the selector labela of a deployment/statefulset resource are immutable.

This can be done still by deleting the deployment/statefulset resource ahead of the helm upgrade manually.

When deleting the deployment, one could declare that the resources created for it, the replicasets that in turn create pods, should still be around but orphaned instead of deleted as is the default. That makes sense if the new deployments selector labels would select the old labels, otherwise not.

Since there is no changelog etc for the charts, this is problematic in mind as it is a breaking change troublesome to document, especially since we use calver and cant indicate a breaking change via the version number either.

A non-breaking change would be to add the new labels next to the old in pod definitions, without changing the selector labels. In jupyterhub helm chart, i proposed this once but we ended up not making a change like this. The recommended labels from helm/k8s community has changed twice since jupyterhub helm chart was created, probably settled down now. A few charts has made these breaking changes, a few havent.

I think if this is to be merged, documentation about it being a breaking change should be established someplace. For example in a CHANGELOG.md file associated with the helm chart in question.

Absolutely correct, this change (switching to new label set on a running system) is a breaking action, since it needs to delete all existing deployments together with their running pods.. It is one of the main reasons I make it an option and default to current label set.
Also it is not very clear how app should be defined, in current code I see it more like system (app=dask). In other documentations people try to match it with name/instance in kubernetes labels.
Makes sense to manage all labels in the way demonstrated by this PR: one change applies to everywhere.

[consideRatio]

I make it an option and default to current label set.

Ahhhh nice!

If rendering templates with default configuration doesn't lead to changes in the matchLabels aka selector labels, this is good in my mind.

Sorry for my sloppy review to not realize precaution had been taken to avoid the breaking change!