Add workers.celery.kerberosInitContainer field #60427
Conversation
henry3260
requested review from
dstandish,
hussein-awala,
jedcunningham and
jscheffl
as code owners
Miretpl
left a comment
Miretpl
left a comment
There was a problem hiding this comment.
@henry3260 there are some things which I believe could be improved to make moving this section consistent with previously moved sections. Could you take a look?
|
|
||
| {{- end }} | ||
|
|
||
| {{- if not (empty .Values.workers.kerberosInitContainer) }} |
There was a problem hiding this comment.
Is there any scenario that this condition will not be met? Cause I believe there isn't, and I think we should not print a deprecation message if the user will not use the deprecated section (e.g. that situation is when the section has all default values set).
There was a problem hiding this comment.
Is there any scenario that this condition will not be met? Cause I believe there isn't, and I think we should not print a deprecation message if the user will not use the deprecated section (e.g. that situation is when the section has all default values set).
Indeed, maybe we should print a deprecation message {{- if .Values.workers.kerberosInitContainer.enabled }}? WDYT
| """Test that workers.celery.kerberosInitContainer configuration works and takes precedence.""" | ||
| docs = render_chart( | ||
| values={ | ||
| "airflowVersion": "2.8.0", |
There was a problem hiding this comment.
I think its not needed, it should work on default Airflow version
| "workers": { | ||
| **workers_values, | ||
| "celery": { | ||
| **workers_values.get("celery", {}), | ||
| "persistence": {"fixPermissions": True}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
"workers": workers_valuespersistence is not required for kerberosInitContainer to be present
| initContainers = jmespath.search("spec.template.spec.initContainers", docs[0]) | ||
| # Should have 3 init containers: wait-for-migrations, kerberos-init, volume-permissions | ||
| assert len(initContainers) == 3 | ||
| assert initContainers[1]["name"] == "kerberos-init" | ||
| assert initContainers[1]["args"] == ["kerberos", "-o"] |
There was a problem hiding this comment.
Personally, I would split tests to test particular arguments and this one left only for testing enabled flag
| securityContexts: | ||
| container: {} | ||
|
|
||
| # Container level lifecycle hooks | ||
| containerLifecycleHooks: {} |
There was a problem hiding this comment.
I don't see that these sections are used anywhere. Am I missing something?
| {{- end }} | ||
| {{- end }} | ||
| {{- if and (semverCompare ">=2.8.0" .Values.airflowVersion) .Values.workers.kerberosInitContainer.enabled }} | ||
| {{- $kerberosInitContainerEnabled := or (.Values.workers.celery.kerberosInitContainer).enabled (.Values.workers.kerberosInitContainer).enabled }} |
There was a problem hiding this comment.
This behaviour does not match the behaviour which was previously done. for moving workers to workers.celery/kubernetes. Regarding the behaviour for false flag you can check #60238
3 participants
Was generative AI tooling used to co-author this PR?
copilot
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.