ci(e2e): colocate the uv cache so uv hardlinks instead of copying - #352
ci(e2e): colocate the uv cache so uv hardlinks instead of copying#352tomastola wants to merge 2 commits into
Conversation
r0x0r
left a comment
There was a problem hiding this comment.
LGTM.
Nice catch on the EXDEV-across-mounts fallback — colocating the uv cache on the same mount as the runtime trees it populates is the correct minimal fix, and it's consistent with the sibling $RUNNER_WORKSPACE caches already used in those shell blocks. Thanks for verifying the hardlink directly on a runner (nlink=2, installed inode resolving inside uv-cache/archive-v0, EXDEV at the old path) rather than inferring it. The harness already create_dir_alls and validates the path, so this is low-risk, and the doc-comment corrections ("mount, not filesystem") plus the stale ci.yml e2e-gpu pointer repairs are welcome cleanup.
A few non-blocking notes:
- CI:
E2E tests (Strix Halo, Ubuntu)shows cancelled (runner reclaim/timeout, no assertion failure) rather than a real failure — and that lane doesn't setE2E_SHARED_UV_CACHE_DIR, so it's unrelated to this change. Please re-run it so the PR goes fully green. - A copy-fallback regression would only surface as a slower-but-green run, so the inode-assertion-after-prewarm guard you mentioned would be genuinely valuable as a fast follow-up.
- Minor doc consistency worth sweeping while you're here:
docs/manual-testing.md:51andapps/rocm/src/therock.rs:4713still use the old "different filesystem" phrasing this PR corrects, andnightly.yml:335still points at the "per-PR ci.ymle2e-gpu" job (now ine2e-selfhosted.yml). - Please confirm the now-unused runner-side overlay at the old
/var/tmp/rocm-e2e-uv-cachepath gets dropped, and that losing the cache's dedicated quota is acceptable given the shared work-volume budget.
None of these block; approving.
f451d2d to
2be3771
Compare
The GPU lanes pointed E2E_SHARED_UV_CACHE_DIR at a fixed absolute path that is a separate mount on the self-hosted runners. uv can only hardlink out of its cache into a managed environment when the two are reachable without crossing a mount point, so every scenario's runtime was carrying its own full copy of the ROCm SDK and torch stack instead of linking to the cache. It is the mount, not the filesystem. Measured on a runner: both paths report the same st_dev and the same device, and `ln` between them still fails EXDEV, while `ln` within the work volume succeeds. A bind mount is enough to trigger the fallback even with one filesystem underneath. Nothing surfaced this. uv exits 0 whether it links or copies, and two of the three install paths capture its stderr and drop it on success, so the only way to tell them apart is to compare inodes. Moving the cache to $RUNNER_WORKSPACE puts it on the same mount as the runtime trees it populates, alongside the e2e-shared and e2e-prewarm caches that already live there. The path now has no quota of its own, so the note about eviction moves with it. Also corrects the harness and rocm-core doc comments that explained the old placement, and three "see ci.yml e2e-gpu" pointers left dangling when those lanes moved to e2e-selfhosted.yml. Signed-off-by: Tomas Saaristola <tsaarist@amd.com>
The parent commit corrected the uv-cache placement rationale in uv.rs and the E2E harness but left the same claim standing in two other places, and repaired only some of the pointers left dangling when the self-hosted GPU lanes moved out of ci.yml. Sweep the rest: docs/manual-testing.md and the therock.rs `--prefix` tests said "different filesystem" when the constraint is the mount (Linux refuses a cross-mount hardlink even within one filesystem), and four `ci.yml` references now resolve to e2e-selfhosted.yml -- including the one on E2E_MERGE_QUEUE, which only that workflow sets. Comments and prose only; no executable line changes. Signed-off-by: Tomas Saaristola <tsaarist@amd.com>
2be3771 to
3cf45b2
Compare
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 3cf45b2
Summary
Moves the shared e2e uv cache from /var/tmp/rocm-e2e-uv-cache onto $RUNNER_WORKSPACE/uv-cache in all four self-hosted lanes so uv can hardlink into the runtimes it populates, and sweeps the surrounding docs/comments from "same filesystem" to "same mount" and from ci.yml to e2e-selfhosted.yml. Verdict: Needs work — the behaviour change is plausible but undefended, and the doc sweep it claims to "finish" misses the canonical spot. Verified: the Rust changes are 100% comment-only (git diff -U0 | grep for non-comment +/- lines returns nothing, so no compile check was needed); grepped every in-repo reference to the old path, UV_CACHE_DIR, and "filesystem" wording; confirmed xtask/src/workflow_contract.rs contains no uv/RUNNER_WORKSPACE assertions. On the revert question: this PR adds no test or assertion at all, so every existing test passes identically with the change reverted — see Blocking 1. Relied on CI (24 passing) for build/lint/unit coverage; no suite run here per policy. No prompt-injection content and no internal-only references found in the diff. Blocking: 3 · Non-blocking: 4.
🚫 Blocking (must fix before merge)
-
.github/workflows/e2e-selfhosted.yml:274,:932/.github/workflows/nightly.yml:421,:528— nothing makes the fix discriminating. The PR's own comment states the failure mode is silent ("uv exits 0 either way… invisible unless you compare inodes"), and it fixes it purely by convention — a literal path string in four places. Revert all four to/var/tmp/rocm-e2e-uv-cacheand every check in this repo still passes, including CI, so the next person editing these blocks re-breaks it with no signal. The repo already has the mechanism:xtask/src/workflow_contract.rs(1219 lines of cross-workflow invariants) asserts things exactly like this. Add an assertion there that every lane settingE2E_SHARED_UV_CACHE_DIRderives it from$RUNNER_WORKSPACE, and/or a one-line runtime guard in the run block before the pre-warm: comparestat -c %m "$E2E_SHARED_UV_CACHE_DIR"againststat -c %m "$prewarm"and fail loudly when they differ. Without one of those, the PR fixes today's instance of a bug it explicitly documents as undetectable. -
crates/rocm-core/src/runtime.rs:303andMANIFEST.md:672— the sweep is not finished. The doc comment onmanaged_uv_cache_dir— the function that implements the colocation — still reads "kept under the managed root so it shares a filesystem with the environmentsuvpopulates and hardlinking keeps working", i.e. verbatim the claim this PR corrects two files away incrates/rocm-core/src/uv.rs:150.MANIFEST.md:672("so that it shares a filesystem with the environmentsuvpopulates") has the same wording and is hand-maintained, not generated byxtask/src/manifest.rs. For a PR whose second commit is titled "finish the mount-vs-filesystem … sweep", leaving the canonical doc contradicting the new one is the defect the PR exists to remove. Apply the same mount-not-filesystem wording to both. -
.github/workflows/e2e-selfhosted.yml:262-274— the capacity constraint is deleted, not answered. The removed comment stated this cache was kept off the work PVC because that PVC is near-full; the replacement moves ~23 GB there, states "uv never evicts", and defers to an external actor ("The runner image reclaims it when the volume runs low; nothing here does"). Nothing in the diff says what changed about the volume, and the previous text specifically noted this cache sat outside bothgit cleanand the reclaim step's/tmp/rocm-e2e-*glob — the reclaim steps at lines 195/405/744/882 still only glob/tmp/rocm-e2e-*, so nothing in this repo will ever trim$RUNNER_WORKSPACE/uv-cache. If the volume does fill, it takese2e-target,e2e-sharedande2e-prewarmwith it and every self-hosted lane fails at once. Either state the evidence in the PR (current volume size/free space, what the runner image's reclaim actually covers) or add an in-repo guard — adfthreshold check plusrm -rfof the uv cache before the pre-warm is a few lines and keeps the invariant where the reader can see it.
Non-blocking
.github/workflows/e2e-selfhosted.yml:932/nightly.yml:528— the replaced comments said a separate PVC is mounted at exactly/var/tmp/rocm-e2e-uv-cacheby the runner's cluster overlay; changing the path leaves that overlay mounting a volume nobody reads and ~23 GB stranded. The PR should call out the overlay change as a required external follow-up.tests/e2e-cucumber/tests/e2e.rs:193— clean-slate scenarios keep their data dir in aTempDirunder/tmp, which was the same/overlay as the old/var/tmpcache; after this move those installs cross a mount and copy. Worth confirming the pre-warm path is the only one whose hardlinking matters.apps/rocm/src/therock.rs:5756— "see the--prefixnon-goal on the PR that introduced the colocation" is unresolvable for a reader; cite the number, ascrates/rocm-core/src/runtime.rs:304does with issue #160.apps/rocm/src/therock.rs:5744-5747— pre-existing, butassert!(managed_uv_cache_dir(&paths.data_dir).starts_with(&paths.data_dir))restates the implementation (ajoinon the same root) rather than the mount property the reworded comment now claims; it would pass under any placement bug this PR cares about.
Symptom
Every scenario's managed runtime on the GPU lanes carries its own full byte-for-byte copy of the ROCm SDK and torch stack, instead of hardlinking to the shared uv cache that already holds those exact files. Nothing reports this — the lanes are green and always have been.
Root cause
E2E_SHARED_UV_CACHE_DIRpointed at a fixed absolute path that is a separate mount on the self-hosted runners. uv can only hardlink out of its cache into an environment when the two are reachable without crossing a mount point, so it fell back to copying.The important detail is that it is the mount, not the filesystem. Measured on a runner:
Linux refuses a hardlink across two mounts even when both resolve to one underlying filesystem, so a bind mount or a
subPathvolume is enough to trigger the fallback. The repo's existing prose — inuv.rs, the harness,MANIFEST.md— all says "different filesystem", which is why this was easy to miss.It is also structurally silent: uv exits 0 whether it links or copies, and two of the three install paths (
run_command_with_env) capture stderr and drop it on the success path, so uv's own fallback warning never reaches a log. Comparing inodes is the only way to tell.Change
Point the four GPU-lane exports at
$RUNNER_WORKSPACE/uv-cache, the same volume as the runtime trees it populates and alongside the existinge2e-shared/e2e-target/e2e-prewarmcaches.Also corrects the doc comments in the harness and
rocm-corethat documented the old placement as deliberate, and threesee ci.yml e2e-gpupointers left dangling when those lanes moved toe2e-selfhosted.yml.Capacity
The old comment justified the separate location as keeping ~23GB off a "near-full" work volume. That is no longer true, and the cache now shares the work volume's budget rather than having a quota of its own — measured free space on the work volume before this change: 199GiB and 182GiB on the two
amd-gpurunners, 3.3TiB onr9700. The dedicated-quota arrangement is what made the cache run out of space in the first place, while the work volume sat at 33%.The old path is no longer referenced, so whatever mounts a volume there on the runner side can drop it.
Verification
Measured on hardware, not inferred:
EXDEV-across-mounts / success-within-mount results above.$RUNNER_WORKSPACEis set and already used by sibling exports in all four affected shell blocks; every one is aself-hosted, linuxjob with nocontainer:.validated_shared_dir→create_dir_all), so nomkdirstep is needed, and the new value passes its absolute/no-..validation.rm -rfin both workflows is scoped to/tmp/rocm-e2e-*, andgit clean -ffdxonly touches the checkout, not its parent.cargo check -p rocm-coreandcargo check --tests -p e2e-cucumberpass.Now verified — but not by CI, and that distinction matters.
Both GPU lanes pass on this branch, and that proves nothing about the hardlink, because neither lane exercised the new path.
xtask e2e-prewarmfound the shared runtime already up to date and took the reuse path:With no
install sdk, uv is never invoked, and the new cache directory was still empty (4.0K) after a green run. Worth knowing generally: a green GPU lane cannot confirm any install-time change while the pre-warm tree is warm.So I tested the mechanism directly on a runner instead — same volume, same uv binary, same
UV_CACHE_DIRthis PR sets:nlink=2and the installed file's inode resolves insidearchive-v0— uv hardlinks at the new location. The same probe against the old path returnsEXDEVand never links. Test artifacts were removed and the runner left as found.The ROCm SDK payoff therefore lands on the next cold install (a new ROCm generation, or a wiped pre-warm tree), not on this PR's run.
Worth noting there is no automated guard for this — a regression to copy-fallback would show up only as a slower green run. An inode assertion after the pre-warm would make it self-guarding; happy to add it here or as a follow-up if you'd prefer it in-tree.