Miretpl commented on code in PR #60427:
URL: https://github.com/apache/airflow/pull/60427#discussion_r2696183728
##########
helm-tests/tests/helm_tests/airflow_core/test_worker.py:
##########
@@ -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",
+ "workers": {
+ **workers_values,
+ "celery": {
+ **workers_values.get("celery", {}),
+ "persistence": {"fixPermissions": True},
+ },
+ },
Review Comment:
```python
"workers": workers_values
```
`persistence` is not required for `kerberosInitContainer` to be present
##########
helm-tests/tests/helm_tests/airflow_core/test_worker.py:
##########
@@ -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",
Review Comment:
I think its not needed, it should work on default Airflow version
##########
chart/templates/workers/worker-deployment.yaml:
##########
@@ -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 }}
Review 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
##########
chart/values.yaml:
##########
@@ -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: {}
Review Comment:
I don't see that these sections are used anywhere. Am I missing something?
##########
chart/templates/NOTES.txt:
##########
@@ -204,6 +204,14 @@ DEPRECATION WARNING:
{{- end }}
+{{- if not (empty .Values.workers.kerberosInitContainer) }}
Review Comment:
@henry3260 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).
##########
helm-tests/tests/helm_tests/airflow_core/test_worker.py:
##########
@@ -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",
+ "workers": {
+ **workers_values,
+ "celery": {
+ **workers_values.get("celery", {}),
+ "persistence": {"fixPermissions": True},
+ },
+ },
+ },
+ 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"]
Review Comment:
Personally, I would split tests to test particular arguments and this one
left only for testing `enabled` flag
--
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]
