Skip to content

Add refresh_on_initialize option to GitDagBundle - #70058

Closed
kjh0623 wants to merge 9 commits into
apache:mainfrom
kjh0623:feat/git-bundle-refresh-on-initialize
Closed

Add refresh_on_initialize option to GitDagBundle#70058
kjh0623 wants to merge 9 commits into
apache:mainfrom
kjh0623:feat/git-bundle-refresh-on-initialize

Conversation

@kjh0623

@kjh0623 kjh0623 commented Jul 17, 2026

Copy link
Copy Markdown

What this adds

An opt-in refresh_on_initialize kwarg (default True, no behavior change for existing users) on GitDagBundle, for deployments where many ephemeral processes initialize the same bundle over shared storage:

GitDagBundle(tracking_ref="main", refresh_on_initialize=False)

Today, unversioned initialize() always ends in a remote fetch. That is the right default for a dag processor keeping the bundle fresh, but with KubernetesExecutor every worker pod initializing the bundle pays that fetch on startup — for our repository this adds ~15s of cold-start to every single task, even though the shared volume already holds a tracking repo the dag processor refreshes continuously. Shallow/sparse clones shave the cost down but every pod still pays a network round-trip.

With refresh_on_initialize=False, initialize() reuses the tracking repo already on disk as-is and skips all network operations. Scope guards:

  • the repository is still cloned (and fetched) when nothing is on disk yet,
  • explicit refresh() calls still fetch as usual (both repo handles are assigned in the reuse path),
  • version-pinned initialization is unaffected (it has its own reuse paths already),
  • any failure reusing the on-disk repo falls back to the full initialization path.

This generalizes what we currently run in production as a small GitDagBundle subclass (workers reuse the PVC-resident tracking repo, only the dag processor fetches). Related context on the worker cold-start motivation was discussed in #70003.

Included

  • refresh_on_initialize kwarg + docstring
  • Unit tests: reuse path serves the on-disk state and skips the fetch while explicit refresh() still fetches; default behavior unchanged (second initialize fetches latest)
Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

Generated-by: Claude Code following the guidelines

  • 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.

LOGAN\gktxk @claude
Add refresh_on_initialize option to GitDagBundle
9a87461 
When many ephemeral processes initialize the same bundle over shared storage
(e.g. KubernetesExecutor worker pods mounting a volume the dag processor keeps
fresh), every initialize() pays a remote fetch it doesn't need: for our
repository that adds ~15s of cold-start to every single task.

With refresh_on_initialize=False, initialize() reuses the tracking repo
already on disk as-is and skips all network operations. The repository is
still cloned when absent, explicit refresh() calls still fetch, and
version-pinned initialization is unaffected. Defaults to True - no behavior
change for existing users.

Co-Authored-By: Claude Fable 5 <[email protected]>
@kjh0623
Fall back to full initialization on any error while reusing the on-di…
08b321e 
…sk repo

The reuse path's inner block re-raises non-git errors (bare `raise` after
repo.close()), but the outer except only caught InvalidGitRepositoryError /
NoSuchPathError / GitCommandError. A checkout failing with e.g. OSError /
PermissionError on the shared bundle storage therefore escaped instead of
falling through to full initialization, contradicting the documented
"reuse it as-is, fall back if unusable" intent. Broaden the outer except to
Exception so any unusable on-disk repo falls back as intended (review feedback
from @Sanjays2402).

Signed-off-by: kjh0623 <[email protected]>
@kjh0623

kjh0623 commented Jul 20, 2026

Copy link
Copy Markdown
Author

Good catch — you're right, that's a real gap, not intended.

The inner block catches everything and re-raises after repo.close() (so the handle doesn't leak), but the outer except only listed the three git errors, so a re-raised OSError/PermissionError from the checkout escapes instead of falling through to full initialization — exactly contradicting the "reuse it as-is, fall back if unusable" comment. And it matters most for the target use case here (ephemeral pods over shared bundle storage), where a filesystem hiccup on the on-disk repo is the likely failure.

Fixed in 08b321e by broadening the outer except to Exception, so any unusable on-disk repo falls back to full initialization as documented. Falling back broadly is safe here because the fallback is the normal init path (a genuine problem would resurface there), and it's logged at info with the exception. BaseException (KeyboardInterrupt/SystemExit) is intentionally left to propagate.

Happy to add a regression test that forces a non-git error on the reuse checkout and asserts the fallback, if you'd like it in this PR.

kjh0623 and others added 2 commits July 21, 2026 21:08
@kjh0623
Merge branch 'main' into feat/git-bundle-refresh-on-initialize
f98585b
@kjh0623
Add regression test for the non-git fallback on the reuse path
a479772 
Covers the gap fixed in 08b321e: a PermissionError raised while opening the
on-disk tracking repo must fall through to full initialization rather than
propagate. The test asserts the fallback actually ran by checking the bundle
ends up on the newer remote commit, which the reuse path would not have picked
up (offered in review to @Sanjays2402).

Signed-off-by: kjh0623 <[email protected]>
@kjh0623
kjh0623 requested a review from Sanjays2402 July 21, 2026 13:06
@kjh0623
Reword the docstring to keep the docs spellcheck green
f2e00d3 
"unversioned" is not in docs/spelling_wordlist.txt, so the sphinx spellcheck
failed on the generated API docs for this docstring. Say "when the bundle has
no pinned version" instead of adding a word to the shared wordlist for a single
sentence.

Signed-off-by: kjh0623 <[email protected]>
aaron-y-chen
aaron-y-chen reviewed Jul 28, 2026
View reviewed changes

@aaron-y-chen aaron-y-chen 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.

Hi, thanks for the PR :)

