You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "mrrsm (via GitHub)" <gi...@apache.org> on 2025/12/04 19:52:19 UTC
[PR] fix(chart): Add pdbs for triggerer and workers [airflow]
mrrsm opened a new pull request, #59068:
URL: https://github.com/apache/airflow/pull/59068
## Description
This will add the ability to set pod disruption budget resources for either the triggerer and/or worker pods. These are disabled by default as to not cause a breaking change in the helm chart.
## Why
We are running multiple instances of triggerer pods as well as worker pods. When we have a cluster event that may take out a node having the PDB allows us to not have multiple instances of the same pods go down at the same time.
---
--
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 pdbs for triggerer and workers [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl merged PR #59068:
URL: https://github.com/apache/airflow/pull/59068
--
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 pdbs for triggerer and workers [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on PR #59068:
URL: https://github.com/apache/airflow/pull/59068#issuecomment-3651188488
@Miretpl as of the updated, previously you requested changes - OK to merge? (In my view it is good)
--
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 pdbs for triggerer and workers [airflow]
Posted by "Miretpl (via GitHub)" <gi...@apache.org>.
Miretpl commented on PR #59068:
URL: https://github.com/apache/airflow/pull/59068#issuecomment-3644072193
@mrrsm, consistency across the entire Helm Chart would be something nice to have, but I believe that combining everything in one PR can become a little harder to merge (based on my experience). Personally, I would probably do 2 additional PRs after this one:
1. one for making the PDB consistent between components
2. removal of other unneeded tests which are testing stuff for currently unsupported Kubernetes versions
--
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] fix(chart): Add pdbs for triggerer and workers [airflow]
Posted by "Miretpl (via GitHub)" <gi...@apache.org>.
Miretpl commented on PR #59068:
URL: https://github.com/apache/airflow/pull/59068#issuecomment-3618013053
Hi @mrrsm, could you add a test cases?
--
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 pdbs for triggerer and workers [airflow]
Posted by "Miretpl (via GitHub)" <gi...@apache.org>.
Miretpl commented on code in PR #59068:
URL: https://github.com/apache/airflow/pull/59068#discussion_r2599422737
##########
chart/templates/triggerer/triggerer-poddisruptionbudget.yaml:
##########
@@ -0,0 +1,46 @@
+{{/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+*/}}
+
+################################
+## Airflow Triggerer PodDisruptionBudget
+#################################
+{{- if .Values.triggerer.enabled }}
+{{- if .Values.triggerer.podDisruptionBudget.enabled }}
Review Comment:
```suggestion
{{- if and .Values.triggerer.enabled .Values.triggerer.podDisruptionBudget.enabled }}
```
##########
chart/templates/triggerer/triggerer-poddisruptionbudget.yaml:
##########
@@ -0,0 +1,46 @@
+{{/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+*/}}
+
+################################
+## Airflow Triggerer PodDisruptionBudget
+#################################
+{{- if .Values.triggerer.enabled }}
+{{- if .Values.triggerer.podDisruptionBudget.enabled }}
+apiVersion: policy/v1
+kind: PodDisruptionBudget
+metadata:
+ name: {{ include "airflow.fullname" . }}-triggerer-pdb
+ labels:
+ tier: airflow
+ component: triggerer
+ release: {{ .Release.Name }}
+ chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
+ heritage: {{ .Release.Service }}
+ {{- if or (.Values.labels) (.Values.triggerer.labels) }}
Review Comment:
```suggestion
{{- if or .Values.labels .Values.triggerer.labels }}
```
##########
helm-tests/tests/helm_tests/airflow_core/test_pdb_worker.py:
##########
@@ -0,0 +1,64 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+import jmespath
+from chart_utils.helm_template_generator import render_chart
+
+
+class TestWorkerPdb:
+ """Tests Worker PDB."""
+
+ def test_should_pass_validation_with_just_pdb_enabled_v1(self):
+ render_chart(
+ values={"workers": {"podDisruptionBudget": {"enabled": True}}},
+ show_only=["templates/workers/worker-poddisruptionbudget.yaml"],
+ ) # checks that no validation exception is raised
+
+ def test_should_pass_validation_with_just_pdb_enabled_v1beta1(self):
+ render_chart(
+ values={"workers": {"podDisruptionBudget": {"enabled": True}}},
+ show_only=["templates/workers/worker-poddisruptionbudget.yaml"],
+ kubernetes_version="1.16.0",
Review Comment:
Same as above, we do not support Kubernetes in 1.16 version
##########
helm-tests/tests/helm_tests/airflow_core/test_pdb_triggerer.py:
##########
@@ -0,0 +1,64 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+import jmespath
+from chart_utils.helm_template_generator import render_chart
+
+
+class TestTriggererPdb:
+ """Tests Triggerer PDB."""
+
+ def test_should_pass_validation_with_just_pdb_enabled_v1(self):
+ render_chart(
+ values={"triggerer": {"podDisruptionBudget": {"enabled": True}}},
+ show_only=["templates/triggerer/triggerer-poddisruptionbudget.yaml"],
+ ) # checks that no validation exception is raised
+
+ def test_should_pass_validation_with_just_pdb_enabled_v1beta1(self):
+ render_chart(
+ values={"triggerer": {"podDisruptionBudget": {"enabled": True}}},
+ show_only=["templates/triggerer/triggerer-poddisruptionbudget.yaml"],
+ kubernetes_version="1.16.0",
Review Comment:
According to the documentation, the Helm Chart supports Kubernetes clusters 1.30+, so I would say that this test should not be necessary
##########
chart/templates/workers/worker-poddisruptionbudget.yaml:
##########
@@ -0,0 +1,44 @@
+{{/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+*/}}
+
+################################
+## Airflow Worker PodDisruptionBudget
+#################################
+{{- if .Values.workers.podDisruptionBudget.enabled }}
+apiVersion: policy/v1
+kind: PodDisruptionBudget
+metadata:
+ name: {{ include "airflow.fullname" . }}-worker-pdb
+ labels:
+ tier: airflow
+ component: worker
+ release: {{ .Release.Name }}
+ chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
+ heritage: {{ .Release.Service }}
+ {{- if or (.Values.labels) (.Values.workers.labels) }}
Review Comment:
```suggestion
{{- if or .Values.labels .Values.workers.labels }}
```
--
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] fix(chart): Add pdbs for triggerer and workers [airflow]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #59068:
URL: https://github.com/apache/airflow/pull/59068#issuecomment-3614080140
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:
- Pay attention to the quality of your code (ruff, mypy and type annotations). Our [prek-hooks]( https://github.com/apache/airflow/blob/main/contributing-docs/08_static_code_checks.rst#prerequisites-for-prek-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/airflow-core/docs/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/dev/breeze/doc/README.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#coding-style-and-best-practices).
- Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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 pdbs for triggerer and workers [airflow]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #59068:
URL: https://github.com/apache/airflow/pull/59068#issuecomment-3657611006
Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional 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 pdbs for triggerer and workers [airflow]
Posted by "mrrsm (via GitHub)" <gi...@apache.org>.
mrrsm commented on PR #59068:
URL: https://github.com/apache/airflow/pull/59068#issuecomment-3637516220
@Miretpl Thank you for the review/changes.
In an effort to not add a different way to do PDBs should I also update the PDB's and tests that already exist for other components (scheduler, api-server, etc) to match the style in this PR and also update the tests to remove the unneeded kubernetes version test?
--
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 pdbs for triggerer and workers [airflow]
Posted by "Miretpl (via GitHub)" <gi...@apache.org>.
Miretpl commented on code in PR #59068:
URL: https://github.com/apache/airflow/pull/59068#discussion_r2617536091
##########
chart/templates/triggerer/triggerer-poddisruptionbudget.yaml:
##########
@@ -0,0 +1,45 @@
+{{/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+*/}}
+
+################################
+## Airflow Triggerer PodDisruptionBudget
+#################################
+{{- if and .Values.triggerer.enabled .Values.triggerer.podDisruptionBudget.enabled }}
+apiVersion: policy/v1
+kind: PodDisruptionBudget
+metadata:
+ name: {{ include "airflow.fullname" . }}-triggerer-pdb
+ labels:
+ tier: airflow
+ component: triggerer
+ release: {{ .Release.Name }}
+ chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
+ heritage: {{ .Release.Service }}
+ {{- if or .Values.labels .Values.triggerer.labels }}
+ {{- mustMerge .Values.triggerer.labels .Values.labels | toYaml | nindent 4 }}
+ {{- end }}
+spec:
+ selector:
+ matchLabels:
+ tier: airflow
+ component: triggerer
+ release: {{ .Release.Name }}
+ {{- toYaml .Values.triggerer.podDisruptionBudget.config | nindent 2 }}
+{{- end }}
+{{- end }}
Review Comment:
```suggestion
{{- end }}
```
I guess, this fixes some failed tests
--
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