Miretpl commented on code in PR #70213:
URL: https://github.com/apache/airflow/pull/70213#discussion_r3646781022
##########
chart/templates/_helpers.yaml:
##########
@@ -158,6 +148,21 @@ If release name contains chart name it will be used as a
full name.
{{- end }}
{{- end }}
+{{/* KEDA scaler database connection for the worker and triggerer autoscalers
*/}}
+{{- define "keda_airflow_environment" }}
+ {{- $triggererKedaEnabled := and .Values.triggerer.enabled
.Values.triggerer.keda.enabled }}
+ {{- $workersKedaNeedsDbConn := and .Values.workers.celery.keda.enabled (or
(eq .Values.data.metadataConnection.protocol "mysql") (and
.Values.pgbouncer.enabled (not .Values.workers.celery.keda.usePgbouncer))) }}
+ {{- $triggererKedaNeedsDbConn := and $triggererKedaEnabled (or (eq
.Values.data.metadataConnection.protocol "mysql") (and
.Values.pgbouncer.enabled (not .Values.triggerer.keda.usePgbouncer))) }}
+ {{- $component := .Component | default "" }}
+ {{- if or (and (eq $component "worker") $workersKedaNeedsDbConn) (and (eq
$component "triggerer") $triggererKedaNeedsDbConn) }}
Review Comment:
I think that we could simplify this by:
1. Moving checking if keda is enabled for a particular component to the
template files
2. Passing as an additional argument here the value of
`.Values.triggerer.keda.usePgBouncer` or
`.Values.workers.celery.keda.usePgBouncer`
and the rest of the logic is common.
##########
chart/tests/helm_tests/other/test_keda.py:
##########
@@ -406,3 +406,49 @@ def test_overwrite_keda_max_replica_count(self):
)
assert jmespath.search("spec.maxReplicaCount", docs[0]) == 5
+
+ @staticmethod
+ def _env_names(doc):
+ return jmespath.search("spec.template.spec.containers[].env[].name",
doc) or []
+
+ def test_worker_keda_db_conn_not_leaked_to_other_components(self):
+ """Worker KEDA_DB_CONN should render on the worker only, not on
unrelated components."""
Review Comment:
```suggestion
```
Name of the test case is descriptive enough that we don't need additional
comment I think.
##########
chart/tests/helm_tests/other/test_keda.py:
##########
@@ -406,3 +406,49 @@ def test_overwrite_keda_max_replica_count(self):
)
assert jmespath.search("spec.maxReplicaCount", docs[0]) == 5
+
+ @staticmethod
+ def _env_names(doc):
+ return jmespath.search("spec.template.spec.containers[].env[].name",
doc) or []
+
+ def test_worker_keda_db_conn_not_leaked_to_other_components(self):
+ """Worker KEDA_DB_CONN should render on the worker only, not on
unrelated components."""
+ docs = render_chart(
+ values={
+ "workers": {"celery": {"keda": {"enabled": True,
"usePgbouncer": False}}},
+ "executor": "CeleryExecutor",
+ "pgbouncer": {"enabled": True},
+ },
+ show_only=[
+ "templates/workers/worker-deployment.yaml",
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/api-server/api-server-deployment.yaml",
+ "templates/dag-processor/dag-processor-deployment.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ ],
+ )
+ worker, scheduler, api_server, dag_processor, triggerer = docs
+ assert "KEDA_DB_CONN" in self._env_names(worker)
+ assert "KEDA_DB_CONN" not in self._env_names(scheduler)
+ assert "KEDA_DB_CONN" not in self._env_names(api_server)
+ assert "KEDA_DB_CONN" not in self._env_names(dag_processor)
+ assert "KEDA_DB_CONN" not in self._env_names(triggerer)
+
+ def test_triggerer_keda_db_conn_not_leaked_to_other_components(self):
+ """Triggerer KEDA_DB_CONN should render on the triggerer only, not on
the worker or others."""
Review Comment:
```suggestion
```
Same as above comment.
--
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]
