Miretpl commented on code in PR #72555:
URL: https://github.com/apache/airflow/pull/72555#discussion_r4041471447
##########
chart/templates/_helpers.yaml:
##########
@@ -1127,15 +1127,10 @@ Usage:
{{- end }}
{{/*
-Custom merge function which enables full map overwrite and `or` logic for
boolean overwrite.
Review Comment:
That's a bit of an unrelated change.
##########
chart/templates/NOTES.txt:
##########
@@ -150,3 +150,15 @@
https://airflow.apache.org/docs/helm-chart/stable/production-guide.html#api-secr
{{- if ne .Values.executor (tpl .Values.config.core.executor $) }}
{{ fail "Please configure the executor with `executor`, not
`config.core.executor`." }}
{{- end }}
+
Review Comment:
Rather for 1.2x line as a warning than a hard fail. On main, it will be
sufficient to have a proper values.schema.json file to handle that.
##########
chart/tests/helm_tests/airflow_aux/test_pod_template_file.py:
##########
@@ -1293,6 +1296,80 @@ def
test_runtime_class_name_values_are_configurable(self):
assert jmespath.search("spec.runtimeClassName", docs[0]) == "nvidia"
+ def test_kerberos_sidecar_is_native_sidecar(self):
+ docs = render_chart(
+ values={"workers": {"kubernetes": {"kerberosSidecar": {"enabled":
True}}}},
+ show_only=["templates/pod-template-file.yaml"],
+ chart_dir=self.temp_chart_dir,
+ )
+ sidecar =
jmespath.search("spec.initContainers[?name=='worker-kerberos'] | [0]", docs[0])
+ assert sidecar is not None
+ assert sidecar["restartPolicy"] == "Always"
+ assert jmespath.search("spec.containers[?name=='worker-kerberos'] |
[0]", docs[0]) is None
+
+ @pytest.mark.parametrize(
+ ("sidecar_enabled", "probe_enabled", "expected_names"),
+ [
+ (False, True, []),
+ (True, True, ["worker-kerberos"]),
+ (True, False, ["worker-kerberos"]),
+ ],
+ )
+ def test_kerberos_initialization(self, sidecar_enabled, probe_enabled,
expected_names):
Review Comment:
I would separate this test case per `sidecar_enabled` flag. It will simplify
the logic a bit.
##########
chart/tests/helm_tests/security/test_kerberos.py:
##########
@@ -60,6 +76,64 @@ def
test_kerberos_envs_available_in_worker_with_persistence(self):
"spec.template.spec.containers[0].env", docs[0]
)
+ def test_kerberos_sidecar_is_native_sidecar(self):
+ docs = render_chart(
+ values={
+ "executor": "CeleryExecutor",
+ "workers": {"celery": {"kerberosSidecar": {"enabled": True}}},
+ },
+ show_only=["templates/workers/worker-deployment.yaml"],
+ )
+ sidecar = jmespath.search(
+ "spec.template.spec.initContainers[?name=='worker-kerberos'] |
[0]", docs[0]
+ )
+ assert sidecar is not None
+ assert sidecar["restartPolicy"] == "Always"
+ assert (
Review Comment:
This verification is rather not needed.
##########
chart/tests/helm_tests/airflow_aux/test_pod_template_file.py:
##########
@@ -1421,10 +1501,10 @@ def test_kerberos_sidecar_startup_probe(self, override,
expected):
chart_dir=self.temp_chart_dir,
)
- assert (
- jmespath.search("spec.containers[?name=='worker-kerberos'] |
[0].startupProbe", docs[0])
- == expected
- )
+ sidecar =
jmespath.search("spec.initContainers[?name=='worker-kerberos'] | [0]", docs[0])
+ assert sidecar is not None
+ assert sidecar.get("restartPolicy") == "Always"
+ assert sidecar.get("startupProbe") == expected
Review Comment:
```suggestion
assert sidecar["restartPolicy"] == "Always"
assert sidecar["startupProbe"] == expected
```
I find `KeyError` message clearer than `None == Always`, but maybe that is
opinionaited a bit 🤔
##########
chart/values.yaml:
##########
@@ -844,37 +844,16 @@ workers:
# Container level lifecycle hooks
containerLifecycleHooks: {}
- # Startup probe for the kerberos sidecar: `klist -s` succeeds once the
credential
- # cache holds a valid, unexpired ticket. Disable for custom images
without `klist`.
+ # Wait for a valid Kerberos ticket before starting worker or task
containers.
Review Comment:
Similar change as in `values.schema.json` file.
##########
chart/values.schema.json:
##########
@@ -2123,7 +2123,7 @@
"default": false
},
"startupProbe": {
- "description": "Startup probe for the
Kerberos worker sidecar (runs `klist -s`).",
+ "description": "Wait for a valid Kerberos
ticket before starting worker or task containers (runs `klist -s`). Disabling
this probe allows them to start before a ticket is available.",
Review Comment:
```suggestion
"description": "Wait for a valid
Kerberos ticket before starting the Airflow Celery worker (runs `klist -s`).
Disabling this probe allows them to start before a ticket is available.",
```
##########
chart/tests/helm_tests/airflow_aux/test_pod_template_file.py:
##########
@@ -1293,6 +1296,80 @@ def
test_runtime_class_name_values_are_configurable(self):
assert jmespath.search("spec.runtimeClassName", docs[0]) == "nvidia"
+ def test_kerberos_sidecar_is_native_sidecar(self):
+ docs = render_chart(
+ values={"workers": {"kubernetes": {"kerberosSidecar": {"enabled":
True}}}},
+ show_only=["templates/pod-template-file.yaml"],
+ chart_dir=self.temp_chart_dir,
+ )
+ sidecar =
jmespath.search("spec.initContainers[?name=='worker-kerberos'] | [0]", docs[0])
+ assert sidecar is not None
+ assert sidecar["restartPolicy"] == "Always"
+ assert jmespath.search("spec.containers[?name=='worker-kerberos'] |
[0]", docs[0]) is None
+
+ @pytest.mark.parametrize(
+ ("sidecar_enabled", "probe_enabled", "expected_names"),
+ [
+ (False, True, []),
+ (True, True, ["worker-kerberos"]),
+ (True, False, ["worker-kerberos"]),
+ ],
+ )
+ def test_kerberos_initialization(self, sidecar_enabled, probe_enabled,
expected_names):
+ docs = render_chart(
+ values={
+ "workers": {
+ "kubernetes": {
+ "kerberosSidecar": {
+ "enabled": sidecar_enabled,
+ "startupProbe": {"enabled": probe_enabled},
+ },
+ }
+ }
+ },
+ show_only=["templates/pod-template-file.yaml"],
+ chart_dir=self.temp_chart_dir,
+ )
+ assert (jmespath.search("spec.initContainers[].name", docs[0]) or [])
== expected_names
+ assert (
+ jmespath.search('metadata.annotations."checksum/kerberos-keytab"',
docs[0]) is not None
+ ) == sidecar_enabled
+ if sidecar_enabled:
+ sidecar =
jmespath.search("spec.initContainers[?name=='worker-kerberos'] | [0]", docs[0])
+ assert sidecar["args"] == ["kerberos"]
+ assert sidecar["restartPolicy"] == "Always"
+ assert ("startupProbe" in sidecar) == probe_enabled
+
+ def test_pod_override_reconciliation_with_kerberos_sidecar(self):
+ docs = render_chart(
+ values={"workers": {"kubernetes": {"kerberosSidecar": {"enabled":
True}}}},
+ show_only=["templates/pod-template-file.yaml"],
+ chart_dir=self.temp_chart_dir,
+ )
+ base_pod = PodGenerator.deserialize_model_dict(docs[0])
Review Comment:
Why that way?
##########
chart/values.schema.json:
##########
@@ -2915,7 +2832,7 @@
"default": false
},
"startupProbe": {
- "description": "Startup probe for the
Kerberos worker sidecar (runs `klist -s`).",
+ "description": "Wait for a valid Kerberos
ticket before starting worker or task containers (runs `klist -s`). Disabling
this probe allows them to start before a ticket is available.",
Review Comment:
```suggestion
"description": "Wait for a valid
Kerberos ticket before starting pod-template-file base container (runs `klist
-s`). Disabling this probe allows them to start before a ticket is available.",
```
--
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]
