[GitHub] [airflow] dstandish commented on a change in pull request #20641: Fix precedence of affinity, nodeSelector, and tolerations

[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