You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Owen-CH-Leung (via GitHub)" <gi...@apache.org> on 2023/10/03 15:20:10 UTC
[PR] Add k8s recommended labels [airflow]
Owen-CH-Leung opened a new pull request, #34735:
URL: https://github.com/apache/airflow/pull/34735
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](https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/#labels) 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add k8s recommended labels [airflow]
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on PR #34735:
URL: https://github.com/apache/airflow/pull/34735#issuecomment-1753105429
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add k8s recommended labels [airflow]
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on PR #34735:
URL: https://github.com/apache/airflow/pull/34735#issuecomment-1755387465
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add k8s recommended labels [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #34735: Add k8s recommended labels
URL: https://github.com/apache/airflow/pull/34735
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add k8s recommended labels [airflow]
Posted by "kimminw00 (via GitHub)" <gi...@apache.org>.
kimminw00 commented on PR #34735:
URL: https://github.com/apache/airflow/pull/34735#issuecomment-1752000485
I think it would be helpful to refer to the prometheus helm chart.
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add k8s recommended labels [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34735:
URL: https://github.com/apache/airflow/pull/34735#issuecomment-1858993645
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add k8s recommended labels [airflow]
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on PR #34735:
URL: https://github.com/apache/airflow/pull/34735#issuecomment-1768322831
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.
Let me know your thought on this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add k8s recommended labels [airflow]
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on PR #34735:
URL: https://github.com/apache/airflow/pull/34735#issuecomment-1749902752
> 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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add k8s recommended labels [airflow]
Posted by "kimminw00 (via GitHub)" <gi...@apache.org>.
kimminw00 commented on PR #34735:
URL: https://github.com/apache/airflow/pull/34735#issuecomment-1749085126
I think it is necessary to differentiate <code>app.kubernetes.io/name</code> of each object.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add k8s recommended labels [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34735:
URL: https://github.com/apache/airflow/pull/34735#issuecomment-2027836374
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add k8s recommended labels [airflow]
Posted by "kimminw00 (via GitHub)" <gi...@apache.org>.
kimminw00 commented on PR #34735:
URL: https://github.com/apache/airflow/pull/34735#issuecomment-1772292764
If <code>airflow.fullname</code> is longer than 63 chars, then <code>app.kubernetes.io/name</code> is longer than 63 chars.
(For example, <code>{{ include "airflow.fullname" . }}-dag-processor</code> should not 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 <code>{{ include "airflow.fullname" . }}</code> first. And
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add k8s recommended labels [airflow]
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on PR #34735:
URL: https://github.com/apache/airflow/pull/34735#issuecomment-1782188337
@jedcunningham @hussein-awala Can I seek your feedback for this PR also ? Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add k8s recommended labels [airflow]
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on PR #34735:
URL: https://github.com/apache/airflow/pull/34735#issuecomment-1788933112
> > I think you should also have to consider `Label selectors`.
>
> This means replacing the previous labels with the K8S recommended labels in `label selector`s. 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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add k8s recommended labels [airflow]
Posted by "kimminw00 (via GitHub)" <gi...@apache.org>.
kimminw00 commented on PR #34735:
URL: https://github.com/apache/airflow/pull/34735#issuecomment-1753271296
And please take a close look at how each object(secret, pv, ingress etc.) is labeled in prometheus helm chart.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add k8s recommended labels [airflow]
Posted by "kimminw00 (via GitHub)" <gi...@apache.org>.
kimminw00 commented on PR #34735:
URL: https://github.com/apache/airflow/pull/34735#issuecomment-1763290879
Prometheus and alertmanager are different apps. So they have different <code>app.kubernetes.io/name</code>
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 <code>app.kubernetes.io/name</code> in the same way.
(But I'm not sure that webserver, scheduler, dag processor, triggerer, etc., are different apps as they run 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add k8s recommended labels [airflow]
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on PR #34735:
URL: https://github.com/apache/airflow/pull/34735#issuecomment-1783689269
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add k8s recommended labels [airflow]
Posted by "kimminw00 (via GitHub)" <gi...@apache.org>.
kimminw00 commented on PR #34735:
URL: https://github.com/apache/airflow/pull/34735#issuecomment-1783677924
I think you should also have to consider <code>Label selectors</code>.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add k8s recommended labels [airflow]
Posted by "kimminw00 (via GitHub)" <gi...@apache.org>.
kimminw00 commented on PR #34735:
URL: https://github.com/apache/airflow/pull/34735#issuecomment-1784790148
> I think you should also have to consider <code>Label selectors</code>.
This means replacing the previous labels with the K8S recommended labels in <code>label selector</code>s.
There may be problems due to replacement, so I think it would be good idea to leave it as is.
I'd like to hear what others think!
Please add recommemded labels to the <code>chart/files/pod-template-file.kubernetes-helm-yaml</code> file as well!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add k8s recommended labels [airflow]
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on PR #34735:
URL: https://github.com/apache/airflow/pull/34735#issuecomment-1773860191
I've applied the fix here to truncate `airflow.fullname` to return at most 40 chars.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add k8s recommended labels [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34735:
URL: https://github.com/apache/airflow/pull/34735#issuecomment-1924928004
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org