Miretpl commented on code in PR #61040:
URL: https://github.com/apache/airflow/pull/61040#discussion_r2726010389
##########
helm-tests/tests/helm_tests/security/test_security_context.py:
##########
@@ -1119,21 +1123,22 @@ def test_workers_sets_overwrite_local_container(self,
values):
],
},
},
- {
- "celery": {
- "enableDefault": False,
- "persistence": {"securityContexts": {"container":
{"allowPrivilegeEscalation": True}}},
- "sets": [
- {
- "name": "set1",
- "persistence": {
- "fixPermissions": True,
- "securityContexts": {"container":
{"allowPrivilegeEscalation": False}},
- },
- }
- ],
- }
- },
+ # securityContexts is not working in worker sets, to be fixed in
Helm Chart 2.0
+ # {
+ # "celery": {
+ # "enableDefault": False,
+ # "persistence": {"securityContexts": {"container":
{"allowPrivilegeEscalation": True}}},
+ # "sets": [
+ # {
+ # "name": "set1",
+ # "persistence": {
+ # "fixPermissions": True,
+ # "securityContexts": {"container":
{"allowPrivilegeEscalation": False}},
+ # },
+ # }
+ # ],
+ # }
+ # },
Review Comment:
It doesn't work due to a change in the logic. For the security context, the
overwrite behaviour looks like:
```
workers.celery -> workers.celery.sets -> workers
```
instead of:
```
workers.celery.sets -> workers.celery -> workers
```
where overwrite logic is from left to right
##########
helm-tests/tests/helm_tests/airflow_core/test_worker_sets.py:
##########
@@ -760,13 +760,15 @@ def test_overwrite_kerberos_init_container_enabled(self):
@pytest.mark.parametrize(
"values",
[
- {
- "celery": {
- "enableDefault": False,
- "kerberosInitContainer": {"enabled": True},
- "sets": [{"name": "test", "kerberosInitContainer":
{"enabled": False}}],
- }
- },
+ # kerberosInitContainer is not working in worker sets, to be fixed
in Helm Chart 2.0
+ # if this actually makes sense and is needed at all. I mostly
doubt.
+ # {
+ # "celery": {
+ # "enableDefault": False,
+ # "kerberosInitContainer": {"enabled": True},
+ # "sets": [{"name": "test", "kerberosInitContainer":
{"enabled": False}}],
+ # }
+ # },
Review Comment:
I would say disabling makes sense; the other options do not (Kerberos
container can be not needed e.g. in 2 worker sets out of 10, so it would be
easier to specify a global setting and disable for some, but it can also be the
other way around - at the end, I don't have a preference really).
It doesn't work due to the same change of logic, which I've mentioned in
workers, but here also the if statement in templates has an effect.
##########
helm-tests/tests/helm_tests/airflow_core/test_worker.py:
##########
@@ -1724,11 +1712,11 @@ def test_pod_management_policy_default(self):
({"celery": {"podManagementPolicy": "OrderedReady"}},
"OrderedReady"),
(
{"podManagementPolicy": "OrderedReady", "celery":
{"podManagementPolicy": "Parallel"}},
- "Parallel",
+ "OrderedReady",
Review Comment:
`celery` config, if specified, should not overwrite the `workers` config? 🤔
It makes the logic a little harder to follow, because in some cases we with
have overwrite behaviour like (e.g. `workers.command` field):
```
workers.celery.sets -> workers.celery -> workers
```
and in the other cases (e.g. `workers.podManagementPolicy`):
```
workers.celery -> workers.celery.sets -> workers
```
--
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]
