[grafana] Use .Values.podLabels in service definition.

[ikogan]

Currently, if you set .Values.podLabels in your values.yaml, they are not reflected in the service selectors so traffic does not reach Grafana pods.

This simply adds .Values.podLabels to the service selectors.

[CLAassistant]

All committers have signed the CLA.

[Sheikh-Abubaker]

@ikogan Could you please sign the DCO ? Thanks!

[jkroepke]

grafana.selectorLabels should still sufficient for matching pods

[Sheikh-Abubaker]

grafana.selectorLabels should still sufficient for matching pods

@jkroepke That's right! but what if the some users find it more convinient to include the labels directly from the values.yaml instead of _helpers.tpl ?

[jkroepke]

No. .Values.podLabels is designed to put additional labels on a pod.

The helper grafana.selectorLabels is used on deploy.spec.selector.matchLabels which is an immutable field.

If .Values.podLabels would be part of grafana.selectorLabels, the end-user always needs to manually delete the grafana deployment before changing .Values.podLabels.

The currently implementation is fine and should not be changed in my opinion.

[ikogan]

For the DCO, do you just want an empty commit with the sign-off or to amend the one commit I made for this?

[ikogan]

No. .Values.podLabels is designed to put additional labels on a pod.

The helper grafana.selectorLabels is used on deploy.spec.selector.matchLabels which is an immutable field.

If .Values.podLabels would be part of grafana.selectorLabels, the end-user always needs to manually delete the grafana deployment before changing .Values.podLabels.

The currently implementation is fine and should not be changed in my opinion.

By "the current implementation" do you mean in the PR or in main?

[jkroepke]

current implementation in main. You can add additional pod labels on your choice, however not all labels must be defined in a selector on a service.

[Sheikh-Abubaker]

No. .Values.podLabels is designed to put additional labels on a pod.

The helper grafana.selectorLabels is used on deploy.spec.selector.matchLabels which is an immutable field.

If .Values.podLabels would be part of grafana.selectorLabels, the end-user always needs to manually delete the grafana deployment before changing .Values.podLabels.

The currently implementation is fine and should not be changed in my opinion.

That's correct! seems like I mistaken it for something else.

[Sheikh-Abubaker]

current implementation in main. You can add additional pod labels on your choice, however not all labels must be defined in a selector on a service.

@jkroepke guess we're closing this then ?

[ikogan]

current implementation in main. You can add additional pod labels on your choice, however not all labels must be defined in a selector on a service.

But doesn't this mean that if you use .Values.podLabels then there's no way for the service generated by the helm chart to actually ever work? What's the point of generating a knowingly broken service?

[jkroepke]

Please, explain, why the service would be broken? The service selector based on grafana.selectorLabels template and the pod has the labels from grafana.selectorLabels template. With Values.podLabels, you could define additional labels, but the selector still matches, even just a sub-set of labels matched.

[ikogan]

Please, explain, why the service would be broken? The service selector based on grafana.selectorLabels template and the pod has the labels from grafana.selectorLabels template. With Values.podLabels, you could define additional labels, but the selector still matches, even just a sub-set of labels matched.

Sorry, it's been a minute since I had the issue and I had my issue wrong. Since I'm deploying Grafana as a subchart of another chart, the selectorLabels generated match all of the pods generated by that chart, including non-Grafana pods. So, in fact, the service ends up matching more than it should.

I don't see a way in the helm chart to impact the selector labels that the service uses. Right now my only recourse is to disable the service entirely and generate my own.

[jkroepke]

Since I'm deploying Grafana as a subchart of another chart, the selectorLabels generated match all of the pods generated by that chart, including non-Grafana pods. So, in fact, the service ends up matching more than it should.

This case happens if your umbralla chart contains multiple charts named grafana?

Because of part of the grafana selector labels named app.kubernetes.io/name: grafana where the app.kubernetes.io/name is the name of the chart.

For such scenarios, nameOverride can be used to etablish an unique selector

So, in fact, the service ends up matching more than it should.

That case, you current deployment strategy is broken. If grafana.selectorLabels matches more pod than it should, the selector at Deployment level is affected from that issue, too.

helm-charts/charts/grafana/templates/deployment.yaml

Lines 21 to 23 in 323e843

	selector:
	matchLabels:
	{{- include "grafana.selectorLabels" . | nindent 6 }}

If we are merging this PR, this issue would more hidden and lead to undefined behavior in Kubernetes.

Note

You should not create other Pods whose labels match this selector, either directly, by creating another Deployment, or by creating another controller such as a ReplicaSet or a ReplicationController. If you do so, the first Deployment thinks that it created these other Pods. Kubernetes does not stop you from doing this.

Ref: https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#selector

---

To address that kind of issue, .Values.podLabels must be also part of the Deployment.spec.selector configuration. The issue here is that this field is immutable and could not update after initial deployment.

[ikogan]

It contains a single chart named Grafana but I'm also adding oncall as a subchart. This causes all of the oncall deployments to have the same pod labels. In my values, I simply have fullnameOverride under the grafana key set to grafana and fullnameOverride under the oncall key set to oncall. Nevertheless, I end up with, for example, the RabbitMQ service that comes with oncall targeting these selectors:

app.kubernetes.io/name: grafana
app.kubernetes.io/instance: grafana

You're right that this all runs deeper than the podLabels thing posted earlier. I suppose I'll have to dig more into why these selectors are being generated this way.

[jkroepke]

This causes all of the oncall deployments to have the same pod labels.

But that is strange. Is the sub-chart name of oncall, grafana too?

Did you define nameOverride somewhere? Normally, the name of the chart is used as selector. Even for sub chart components, the name of the sub chart it used unless nameOverride is set.

fullnameOverride has no effect for selectorLabels.