Ensure back-compat for new Celery settings #61040
Conversation
jscheffl
force-pushed
the
bugfix/ensure-backcompat-for-helm-celery-settings
branch
from
3b726d9 to
8c20bef
Compare
jscheffl
requested review from
dstandish,
hussein-awala and
jedcunningham
as code owners
| {{- $mergedWorkers := (include "workersMergeValues" (list .Values.workers $filteredCelery "" (list "kerberosInitContainer")) | fromYaml) -}} | ||
| {{- $mergedWorkers := (include "workersMergeValues" (list $filteredCelery .Values.workers "" (list "kerberosInitContainer")) | fromYaml) -}} |
There was a problem hiding this comment.
Reviewer note: THis is the core change. All other adjustments are mainly fixes needed to make CI green
| # 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}}, | ||
| # }, | ||
| # } | ||
| # ], | ||
| # } | ||
| # }, |
There was a problem hiding this 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
| # 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}}], | ||
| # } | ||
| # }, |
There was a problem hiding this 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.
| {"podManagementPolicy": "OrderedReady", "celery": {"podManagementPolicy": "Parallel"}}, | ||
| "Parallel", | ||
| "OrderedReady", |
There was a problem hiding this 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
|
Closing as #61049 has been merged. |
2 participants
As mentioned in Slack in release-management and discussed the recent changes in Helm Chart which move settings from
workersection toworker.celeryare breaking previous configurations for people upgrading...This PR attempts to fix the Helm chart such that config is not a breaking change but stays backwards compatible.
It does so by reversing the order of priority and removing the defaults of the deprecated values.
FYI @jedcunningham @Miretpl
Note: I needed to disable 3 tests as it seems the implementation of "worker sets" is a bit limited. But for me fixing (short term) is not reasonable to make 1.19.0 release... as I see also a questionmark why different security settings per worker set or different configs for kerberosInit containers would be needed. This is "just a pragmatic move" and would propose the complexity to be cleaned in Helm Chart 2.0
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.