Add k8s recommended labels

[Owen-CH-Leung]

fixes #34048

As per the discussion, this PR modifes all existing yaml files inside airflow helm chart to include the recommended k8s labels. See this for more details

A lot of files are being affected but in short

• a new definition kubernetes_recommended_labels is added into _helpers.yaml
• Apply this new definition to each yaml file, and if applicable, add a new label app.kubernetes.io/component (with the value being the same as component if the resource was originally labeled with component
• Modify a few helm tests as a result of this change

[kimminw00]

I think it is necessary to differentiate app.kubernetes.io/name of each object.

[Owen-CH-Leung]

I think it is necessary to differentiate app.kubernetes.io/name of each object.

How about we name each resource like airflow-{name of the component}-{type of the resource} ?

e.g. airflow-webserver-deployment, airflow-webserver-ingress

[kimminw00]

I think it would be helpful to refer to the prometheus helm chart. (not only app.kubernetes.io/name but also other labels)

https://github.com/prometheus-community/helm-charts/blob/f57ff6651817b23c21daa7d5cd087649add5836e/charts/prometheus-druid-exporter/templates/_helpers.tpl#L9-L25

https://github.com/search?q=repo%3Aprometheus-community%2Fhelm-charts%20app.kubernetes.io%2Fname&type=code

https://github.com/prometheus-community/helm-charts/tree/f57ff6651817b23c21daa7d5cd087649add5836e/charts/kube-prometheus-stack

[Owen-CH-Leung]

I think it would be helpful to refer to the prometheus helm chart. (not only app.kubernetes.io/name but also other labels)

https://github.com/prometheus-community/helm-charts/blob/f57ff6651817b23c21daa7d5cd087649add5836e/charts/prometheus-druid-exporter/templates/_helpers.tpl#L9-L25

https://github.com/search?q=repo%3Aprometheus-community%2Fhelm-charts%20app.kubernetes.io%2Fname&type=code

https://github.com/prometheus-community/helm-charts/tree/f57ff6651817b23c21daa7d5cd087649add5836e/charts/kube-prometheus-stack

@kimminw00 Thanks. I've examined the 6 k8s recommended labels in the prometheus chart and I find that there's a difference for the labels app.kubernetes.io/name and app.kubernetes.io/part-of.

In prometheus chart, these 2 labels are set as default Chart.Name, and can be overriden by nameOverride in the values.yaml file. In airflow chart, this field also exists so I think we can implement the same logic as well.

For the other 4 labels (app.kubernetes.io/version, helm.sh/chart, app.kubernetes.io/managed-by and app.kubernetes.io/instance), I think my current logic is the same as the prometheus chart. Feel free to let me know if I'm wrong or I miss anything.

[kimminw00]

And please take a close look at how each object(secret, pv, ingress etc.) is labeled in prometheus helm chart too!

[Owen-CH-Leung]

And please take a close look at how each object(secret, pv, ingress etc.) is labeled in prometheus helm chart too!

Hi @kimminw00, thank you for your comments. I've gone through the Prometheus helm chart as you suggested. While I tried to grasp the key points, I'm keen to ensure I address your concerns accurately. If there are areas you feel could benefit from closer alignment with the Prometheus helm chart, I'd appreciate any additional insights you might share. Thanks for your understanding and assistance!

[kimminw00]

Prometheus and alertmanager are different apps. So they have different app.kubernetes.io/name

https://github.com/prometheus-community/helm-charts/blob/f57ff6651817b23c21daa7d5cd087649add5836e/charts/kube-prometheus-stack/templates/alertmanager/serviceaccount.yaml#L9

https://github.com/prometheus-community/helm-charts/blob/f57ff6651817b23c21daa7d5cd087649add5836e/charts/kube-prometheus-stack/templates/prometheus/serviceaccount.yaml#L9C1-L9C85

Ariflow, pgbouncer, redis, and statsd(may be there is more apps!) need to have different app.kubernetes.io/name in the same way.
(But I'm not sure that webserver, scheduler, dag processor, triggerer, create-user-job, etc., are different apps as each app runs in the same container image)

And you have to tests if app name is not longer than 63 chars.
https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-label-names

[Owen-CH-Leung]

Thanks @kimminw00. Yes it makes sense to me. Different apps should have different names for differentiation. I've customized the yaml files to have different app.kubernetes.io/name instead of a single definition for all (i.e. differernt resource will have app.kubernetes.io/name like airflow-flower, airflow-redis, airflow-statsd and so on).

I also put webserver, scheduler, dag processor, triggerer, worker to have similar app name (i.e. airflow-webserver, airflow-scheduler etc) even though they use the same image since they are seperate sub-applications. But for jobs like create-user-job, run-airflow-migration etc, I didn't put this label for now.

I also added a new test to check if the app.kubernetes.io/name exceeds 63 chars.

Let me know your thought on this.

[kimminw00]

If airflow.fullname length is 63 chars, then app.kubernetes.io/name will be longer than 63 chars.
(For example, {{ include "airflow.fullname" . }}-dag-processor should not be longer than 63 chars.)
You have to check https://github.com/prometheus-community/helm-charts/blob/1a8f84ae3621937cf43cfb0e97956c4690d9d96b/charts/kube-prometheus-stack/templates/_helpers.tpl#L7-L25.

I think it is better to open another PR which fixes {{ include "airflow.fullname" . }} first.

[Owen-CH-Leung]

Thanks. I've applied the fix here to truncate airflow.fullname to return at most 40 chars.

[Owen-CH-Leung]

@jedcunningham @hussein-awala Can I seek your feedback for this PR also ? Thanks

[kimminw00]

I think you should also have to consider Label selectors.

[Owen-CH-Leung]

I think you should also have to consider Label selectors.

You mean add the newly created labels inside label selector ? I think label selector should continue to work as long as I didn't remove the existing labels. We don't need to select all labels that are newly created.

[kimminw00]

I think you should also have to consider Label selectors.

This means replacing the previous labels with the K8S recommended labels in label selectors.
There may be problems due to replacement, so I think it would be good idea to leave it as is.
(I think we can replace labels when the chart major version is bumped)
I'd like to hear what others think!

Please add recommemded labels to the chart/files/pod-template-file.kubernetes-helm-yaml file as well!

Thank you for going above and beyond!

[Owen-CH-Leung]

I think you should also have to consider Label selectors.

This means replacing the previous labels with the K8S recommended labels in label selectors. There may be problems due to replacement, so I think it would be good idea to leave it as is. (I think we can replace labels when the chart major version is bumped) I'd like to hear what others think!

Please add recommemded labels to the chart/files/pod-template-file.kubernetes-helm-yaml file as well!

Thank you for going above and beyond!

No problem =). Added recommended labels to chart/files/pod-template-file.kubernetes-helm-yaml

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.