Skip to content

Conversation

@jstaf
Copy link
Contributor

@jstaf jstaf commented Dec 5, 2025

In the official documentation, deploying Airflow with both git-sync (dags.gitSync.enabled=true) and persistence enabled (dags.persistence.enabled=true) is a supported deployment pattern: https://airflow.apache.org/docs/helm-chart/1.8.0/manage-dags-files.html#mounting-dags-using-git-sync-sidecar-with-persistence-enabled. On Airflow 2.x this works just fine, the scheduler pod runs a git-sync sidecar container which downloads DAGs onto a shared volume on behalf of the entire Airflow installation.

On Airflow 3.x, there is an if-statement that turns off the git-sync sidecar in the scheduler pod since the dag-processor pod is now present https://github.com/apache/airflow/blob/main/chart/templates/scheduler/scheduler-deployment.yaml#L254-L256. However, when we look at the dag-processor pod, it incorrectly disables the git-sync sidecar when DAG persistence is enabled https://github.com/apache/airflow/blob/main/chart/templates/dag-processor/dag-processor-deployment.yaml#L198. This sadly means that git-sync doesn't run anywhere, because the dag-processor was supposed to be the one doing the syncing.

This PR fixes the if statement so that the dag-processor always runs the git-sync sidecar irrespective of if persistence is enabled or not (it's the pod that's supposed to be syncing things for all of the others if persistence is enabled + it needs to sync for itself if persistence is disabled). So with this change, Airflow 3.x actually works with both git-sync and DAG persistence enabled again.

You can check this for yourself by doing helm template airflow chart --set=dags.gitSync.enabled=true --set=dags.persistence.enabled=true | less with and without this PR. (With this PR's change, there is a git-sync container in the rendered manifest, and without this PR the git-sync container is totally missing.)


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

@boring-cyborg boring-cyborg bot added the area:helm-chart Airflow Helm Chart label Dec 5, 2025
Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! This makes sense!

@jscheffl jscheffl added this to the Airflow Helm Chart 1.19.0 milestone Dec 7, 2025
@jscheffl
Copy link
Contributor

jscheffl commented Dec 7, 2025

Oh, small changes in unit tests required, can you fix this @jstaf ? Then we can merge...

Copy link
Contributor

@Miretpl Miretpl left a comment

Choose a reason for hiding this comment

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

The change looks good, but could you add a test case for it (+ fix currently failing test)?

@jstaf jstaf requested a review from dstandish as a code owner December 8, 2025 20:21
@jstaf
Copy link
Contributor Author

jstaf commented Dec 8, 2025

@Miretpl @jscheffl Ok, I've fixed the existing unit test. I didn't add a new one because the existing unit test actually tests the exact scenario we are doing here (git-sync + persistence), it just was testing for the wrong value (it was incorrectly checking that the git-sync sidecar absent instead of present).

@jscheffl
Copy link
Contributor

jscheffl commented Dec 8, 2025

Oh, just a small fail in static checks. If you install prek as the local pre-commit tool, then it will fix the diff easily.

@jscheffl jscheffl merged commit 006b7e8 into apache:main Dec 10, 2025
92 checks passed
@jstaf jstaf deleted the dag-processor-pod-syncs branch December 11, 2025 15:17
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