-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Add workers.celery.kerberosInitContainer field #60427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -196,12 +196,14 @@ spec: | |
| subPath: {{ .Values.logs.persistence.subPath }} | ||
| {{- 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 }} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This behaviour does not match the behaviour which was previously done. for moving |
||
| {{- $kerberosInitContainerResources := (.Values.workers.celery.kerberosInitContainer).resources | default (.Values.workers.kerberosInitContainer).resources | default dict }} | ||
| {{- if and (semverCompare ">=2.8.0" .Values.airflowVersion) $kerberosInitContainerEnabled }} | ||
| - name: kerberos-init | ||
| image: {{ template "airflow_image" . }} | ||
| imagePullPolicy: {{ .Values.images.airflow.pullPolicy }} | ||
| args: ["kerberos", "-o"] | ||
| resources: {{- toYaml .Values.workers.kerberosInitContainer.resources | nindent 12 }} | ||
| resources: {{- toYaml $kerberosInitContainerResources | nindent 12 }} | ||
| volumeMounts: | ||
| - name: logs | ||
| mountPath: {{ template "airflow_logs" . }} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -871,6 +871,7 @@ workers: | |
| containerLifecycleHooks: {} | ||
|
|
||
| # Kerberos init container configuration for Airflow Celery workers and pods created with pod-template-file | ||
| # (deprecated, use `workers.celery.kerberosInitContainer` instead) | ||
| kerberosInitContainer: | ||
| # Enable kerberos init container | ||
| enabled: false | ||
|
|
@@ -1152,6 +1153,25 @@ workers: | |
| securityContexts: | ||
| container: {} | ||
|
|
||
| # Kerberos init container configuration for Airflow Celery workers | ||
| kerberosInitContainer: | ||
| # Enable kerberos init container | ||
| enabled: false | ||
| resources: {} | ||
| # limits: | ||
| # cpu: 100m | ||
| # memory: 128Mi | ||
| # requests: | ||
| # cpu: 100m | ||
| # memory: 128Mi | ||
|
|
||
| # Detailed default security context for kerberos init container on container level | ||
| securityContexts: | ||
| container: {} | ||
|
|
||
| # Container level lifecycle hooks | ||
| containerLifecycleHooks: {} | ||
|
Comment on lines
+1169
to
+1173
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see that these sections are used anywhere. Am I missing something? |
||
|
|
||
| kubernetes: | ||
| # Command to use in pod-template-file (templated) | ||
| command: ~ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -970,6 +970,67 @@ def test_airflow_kerberos_init_container( | |
| assert initContainers[1]["name"] == "kerberos-init" | ||
| assert initContainers[1]["args"] == ["kerberos", "-o"] | ||
|
|
||
| @pytest.mark.parametrize( | ||
| "workers_values", | ||
| [ | ||
| {"kerberosInitContainer": {"enabled": True}}, | ||
| {"celery": {"kerberosInitContainer": {"enabled": True}}}, | ||
| { | ||
| "kerberosInitContainer": {"enabled": False}, | ||
| "celery": {"kerberosInitContainer": {"enabled": True}}, | ||
| }, | ||
| ], | ||
| ) | ||
| def test_airflow_kerberos_init_container_celery_values(self, workers_values): | ||
| """Test that workers.celery.kerberosInitContainer configuration works and takes precedence.""" | ||
| docs = render_chart( | ||
| values={ | ||
| "airflowVersion": "2.8.0", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think its not needed, it should work on default Airflow version |
||
| "workers": { | ||
| **workers_values, | ||
| "celery": { | ||
| **workers_values.get("celery", {}), | ||
| "persistence": {"fixPermissions": True}, | ||
| }, | ||
| }, | ||
|
Comment on lines
+989
to
+995
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "workers": workers_values
|
||
| }, | ||
| show_only=["templates/workers/worker-deployment.yaml"], | ||
| ) | ||
|
|
||
| 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"] | ||
|
Comment on lines
+1000
to
+1004
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally, I would split tests to test particular arguments and this one left only for testing |
||
|
|
||
| def test_airflow_kerberos_init_container_resources(self): | ||
| """Test that kerberos init container resources can be configured via workers.celery.kerberosInitContainer.""" | ||
| docs = render_chart( | ||
| values={ | ||
| "airflowVersion": "2.8.0", | ||
| "workers": { | ||
| "celery": { | ||
| "kerberosInitContainer": { | ||
| "enabled": True, | ||
| "resources": { | ||
| "limits": {"cpu": "100m", "memory": "128Mi"}, | ||
| "requests": {"cpu": "50m", "memory": "64Mi"}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| show_only=["templates/workers/worker-deployment.yaml"], | ||
| ) | ||
|
|
||
| initContainers = jmespath.search("spec.template.spec.initContainers", docs[0]) | ||
| kerberos_init = next((c for c in initContainers if c["name"] == "kerberos-init"), None) | ||
| assert kerberos_init is not None | ||
| assert kerberos_init["resources"]["limits"]["cpu"] == "100m" | ||
| assert kerberos_init["resources"]["limits"]["memory"] == "128Mi" | ||
| assert kerberos_init["resources"]["requests"]["cpu"] == "50m" | ||
| assert kerberos_init["resources"]["requests"]["memory"] == "64Mi" | ||
|
|
||
| @pytest.mark.parametrize( | ||
| ("airflow_version", "expected_arg"), | ||
| [ | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, maybe we should print a deprecation message
{{- if .Values.workers.kerberosInitContainer.enabled }}? WDYTThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WIll be good