Miretpl commented on code in PR #70180:
URL: https://github.com/apache/airflow/pull/70180#discussion_r4041272949
##########
chart/templates/_helpers.yaml:
##########
@@ -645,7 +649,16 @@ server_tls_key_file = /etc/pgbouncer/server.key
{{- printf "%s-config" (include "airflow.fullname" .) }}
{{- end }}
+{{- define "airflow_config_dir" -}}
+ {{- printf "%s/config" .Values.airflowHome | quote }}
+{{- end }}
+
{{- define "airflow_config_mount" -}}
+{{- if .Values.mountConfigAsDir }}
Review Comment:
When this is true, there will be no mount for the
`airflow_local_settings.py` file.
##########
chart/values.schema.json:
##########
@@ -685,6 +685,12 @@
"x-docsSection": "Common",
"default": "See values.yaml"
},
+ "mountConfigAsDir": {
+ "description": "Mount the Airflow config ConfigMap as a directory
(at ``{airflowHome}/config``) and set ``AIRFLOW_CONFIG`` accordingly, instead
of mounting ``airflow.cfg`` via ``subPath``. Avoids the Kubernetes ``subPath``
+ ConfigMap stale-mount race that can fail containers at init with a ``runc``
``StartError``. Opt-in; the default preserves the historical ``subPath``
mount.",
Review Comment:
```suggestion
"description": "Mount the Airflow config ConfigMap as a
directory (at ``{airflowHome}/config``) and set ``AIRFLOW_CONFIG`` accordingly,
instead of mounting ``airflow.cfg`` via ``subPath``.",
```
It is description of the config option, not the justification why it was
added.
##########
chart/tests/helm_tests/airflow_aux/test_airflow_common.py:
##########
@@ -416,6 +416,42 @@ def test_have_all_config_mounts_on_init_containers(self):
for doc in docs:
assert expected_mount in
jmespath.search("spec.template.spec.initContainers[0].volumeMounts", doc)
+ def test_mount_config_as_dir(self):
+ components = [
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/workers/worker-deployment.yaml",
+ "templates/api-server/api-server-deployment.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ "templates/dag-processor/dag-processor-deployment.yaml",
+ ]
+ docs = render_chart(
+ values={"mountConfigAsDir": True, "airflowLocalSettings": "# local
settings"},
+ show_only=components,
+ )
+ assert len(docs) == len(components)
Review Comment:
It doesn't check anything, really. All of these components are rendered by
default and if they will not, there is coverage in different tests.
##########
chart/values.yaml:
##########
@@ -305,6 +305,13 @@ airflowConfigAnnotations: {}
# 'airflow_local_settings' file as a string (templated)
airflowLocalSettings: ~
+# Mount the Airflow config ConfigMap as a directory (at
``{airflowHome}/config``) and set
+# ``AIRFLOW_CONFIG`` accordingly, instead of mounting ``airflow.cfg`` via
``subPath``. This avoids
+# the Kubernetes ``subPath`` + ConfigMap stale-mount race that can fail
containers at init with a
+# ``runc`` ``StartError`` on node/volume lifecycle events.
``airflow_local_settings.py`` keeps its
+# default path. Opt-in; the default preserves the historical ``subPath`` mount.
Review Comment:
```suggestion
# Mount the Airflow config ConfigMap as a directory (at
``{airflowHome}/config``) and set
# ``AIRFLOW_CONFIG`` accordingly, instead of mounting ``airflow.cfg`` via
``subPath``.
```
##########
chart/tests/helm_tests/airflow_aux/test_airflow_common.py:
##########
@@ -416,6 +416,42 @@ def test_have_all_config_mounts_on_init_containers(self):
for doc in docs:
assert expected_mount in
jmespath.search("spec.template.spec.initContainers[0].volumeMounts", doc)
+ def test_mount_config_as_dir(self):
+ components = [
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/workers/worker-deployment.yaml",
+ "templates/api-server/api-server-deployment.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ "templates/dag-processor/dag-processor-deployment.yaml",
+ ]
+ docs = render_chart(
+ values={"mountConfigAsDir": True, "airflowLocalSettings": "# local
settings"},
+ show_only=components,
+ )
+ assert len(docs) == len(components)
+ dir_mount = {"name": "config", "mountPath": "/opt/airflow/config",
"readOnly": True}
+ for doc in docs:
+ mounts =
jmespath.search("spec.template.spec.containers[0].volumeMounts", doc)
+ # airflow.cfg / airflow_local_settings.py are now served from the
mounted directory
+ assert dir_mount in mounts
+ assert not [m for m in mounts if m.get("subPath") in
("airflow.cfg", "airflow_local_settings.py")]
+ # AIRFLOW_CONFIG points Airflow at the config in the mounted
directory
Review Comment:
```suggestion
```
##########
chart/tests/helm_tests/airflow_aux/test_airflow_common.py:
##########
@@ -416,6 +416,42 @@ def test_have_all_config_mounts_on_init_containers(self):
for doc in docs:
assert expected_mount in
jmespath.search("spec.template.spec.initContainers[0].volumeMounts", doc)
+ def test_mount_config_as_dir(self):
+ components = [
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/workers/worker-deployment.yaml",
+ "templates/api-server/api-server-deployment.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ "templates/dag-processor/dag-processor-deployment.yaml",
+ ]
+ docs = render_chart(
+ values={"mountConfigAsDir": True, "airflowLocalSettings": "# local
settings"},
+ show_only=components,
+ )
+ assert len(docs) == len(components)
+ dir_mount = {"name": "config", "mountPath": "/opt/airflow/config",
"readOnly": True}
+ for doc in docs:
+ mounts =
jmespath.search("spec.template.spec.containers[0].volumeMounts", doc)
+ # airflow.cfg / airflow_local_settings.py are now served from the
mounted directory
+ assert dir_mount in mounts
+ assert not [m for m in mounts if m.get("subPath") in
("airflow.cfg", "airflow_local_settings.py")]
+ # AIRFLOW_CONFIG points Airflow at the config in the mounted
directory
+ env = jmespath.search("spec.template.spec.containers[0].env", doc)
+ assert {"name": "AIRFLOW_CONFIG", "value":
"/opt/airflow/config/airflow.cfg"} in env
+
+ def test_config_mounted_via_subpath_by_default(self):
+ docs = render_chart(
+ values={"airflowLocalSettings": "# local settings"},
Review Comment:
If you are setting this value here, you don't really checking the default as
it overrides default config for one of the affected by this change areas.
##########
chart/tests/helm_tests/airflow_aux/test_airflow_common.py:
##########
@@ -416,6 +416,42 @@ def test_have_all_config_mounts_on_init_containers(self):
for doc in docs:
assert expected_mount in
jmespath.search("spec.template.spec.initContainers[0].volumeMounts", doc)
+ def test_mount_config_as_dir(self):
+ components = [
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/workers/worker-deployment.yaml",
+ "templates/api-server/api-server-deployment.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ "templates/dag-processor/dag-processor-deployment.yaml",
+ ]
+ docs = render_chart(
+ values={"mountConfigAsDir": True, "airflowLocalSettings": "# local
settings"},
+ show_only=components,
+ )
+ assert len(docs) == len(components)
+ dir_mount = {"name": "config", "mountPath": "/opt/airflow/config",
"readOnly": True}
+ for doc in docs:
+ mounts =
jmespath.search("spec.template.spec.containers[0].volumeMounts", doc)
Review Comment:
We could have done that directly in the `jmespath`.
##########
chart/tests/helm_tests/airflow_aux/test_airflow_common.py:
##########
@@ -416,6 +416,42 @@ def test_have_all_config_mounts_on_init_containers(self):
for doc in docs:
assert expected_mount in
jmespath.search("spec.template.spec.initContainers[0].volumeMounts", doc)
+ def test_mount_config_as_dir(self):
+ components = [
Review Comment:
We should test all components that use this mount config, not just the core.
##########
chart/newsfragments/70180.feature.rst:
##########
@@ -0,0 +1 @@
+Add ``mountConfigAsDir`` to mount the Airflow config ConfigMap as a directory
(setting ``AIRFLOW_CONFIG``) instead of via ``subPath``, avoiding the
Kubernetes ``subPath`` + ConfigMap stale-mount race that can fail containers at
init with a ``runc`` ``StartError``.
Review Comment:
Rather not needed newsfragment.
##########
chart/tests/helm_tests/airflow_aux/test_airflow_common.py:
##########
@@ -416,6 +416,42 @@ def test_have_all_config_mounts_on_init_containers(self):
for doc in docs:
assert expected_mount in
jmespath.search("spec.template.spec.initContainers[0].volumeMounts", doc)
+ def test_mount_config_as_dir(self):
+ components = [
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/workers/worker-deployment.yaml",
+ "templates/api-server/api-server-deployment.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ "templates/dag-processor/dag-processor-deployment.yaml",
+ ]
+ docs = render_chart(
+ values={"mountConfigAsDir": True, "airflowLocalSettings": "# local
settings"},
+ show_only=components,
+ )
+ assert len(docs) == len(components)
+ dir_mount = {"name": "config", "mountPath": "/opt/airflow/config",
"readOnly": True}
+ for doc in docs:
+ mounts =
jmespath.search("spec.template.spec.containers[0].volumeMounts", doc)
+ # airflow.cfg / airflow_local_settings.py are now served from the
mounted directory
+ assert dir_mount in mounts
+ assert not [m for m in mounts if m.get("subPath") in
("airflow.cfg", "airflow_local_settings.py")]
Review Comment:
We can do it with `jmespath`.
##########
chart/templates/_helpers.yaml:
##########
@@ -63,6 +63,10 @@ If release name contains chart name it will be used as a
full name.
# Hard Coded Airflow Envs
- name: AIRFLOW_HOME
value: {{ .Values.airflowHome }}
+ {{- if .Values.mountConfigAsDir }}
+ - name: AIRFLOW_CONFIG
+ value: {{ printf "%s/config/airflow.cfg" .Values.airflowHome | quote }}
Review Comment:
It should rather user proposed `airflow_config_dir` helper.
##########
chart/tests/helm_tests/airflow_aux/test_airflow_common.py:
##########
@@ -416,6 +416,42 @@ def test_have_all_config_mounts_on_init_containers(self):
for doc in docs:
assert expected_mount in
jmespath.search("spec.template.spec.initContainers[0].volumeMounts", doc)
+ def test_mount_config_as_dir(self):
+ components = [
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/workers/worker-deployment.yaml",
+ "templates/api-server/api-server-deployment.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ "templates/dag-processor/dag-processor-deployment.yaml",
+ ]
+ docs = render_chart(
+ values={"mountConfigAsDir": True, "airflowLocalSettings": "# local
settings"},
+ show_only=components,
+ )
+ assert len(docs) == len(components)
+ dir_mount = {"name": "config", "mountPath": "/opt/airflow/config",
"readOnly": True}
+ for doc in docs:
+ mounts =
jmespath.search("spec.template.spec.containers[0].volumeMounts", doc)
+ # airflow.cfg / airflow_local_settings.py are now served from the
mounted directory
Review Comment:
```suggestion
```
--
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]
