diff --git a/cmd/ateom-microvm/csi.go b/cmd/ateom-microvm/csi.go index 4b29d1ac5..9e3b9cc8d 100644 --- a/cmd/ateom-microvm/csi.go +++ b/cmd/ateom-microvm/csi.go @@ -20,12 +20,8 @@ import ( "context" "fmt" "os" - "os/exec" - "path/filepath" - "strings" "github.com/agent-substrate/substrate/cmd/ateom-microvm/internal/kata" - "github.com/agent-substrate/substrate/cmd/ateom-microvm/internal/reaper" "github.com/agent-substrate/substrate/internal/ateompath" "github.com/agent-substrate/substrate/internal/proto/ateompb" specs "github.com/opencontainers/runtime-spec/specs-go" @@ -64,19 +60,8 @@ func (s *AteomService) stageCsiVolumes(ctx context.Context, actorUID string) err if _, err := os.Stat(src); err != nil { return fmt.Errorf("while checking CSI volumes dir %q: %w", src, err) } - dst := filepath.Join(kata.SharedDir(actorUID), "csi") - // Drop any stale mount first (lazy if busy), then ensure clean mountpoint. - if err := reaper.Run(exec.Command("umount", dst)); err != nil { - _ = reaper.Run(exec.Command("umount", "-l", dst)) - } - if err := os.MkdirAll(dst, 0o755); err != nil { - return fmt.Errorf("creating %q: %w", dst, err) - } - cmd := exec.CommandContext(ctx, "mount", "--bind", src, dst) - var stderr strings.Builder - cmd.Stderr = &stderr - if err := reaper.Run(cmd); err != nil { - return fmt.Errorf("bind-mounting CSI volumes at %q: %w (%s)", dst, err, strings.TrimSpace(stderr.String())) + if err := kata.BindIntoShare(ctx, src, actorUID, "csi"); err != nil { + return fmt.Errorf("while binding CSI volumes into the shared tree: %w", err) } return nil } diff --git a/cmd/ateom-microvm/durable.go b/cmd/ateom-microvm/durable.go index 18f997447..d1484d95d 100644 --- a/cmd/ateom-microvm/durable.go +++ b/cmd/ateom-microvm/durable.go @@ -41,12 +41,9 @@ import ( "context" "fmt" "os" - "os/exec" "path/filepath" - "strings" "github.com/agent-substrate/substrate/cmd/ateom-microvm/internal/kata" - "github.com/agent-substrate/substrate/cmd/ateom-microvm/internal/reaper" "github.com/agent-substrate/substrate/cmd/ateom-microvm/internal/tarutil" "github.com/agent-substrate/substrate/internal/ateompath" "github.com/agent-substrate/substrate/internal/proto/ateompb" @@ -112,19 +109,8 @@ func (s *AteomService) stageDurableVolumes(ctx context.Context, actorUID string) if _, err := os.Stat(src); err != nil { return fmt.Errorf("while checking durable-dir volumes dir %q: %w", src, err) } - dst := filepath.Join(kata.SharedDir(actorUID), "durable") - // Drop any stale mount first (lazy if busy), then ensure clean mountpoint. - if err := reaper.Run(exec.Command("umount", dst)); err != nil { - _ = reaper.Run(exec.Command("umount", "-l", dst)) - } - if err := os.MkdirAll(dst, 0o755); err != nil { - return fmt.Errorf("creating %q: %w", dst, err) - } - cmd := exec.CommandContext(ctx, "mount", "--bind", src, dst) - var stderr strings.Builder - cmd.Stderr = &stderr - if err := reaper.Run(cmd); err != nil { - return fmt.Errorf("bind-mounting durable-dir volumes at %q: %w (%s)", dst, err, strings.TrimSpace(stderr.String())) + if err := kata.BindIntoShare(ctx, src, actorUID, "durable"); err != nil { + return fmt.Errorf("while binding durable-dir volumes into the shared tree: %w", err) } return nil } diff --git a/cmd/ateom-microvm/internal/kata/cleanup_linux.go b/cmd/ateom-microvm/internal/kata/cleanup_linux.go index 7b0f01049..486c56893 100644 --- a/cmd/ateom-microvm/internal/kata/cleanup_linux.go +++ b/cmd/ateom-microvm/internal/kata/cleanup_linux.go @@ -37,13 +37,31 @@ import ( // .../virtiofsd.sock: bind: address already in use", "Could not bind mount // .../shared/sandboxes//mounts", "directory not empty". Calling this // before each run gives a clean slate. +// +// Removal is gated on the unmounting: the shared tree holds bind mounts whose +// SOURCES belong to someone else (the durable-dir and CSI volume dirs are +// atelet's — see BindIntoShare), and a RemoveAll that walks into a live bind +// deletes the actor's data through it. So a dir is removed only once every +// mount beneath it is known detached; otherwise it is left in place for the +// next sweep (the stagers tolerate a leftover dir — they unmount stale binds +// and reuse mountpoints). func CleanupSandboxState(ctx context.Context, id string) { dirs := []string{ filepath.Join("/run/kata-containers/shared/sandboxes", id), filepath.Join(vcVMDir, id), } - if b, err := os.ReadFile("/proc/self/mountinfo"); err == nil { - var mounts []string + removable := make(map[string]bool, len(dirs)) + if b, err := os.ReadFile("/proc/self/mountinfo"); err != nil { + // Without the mount table there is no telling what is still mounted + // under the dirs, so none of them is provably safe to remove. + slog.WarnContext(ctx, "Cannot read mountinfo; leaving sandbox dirs in place", + slog.Any("err", err)) + } else { + for _, d := range dirs { + removable[d] = true + } + type mount struct{ mp, dir string } + var mounts []mount for _, line := range strings.Split(string(b), "\n") { fields := strings.Fields(line) if len(fields) < 5 { @@ -52,21 +70,27 @@ func CleanupSandboxState(ctx context.Context, id string) { mp := fields[4] // mount point for _, d := range dirs { if mp == d || strings.HasPrefix(mp, d+"/") { - mounts = append(mounts, mp) + mounts = append(mounts, mount{mp: mp, dir: d}) break } } } // Deepest paths first so child mounts unmount before their parents. - sort.Slice(mounts, func(i, j int) bool { return len(mounts[i]) > len(mounts[j]) }) - for _, mp := range mounts { - if err := unix.Unmount(mp, unix.MNT_DETACH); err != nil { + sort.Slice(mounts, func(i, j int) bool { return len(mounts[i].mp) > len(mounts[j].mp) }) + for _, m := range mounts { + if err := unix.Unmount(m.mp, unix.MNT_DETACH); err != nil { slog.WarnContext(ctx, "Failed to unmount leftover sandbox mount", - slog.String("mount", mp), slog.Any("err", err)) + slog.String("mount", m.mp), slog.Any("err", err)) + removable[m.dir] = false } } } for _, d := range dirs { + if !removable[d] { + slog.WarnContext(ctx, "Leaving sandbox dir in place: mounts under it were not all detached", + slog.String("dir", d)) + continue + } if err := os.RemoveAll(d); err != nil { slog.WarnContext(ctx, "Failed to remove leftover sandbox dir", slog.String("dir", d), slog.Any("err", err)) diff --git a/cmd/ateom-microvm/internal/kata/overlay_linux.go b/cmd/ateom-microvm/internal/kata/overlay_linux.go index 4c7154fe0..f6f142192 100644 --- a/cmd/ateom-microvm/internal/kata/overlay_linux.go +++ b/cmd/ateom-microvm/internal/kata/overlay_linux.go @@ -241,6 +241,45 @@ func UnmountMergedRootfs(restoreID, cid string) { } } +// BindIntoShare bind-mounts a host directory at SharedDir(id)/, so the +// ONE virtiofsd serves it to the guest as a subtree of the kataShared mount +// (--announce-submounts presents it to the guest as its own filesystem). +// +// This is THE pattern for exposing another host directory to the guest — the +// durable-dir and CSI volumes ride it today: a separate share would otherwise +// pay for its own virtiofsd (a process, a vhost socket, an fs device in every +// snapshot config and a restore-time revival of all three) per actor, forever. +// A bind costs one mount, and teardown already covers it: CleanupSandboxState +// lazily detaches every mount under the sandbox dir first, and will not remove +// a dir whose mounts it could not all detach, so the source directory (which +// may belong to atelet, as the volume dirs do) is never deleted through a live +// bind. Callers must stage binds before StartVirtiofsd, both to keep +// the served tree complete from the first request and because find-paths +// migration re-opens a restored guest's open files by path at reconnect. +// +// name sits beside the per-container /... entries of the tree, so it must +// not collide with a container id. +func BindIntoShare(ctx context.Context, src, id, name string) error { + if name == "" { + return fmt.Errorf("BindIntoShare: empty share subdir name") + } + dst := filepath.Join(SharedDir(id), name) + // Drop any stale bind first (lazy if busy), then ensure a clean mountpoint. + if err := reaper.Run(exec.Command("umount", dst)); err != nil { + _ = reaper.Run(exec.Command("umount", "-l", dst)) + } + if err := os.MkdirAll(dst, 0o755); err != nil { + return fmt.Errorf("creating share subdir %q: %w", dst, err) + } + cmd := exec.CommandContext(ctx, "mount", "--bind", src, dst) + var stderr strings.Builder + cmd.Stderr = &stderr + if err := reaper.Run(cmd); err != nil { + return fmt.Errorf("bind-mounting %q into the shared tree at %q: %w (%s)", src, dst, err, strings.TrimSpace(stderr.String())) + } + return nil +} + // ReconstructSharedDirFromImage bind-mounts a container's OCI image rootfs at // /rootfs under SharedDir(restoreID) so virtiofsd serves it as the read-only // lower. LEGACY restores only: guests from retired guest-tmpfs-upper snapshots hold diff --git a/cmd/ateom-microvm/internal/kata/overlay_linux_test.go b/cmd/ateom-microvm/internal/kata/overlay_linux_test.go index 368967e63..6af17aec1 100644 --- a/cmd/ateom-microvm/internal/kata/overlay_linux_test.go +++ b/cmd/ateom-microvm/internal/kata/overlay_linux_test.go @@ -47,6 +47,27 @@ func TestUpperWorkDirsAreSiblings(t *testing.T) { } } +// The volume subtrees ride the ONE kataShared device (BindIntoShare): the host +// side is bind-mounted under the directory virtiofsd serves, and the guest +// re-opens the same relative path under its kataShared mount — find-paths +// re-opens open volume files by path on restore, so host and guest must agree. +func TestVolumeSubtreePaths(t *testing.T) { + for name, guest := range map[string]string{ + "durable": GuestDurableVolumeDir("data"), + "csi": GuestCSIVolumeDir("data"), + } { + host := filepath.Join(SharedDir("uid"), name, "data") + hostRel, err := filepath.Rel(SharedDir("uid"), host) + if err != nil { + t.Fatalf("Rel(host): %v", err) + } + guestRel := strings.TrimPrefix(guest, guestSharedDir) + if hostRel != guestRel { + t.Errorf("%s: host-relative path %q != guest-relative path %q; find-paths would re-open the wrong file", name, hostRel, guestRel) + } + } +} + func TestVirtiofsdArgs(t *testing.T) { args := virtiofsdArgs(VirtiofsdOptions{ SocketPath: "/run/vm/virtiofsd.sock", diff --git a/cmd/ateom-microvm/internal/kata/restore.go b/cmd/ateom-microvm/internal/kata/restore.go index 060b1bb2e..2dccbca8f 100644 --- a/cmd/ateom-microvm/internal/kata/restore.go +++ b/cmd/ateom-microvm/internal/kata/restore.go @@ -29,15 +29,3 @@ func VMDir(id string) string { return filepath.Join(vcVMDir, id) } // VsockSocketPath is the hybrid-vsock socket the CH snapshot's vsock device // references; CH recreates the listener here on restore. func VsockSocketPath(id string) string { return filepath.Join(VMDir(id), "clh.sock") } - -// DurableVirtiofsdSocketPath is the vhost-user-fs socket for the actor's writable -// durable-dir share, served by a second virtiofsd alongside the rootfs share's. -func DurableVirtiofsdSocketPath(id string) string { - return filepath.Join(VMDir(id), "virtiofsd-durable.sock") -} - -// CsiVirtiofsdSocketPath is the vhost-user-fs socket for the actor's writable -// CSI volumes share. -func CsiVirtiofsdSocketPath(id string) string { - return filepath.Join(VMDir(id), "virtiofsd-csi.sock") -} diff --git a/cmd/ateom-microvm/restore.go b/cmd/ateom-microvm/restore.go index 2e030af95..a7342c9f5 100644 --- a/cmd/ateom-microvm/restore.go +++ b/cmd/ateom-microvm/restore.go @@ -474,6 +474,12 @@ func rewriteSnapshotSocketPaths(snapshotDir, id string) error { } // The virtio-fs share is served by its per-VMDir virtiofsd socket; the // snapshot recorded the golden actor's, so repoint it at this actor's VMDir. + // kataShared is the ONLY tag a restorable snapshot can carry: anything else + // (the retired multi-virtiofsd tags "ateDurable"/"ateCSI"/"ateUpper" among + // them) must fail loudly here. Repointing such a device used to pass for + // backward compatibility, but nothing spawns those extra virtiofsds anymore, + // so the "compatible" restore just failed later and worse — at CH's vhost + // socket connect, or as a guest wedged on a dead mount. if fss, ok := cfg["fs"].([]any); ok { for _, f := range fss { fm, ok := f.(map[string]any) @@ -483,14 +489,8 @@ func rewriteSnapshotSocketPaths(snapshotDir, id string) error { switch tag, _ := fm["tag"].(string); tag { case kata.FsTag: fm["socket"] = kata.VirtiofsdSocketPath(id) - case "ateDurable": - // Legacy multi-virtiofs snapshot backward compatibility. - fm["socket"] = kata.DurableVirtiofsdSocketPath(id) - case "ateCSI": - // Legacy multi-virtiofs snapshot backward compatibility. - fm["socket"] = kata.CsiVirtiofsdSocketPath(id) default: - return fmt.Errorf("snapshot config %q has fs device with unknown tag %q", cfgPath, tag) + return fmt.Errorf("snapshot config %q has fs device with unknown tag %q (multi-virtiofsd snapshots are no longer restorable)", cfgPath, tag) } } } diff --git a/cmd/ateom-microvm/restore_test.go b/cmd/ateom-microvm/restore_test.go index 1cceaf87c..11d24bb84 100644 --- a/cmd/ateom-microvm/restore_test.go +++ b/cmd/ateom-microvm/restore_test.go @@ -85,35 +85,16 @@ func TestRewriteSnapshotSocketPaths(t *testing.T) { } }) - t.Run("legacy multi-share snapshots repoint each tag", func(t *testing.T) { - // Ordered with the rootfs share last to catch a rewrite that assumes it - // comes first, which would hand the guest the wrong filesystem. - dir := writeSnapshotConfig(t, []map[string]any{ - {"tag": "ateDurable", "socket": "/run/vc/vm/golden/virtiofsd-durable.sock"}, - {"tag": kata.FsTag, "socket": "/run/vc/vm/golden/virtiofsd.sock"}, - }) - if err := rewriteSnapshotSocketPaths(dir, id); err != nil { - t.Fatalf("rewriteSnapshotSocketPaths: %v", err) - } - got := readFsSockets(t, dir) - if got[kata.FsTag] != kata.VirtiofsdSocketPath(id) { - t.Errorf("%s socket = %q, want %q", kata.FsTag, got[kata.FsTag], kata.VirtiofsdSocketPath(id)) - } - if got["ateDurable"] != kata.DurableVirtiofsdSocketPath(id) { - t.Errorf("ateDurable socket = %q, want %q", got["ateDurable"], kata.DurableVirtiofsdSocketPath(id)) - } - if got[kata.FsTag] == got["ateDurable"] { - t.Error("both shares were pointed at the same socket") - } - }) - - t.Run("unknown tag is an error", func(t *testing.T) { - // "ateUpper" is the retired third share's tag: it never appears in - // snapshots this code produces, and one showing up must fail loudly - // rather than be silently repointed. - for _, tag := range []string{"somethingElse", "ateUpper"} { + t.Run("non-kataShared tags are an error", func(t *testing.T) { + // The retired multi-virtiofsd tags ("ateDurable"/"ateCSI" second and + // third shares, "ateUpper" from the retired guest-mounted-upper + // arrangement) must fail loudly, exactly like a tag that never existed: + // nothing spawns those extra virtiofsds anymore, so repointing such a + // device would just defer the failure to CH's vhost socket connect. + for _, tag := range []string{"somethingElse", "ateUpper", "ateDurable", "ateCSI"} { dir := writeSnapshotConfig(t, []map[string]any{ {"tag": tag, "socket": "/run/vc/vm/golden/other.sock"}, + {"tag": kata.FsTag, "socket": "/run/vc/vm/golden/virtiofsd.sock"}, }) if err := rewriteSnapshotSocketPaths(dir, id); err == nil { t.Fatalf("rewriteSnapshotSocketPaths accepted fs tag %q, want an error", tag)