Since the motivation mentions KubernetesExecutor worker pod cold-starts, have you had a chance to test this in a local K8s cluster or a real KubernetesExecutor setup?

@kjh0623

kjh0623 commented Jul 28, 2026

Copy link
Copy Markdown
Author

Yeah, I've been running this on our on-prem K8s cluster for a while now — it's actually already deployed in our production Airflow and has been working without issues.

In our setup the DAG processor keeps the tracking repo fresh on the shared volume, so having every worker pod re-fetch on init was pure overhead. With refresh_on_initialize=False that ~15s per-pod cold start on task startup basically goes away, since it just reuses what's already on disk. The clone-if-missing and explicit refresh() paths still behave the same, so we haven't seen any staleness problems in practice.

Happy to share more details about the environment if that'd help.

LOGAN\gktxk and others added 4 commits August 23, 2026 15:15
LOGAN\gktxk
Merge remote-tracking branch 'upstream/main' into feat/git-bundle-ref…
66d64e7 
…resh-on-initialize
@kjh0623 @claude
Reset the working tree when reusing the tracking repo
fdbf654 
The reuse path replaces the refresh() that ends the unversioned initialize,
but refresh() also does head.reset(..., working_tree=True) - the reuse path
only checked out tracking_ref, so a working tree a previous task left dirty
was served as bundle content. _initialize()'s other short-circuit, the
version-pinned one, already resets for exactly this reason.

reset --hard HEAD, not clean -fd: untracked files survive refresh() too, and
deleting them on shared bundle storage is not this change's call to make.

Also log the best-effort fallback at warning, matching the other
clone/open failure fallbacks in this file.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: kjh0623 <[email protected]>
@kjh0623 @claude
Document refresh_on_initialize and when it is worth setting
d83a7c4 
The bundles page lists the GitDagBundle kwargs, and this one was only in the
docstring - where it also did not say what makes it safe. Both now spell out
that it needs storage some other component refreshes, and that it is a no-op
for version-pinned bundles, so it only helps deployments running with
disable_bundle_versioning.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: kjh0623 <[email protected]>
@kjh0623 @claude
Target the fallback test at the reuse path, not at a call count
caa95f9 
The fallback test failed the first Repo() construction of the whole
initialize and then asserted len(opened) > 1, which encodes how many times
_initialize happens to construct a Repo. Fail the first open of repo_path
instead - that is the reuse attempt itself - and assert it was reached.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: kjh0623 <[email protected]>
@kaxil

kaxil commented Oct 1, 2026

Copy link
Copy Markdown
Member

Who besides your deployment needs this? refresh_on_initialize=False only helps with versioning disabled and shared storage, and as the new docs say, it gains nothing for the default per-pod layout and serves stale code indefinitely if nothing else refreshes the storage. It's a permanent GitDagBundle kwarg for that one layout. Could workers on shared storage point a LocalDagBundle at the shared checkout instead? Is there an issue or another user asking for this? Converting to draft until there is.

@kaxil
kaxil marked this pull request as draft October 1, 2026 23:44
@kjh0623

kjh0623 commented Oct 14, 2026

Copy link
Copy Markdown
Author

Thanks, that's fair. You're right that this only pays off in one fairly specific layout, with versioning disabled and bundle storage shared between the Dag processor and workers. I don't have other users asking for it, so a permanent GitDagBundle kwarg is hard to justify. I'll close this PR and keep running it as a small subclass on our side, where it's been working fine.

Thanks for the review, and thanks @Sanjays2402 for catching the exception handling gap.

@kjh0623 kjh0623 closed this Oct 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Sanjays2402 Sanjays2402 Awaiting requested review from Sanjays2402
1 more reviewer
@aaron-y-chen aaron-y-chen aaron-y-chen left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area:providers provider:git

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kjh0623 @kaxil @aaron-y-chen @Sanjays2402