fix(runtime): stop the reaper sweeping LIVE jobs' dind bind staging dirs - #201
Merged
Merged
Conversation
Regression introduced by #187 (v0.2.8). Its periodic dead-container reaper called dind.SweepStagedBinds once per pass. That function is documented STARTUP ONLY, and its own comment says why: "it does not know which jobs are live, and unmounting a running job's staged bind would not break that job's already-running containers but would break any container it starts next." Which is exactly what happened on the fleet's Linux amd64 nodes: docker: Error response from daemon: bind mount /home/runner/_work/... -> /w rejected: creating bind staging mountpoint /var/lib/ephemerd/dind-binds/ephemerd-github-ephpm-sure_hopper/2: mkdir ...: no such file or directory A reaper tick reaps some unrelated dead container, sweeps the whole <data>/dind-binds tree, and deletes the staging directory of every LIVE job with it. The running job's existing containers keep working (their mounts are already in their namespaces), so nothing fails loudly -- but its NEXT `docker run -v` cannot create its mountpoint, because ensureDirLocked has latched m.ready and will not recreate the parent. Hence the index /2: binds 0 and 1 staged fine before the tick landed. ephpm's release workflow is the heaviest user of `docker run -v "$PWD":/w" (since the node24 de-containerization), which is why it surfaced there. Fix: sweep only the staging dir of the container actually being reaped, via a new SweepStagedBindsForJob. The reaping motive is unchanged -- a leaked staged mount pins that container's rootfs so its snapshot cannot be deleted -- and scoping it keeps that without touching anyone else's. The startup-only whole-tree sweep in CleanOrphans is left alone; that one is correct, because nothing is live yet.
|
ePHPm Preview — removed Preview deployment has been torn down. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Regression introduced by #187 (shipped in v0.2.8, present in every release since including v0.2.12). Actively breaking jobs on the Linux amd64 nodes.
Reported symptom
Cause
#187's periodic dead-container reaper calls the whole-treedind.SweepStagedBindsonce per pass (pkg/runtime/runtime.go). That function is documented STARTUP ONLY, and its own comment states the failure mode precisely:Sequence:
<data>/dind-binds/<jobID>/dind-bindstree first — deleting live jobs' directories toodocker run -vcannot create its mountpoint:ensureDirLockedhas latchedm.readyand will not recreate the parent, andos.Mkdiris non-recursive →ENOENTThe index
/2in the error is the tell: binds 0 and 1 staged fine, then a tick landed.Why ephpm surfaced it: its release workflow runs several
docker run -v "$PWD":/wsteps per job (since the node24 de-containerization), so it has the most exposure.Fix
Sweep only the staging dir of the container actually being reaped, via a new
SweepStagedBindsForJob. The reaping motive is unchanged — a leaked staged mount pins that container's rootfs soWithSnapshotCleanupfails with "device or resource busy" — and scoping it preserves that without touching anyone else's directory.The startup-only whole-tree sweep in
CleanOrphansis left exactly as-is; that one is correct, because nothing is live at startup.Tests
TestSweepStagedBindsForJob_SparesOtherJobscreates a dead and a live job staging dir, sweeps the dead one, and asserts the live one survives with its mountpoint subdirectory intact — the precise property that was violated. Plus a tolerates-missing case (a job with no-vanywhere, and empty-arg guards). Deliberately mount-free so it runs in CI without privileges.Verification
GOOS=linux go vet ./pkg/dind/... ./pkg/runtime/...clean;go test ./pkg/dind/... ./pkg/runtime/...green on the dev box.Caveat: the dev box is Windows, so the new Linux tests were compile-verified (
GOOS=linux go vet) but not executed locally — CI's "Lint, Unit, E2E & Build (Linux)" is the gate. The existing bind-staging tests need real mount privileges and can only run there.