fix(runners): import B300 container images on the login host - #2893
fix(runners): import B300 container images on the login host#2893cquil11 wants to merge 3 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fffdaa1. Configure here.
| fi | ||
|
|
||
| srun -N 1 -A "$SLURM_ACCOUNT" -p "$SLURM_PARTITION" \ | ||
| --time="${ENROOT_IMPORT_TIME_LIMIT:-120}" bash -c " |
There was a problem hiding this comment.
Import timeout no longer enforced
Medium Severity
Replacing srun --time with unbounded bash -c drops the import deadline. A stalled enroot import now holds the per-image lock indefinitely, and other runners wait on flock for an hour before failing while the hung import keeps the login-host runner busy.
Reviewed by Cursor Bugbot for commit fffdaa1. Configure here.
| fi | ||
|
|
||
| srun -N 1 -A "$SLURM_ACCOUNT" -p "$SLURM_PARTITION" \ | ||
| --time="${ENROOT_IMPORT_TIME_LIMIT:-120}" bash -c " |
There was a problem hiding this comment.
Login-host imports can exhaust resources
High Severity
Cold enroot import now runs on the 30 GiB login host with Enroot’s defaults (nproc processors, 10 download connections) and only a per-image lock. A production-size image, or two different images at once, can OOM or fill local disk and take down all 18 GHA runners on that host.
Reviewed by Cursor Bugbot for commit fffdaa1. Configure here.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also looked at whether a SIGKILL from timeout --kill-after=30 on a hung enroot import could leave stale overlay/bind mounts under the local scratch dir that rm -rf "$work_dir" can't remove; since each import attempt gets its own fresh mktemp -d work_dir, any such leftover is scoped to that one directory and won't block or corrupt a subsequent import, though it could leak disk on the login host until cleaned up manually.
Extended reasoning...
This run's inline findings already flag the real correctness issues in the new import_squash_image locking/timeout design (mismatched lock-wait vs. import timeouts, unbounded post-import validation/publish on shared Lustre storage, and a lock-ordering issue with the old per-image lock during mixed-fleet rollout), so those carry the weight for why a human should look here rather than approving. I additionally traced the failure-cleanup path for the case this PR is explicitly designed to survive (a hung/stalled import getting killed): the timeout --kill-after=30 sends SIGKILL to the enroot import process if it doesn't exit on SIGTERM, and the EXIT trap then runs rm -f "$partial"; rm -rf "$work_dir". Because ENROOT_TEMP_PATH/ENROOT_RUNTIME_PATH/ENROOT_DATA_PATH are all rooted under a per-invocation mktemp -d directory, a mount that survives the kill (if enroot's own unmount-on-signal handling doesn't run) would only affect that one throwaway directory and not interfere with the next import's fresh work_dir, so it's a possible local disk/mount leak on the login host rather than a correctness or serialization bug. Given the inline findings already cover the higher-severity issues in the concurrency and timeout design, I'm not raising this as a separate finding, just noting it was checked.
| flock -w 7200 9 || { echo "Timed out waiting for the image import lock" >&2; exit 1; } | ||
| # Keep the per-image lock compatible with runners on the old launcher. | ||
| exec 8>"${sqsh}.lock" || exit 1 | ||
| flock -w 7200 8 || { echo "Timed out waiting for $sqsh" >&2; exit 1; } | ||
| # Another runner may have populated this image while we waited. | ||
| if unsquashfs -l "$sqsh" > /dev/null 2>&1; then | ||
| echo "Squash file already present, skipping import: $sqsh" | ||
| exit 0 | ||
| fi | ||
| rm -f \"$sqsh\" | ||
| enroot import -o \"$sqsh\" \"docker://$image_ref\" | ||
| unsquashfs -l \"$sqsh\" > /dev/null | ||
| " || { echo "Error: enroot import failed for $image_ref -> $sqsh" >&2; exit 1; } | ||
|
|
||
| test -r "$sqsh" || { echo "Error: squash file not readable: $sqsh" >&2; exit 1; } | ||
| local work_dir partial | ||
| work_dir=$(mktemp -d "${ENROOT_TEMP_PATH:-/tmp}/inferencex-enroot.XXXXXX") || exit 1 | ||
| partial="${sqsh}.tmp.${work_dir##*/}" | ||
| trap 'rm -f -- "$partial"; rm -rf -- "$work_dir"' EXIT | ||
| trap 'exit 143' TERM | ||
| trap 'exit 130' INT | ||
| export ENROOT_TEMP_PATH="$work_dir" | ||
| export ENROOT_RUNTIME_PATH="$work_dir/runtime" | ||
| export ENROOT_DATA_PATH="$work_dir/data" | ||
| export ENROOT_CACHE_PATH="${ENROOT_CACHE_PATH:-$SQUASH_DIR/.enroot-cache}" | ||
| export ENROOT_MAX_PROCESSORS="${ENROOT_MAX_PROCESSORS:-2}" | ||
| export ENROOT_MAX_CONNECTIONS="${ENROOT_MAX_CONNECTIONS:-2}" | ||
|
|
||
| echo "Importing $image_ref on $(hostname) using local scratch $work_dir" | ||
| timeout --kill-after=30 "${ENROOT_IMPORT_TIME_LIMIT:-120}m" \ |
There was a problem hiding this comment.
🔴 The global (fd9) and per-image (fd8) flock waits are hardcoded to 7200s, but the actual import timeout is the configurable ${ENROOT_IMPORT_TIME_LIMIT:-120}m (also 7200s by default). A waiting caller can now time out ("Timed out waiting for the image import lock") while the current holder is still legitimately importing within its own allowed time, since there is zero margin between the two. Fix: derive both flock -w timeouts from ENROOT_IMPORT_TIME_LIMIT plus a safety margin (e.g. import_timeout + overhead), not a separate hardcoded constant, so raising the import limit for large images also raises the wait budget for everyone contending on either lock.
Extended reasoning...
Holder acquires the global lock at t=0 and imports a large image; its timeout allows up to 7200s (default) or longer if ENROOT_IMPORT_TIME_LIMIT is raised for a big image like SGLang (explicitly recommended in the PR/docs for large images). A second job on the same login host needing a different (or same) cold image starts waiting on flock at t≈0 with a fixed 7200s budget (line 92/95). If the holder's import legitimately runs close to or past 7200s (or the override raises the import timeout without raising the flock wait), the waiter's flock -w 7200 elapses first and the second job fails with a lock-timeout error even though the first import was still within its configured allowance. Before this change, the analogous per-image flock wait (3600s) was half of the outer Slurm --time bound (7200s), giving a structural 2x margin; here the margin is removed and a second, image-agnostic global lock is added with the same defect, widening who is exposed to spurious failures.
Verification: Severity: pre-existing (with one bounded new variant). The mismatch is real and reachable. runners/launch_b300-dsxe.sh line 92 flock -w 7200 9 || { echo "Timed out waiting for the image import lock" >&2; exit 1; } and line 95 flock -w 7200 8 || { echo "Timed out waiting for $sqsh" >&2; exit 1; } are hardcoded, while the holder may import for `timeout ...… | normal. The new global lock…
| timeout --kill-after=30 "${ENROOT_IMPORT_TIME_LIMIT:-120}m" \ | ||
| enroot import -o "$partial" "docker://$image_ref" || exit 1 | ||
| unsquashfs -l "$partial" > /dev/null || exit 1 | ||
| # Publish only a validated image, without exposing partial imports. | ||
| mv -f -- "$partial" "$sqsh" || exit 1 |
There was a problem hiding this comment.
🔴 The timeout wrapper at line 116 bounds only the enroot import call; the following validation (unsquashfs -l "$partial", line 118) and publish (mv, line 120) read/write $SQUASH_DIR on shared Lustre storage -- the same storage this PR's own motivation says can hang -- with no timeout, while still holding the global cross-image flock (fd9). A Lustre stall there now blocks every other image's import on the whole login host indefinitely, not just one job as before. Fix: wrap the whole locked import section (import + validate + publish) in a single timeout/deadline so a stall on shared storage cannot hold the global lock forever.
Extended reasoning...
Old code ran import+validate inside one srun --time=... job, so a stall there only killed that one Slurm step. New code's timeout (line 116-117) covers only enroot import; unsquashfs -l "$partial" (118) and mv -f -- "$partial" "$sqsh" (120) both touch $sqsh's directory, which is $SQUASH_DIR on shared Lustre (top-of-file: 'Must be on storage every compute node mounts'). If that mount stalls -- the exact failure mode this PR was written to work around -- the read/rename blocks in D-state with no timeout, and the process is still holding the global .import.lock (fd9, acquired at line ~92) which serializes ALL images across ALL callers on the login host, so every other cold import on the host hangs too, with nothing to end it.
Verification: normal. The global cross-image lock is taken at lines 91-92 (exec 9>"$SQUASH_DIR/.import.lock"; flock -w 7200 9) and held for the entire subshell. timeout --kill-after=30 "${ENROOT_IMPORT_TIME_LIMIT:-120}m" (lines 116-117) wraps ONLY enroot import. The subsequent validation unsquashfs -l "$partial" (line 118) and publish mv -f -- "$partial" "$sqsh" (line 120) both operate…
| exec 9>"$SQUASH_DIR/.import.lock" || exit 1 | ||
| flock -w 7200 9 || { echo "Timed out waiting for the image import lock" >&2; exit 1; } | ||
| # Keep the per-image lock compatible with runners on the old launcher. | ||
| exec 8>"${sqsh}.lock" || exit 1 | ||
| flock -w 7200 8 || { echo "Timed out waiting for $sqsh" >&2; exit 1; } |
There was a problem hiding this comment.
🔴 The subshell takes the login-host-wide global lock (fd9) before the per-image compatibility lock (fd8), so waiting on fd8 keeps fd9 held. If an old-launcher runner (still importing the same image via srun on a compute node) holds ${sqsh}.lock, a new-launcher caller blocks on fd8 for up to 7200s while still holding the global lock, starving every other new-launcher import of a different, otherwise-idle image on that login host for the whole wait -- defeating the stated goal of bounding login-host resource use. Fix: acquire the per-image lock first and only take/hold the global lock around the actual local download+extract work, not around the compatibility-lock wait.
Extended reasoning...
Mixed fleet during rollout: runner R1 (old script) srun-imports image X on a compute node and holds X.sqsh.lock. Runner R2 (new script, same login host) calls import_squash_image for X: it takes fd9 (.import.lock) at line 91-92, then blocks on fd8 (X.sqsh.lock) at line 94-95 waiting on R1's unrelated compute-node import, holding fd9 the entire time. Runner R3 (new script) calls import_squash_image for an unrelated, uncontended image Y; it blocks on fd9 at line 92 even though nothing on the login host is using CPU/network for Y. R3's import is delayed by R1's compute-node-bound wait, which the new global lock was never meant to gate. This ordering did not exist pre-PR (no global lock existed), so it is a regression introduced by combining the two locks in this order.
Verification: normal. Real lock-ordering defect introduced by this change. Lines 91-92 acquire the shared global lock fd9 ($SQUASH_DIR/.import.lock), then lines 94-95 acquire the per-image lock fd8 (${sqsh}.lock) — i.e. the caller waits on fd8 while still holding fd9. The old launcher (removed base code: exec 9>"$lock"; flock -w 3600 9 with lock="${2}.lock") takes that same ${sqsh}.lock file…
1 participant
B300 image preparation currently allocates a compute node with
srun. Replace that wrapper with localbash -cso the existing import function runs on the runner/login host. Update the outdated comment; the function's locking, cache checks, import command, and error handling stay as they were.Validation: Bash syntax, ShellCheck, and
git diff --checkpass. The exact simplified function successfully imported the seven-layernginx:1.27.4image on the B300 login node using its default Enroot configuration, then reused the cached image on a second call. Full-size SGLang import and end-to-end benchmarks have not been validated.Compute nodes still need working shared-storage access to read the finished image. Supersedes #2892.