Skip to content

Allow Worker Log Groomer Sidecar Without Persistence#65852

Merged
jscheffl merged 2 commits into
apache:mainfrom
ericxiao251:ericxiao/log-groomer-without-persistence
Apr 28, 2026
Merged

Allow Worker Log Groomer Sidecar Without Persistence#65852
jscheffl merged 2 commits into
apache:mainfrom
ericxiao251:ericxiao/log-groomer-without-persistence

Conversation

@ericxiao251

@ericxiao251 ericxiao251 commented Apr 25, 2026

Copy link
Copy Markdown
Contributor

Context

Previously, the worker log groomer sidecar only rendered when workers.persistence.enabled was true (i.e., StatefulSet mode). This prevented using the log groomer on Deployments with emptyDir volumes for logs.

This change removes the persistence requirement from the log groomer condition. Now the log groomer renders whenever logGroomerSidecar.enabled is true, regardless of the persistence setting.

This is useful for deployments that:

  • Use emptyDir for local logs with remote logging (S3, GCS, etc.)
  • Want to clean up local log files to prevent ephemeral storage exhaustion
  • Prefer Deployments over StatefulSets for workers

Users who don't want the log groomer can disable it by setting workers.logGroomerSidecar.enabled: false.

Backward Compatibility

This change may add a new sidecar container to existing worker Deployments. Specifically:

persistence.enabled logGroomerSidecar.enabled Before After
(default: true) (default: true) Log groomer renders No change
(default: true) false No log groomer No change
true (default: true) Log groomer renders No change
true true Log groomer renders No change
true false No log groomer No change
false (default: true) No log groomer Log groomer now renders ⚠️
false true No log groomer Log groomer now renders ⚠️
false false No log groomer No change

Impact: Users with persistence.enabled: false who left logGroomerSidecar.enabled or the
default (true) will now get the log groomer sidecar. This will cause worker pods to be recreated on
upgrade.

Mitigation: Users who don't want the log groomer can explicitly set
workers.logGroomerSidecar.enabled: false.

Rationale:

  • This is much cleaner solution (without introducing a new argument, etc.).
  • The log groomer being silently disabled when persistence is off could be considered a
    bug—users who enable the log groomer expect it to run. The sidecar is lightweight and helps prevent
    ephemeral storage exhaustion.

Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

@boring-cyborg

boring-cyborg Bot commented Apr 25, 2026

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our prek-hooks will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: [email protected]
    Slack: https://s.apache.org/airflow-slack

Previously, the worker log groomer sidecar only rendered when
`workers.persistence.enabled` was true (i.e., StatefulSet mode).
This prevented using the log groomer on Deployments with emptyDir
volumes for logs.

This change removes the persistence requirement from the log groomer
condition. Now the log groomer renders whenever `logGroomerSidecar.enabled`
is true, regardless of the persistence setting.

This is useful for deployments that:
- Use emptyDir for local logs with remote logging (S3, GCS, etc.)
- Want to clean up local log files to prevent ephemeral storage exhaustion
- Prefer Deployments over StatefulSets for workers

Users who don't want the log groomer can disable it by setting
`workers.logGroomerSidecar.enabled: false`.

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@ericxiao251 ericxiao251 force-pushed the ericxiao/log-groomer-without-persistence branch from c7225ba to 201fadd Compare April 25, 2026 22:57
@jscheffl jscheffl added the backport-to-chart/v1-2x-test Automatic backport to chart 1.2x maintenance branch label Apr 26, 2026

@jscheffl jscheffl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me this perfectly makes sense as also w/o persistence the emptyDir can run full for a long running worker. Limiting log groomer only to persistence... not sure why it had been made like this.

Leaving the PR open for a few days for others to object but for me this looks fine.

Comment thread chart/templates/workers/worker-deployment.yaml
@jscheffl jscheffl removed the backport-to-chart/v1-2x-test Automatic backport to chart 1.2x maintenance branch label Apr 27, 2026
@jscheffl

Copy link
Copy Markdown
Contributor

Note: Backport not needed, would be #65884

@ericxiao251

Copy link
Copy Markdown
Contributor Author

So this PR will also be merged, but to main?

Is there anything I need to do - or the merging will be done by you folks :).

@jscheffl

Copy link
Copy Markdown
Contributor

So this PR will also be merged, but to main?

Is there anything I need to do - or the merging will be done by you folks :).

Nothing needed to be done. Waited now 24h for ther feedback - as no other objection will merge now...

@jscheffl jscheffl merged commit d9c5804 into apache:main Apr 28, 2026
105 checks passed
@boring-cyborg

boring-cyborg Bot commented Apr 28, 2026

Copy link
Copy Markdown

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

@ericxiao251 ericxiao251 deleted the ericxiao/log-groomer-without-persistence branch April 29, 2026 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:helm-chart Airflow Helm Chart

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants