Re: [PR] Add workers.celery.topologySpreadConstraints & workers.kubernetes.topologySpreadConstraints [airflow]
jscheffl merged PR #64980: URL: https://github.com/apache/airflow/pull/64980 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add workers.celery.topologySpreadConstraints & workers.kubernetes.topologySpreadConstraints [airflow]
github-actions[bot] commented on PR #64980: URL: https://github.com/apache/airflow/pull/64980#issuecomment-4229344198 ### Backport successfully created: chart/v1-2x-test Note: As of [Merging PRs targeted for Airflow 3.X](https://github.com/apache/airflow/blob/main/dev/README_AIRFLOW3_DEV.md#merging-prs-targeted-for-airflow-3x) the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches. In matter of doubt please ask in [#release-management](https://apache-airflow.slack.com/archives/C03G9H97MM2) Slack channel. Status Branch Result ✅ chart/v1-2x-test https://github.com/apache/airflow/pull/65055";>https://img.shields.io/badge/PR-65055-blue"; alt="PR Link"> -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add workers.celery.topologySpreadConstraints & workers.kubernetes.topologySpreadConstraints [airflow]
Miretpl commented on code in PR #64980:
URL: https://github.com/apache/airflow/pull/64980#discussion_r3066902979
##
helm-tests/tests/helm_tests/airflow_core/test_worker.py:
##
@@ -664,23 +664,67 @@ def test_tolerations(self):
{"key": "dynamic-pods", "operator": "Equal", "value": "true",
"effect": "NoSchedule"}
]
-def test_topology_spread_constraints(self):
-expected_topology_spread_constraints = [
[email protected](
+"workers_values",
+[
+{
+"topologySpreadConstraints": [
+{
+"maxSkew": 1,
+"topologyKey": "foo",
+"whenUnsatisfiable": "ScheduleAnyway",
+"labelSelector": {"matchLabels": {"tier": "airflow"}},
+}
+]
+},
+{
+"celery": {
+"topologySpreadConstraints": [
+{
+"maxSkew": 1,
+"topologyKey": "foo",
+"whenUnsatisfiable": "ScheduleAnyway",
+"labelSelector": {"matchLabels": {"tier":
"airflow"}},
+}
+]
+}
+},
Review Comment:
Not sure if it is worth it, as everything is done in this way and will be
removed with version 2.0 of the chart. I will think about that.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Add workers.celery.topologySpreadConstraints & workers.kubernetes.topologySpreadConstraints [airflow]
Miretpl commented on code in PR #64980:
URL: https://github.com/apache/airflow/pull/64980#discussion_r3066897232
##
chart/values.schema.json:
##
@@ -2372,7 +2372,7 @@
}
},
"topologySpreadConstraints": {
-"description": "Specify topology spread constraints for
Airflow Celery worker pods and pods created with pod-template-file.",
+"description": "Specify topology spread constraints for
Airflow Celery worker pods and pods created with pod-template-file (deprecated,
use ``workers.celery.topologySpreadConstraints`` and/or
``workers.kubernetes.topologySpreadConstraints`` instead).",
Review Comment:
> The schema description uses double backticks (...), which is
reStructuredText-style and may render oddly/inconsistently in tooling that
expects plain text/JSON Schema descriptions. Consider switching to single
backticks (or no backticks) consistent with other schema descriptions in the
chart
Our docs render it correctly, so I'm leaving it as it is.
> optionally add a JSON Schema deprecated: true marker for machine-readable
deprecation.
I'm planning to prepare a separate PR for it with proper addition to the
documentation for users to have all deprecation information in one place in the
doc.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Add workers.celery.topologySpreadConstraints & workers.kubernetes.topologySpreadConstraints [airflow]
Miretpl commented on code in PR #64980:
URL: https://github.com/apache/airflow/pull/64980#discussion_r3066897232
##
chart/values.schema.json:
##
@@ -2372,7 +2372,7 @@
}
},
"topologySpreadConstraints": {
-"description": "Specify topology spread constraints for
Airflow Celery worker pods and pods created with pod-template-file.",
+"description": "Specify topology spread constraints for
Airflow Celery worker pods and pods created with pod-template-file (deprecated,
use ``workers.celery.topologySpreadConstraints`` and/or
``workers.kubernetes.topologySpreadConstraints`` instead).",
Review Comment:
Backticks -> our docs render it correctly, so I'm leaving it as it is.
`deprecated` addition -> I'm planning to prepare a separate PR for it with
proper addition to the documentation for users to have all deprecation
information in one place in the doc.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Add workers.celery.topologySpreadConstraints & workers.kubernetes.topologySpreadConstraints [airflow]
Miretpl commented on code in PR #64980:
URL: https://github.com/apache/airflow/pull/64980#discussion_r3066892695
##
chart/templates/NOTES.txt:
##
@@ -741,6 +741,14 @@ DEPRECATION WARNING:
{{- end }}
+{{- if not (empty .Values.workers.topologySpreadConstraints) }}
+
+ DEPRECATION WARNING:
+`workers.topologySpreadConstraints` has been renamed to
`workers.celery.topologySpreadConstraints`/`workers.kubernetes.topologySpreadConstraints`.
+Please change your values as support for the old name will be dropped in a
future release.
Review Comment:
Probably to do in a separate PR for the whole NOTES.txt file.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Add workers.celery.topologySpreadConstraints & workers.kubernetes.topologySpreadConstraints [airflow]
Copilot commented on code in PR #64980:
URL: https://github.com/apache/airflow/pull/64980#discussion_r3066477962
##
chart/templates/NOTES.txt:
##
@@ -741,6 +741,14 @@ DEPRECATION WARNING:
{{- end }}
+{{- if not (empty .Values.workers.topologySpreadConstraints) }}
+
+ DEPRECATION WARNING:
+`workers.topologySpreadConstraints` has been renamed to
`workers.celery.topologySpreadConstraints`/`workers.kubernetes.topologySpreadConstraints`.
+Please change your values as support for the old name will be dropped in a
future release.
Review Comment:
The deprecation warning uses a slash (`...celery...`/`...kubernetes...`)
which can read like a literal path rather than two alternatives. Prefer wording
that mirrors the PR description (e.g., '... renamed; use
`workers.celery.topologySpreadConstraints` and/or
`workers.kubernetes.topologySpreadConstraints`') to reduce ambiguity for users.
##
chart/values.schema.json:
##
@@ -2372,7 +2372,7 @@
}
},
"topologySpreadConstraints": {
-"description": "Specify topology spread constraints for
Airflow Celery worker pods and pods created with pod-template-file.",
+"description": "Specify topology spread constraints for
Airflow Celery worker pods and pods created with pod-template-file (deprecated,
use ``workers.celery.topologySpreadConstraints`` and/or
``workers.kubernetes.topologySpreadConstraints`` instead).",
Review Comment:
The schema description uses double backticks (``...``), which is
reStructuredText-style and may render oddly/inconsistently in tooling that
expects plain text/JSON Schema descriptions. Consider switching to single
backticks (or no backticks) consistent with other schema descriptions in the
chart, and optionally add a JSON Schema `deprecated: true` marker for
machine-readable deprecation.
```suggestion
"description": "Specify topology spread constraints for
Airflow Celery worker pods and pods created with pod-template-file (deprecated,
use `workers.celery.topologySpreadConstraints` and/or
`workers.kubernetes.topologySpreadConstraints` instead).",
"deprecated": true,
```
##
helm-tests/tests/helm_tests/airflow_core/test_worker.py:
##
@@ -664,23 +664,67 @@ def test_tolerations(self):
{"key": "dynamic-pods", "operator": "Equal", "value": "true",
"effect": "NoSchedule"}
]
-def test_topology_spread_constraints(self):
-expected_topology_spread_constraints = [
[email protected](
+"workers_values",
+[
+{
+"topologySpreadConstraints": [
+{
+"maxSkew": 1,
+"topologyKey": "foo",
+"whenUnsatisfiable": "ScheduleAnyway",
+"labelSelector": {"matchLabels": {"tier": "airflow"}},
+}
+]
+},
+{
+"celery": {
+"topologySpreadConstraints": [
+{
+"maxSkew": 1,
+"topologyKey": "foo",
+"whenUnsatisfiable": "ScheduleAnyway",
+"labelSelector": {"matchLabels": {"tier":
"airflow"}},
+}
+]
+}
+},
Review Comment:
The topology spread constraint object is repeated multiple times within the
parametrization. To make future edits less error-prone, consider extracting the
repeated constraint dict/list into a shared constant (or fixture) and reusing
it across the parametrized 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
