You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/01/03 21:20:10 UTC
[GitHub] [airflow] jedcunningham opened a new pull request #20641: Fix precedence of affinity, nodeSelector, and tolerations
jedcunningham opened a new pull request #20641:
URL: https://github.com/apache/airflow/pull/20641
Move the default affinity for components out of the component-level override, so we can have proper precedence of component-level, global, and default for each component.
Closes: #20538
--
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
[GitHub] [airflow] jedcunningham merged pull request #20641: Fix precedence of affinity, nodeSelector, and tolerations
Posted by GitBox <gi...@apache.org>.
jedcunningham merged pull request #20641:
URL: https://github.com/apache/airflow/pull/20641
--
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
[GitHub] [airflow] jedcunningham commented on a change in pull request #20641: Fix precedence of affinity, nodeSelector, and tolerations
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #20641:
URL: https://github.com/apache/airflow/pull/20641#discussion_r777761164
##########
File path: chart/tests/test_triggerer.py
##########
@@ -145,6 +145,67 @@ def test_should_create_valid_affinity_tolerations_and_node_selector(self):
docs[0],
)
+ def test_affinity_tolerations_and_node_selector_precedence(self):
+ """When given both global and triggerer affinity etc, triggerer affinity etc is used"""
+ docs = render_chart(
+ values={
+ "triggerer": {
+ "affinity": {
+ "nodeAffinity": {
+ "requiredDuringSchedulingIgnoredDuringExecution": {
+ "nodeSelectorTerms": [
+ {
+ "matchExpressions": [
+ {"key": "foo", "operator": "In", "values": ["true"]},
+ ]
+ }
+ ]
+ }
+ }
+ },
+ "tolerations": [
+ {"key": "dynamic-pods", "operator": "Equal", "value": "true", "effect": "NoSchedule"}
+ ],
+ "nodeSelector": {"type": "ssd"},
+ },
+ "affinity": {
+ "nodeAffinity": {
+ "requiredDuringSchedulingIgnoredDuringExecution": {
+ "nodeSelectorTerms": [
+ {
+ "matchExpressions": [
+ {"key": "not-me", "operator": "In", "values": ["true"]},
+ ]
+ }
+ ]
+ }
+ }
+ },
+ "tolerations": [
+ {"key": "not-me", "operator": "Equal", "value": "true", "effect": "NoSchedule"}
+ ],
+ "nodeSelector": {"type": "not-me"},
+ },
+ show_only=["templates/triggerer/triggerer-deployment.yaml"],
+ )
+
+ assert "foo" == jmespath.search(
+ "spec.template.spec.affinity.nodeAffinity."
+ "requiredDuringSchedulingIgnoredDuringExecution."
+ "nodeSelectorTerms[0]."
+ "matchExpressions[0]."
+ "key",
+ docs[0],
+ )
Review comment:
Take a look now.
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #20641: Fix precedence of affinity, nodeSelector, and tolerations
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20641:
URL: https://github.com/apache/airflow/pull/20641#discussion_r777803674
##########
File path: chart/tests/test_triggerer.py
##########
@@ -145,6 +145,67 @@ def test_should_create_valid_affinity_tolerations_and_node_selector(self):
docs[0],
)
+ def test_affinity_tolerations_and_node_selector_precedence(self):
+ """When given both global and triggerer affinity etc, triggerer affinity etc is used"""
+ docs = render_chart(
+ values={
+ "triggerer": {
+ "affinity": {
+ "nodeAffinity": {
+ "requiredDuringSchedulingIgnoredDuringExecution": {
+ "nodeSelectorTerms": [
+ {
+ "matchExpressions": [
+ {"key": "foo", "operator": "In", "values": ["true"]},
+ ]
+ }
+ ]
+ }
+ }
+ },
+ "tolerations": [
+ {"key": "dynamic-pods", "operator": "Equal", "value": "true", "effect": "NoSchedule"}
+ ],
+ "nodeSelector": {"type": "ssd"},
+ },
+ "affinity": {
+ "nodeAffinity": {
+ "requiredDuringSchedulingIgnoredDuringExecution": {
+ "nodeSelectorTerms": [
+ {
+ "matchExpressions": [
+ {"key": "not-me", "operator": "In", "values": ["true"]},
+ ]
+ }
+ ]
+ }
+ }
+ },
+ "tolerations": [
+ {"key": "not-me", "operator": "Equal", "value": "true", "effect": "NoSchedule"}
+ ],
+ "nodeSelector": {"type": "not-me"},
+ },
+ show_only=["templates/triggerer/triggerer-deployment.yaml"],
+ )
+
+ assert "foo" == jmespath.search(
+ "spec.template.spec.affinity.nodeAffinity."
+ "requiredDuringSchedulingIgnoredDuringExecution."
+ "nodeSelectorTerms[0]."
+ "matchExpressions[0]."
+ "key",
+ docs[0],
+ )
Review comment:
nice, looks 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
[GitHub] [airflow] dstandish commented on a change in pull request #20641: Fix precedence of affinity, nodeSelector, and tolerations
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20641:
URL: https://github.com/apache/airflow/pull/20641#discussion_r777716680
##########
File path: chart/UPDATING.rst
##########
@@ -50,6 +50,18 @@ The default Airflow image that is used with the Chart is now ``2.2.3``, previous
The old parameter names will continue to work, however support for them will be removed in a future release so please update your values file.
+Fixed precedence of ``nodeSelector``, ``affinity`` and ``tolerations`` params
+"""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""
+
+``nodeSelector``, ``affinity`` and ``tolerations`` params precedence has been fixed on all components. Now component-specific params
+(e.g. ``webserver.affinity``) takes precedence over the global param (e.g. ``affinity``).
+
+Default ``KubernetesExecutor`` worker affinity removed
+""""""""""""""""""""""""""""""""""""""""""""""""""""""
+
+Previously a default affinity was added to ``KubernetesExecutor`` workers to spread the workers out across nodes. This default affinity is no
+longer set as, in general, there is no reason to spread task-specific workers across nodes.
Review comment:
```suggestion
longer set because, in general, there is no reason to spread task-specific workers across nodes.
```
a nit for you
cus when you read `set as` it looks like it's supposed to be `set as _something_` and you just forgot the _something_... and `because` avoids this
##########
File path: chart/tests/test_triggerer.py
##########
@@ -145,6 +145,67 @@ def test_should_create_valid_affinity_tolerations_and_node_selector(self):
docs[0],
)
+ def test_affinity_tolerations_and_node_selector_precedence(self):
+ """When given both global and triggerer affinity etc, triggerer affinity etc is used"""
+ docs = render_chart(
+ values={
+ "triggerer": {
+ "affinity": {
+ "nodeAffinity": {
+ "requiredDuringSchedulingIgnoredDuringExecution": {
+ "nodeSelectorTerms": [
+ {
+ "matchExpressions": [
+ {"key": "foo", "operator": "In", "values": ["true"]},
+ ]
+ }
+ ]
+ }
+ }
+ },
+ "tolerations": [
+ {"key": "dynamic-pods", "operator": "Equal", "value": "true", "effect": "NoSchedule"}
+ ],
+ "nodeSelector": {"type": "ssd"},
+ },
+ "affinity": {
+ "nodeAffinity": {
+ "requiredDuringSchedulingIgnoredDuringExecution": {
+ "nodeSelectorTerms": [
+ {
+ "matchExpressions": [
+ {"key": "not-me", "operator": "In", "values": ["true"]},
+ ]
+ }
+ ]
+ }
+ }
+ },
+ "tolerations": [
+ {"key": "not-me", "operator": "Equal", "value": "true", "effect": "NoSchedule"}
+ ],
+ "nodeSelector": {"type": "not-me"},
+ },
+ show_only=["templates/triggerer/triggerer-deployment.yaml"],
+ )
+
+ assert "foo" == jmespath.search(
+ "spec.template.spec.affinity.nodeAffinity."
+ "requiredDuringSchedulingIgnoredDuringExecution."
+ "nodeSelectorTerms[0]."
+ "matchExpressions[0]."
+ "key",
+ docs[0],
+ )
Review comment:
you have verified that local beats global but it might be worth adding an assert to verify that, when local beats global, global is not _also_ there
we could imagine that they may or may not get merged some how
e.g. what if global has `requiredDuringSchedulingIgnoredDuringExecution` and local has `preferredDuringSchedulingIgnoredDuringExecution`
not suggesting a change in logic here, just that maybe we could amend this test (e.g. by changing one to preferred, and then in an assert showing that it is not there) to also prove what happens in this case
--
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
[GitHub] [airflow] jedcunningham commented on a change in pull request #20641: Fix precedence of affinity, nodeSelector, and tolerations
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #20641:
URL: https://github.com/apache/airflow/pull/20641#discussion_r777731406
##########
File path: chart/tests/test_triggerer.py
##########
@@ -145,6 +145,67 @@ def test_should_create_valid_affinity_tolerations_and_node_selector(self):
docs[0],
)
+ def test_affinity_tolerations_and_node_selector_precedence(self):
+ """When given both global and triggerer affinity etc, triggerer affinity etc is used"""
+ docs = render_chart(
+ values={
+ "triggerer": {
+ "affinity": {
+ "nodeAffinity": {
+ "requiredDuringSchedulingIgnoredDuringExecution": {
+ "nodeSelectorTerms": [
+ {
+ "matchExpressions": [
+ {"key": "foo", "operator": "In", "values": ["true"]},
+ ]
+ }
+ ]
+ }
+ }
+ },
+ "tolerations": [
+ {"key": "dynamic-pods", "operator": "Equal", "value": "true", "effect": "NoSchedule"}
+ ],
+ "nodeSelector": {"type": "ssd"},
+ },
+ "affinity": {
+ "nodeAffinity": {
+ "requiredDuringSchedulingIgnoredDuringExecution": {
+ "nodeSelectorTerms": [
+ {
+ "matchExpressions": [
+ {"key": "not-me", "operator": "In", "values": ["true"]},
+ ]
+ }
+ ]
+ }
+ }
+ },
+ "tolerations": [
+ {"key": "not-me", "operator": "Equal", "value": "true", "effect": "NoSchedule"}
+ ],
+ "nodeSelector": {"type": "not-me"},
+ },
+ show_only=["templates/triggerer/triggerer-deployment.yaml"],
+ )
+
+ assert "foo" == jmespath.search(
+ "spec.template.spec.affinity.nodeAffinity."
+ "requiredDuringSchedulingIgnoredDuringExecution."
+ "nodeSelectorTerms[0]."
+ "matchExpressions[0]."
+ "key",
+ docs[0],
+ )
Review comment:
Good point, I admit I was just following the existing tests. Let me look at 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
[GitHub] [airflow] github-actions[bot] commented on pull request #20641: Fix precedence of affinity, nodeSelector, and tolerations
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20641:
URL: https://github.com/apache/airflow/pull/20641#issuecomment-1004394717
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
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
[GitHub] [airflow] kaxil commented on a change in pull request #20641: Fix precedence of affinity, nodeSelector, and tolerations
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #20641:
URL: https://github.com/apache/airflow/pull/20641#discussion_r777842983
##########
File path: chart/templates/scheduler/scheduler-deployment.yaml
##########
@@ -93,7 +93,18 @@ spec:
nodeSelector:
{{ toYaml $nodeSelector | indent 8 }}
affinity:
+{{ if $affinity }}
{{ toYaml $affinity | indent 8 }}
+{{ else }}
Review comment:
```suggestion
{{- if $affinity }}
{{ toYaml $affinity | indent 8 }}
{{- else }}
```
nit
--
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