From 8bf23f503d9554c72dbef88abe8ab4bbefaa3d9e Mon Sep 17 00:00:00 2001 From: Eric Xiao Date: Sun, 26 Apr 2026 13:13:39 -0400 Subject: [PATCH 1/4] Add requirePersistence option to worker logGroomerSidecar Currently, the worker log groomer sidecar only renders when persistence is enabled (i.e., when using StatefulSets). This prevents using the log groomer on Deployments with emptyDir volumes for logs. This change adds a new `requirePersistence` option that defaults to `true` (preserving backwards compatibility). When set to `false`, the log groomer can be enabled regardless of whether persistence is enabled. This is useful for deployments that: - Use emptyDir for local logs with remote logging (S3, GCS, etc.) - Want to clean up local log files without using StatefulSets - Prefer Deployments over StatefulSets for workers Configuration: - `workers.logGroomerSidecar.requirePersistence: false` - `workers.celery.logGroomerSidecar.requirePersistence: false` Co-Authored-By: Claude Opus 4.5 --- .../templates/workers/worker-deployment.yaml | 3 +- chart/values.schema.json | 13 ++++ chart/values.yaml | 9 +++ .../helm_tests/airflow_core/test_worker.py | 76 +++++++++++++++++++ 4 files changed, 100 insertions(+), 1 deletion(-) diff --git a/chart/templates/workers/worker-deployment.yaml b/chart/templates/workers/worker-deployment.yaml index 340c9aaea3e13..92f30253b236c 100644 --- a/chart/templates/workers/worker-deployment.yaml +++ b/chart/templates/workers/worker-deployment.yaml @@ -35,6 +35,7 @@ {{- if or (contains "CeleryExecutor" .Values.executor) (contains "CeleryKubernetesExecutor" .Values.executor) }} --- {{- $persistence := or .Values.workers.persistence.enabled }} +{{- $logGroomerRequirePersistence := .Values.workers.logGroomerSidecar.requirePersistence }} {{- $keda := .Values.workers.keda.enabled }} {{- $hpa := and .Values.workers.hpa.enabled (not .Values.workers.keda.enabled) }} {{- $nodeSelector := or .Values.workers.nodeSelector .Values.nodeSelector }} @@ -348,7 +349,7 @@ spec: {{- if and .Values.dags.gitSync.enabled (not .Values.dags.persistence.enabled) }} {{- include "git_sync_container" . | nindent 8 }} {{- end }} - {{- if and $persistence .Values.workers.logGroomerSidecar.enabled }} + {{- if and .Values.workers.logGroomerSidecar.enabled (or $persistence (not $logGroomerRequirePersistence)) }} - name: worker-log-groomer image: {{ template "airflow_image" . }} imagePullPolicy: {{ .Values.images.airflow.pullPolicy }} diff --git a/chart/values.schema.json b/chart/values.schema.json index 6ee2b5217231a..d1dcb2fc338eb 100644 --- a/chart/values.schema.json +++ b/chart/values.schema.json @@ -2445,6 +2445,11 @@ "type": "boolean", "default": true }, + "requirePersistence": { + "description": "Whether persistence is required for log groomer sidecar. When false, the log groomer can run on Deployments (without persistence) using an emptyDir volume for logs (deprecated, use ``workers.celery.logGroomerSidecar.requirePersistence`` instead).", + "type": "boolean", + "default": true + }, "command": { "description": "Command to use when running the Airflow log groomer sidecar (templated) (deprecated, use ``workers.celery.logGroomerSidecar.command`` instead).", "type": [ @@ -3669,6 +3674,14 @@ ], "default": null }, + "requirePersistence": { + "description": "Whether persistence is required for log groomer sidecar. When false, the log groomer can run on Deployments (without persistence) using an emptyDir volume for logs.", + "type": [ + "boolean", + "null" + ], + "default": null + }, "command": { "description": "Command to use when running the Airflow log groomer sidecar (templated).", "type": [ diff --git a/chart/values.yaml b/chart/values.yaml index aad52cb3d6260..bece88b1ebb40 100644 --- a/chart/values.yaml +++ b/chart/values.yaml @@ -1148,6 +1148,11 @@ workers: # (deprecated, use `workers.celery.logGroomerSidecar.enabled` instead) enabled: true + # Whether persistence is required for log groomer sidecar. When false, the log groomer + # can run on Deployments (without persistence) using an emptyDir volume for logs. + # (deprecated, use `workers.celery.logGroomerSidecar.requirePersistence` instead) + requirePersistence: true + # Command to use when running the Airflow Celery worker log groomer sidecar (templated) # (deprecated, use `workers.celery.logGroomerSidecar.command` instead) command: ~ @@ -1559,6 +1564,10 @@ workers: # Whether to deploy the Airflow Celery worker log groomer sidecar enabled: ~ + # Whether persistence is required for log groomer sidecar. When false, the log groomer + # can run on Deployments (without persistence) using an emptyDir volume for logs. + requirePersistence: ~ + # Command to use when running the Airflow Celery worker log groomer sidecar (templated) command: ~ diff --git a/helm-tests/tests/helm_tests/airflow_core/test_worker.py b/helm-tests/tests/helm_tests/airflow_core/test_worker.py index 3e31d2d454efe..fa41196368ee7 100644 --- a/helm-tests/tests/helm_tests/airflow_core/test_worker.py +++ b/helm-tests/tests/helm_tests/airflow_core/test_worker.py @@ -2352,6 +2352,82 @@ class TestWorkerCeleryLogGroomer(LogGroomerTestBase): obj_name = "workers-celery" folder = "workers" + @pytest.mark.parametrize( + ("persistence_enabled", "log_groomer_enabled", "require_persistence", "expect_log_groomer"), + [ + # With requirePersistence=true (default behavior, backwards compatible) + # persistence default (true) + logGroomer default (true) -> renders + (None, None, None, True), + # persistence default (true) + logGroomer false -> no render + (None, False, None, False), + # persistence true + logGroomer default (true) -> renders + (True, None, None, True), + # persistence true + logGroomer true -> renders + (True, True, None, True), + # persistence true + logGroomer false -> no render + (True, False, None, False), + # persistence false + logGroomer default (true) + requirePersistence default (true) -> NO render + (False, None, None, False), + # persistence false + logGroomer true + requirePersistence default (true) -> NO render + (False, True, None, False), + # persistence false + logGroomer true + requirePersistence true -> NO render + (False, True, True, False), + # persistence false + logGroomer false -> no render + (False, False, None, False), + # With requirePersistence=false (new behavior) + # persistence false + logGroomer true + requirePersistence false -> renders + (False, True, False, True), + # persistence false + logGroomer default (true) + requirePersistence false -> renders + (False, None, False, True), + # persistence true + logGroomer true + requirePersistence false -> renders + (True, True, False, True), + ], + ) + def test_log_groomer_with_persistence_and_require_persistence_combinations( + self, persistence_enabled, log_groomer_enabled, require_persistence, expect_log_groomer + ): + """Test log groomer sidecar rendering with various persistence, enabled, and requirePersistence combinations.""" + celery_values = {} + if persistence_enabled is not None: + celery_values["persistence"] = {"enabled": persistence_enabled} + + log_groomer_values = {} + if log_groomer_enabled is not None: + log_groomer_values["enabled"] = log_groomer_enabled + if require_persistence is not None: + log_groomer_values["requirePersistence"] = require_persistence + if log_groomer_values: + celery_values["logGroomerSidecar"] = log_groomer_values + + docs = render_chart( + values={ + "executor": "CeleryExecutor", + "workers": {"celery": celery_values} if celery_values else {}, + }, + show_only=["templates/workers/worker-deployment.yaml"], + ) + + # Verify resource kind based on persistence + expected_kind = "StatefulSet" if persistence_enabled is not False else "Deployment" + assert jmespath.search("kind", docs[0]) == expected_kind + + # Verify log groomer presence + containers = jmespath.search("spec.template.spec.containers", docs[0]) + container_names = [c["name"] for c in containers] + + if expect_log_groomer: + assert "worker-log-groomer" in container_names, ( + f"Expected log groomer with persistence={persistence_enabled}, " + f"enabled={log_groomer_enabled}, requirePersistence={require_persistence}" + ) + assert len(containers) == 2 + else: + assert "worker-log-groomer" not in container_names, ( + f"Did not expect log groomer with persistence={persistence_enabled}, " + f"enabled={log_groomer_enabled}, requirePersistence={require_persistence}" + ) + assert len(containers) == 1 + class TestWorkerKedaAutoScaler: """Tests worker keda auto scaler.""" From 025edd561d92cc515cfc64a7da0fbfbd476c63a8 Mon Sep 17 00:00:00 2001 From: Eric Xiao Date: Sun, 26 Apr 2026 13:38:49 -0400 Subject: [PATCH 2/4] Update test. Updates test cases descriptions. Remove unrelated check in test. --- .../tests/helm_tests/airflow_core/test_worker.py | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/helm-tests/tests/helm_tests/airflow_core/test_worker.py b/helm-tests/tests/helm_tests/airflow_core/test_worker.py index fa41196368ee7..5c4f07fddf000 100644 --- a/helm-tests/tests/helm_tests/airflow_core/test_worker.py +++ b/helm-tests/tests/helm_tests/airflow_core/test_worker.py @@ -2356,15 +2356,15 @@ class TestWorkerCeleryLogGroomer(LogGroomerTestBase): ("persistence_enabled", "log_groomer_enabled", "require_persistence", "expect_log_groomer"), [ # With requirePersistence=true (default behavior, backwards compatible) - # persistence default (true) + logGroomer default (true) -> renders + # persistence default (true) + logGroomer default (true) + requirePersistence default (true) -> renders (None, None, None, True), - # persistence default (true) + logGroomer false -> no render + # persistence default (true) + logGroomer false + requirePersistence default (true) -> no render (None, False, None, False), - # persistence true + logGroomer default (true) -> renders + # persistence true + logGroomer default (true) + requirePersistence default (true) -> renders (True, None, None, True), - # persistence true + logGroomer true -> renders + # persistence true + logGroomer true + requirePersistence default (true) -> renders (True, True, None, True), - # persistence true + logGroomer false -> no render + # persistence true + logGroomer false + requirePersistence default (true) -> no render (True, False, None, False), # persistence false + logGroomer default (true) + requirePersistence default (true) -> NO render (False, None, None, False), @@ -2372,7 +2372,7 @@ class TestWorkerCeleryLogGroomer(LogGroomerTestBase): (False, True, None, False), # persistence false + logGroomer true + requirePersistence true -> NO render (False, True, True, False), - # persistence false + logGroomer false -> no render + # persistence false + logGroomer false + requirePersistence default (true) -> no render (False, False, None, False), # With requirePersistence=false (new behavior) # persistence false + logGroomer true + requirePersistence false -> renders @@ -2407,10 +2407,6 @@ def test_log_groomer_with_persistence_and_require_persistence_combinations( show_only=["templates/workers/worker-deployment.yaml"], ) - # Verify resource kind based on persistence - expected_kind = "StatefulSet" if persistence_enabled is not False else "Deployment" - assert jmespath.search("kind", docs[0]) == expected_kind - # Verify log groomer presence containers = jmespath.search("spec.template.spec.containers", docs[0]) container_names = [c["name"] for c in containers] From a698b77a9aaaf8fc7de7137988f56aebfb54bcd8 Mon Sep 17 00:00:00 2001 From: Eric Xiao Date: Mon, 27 Apr 2026 15:36:17 -0400 Subject: [PATCH 3/4] Add transitional note for requirePersistence parameter Document that requirePersistence is transitional for chart 1.2x and will be removed in chart 2.x when log groomer works without persistence by default. Co-Authored-By: Claude Opus 4.5 --- chart/values.schema.json | 4 ++-- chart/values.yaml | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/chart/values.schema.json b/chart/values.schema.json index d1dcb2fc338eb..f827ea1a54a5b 100644 --- a/chart/values.schema.json +++ b/chart/values.schema.json @@ -2446,7 +2446,7 @@ "default": true }, "requirePersistence": { - "description": "Whether persistence is required for log groomer sidecar. When false, the log groomer can run on Deployments (without persistence) using an emptyDir volume for logs (deprecated, use ``workers.celery.logGroomerSidecar.requirePersistence`` instead).", + "description": "Whether persistence is required for log groomer sidecar. When false, the log groomer can run on Deployments (without persistence) using an emptyDir volume for logs. Note: This parameter is transitional for chart 1.2x. In chart 2.x, log groomer will work without persistence by default and this parameter will be removed (deprecated, use ``workers.celery.logGroomerSidecar.requirePersistence`` instead).", "type": "boolean", "default": true }, @@ -3675,7 +3675,7 @@ "default": null }, "requirePersistence": { - "description": "Whether persistence is required for log groomer sidecar. When false, the log groomer can run on Deployments (without persistence) using an emptyDir volume for logs.", + "description": "Whether persistence is required for log groomer sidecar. When false, the log groomer can run on Deployments (without persistence) using an emptyDir volume for logs. Note: This parameter is transitional for chart 1.2x. In chart 2.x, log groomer will work without persistence by default and this parameter will be removed.", "type": [ "boolean", "null" diff --git a/chart/values.yaml b/chart/values.yaml index bece88b1ebb40..0d6ab2fade0f3 100644 --- a/chart/values.yaml +++ b/chart/values.yaml @@ -1150,6 +1150,8 @@ workers: # Whether persistence is required for log groomer sidecar. When false, the log groomer # can run on Deployments (without persistence) using an emptyDir volume for logs. + # Note: This parameter is transitional for chart 1.2x. In chart 2.x, log groomer will + # work without persistence by default and this parameter will be removed. # (deprecated, use `workers.celery.logGroomerSidecar.requirePersistence` instead) requirePersistence: true @@ -1566,6 +1568,8 @@ workers: # Whether persistence is required for log groomer sidecar. When false, the log groomer # can run on Deployments (without persistence) using an emptyDir volume for logs. + # Note: This parameter is transitional for chart 1.2x. In chart 2.x, log groomer will + # work without persistence by default and this parameter will be removed. requirePersistence: ~ # Command to use when running the Airflow Celery worker log groomer sidecar (templated) From 3d0f16c8c74049c77511a2fe0c45414a163eb459 Mon Sep 17 00:00:00 2001 From: Eric Xiao Date: Mon, 27 Apr 2026 15:49:12 -0400 Subject: [PATCH 4/4] Fix spellcheck by wrapping emptyDir in backticks Co-Authored-By: Claude Opus 4.5 --- chart/values.schema.json | 4 ++-- chart/values.yaml | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/chart/values.schema.json b/chart/values.schema.json index f827ea1a54a5b..c85923b2120b2 100644 --- a/chart/values.schema.json +++ b/chart/values.schema.json @@ -2446,7 +2446,7 @@ "default": true }, "requirePersistence": { - "description": "Whether persistence is required for log groomer sidecar. When false, the log groomer can run on Deployments (without persistence) using an emptyDir volume for logs. Note: This parameter is transitional for chart 1.2x. In chart 2.x, log groomer will work without persistence by default and this parameter will be removed (deprecated, use ``workers.celery.logGroomerSidecar.requirePersistence`` instead).", + "description": "Whether persistence is required for log groomer sidecar. When false, the log groomer can run on Deployments (without persistence) using an ``emptyDir`` volume for logs. Note: This parameter is transitional for chart 1.2x. In chart 2.x, log groomer will work without persistence by default and this parameter will be removed (deprecated, use ``workers.celery.logGroomerSidecar.requirePersistence`` instead).", "type": "boolean", "default": true }, @@ -3675,7 +3675,7 @@ "default": null }, "requirePersistence": { - "description": "Whether persistence is required for log groomer sidecar. When false, the log groomer can run on Deployments (without persistence) using an emptyDir volume for logs. Note: This parameter is transitional for chart 1.2x. In chart 2.x, log groomer will work without persistence by default and this parameter will be removed.", + "description": "Whether persistence is required for log groomer sidecar. When false, the log groomer can run on Deployments (without persistence) using an ``emptyDir`` volume for logs. Note: This parameter is transitional for chart 1.2x. In chart 2.x, log groomer will work without persistence by default and this parameter will be removed.", "type": [ "boolean", "null" diff --git a/chart/values.yaml b/chart/values.yaml index 0d6ab2fade0f3..14125f3368e05 100644 --- a/chart/values.yaml +++ b/chart/values.yaml @@ -1149,7 +1149,7 @@ workers: enabled: true # Whether persistence is required for log groomer sidecar. When false, the log groomer - # can run on Deployments (without persistence) using an emptyDir volume for logs. + # can run on Deployments (without persistence) using an `emptyDir` volume for logs. # Note: This parameter is transitional for chart 1.2x. In chart 2.x, log groomer will # work without persistence by default and this parameter will be removed. # (deprecated, use `workers.celery.logGroomerSidecar.requirePersistence` instead) @@ -1567,7 +1567,7 @@ workers: enabled: ~ # Whether persistence is required for log groomer sidecar. When false, the log groomer - # can run on Deployments (without persistence) using an emptyDir volume for logs. + # can run on Deployments (without persistence) using an `emptyDir` volume for logs. # Note: This parameter is transitional for chart 1.2x. In chart 2.x, log groomer will # work without persistence by default and this parameter will be removed. requirePersistence: ~