Miretpl commented on code in PR #72206:
URL: https://github.com/apache/airflow/pull/72206#discussion_r4495589961
##########
chart/templates/workers/worker-deployment.yaml:
##########
@@ -262,7 +263,9 @@ spec:
envFrom: {{- include "custom_airflow_environment_from" . | default
"\n []" | indent 10 }}
env:
{{- include "custom_airflow_environment" . | indent 10 }}
+ {{- /* This init container queries the migration state, so it
needs the database regardless of KEDA. */}}
Review Comment:
This comment should rather be one line below.
##########
chart/tests/helm_tests/airflow_aux/test_airflow_common.py:
##########
@@ -455,6 +462,176 @@ def test_jwt_secret_can_be_disabled(self):
)
assert "AIRFLOW__API_AUTH__JWT_SECRET" not in env_names, f"Wrong
vars in {component}"
+ def
test_worker_keeps_sql_alchemy_conn_for_the_default_result_backend(self):
+ """Celery derives its result backend from sql_alchemy_conn when none
is configured."""
+ docs =
render_chart(show_only=["templates/workers/worker-deployment.yaml"])
+ names =
set(jmespath.search("spec.template.spec.containers[].env[].name", docs[0]) or
[])
+ assert names & METADATA_DB_VARS ==
{"AIRFLOW__DATABASE__SQL_ALCHEMY_CONN"}
+
+ @pytest.mark.parametrize(
+ "values",
+ [
+ pytest.param({"data": {"resultBackendSecretName":
"result-backend"}}, id="secret-name"),
+ pytest.param(
+ {
+ "data": {
+ "resultBackendConnection": {
+ "protocol": "postgresql",
+ "host": "results",
+ "port": 5432,
+ "user": "celery",
+ "pass": "x",
+ "db": "results",
+ "sslmode": "disable",
+ }
+ }
+ },
+ id="connection",
+ ),
+ pytest.param({"config": {"celery": {"result_backend":
"redis://r:6379/1"}}}, id="config"),
+ pytest.param({"config": {"celery": {"result_backend_cmd": "cat
/backend"}}}, id="config-cmd"),
+ pytest.param({"config": {"celery": {"result_backend_secret":
"backend"}}}, id="config-secret"),
+ ],
+ )
+ def test_metadata_db_env_absent_in_workers_with_a_result_backend(self,
values):
+ docs = render_chart(values=values,
show_only=["templates/workers/worker-deployment.yaml"])
+ names =
set(jmespath.search("spec.template.spec.containers[].env[].name", docs[0]) or
[])
+ assert not (names & METADATA_DB_VARS)
+
+ def
test_worker_keeps_sql_alchemy_conn_when_the_result_backend_secret_is_not_exposed(self):
+ docs = render_chart(
+ values={
+ "data": {"resultBackendSecretName": "result-backend"},
+ "enableBuiltInSecretEnvVars":
{"AIRFLOW__CELERY__RESULT_BACKEND": False},
+ },
+ show_only=["templates/workers/worker-deployment.yaml"],
+ )
+ names =
jmespath.search("spec.template.spec.containers[?name=='worker'].env[].name",
docs[0])
+ assert "AIRFLOW__DATABASE__SQL_ALCHEMY_CONN" in names
+
+ def test_worker_kerberos_sidecar_has_no_metadata_db_env_with_keda(self):
+ """KEDA reads the worker container's environment only."""
+ docs = render_chart(
+ values={
+ "airflowVersion": "3.4.0",
+ "workers": {"celery": {"keda": {"enabled": True},
"kerberosSidecar": {"enabled": True}}},
+ },
+ show_only=["templates/workers/worker-deployment.yaml"],
+ )
+ names = jmespath.search(
+
"spec.template.spec.initContainers[?name=='worker-kerberos'].env[].name",
docs[0]
+ )
+ assert names
+ assert not (set(names) & METADATA_DB_VARS)
+
+ @pytest.mark.parametrize(
+ ("values", "containers"),
+ [
+ pytest.param(
+ {"workers": {"celery": {"kerberosSidecar": {"enabled":
True}}}},
+ {"worker-kerberos"},
+ id="default-result-backend",
+ ),
+ pytest.param(
+ {
+ "data": {"resultBackendSecretName": "result-backend"},
+ "workers": {"celery": {"kerberosSidecar": {"enabled":
True}}},
+ },
+ {"worker", "worker-kerberos"},
+ id="separate-result-backend",
+ ),
+ ],
+ )
+ def
test_engine_args_cleared_in_worker_containers_without_the_connection(self,
values, containers):
+ """Those containers fall back to SQLite, which rejects engine args
meant for the real database."""
+ docs = render_chart(
+ values={
+ **values,
+ "airflowVersion": "3.4.0",
+ "config": {"database": {"sql_alchemy_engine_args":
'{"executemany_mode": "values"}'}},
+ },
+ show_only=["templates/workers/worker-deployment.yaml"],
+ )
+ pod_spec = docs[0]["spec"]["template"]["spec"]
+ cleared = {
+ container["name"]
+ for container in pod_spec["containers"] +
pod_spec["initContainers"]
+ if {"name": "AIRFLOW__DATABASE__SQL_ALCHEMY_ENGINE_ARGS", "value":
"{}"}
+ in container.get("env", [])
+ }
Review Comment:
Why this way and not jmespath (this one also breaks convention for how tests
are written)?
*This is a question for the majority of tests added in this PR*
--
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]
