diff --git a/pkg/dind/bindstage.go b/pkg/dind/bindstage.go index df089ad..116dbef 100644 --- a/pkg/dind/bindstage.go +++ b/pkg/dind/bindstage.go @@ -75,3 +75,33 @@ func SweepStagedBinds(dataDir string, log *slog.Logger) { } sweepStagedBinds(stagingRootDir(dataDir), log) } + +// SweepStagedBindsForJob removes the bind staging mounts and directory of ONE +// job. Safe to call while other jobs are running, which is what separates it +// from SweepStagedBinds. +// +// This exists because #187's periodic dead-container reaper called the +// whole-tree SweepStagedBinds on its sweeper tick. That function is documented +// STARTUP ONLY for a precise reason, and the reaper violated it: sweeping the +// root unmounts and deletes the staging directory of every LIVE job too. The +// running job's already-started containers keep working (their mounts are +// already in their namespaces), so nothing fails loudly — but the job's NEXT +// `docker run -v` cannot create its mountpoint, because ensureDirLocked has +// already latched m.ready and will not recreate the parent: +// +// docker: Error response from daemon: bind mount ... rejected: +// creating bind staging mountpoint /dind-binds//2: +// mkdir ...: no such file or directory +// +// Observed on the fleet's Linux amd64 nodes against ephpm's release workflow, +// which runs several `docker run -v "$PWD":/w` steps per job — binds 0 and 1 +// stage fine, a reaper tick lands, and bind 2 fails. Scoping the sweep to the +// container actually being reaped keeps the benefit (releasing the mounts that +// pin that container's rootfs snapshot, which is why the reaper swept at all) +// without touching anyone else's. +func SweepStagedBindsForJob(dataDir, jobID string, log *slog.Logger) { + if dataDir == "" || jobID == "" { + return + } + sweepStagedBindsForJob(stagingRootDir(dataDir), jobID, log) +} diff --git a/pkg/dind/bindstage_linux.go b/pkg/dind/bindstage_linux.go index 81475f4..c249606 100644 --- a/pkg/dind/bindstage_linux.go +++ b/pkg/dind/bindstage_linux.go @@ -232,6 +232,25 @@ func stagingRootParent(jobDir string) string { return filepath.Dir(jobDir) } // sweepStagedBinds is the startup half of the lifecycle: it unmounts and // removes staging directories left by a previous ephemerd process. See // SweepStagedBinds for why leaked staging mounts are worse than untidy. +// sweepStagedBindsForJob unmounts and removes exactly one job's staging +// directory. Unlike sweepStagedBinds it never reads the root, so a live job's +// directory sitting next to this one is untouched. +func sweepStagedBindsForJob(root, jobID string, log *slog.Logger) { + dir := filepath.Join(root, jobID) + if _, err := os.Lstat(dir); err != nil { + return // this job never staged a bind, or it is already gone + } + if err := unmountTreeAndRemove(dir); err != nil { + if log != nil { + log.Warn("failed to sweep dind bind staging dir for reaped job", "path", dir, "error", err) + } + return + } + if log != nil { + log.Info("swept dind bind staging dir for reaped job", "path", dir) + } +} + func sweepStagedBinds(root string, log *slog.Logger) { entries, err := os.ReadDir(root) if err != nil { diff --git a/pkg/dind/bindstage_linux_test.go b/pkg/dind/bindstage_linux_test.go index 7657b37..83dcbe3 100644 --- a/pkg/dind/bindstage_linux_test.go +++ b/pkg/dind/bindstage_linux_test.go @@ -726,3 +726,50 @@ func TestBindStaging_MountpointTypeMatchesSource(t *testing.T) { t.Errorf("staged file reads %q (err %v), want %q", body, err, "body") } } + +// TestSweepStagedBindsForJob_SparesOtherJobs pins the fix for the regression +// #187 introduced: its periodic reaper called the whole-tree +// SweepStagedBinds, which is documented STARTUP ONLY because it cannot tell a +// live job's staging dir from a dead one. On the fleet that deleted running +// jobs' directories, and their next `docker run -v` failed with +// "creating bind staging mountpoint /dind-binds//2: +// +// mkdir ...: no such file or directory" +// +// because ensureDirLocked had already latched m.ready and would not recreate +// the parent. No mounts are needed to prove the scoping: plain directories +// exercise the same path selection. +func TestSweepStagedBindsForJob_SparesOtherJobs(t *testing.T) { + data := t.TempDir() + root := stagingRootDir(data) + + dead := filepath.Join(root, "job-dead") + live := filepath.Join(root, "job-live") + for _, d := range []string{filepath.Join(dead, "0"), filepath.Join(live, "0")} { + if err := os.MkdirAll(d, 0o700); err != nil { + t.Fatal(err) + } + } + + SweepStagedBindsForJob(data, "job-dead", discardLog()) + + if _, err := os.Lstat(dead); !os.IsNotExist(err) { + t.Errorf("reaped job's staging dir still present (err=%v), want removed", err) + } + // The whole point: a live job sitting next to the reaped one must survive, + // mountpoint subdirectory and all, or its next docker run cannot stage. + if _, err := os.Lstat(filepath.Join(live, "0")); err != nil { + t.Errorf("LIVE job's staging dir was destroyed by a scoped sweep: %v — "+ + "this is the #187 regression: the next `docker run -v` in that job "+ + "fails with mkdir ENOENT", err) + } +} + +// TestSweepStagedBindsForJob_ToleratesMissing: a job that never staged a bind +// (no -v in the whole job) must not make the reaper log or fail. +func TestSweepStagedBindsForJob_ToleratesMissing(t *testing.T) { + data := t.TempDir() + SweepStagedBindsForJob(data, "never-staged", discardLog()) + SweepStagedBindsForJob("", "job", discardLog()) + SweepStagedBindsForJob(data, "", discardLog()) +} diff --git a/pkg/dind/bindstage_other.go b/pkg/dind/bindstage_other.go index 7895c67..dc05b25 100644 --- a/pkg/dind/bindstage_other.go +++ b/pkg/dind/bindstage_other.go @@ -5,6 +5,7 @@ package dind import ( "log/slog" "os" + "path/filepath" ) // newBindStager returns the off-Linux stager. There is no mount(2) here and, @@ -35,3 +36,9 @@ func (passthroughStager) teardown() {} func sweepStagedBinds(root string, log *slog.Logger) { _ = os.RemoveAll(root) } + +// sweepStagedBindsForJob is the one-job form; same "nothing to unmount" +// reasoning as above. +func sweepStagedBindsForJob(root, jobID string, log *slog.Logger) { + _ = os.RemoveAll(filepath.Join(root, jobID)) +} diff --git a/pkg/runtime/runtime.go b/pkg/runtime/runtime.go index a860683..65d25c4 100644 --- a/pkg/runtime/runtime.go +++ b/pkg/runtime/runtime.go @@ -563,12 +563,11 @@ func (r *Runtime) ReapDeadContainers(ctx context.Context) error { } now := time.Now() - // Release leaked dind bind-staging mounts lazily — only once, and only - // if we actually find something to reap. Each leaked staging mount pins - // the runner rootfs it was bound from, which makes WithSnapshotCleanup - // fail with "device or resource busy"; CleanOrphans sweeps them for the - // same reason. Skipped entirely on an idle node with nothing to reap. - stagedSwept := false + // Each leaked staging mount pins the runner rootfs it was bound from, + // which makes WithSnapshotCleanup fail with "device or resource busy", + // so a container being reaped needs its staging dir released first. That + // happens per-container below, NOT once per pass over the whole tree — + // see the comment at the call site. for _, c := range containers { id := c.ID() log := r.cfg.Log.With("id", id) @@ -592,9 +591,18 @@ func (r *Runtime) ReapDeadContainers(ctx context.Context) error { continue } - if !stagedSwept { - dind.SweepStagedBinds(r.cfg.DataDir, r.cfg.Log) - stagedSwept = true + // Sweep ONLY this container's staging dir. The whole-tree + // SweepStagedBinds used to run here once per pass, which broke live + // jobs: it is documented STARTUP ONLY precisely because it cannot + // tell a live job's staging dir from a dead one, and deleting a + // running job's dir makes its NEXT `docker run -v` fail with + // "creating bind staging mountpoint ...: no such file or directory" + // (ensureDirLocked has latched m.ready and will not recreate the + // parent). The reaping motive is unchanged — a leaked staged mount + // pins this container's rootfs so its snapshot cannot be deleted — + // and scoping to the container being reaped keeps that. + { + dind.SweepStagedBindsForJob(r.cfg.DataDir, id, r.cfg.Log) } log.Info("reaping dead job container", "reason", "task dead past grace, snapshot pinned", "grace", grace)