diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6cfb3b7c..e9df92ea 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -62,6 +62,17 @@ jobs: env: COVERAGE_THRESHOLD: ${{ secrets.COVERAGE_THRESHOLD }} run: make check-coverage + # The volume teardown tests need CAP_SYS_ADMIN: umount(2) and loop + # mounts are unavailable to the ordinary runner user, so the tests that + # matter most skip silently in the job above. Without this step the + # package's central guarantee — that a detach cannot discard writes a + # container still holds — is asserted only by comments. + - name: Volume teardown tests (privileged) + run: | + docker run --rm --privileged -e RUNE_REQUIRE_PRIVILEGED_MOUNT=1 \ + -v "$PWD":/src -w /src golang:1.25 sh -c ' + apt-get -qq update && apt-get -qq install -y e2fsprogs >/dev/null && + go test ./pkg/storage/driver/mountsync/... -v' build: name: Build diff --git a/internal/agent/volumes/subsystem.go b/internal/agent/volumes/subsystem.go index 11061209..62c8b6d4 100644 --- a/internal/agent/volumes/subsystem.go +++ b/internal/agent/volumes/subsystem.go @@ -290,10 +290,16 @@ func (s *Subsystem) Stop(ctx context.Context) error { return nil } -// teardownFallbackTimeout bounds a single volume's teardown when the -// caller's context carries no deadline of its own. Provider clients set -// their own (longer) HTTP timeouts, which must not be what decides how -// long shutdown takes. +// teardownFallbackTimeout bounds a single volume's PROVIDER calls when +// the caller's context carries no deadline of its own. Provider clients +// set their own (longer) HTTP timeouts, which must not be what decides +// how long shutdown takes. +// +// It does not bound the whole teardown. The filesystem flush inside +// Driver.Unmount takes no context and is deliberately unbounded, because +// cutting it short detaches a half-written filesystem — so a shutdown can +// legitimately outlive this and any deadline the caller sets. See +// pkg/storage/driver/mountsync. const teardownFallbackTimeout = 8 * time.Second // drainMounts tears down every tracked mount concurrently. @@ -676,6 +682,16 @@ func (s *Subsystem) bringUp(ctx context.Context, vol *types.Volume, id string) e // error. func (s *Subsystem) tearDown(ctx context.Context, id string, m trackedMount) (detached bool, err error) { opctx := s.teardownOpContext(ctx, id, m) + // Named before the call, not after: Unmount flushes the filesystem + // first, which on a volume with a lot of dirty data is the longest + // unattended pause in a shutdown. Without this line the operator sees + // systemd hang with no indication of which volume, or whether it is + // working at all. + s.log.Info("Unmounting volume", + log.Str("volume_id", id), + log.Str("namespace", m.VolumeNS), + log.Str("name", m.VolumeName), + log.Str("target", string(m.Target))) var firstErr error if uerr := m.Driver.Unmount(ctx, opctx, m.Target); uerr != nil { firstErr = fmt.Errorf("agent.volumes: unmount %s: %w", id, uerr) diff --git a/pkg/storage/driver/awsebs/mount_linux.go b/pkg/storage/driver/awsebs/mount_linux.go index f6a95872..0056f152 100644 --- a/pkg/storage/driver/awsebs/mount_linux.go +++ b/pkg/storage/driver/awsebs/mount_linux.go @@ -7,6 +7,8 @@ import ( "fmt" "golang.org/x/sys/unix" + + "github.com/runestack/rune/pkg/storage/driver/mountsync" ) // Mount on Linux calls mount(2) directly. /bin/mount on util-linux 2.39+ @@ -26,13 +28,7 @@ func (execMounter) Mount(ctx context.Context, dev, target, fsType string, readOn return nil } -// Unmount on Linux calls umount2(2) directly with no flags. -func (execMounter) Unmount(ctx context.Context, target string) error { - if !alreadyMounted(ctx, target) { - return nil - } - if err := unix.Unmount(target, 0); err != nil { - return fmt.Errorf("awsebs: umount(2) %s: %w", target, err) - } - return nil +// Unmount on Linux flushes the filesystem, then calls umount2(2). +func (execMounter) Unmount(_ context.Context, target string) error { + return mountsync.Unmount("awsebs", target) } diff --git a/pkg/storage/driver/dovolume/mount_linux.go b/pkg/storage/driver/dovolume/mount_linux.go index 4a401779..fe03ea8c 100644 --- a/pkg/storage/driver/dovolume/mount_linux.go +++ b/pkg/storage/driver/dovolume/mount_linux.go @@ -7,6 +7,8 @@ import ( "fmt" "golang.org/x/sys/unix" + + "github.com/runestack/rune/pkg/storage/driver/mountsync" ) // Mount on Linux calls mount(2) directly. /bin/mount on util-linux @@ -29,13 +31,7 @@ func (execMounter) Mount(ctx context.Context, dev, target, fsType string, readOn return nil } -// Unmount on Linux calls umount2(2) directly with no flags. -func (execMounter) Unmount(ctx context.Context, target string) error { - if !alreadyMounted(ctx, target) { - return nil - } - if err := unix.Unmount(target, 0); err != nil { - return fmt.Errorf("dovolume: umount(2) %s: %w", target, err) - } - return nil +// Unmount on Linux flushes the filesystem, then calls umount2(2). +func (execMounter) Unmount(_ context.Context, target string) error { + return mountsync.Unmount("dovolume", target) } diff --git a/pkg/storage/driver/gcepd/mount_linux.go b/pkg/storage/driver/gcepd/mount_linux.go index 44d6004c..a6296b08 100644 --- a/pkg/storage/driver/gcepd/mount_linux.go +++ b/pkg/storage/driver/gcepd/mount_linux.go @@ -7,6 +7,8 @@ import ( "fmt" "golang.org/x/sys/unix" + + "github.com/runestack/rune/pkg/storage/driver/mountsync" ) // Mount on Linux calls mount(2) directly. /bin/mount on util-linux 2.39+ @@ -26,13 +28,7 @@ func (execMounter) Mount(ctx context.Context, dev, target, fsType string, readOn return nil } -// Unmount on Linux calls umount2(2) directly with no flags. -func (execMounter) Unmount(ctx context.Context, target string) error { - if !alreadyMounted(ctx, target) { - return nil - } - if err := unix.Unmount(target, 0); err != nil { - return fmt.Errorf("gcepd: umount(2) %s: %w", target, err) - } - return nil +// Unmount on Linux flushes the filesystem, then calls umount2(2). +func (execMounter) Unmount(_ context.Context, target string) error { + return mountsync.Unmount("gcepd", target) } diff --git a/pkg/storage/driver/hcloudvolume/mount_linux.go b/pkg/storage/driver/hcloudvolume/mount_linux.go index 3724a5be..7c01c430 100644 --- a/pkg/storage/driver/hcloudvolume/mount_linux.go +++ b/pkg/storage/driver/hcloudvolume/mount_linux.go @@ -7,6 +7,8 @@ import ( "fmt" "golang.org/x/sys/unix" + + "github.com/runestack/rune/pkg/storage/driver/mountsync" ) func (execMounter) Mount(ctx context.Context, dev, target, fsType string, readOnly bool) error { @@ -23,12 +25,7 @@ func (execMounter) Mount(ctx context.Context, dev, target, fsType string, readOn return nil } -func (execMounter) Unmount(ctx context.Context, target string) error { - if !alreadyMounted(ctx, target) { - return nil - } - if err := unix.Unmount(target, 0); err != nil { - return fmt.Errorf("hcloudvolume: umount(2) %s: %w", target, err) - } - return nil +// Unmount on Linux flushes the filesystem, then calls umount2(2). +func (execMounter) Unmount(_ context.Context, target string) error { + return mountsync.Unmount("hcloudvolume", target) } diff --git a/pkg/storage/driver/mountsync/mountsync.go b/pkg/storage/driver/mountsync/mountsync.go new file mode 100644 index 00000000..62cacdc5 --- /dev/null +++ b/pkg/storage/driver/mountsync/mountsync.go @@ -0,0 +1,49 @@ +// Package mountsync flushes a mounted filesystem and unmounts it, for the +// cloud volume drivers that then detach the underlying disk. +// +// The flush is not belt-and-braces. umount(2) writes the filesystem out +// only when it releases the LAST reference to the superblock, and a +// container started with this volume holds a second one: the runtime +// binds the mount into the container's own mount namespace. So the +// agent's umount(2) returns success, flushes nothing, and the detach +// that follows discards every page still dirty. +// +// Measured on loop-backed ext4, writing a 90-byte file without fsync and +// then reading back the raw device (what a detach hands you): +// +// single mount, bare umount(2) rc=0 90 bytes +// second mount held, bare umount(2) rc=0 0 bytes +// +// Zero-length files with correct names, owners and modes is the +// signature operators see. +// +// The consequence for anyone editing this package: the flush must stay +// BEFORE the unmount and must never become conditional on the unmount — +// moving it after, or behind an error check, reads as a cheap +// optimisation because umount(2) "already flushes", and silently +// reopens the bug for every volume a container is holding. The one +// guard that is safe is isMountPoint, which decides only whether there +// is a volume filesystem here to flush at all. See issue #270. +package mountsync + +// Unmount flushes the filesystem at target and then unmounts it. driver +// names the calling driver for error messages ("gcepd"). +// +// When the unmount fails the error states whether the flush succeeded: +// the caller detaches either way, so that is the difference between a +// volume left attached and unwritten data discarded. +// +// The flush is deliberately unbounded: there is no context parameter and +// no timeout. Cutting it short means detaching on a half-written +// filesystem, which is the failure this package exists to prevent, so a +// slow flush is the correct behaviour and not something to "fix" with a +// deadline. Nothing bounds it: SIGKILL does not interrupt an in-flight +// syncfs, so not even systemd's TimeoutStopSec is a ceiling on this. +// +// Idempotent for a caller holding CAP_SYS_ADMIN — a target that is not a +// mount point, or is already gone, returns nil. Unprivileged, umount(2) +// answers EPERM before it looks at the target, and that surfaces as an +// error rather than a silent success. +func Unmount(driver, target string) error { + return unmountTarget(driver, target) +} diff --git a/pkg/storage/driver/mountsync/mountsync_e2e_linux_test.go b/pkg/storage/driver/mountsync/mountsync_e2e_linux_test.go new file mode 100644 index 00000000..42fab335 --- /dev/null +++ b/pkg/storage/driver/mountsync/mountsync_e2e_linux_test.go @@ -0,0 +1,274 @@ +//go:build linux + +package mountsync + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "golang.org/x/sys/unix" +) + +// TestUnmountFlushesWhenAnotherMountHoldsTheSuperblock is the test this +// package exists to pass. +// +// It builds the production shape on a real filesystem: an ext4 volume +// mounted where the agent mounts one, with a SECOND mount of the same +// superblock standing in for the bind a container runtime makes into its +// own mount namespace. A file is written without fsync — an ordinary +// write-back workload — and then the raw device is captured, which is +// what a detach hands back. +// +// Against a bare umount(2) this fixture loses the file — that is the +// production bug. Unmount must recover it intact. +func TestUnmountFlushesWhenAnotherMountHoldsTheSuperblock(t *testing.T) { + requireLoopMount(t) + + for _, tc := range []struct { + name string + bare bool // tear down with a bare umount(2) + wantBytes int + }{ + {"bare umount(2) loses the write", true, 0}, + {"mountsync.Unmount keeps it", false, 90}, + } { + t.Run(tc.name, func(t *testing.T) { + dir, img, target := heldMountFixture(t) + + // The workload writes and does not fsync. + writeManifest(t, target) + + if tc.bare { + if err := unix.Unmount(target, 0); err != nil { + t.Fatalf("bare umount: %v", err) + } + } else if err := Unmount("test", target); err != nil { + t.Fatalf("Unmount: %v", err) + } + + if got := bytesOnDevice(t, dir, img); got != tc.wantBytes { + t.Errorf("recovered %d bytes from the raw device, want %d", got, tc.wantBytes) + } + }) + } +} + +// heldMountFixture builds the production shape: an ext4 volume mounted +// where the agent mounts one, with a second mount of the same superblock +// standing in for the bind a container runtime makes into its own mount +// namespace. Returns the working dir, the backing image, and the mount +// target. +func heldMountFixture(t *testing.T) (dir, img, target string) { + t.Helper() + dir = t.TempDir() + img = filepath.Join(dir, "disk.img") + target = filepath.Join(dir, "mnt") + second := filepath.Join(dir, "held") + for _, d := range []string{target, second} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + } + makeExt4(t, img) + mustRun(t, "mount", "-o", "loop", img, target) + t.Cleanup(func() { _ = exec.Command("umount", "-l", target).Run() }) + + // Create and commit the file, so only its contents are at risk — the + // reported signature is a correctly-named zero-length file. + if err := os.WriteFile(filepath.Join(target, "SYSTEM"), nil, 0o644); err != nil { + t.Fatalf("create: %v", err) + } + mustRun(t, "sync") + + mustRun(t, "mount", "--bind", target, second) + t.Cleanup(func() { _ = exec.Command("umount", "-l", second).Run() }) + return dir, img, target +} + +// writeManifest writes the 90-byte payload without fsync — an ordinary +// write-back workload. +func writeManifest(t *testing.T, target string) { + t.Helper() + if err := os.WriteFile(filepath.Join(target, "SYSTEM"), []byte(strings.Repeat("0", 90)), 0o644); err != nil { + t.Fatalf("write: %v", err) + } +} + +// bytesOnDevice snapshots the backing file and reports the length of +// SYSTEM as it exists on disk — what a detached volume would come back +// with. +func bytesOnDevice(t *testing.T, dir, img string) int { + t.Helper() + snap := filepath.Join(dir, "snap.img") + mustRun(t, "cp", img, snap) + mnt := filepath.Join(dir, "snapmnt") + if err := os.MkdirAll(mnt, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + mustRun(t, "mount", "-o", "loop", snap, mnt) + defer func() { _ = exec.Command("umount", mnt).Run() }() + + fi, err := os.Stat(filepath.Join(mnt, "SYSTEM")) + if err != nil { + if os.IsNotExist(err) { + return -1 // file never reached the device at all + } + t.Fatalf("stat: %v", err) + } + return int(fi.Size()) +} + +func makeExt4(t *testing.T, img string) { + t.Helper() + f, err := os.Create(img) + if err != nil { + t.Fatalf("create image: %v", err) + } + if err := f.Truncate(32 << 20); err != nil { + t.Fatalf("truncate: %v", err) + } + _ = f.Close() + mustRun(t, "mkfs.ext4", "-q", "-F", img) +} + +func mustRun(t *testing.T, name string, args ...string) { + t.Helper() + if out, err := exec.Command(name, args...).CombinedOutput(); err != nil { + t.Fatalf("%s %s: %v (%s)", name, strings.Join(args, " "), err, strings.TrimSpace(string(out))) + } +} + +// requireLoopMount skips unless this process can actually mount a loop +// device, which needs CAP_SYS_ADMIN, mkfs.ext4 and loop support. That is +// the production condition, not the default CI one — see the privileged +// job in the CI workflow. +func requireLoopMount(t *testing.T) { + t.Helper() + // The CI job that exists to run these sets this. Without it a + // degraded runner — no CAP_SYS_ADMIN, no loop device, no mkfs — + // would skip every one of them and the step would still go green, + // which is the silent-skip failure this package was written to end. + required := os.Getenv("RUNE_REQUIRE_PRIVILEGED_MOUNT") != "" + skip := func(format string, args ...any) { + if required { + t.Fatalf("RUNE_REQUIRE_PRIVILEGED_MOUNT is set but "+format, args...) + } + t.Skipf(format, args...) + } + if unix.Geteuid() != 0 { + skip("needs root to mount a loop device") + } + if _, err := exec.LookPath("mkfs.ext4"); err != nil { + skip("needs mkfs.ext4") + } + dir := t.TempDir() + img := filepath.Join(dir, "probe.img") + f, err := os.Create(img) + if err != nil { + skip("cannot create a probe image") + } + _ = f.Truncate(8 << 20) + _ = f.Close() + if out, err := exec.Command("mkfs.ext4", "-q", "-F", img).CombinedOutput(); err != nil { + skip("cannot mkfs: %v (%s)", err, strings.TrimSpace(string(out))) + } + mnt := filepath.Join(dir, "probe") + _ = os.MkdirAll(mnt, 0o755) + if out, err := exec.Command("mount", "-o", "loop", img, mnt).CombinedOutput(); err != nil { + skip("cannot loop-mount here: %v (%s)", err, strings.TrimSpace(string(out))) + } + _ = exec.Command("umount", mnt).Run() +} + +// TestUnmountSymlinkedTargetStillFlushes guards the gate against the two +// syscalls that act on the path after it. +// +// syncTarget's open(2) and umount(2) both follow symlinks. A mount-point +// check that did not would answer "nothing mounted here" for a symlinked +// target, skip the flush, and then unmount successfully — losing the +// writes it was there to protect. +func TestUnmountSymlinkedTargetStillFlushes(t *testing.T) { + requireLoopMount(t) + + dir := t.TempDir() + img := filepath.Join(dir, "disk.img") + real := filepath.Join(dir, "real") + link := filepath.Join(dir, "link") + if err := os.MkdirAll(real, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + makeExt4(t, img) + mustRun(t, "mount", "-o", "loop", img, real) + if err := os.Symlink(real, link); err != nil { + t.Fatalf("symlink: %v", err) + } + + if err := os.WriteFile(filepath.Join(real, "SYSTEM"), nil, 0o644); err != nil { + t.Fatalf("create: %v", err) + } + mustRun(t, "sync") + // A second reference, as a container's bind would be, so a bare + // umount(2) cannot flush on its own. + second := filepath.Join(dir, "held") + if err := os.MkdirAll(second, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + mustRun(t, "mount", "--bind", real, second) + t.Cleanup(func() { _ = exec.Command("umount", "-l", second).Run() }) + + if err := os.WriteFile(filepath.Join(real, "SYSTEM"), []byte(strings.Repeat("0", 90)), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + + // Tear down through the symlink. + if err := Unmount("test", link); err != nil { + t.Fatalf("Unmount via symlink: %v", err) + } + if got := bytesOnDevice(t, dir, img); got != 90 { + t.Errorf("recovered %d bytes through a symlinked target, want 90 — the flush was skipped", got) + } +} + +// TestUnmountBusyTargetReportsFlushed covers the EBUSY branch, which had +// no test in any environment: a host-side holder keeps the mount busy, so +// umount(2) fails and the caller detaches anyway. The message is the only +// warning an operator gets, so assert its wording. +func TestUnmountBusyTargetReportsFlushed(t *testing.T) { + requireLoopMount(t) + + dir := t.TempDir() + img := filepath.Join(dir, "disk.img") + target := filepath.Join(dir, "mnt") + if err := os.MkdirAll(target, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + makeExt4(t, img) + mustRun(t, "mount", "-o", "loop", img, target) + t.Cleanup(func() { _ = exec.Command("umount", "-l", target).Run() }) + + file := filepath.Join(target, "SYSTEM") + if err := os.WriteFile(file, []byte("held"), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + // An open fd in this namespace is what makes umount(2) return EBUSY; + // a bind mount does not (it is an independent reference). + held, err := os.Open(file) + if err != nil { + t.Fatalf("open: %v", err) + } + defer func() { _ = held.Close() }() + + err = Unmount("test", target) + if err == nil { + t.Fatal("expected EBUSY with an open fd holding the mount") + } + if !strings.Contains(err.Error(), "flushed as of now") { + t.Errorf("a busy target was flushed, and the message must say so: %v", err) + } + if strings.Contains(err.Error(), "nothing was flushed") { + t.Errorf("the mount was real and was flushed; message claims otherwise: %v", err) + } +} diff --git a/pkg/storage/driver/mountsync/mountsync_linux.go b/pkg/storage/driver/mountsync/mountsync_linux.go new file mode 100644 index 00000000..f9f44ab2 --- /dev/null +++ b/pkg/storage/driver/mountsync/mountsync_linux.go @@ -0,0 +1,104 @@ +//go:build linux + +package mountsync + +import ( + "errors" + "fmt" + + "golang.org/x/sys/unix" +) + +// syncTarget opens the mount point and calls syncfs(2) on it, flushing +// only the filesystem containing that path. sync(2) would flush every +// filesystem on the node — on a host with several volumes and a busy +// root disk that turns one volume's teardown into everyone's stall, +// inside a shutdown budget measured in seconds. +func syncTarget(path string) error { + fd, err := unix.Open(path, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + if err != nil { + return fmt.Errorf("mountsync: open %s: %w", path, err) + } + defer func() { _ = unix.Close(fd) }() + if err := unix.Syncfs(fd); err != nil { + return fmt.Errorf("mountsync: syncfs %s: %w", path, err) + } + return nil +} + +func unmountTarget(driver, target string) error { + // Flush first, and before the unmount — see the package doc. + var ( + syncErr error + synced bool + ) + if isMountPoint(target) { + syncErr = syncTarget(target) + synced = syncErr == nil + } + // No pre-probe on the unmount itself. A probe needs a subprocess + // (findmnt), teardown runs during shutdown where a cancelled context + // stops exec starting, and a probe that could not run reads as "not + // mounted" — skipping the unmount entirely. umount(2) answers + // authoritatively with no subprocess. + err := unix.Unmount(target, 0) + switch { + case err == nil, notMounted(err): + return nil + case syncErr != nil: + return fmt.Errorf("%s: umount(2) %s: %w (the filesystem was NOT flushed first: %v; a detach now can lose unwritten data)", + driver, target, err, syncErr) + case !synced: + // Nothing was mounted here, so there was nothing to flush. Saying + // "flushed" would assert work that never happened. + return fmt.Errorf("%s: umount(2) %s: %w (nothing appeared to be mounted at this path, so nothing was flushed)", + driver, target, err) + default: + // Not a reassurance: on this path the holder is usually a running + // container, still writing. + return fmt.Errorf("%s: umount(2) %s: %w (flushed as of now; writes made after this point are not on the disk)", + driver, target, err) + } +} + +// isMountPoint reports whether target has a different filesystem from the +// directory containing it, which for these drivers means a volume is +// mounted there — they always mount a distinct block device. +// +// It exists to keep the idempotent path off the root disk: where nothing +// is mounted, syncTarget would resolve to whatever the mount root sits on +// — on a default node, the root disk — which is the whole-node stall +// syncTarget is written to avoid. Advisory only: any uncertainty answers +// "yes, sync it", because an unnecessary flush costs time and a skipped +// one costs data. +func isMountPoint(target string) bool { + var self, parent unix.Stat_t + // Stat, not Lstat: syncTarget's open(2) and umount(2) both follow + // symlinks, so a gate that did not would answer "nothing mounted here" + // for a symlinked target and skip the flush on a live volume. + if err := unix.Stat(target, &self); err != nil { + return true // cannot tell: flush anyway + } + if err := unix.Stat(target+"/..", &parent); err != nil { + return true + } + return self.Dev != parent.Dev +} + +// notMounted reports whether an error from umount(2) means there was +// nothing mounted at the target — the idempotent success this package +// promises. +// +// EPERM is deliberately absent. Unprivileged umount(2) fails with EPERM +// before the kernel considers whether the path is a mount point, so +// treating it as "nothing to do" would turn a missing CAP_SYS_ADMIN into +// a silent no-op — the same class of bug as the findmnt probe this +// replaced. runed holds CAP_SYS_ADMIN in production (it mounts these +// volumes), so EPERM there is a real misconfiguration and must surface. +// +// EINVAL is trusted as "not a mount point" because runed runs in the host +// mount namespace. A runed inside its own namespace would get EINVAL for a +// volume it can see but not unmount, and that would read as success here. +func notMounted(err error) bool { + return errors.Is(err, unix.EINVAL) || errors.Is(err, unix.ENOENT) +} diff --git a/pkg/storage/driver/mountsync/mountsync_linux_test.go b/pkg/storage/driver/mountsync/mountsync_linux_test.go new file mode 100644 index 00000000..92295510 --- /dev/null +++ b/pkg/storage/driver/mountsync/mountsync_linux_test.go @@ -0,0 +1,157 @@ +//go:build linux + +package mountsync + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "golang.org/x/sys/unix" +) + +// --- syncTarget ------------------------------------------------------ + +func TestSyncTargetFlushesRealDirectory(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "f"), []byte("data"), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + if err := syncTarget(dir); err != nil { + t.Errorf("syncTarget on a real directory should succeed, got %v", err) + } +} + +func TestSyncTargetReportsMissingPath(t *testing.T) { + if err := syncTarget(filepath.Join(t.TempDir(), "does-not-exist")); err == nil { + t.Error("syncTarget must report a path it could not open, not report success") + } +} + +// O_DIRECTORY is a sanity check that the caller passed a mount point +// rather than a file. It does not change which filesystem gets flushed — +// syncfs works through either — so this pins the guard, not a safety +// property. +func TestSyncTargetRejectsNonDirectory(t *testing.T) { + f := filepath.Join(t.TempDir(), "afile") + if err := os.WriteFile(f, []byte("x"), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + if err := syncTarget(f); err == nil { + t.Error("syncTarget on a non-directory should error rather than claim success") + } +} + +// --- isMountPoint ---------------------------------------------------- + +func TestIsMountPointFalseForPlainDirectory(t *testing.T) { + if isMountPoint(t.TempDir()) { + t.Error("a plain directory must not be treated as a mount point") + } +} + +func TestIsMountPointAssumesMountedWhenItCannotTell(t *testing.T) { + if !isMountPoint(filepath.Join(t.TempDir(), "absent")) { + t.Error("an unstattable target must be assumed mounted, so the flush still happens") + } +} + +// --- notMounted ------------------------------------------------------ + +// Getting this wrong is expensive in both directions: too generous and a +// failed unmount reports success while the caller detaches a live +// filesystem; too strict and ordinary idempotent teardown looks broken. +// +// Table-driven so the classifier is covered by the ordinary CI job, +// which cannot call umount(2). +func TestNotMountedClassification(t *testing.T) { + cases := []struct { + name string + err error + want bool + }{ + {"EINVAL — not a mount point", unix.EINVAL, true}, + {"ENOENT — target is gone", unix.ENOENT, true}, + {"EBUSY — something still holds it", unix.EBUSY, false}, + // EPERM means we could not even try. Folding it in would turn a + // missing capability into a silent no-op. + {"EPERM — we lack CAP_SYS_ADMIN", unix.EPERM, false}, + {"EACCES — permission on a path component", unix.EACCES, false}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := notMounted(c.err); got != c.want { + t.Errorf("notMounted(%v) = %v, want %v", c.err, got, c.want) + } + }) + } +} + +// --- Unmount --------------------------------------------------------- + +// Idempotence is only observable to a privileged caller: without +// CAP_SYS_ADMIN the kernel answers EPERM before it evaluates the target. +// The ordinary CI job skips this; the privileged job in the CI workflow +// runs it. +func TestUnmountIsIdempotentOnNonMountpoint(t *testing.T) { + requirePrivilegedUnmount(t) + if err := Unmount("test", t.TempDir()); err != nil { + t.Errorf("Unmount on a non-mountpoint must succeed (idempotent), got %v", err) + } +} + +func TestUnmountMissingTargetIsNotAnError(t *testing.T) { + requirePrivilegedUnmount(t) + if err := Unmount("test", filepath.Join(t.TempDir(), "never-created")); err != nil { + t.Errorf("Unmount on a missing target should be a no-op, got %v", err) + } +} + +// The message on a failed unmount is the only warning an operator gets +// before the caller detaches anyway, so it must state what is on the disk +// rather than reassure. +func TestUnmountReportsUnflushedFilesystem(t *testing.T) { + err := Unmount("test", filepath.Join(t.TempDir(), "absent", "deeper")) + if err == nil { + t.Skip("environment allowed the unmount; the both-failed branch is unreachable here") + } + if !strings.Contains(err.Error(), "NOT flushed") { + t.Errorf("an unflushed failure must warn about data loss, got: %v", err) + } +} + +// requirePrivilegedUnmount skips when the process cannot call umount(2) +// at all, so a CI failure means a real regression rather than a sandbox. +func requirePrivilegedUnmount(t *testing.T) { + t.Helper() + if err := unix.Unmount(t.TempDir(), 0); errors.Is(err, unix.EPERM) { + t.Skip("umount(2) needs CAP_SYS_ADMIN; runed has it in production, this environment does not") + } +} + +// TestUnmountNothingMountedSaysNothingFlushed covers the third message +// branch. Reporting "flushed as of now" when the target was never a mount +// point would assert work that did not happen — and that combination is +// not contrived: it is the shape when runed has lost CAP_SYS_ADMIN, since +// the mount never succeeded either. +// +// Runs unprivileged, which is where the branch is reachable: with the +// capability, umount(2) on a non-mount-point returns EINVAL and the call +// succeeds instead. +func TestUnmountNothingMountedSaysNothingFlushed(t *testing.T) { + if err := unix.Unmount(t.TempDir(), 0); !errors.Is(err, unix.EPERM) { + t.Skip("privileged: umount(2) succeeds on a non-mount-point, so this branch is unreachable") + } + err := Unmount("test", t.TempDir()) + if err == nil { + t.Fatal("expected the unprivileged umount to fail") + } + if !strings.Contains(err.Error(), "nothing appeared to be mounted") { + t.Errorf("must not claim a flush that never happened, got: %v", err) + } + if strings.Contains(err.Error(), "flushed as of now") { + t.Errorf("claims a flush on a path where nothing was mounted: %v", err) + } +} diff --git a/pkg/storage/driver/mountsync/mountsync_other.go b/pkg/storage/driver/mountsync/mountsync_other.go new file mode 100644 index 00000000..1bda30b7 --- /dev/null +++ b/pkg/storage/driver/mountsync/mountsync_other.go @@ -0,0 +1,14 @@ +//go:build !linux + +package mountsync + +import "errors" + +// mountsync.go is untagged, so this keeps the package compiling on +// darwin. Nothing outside this package calls it — each driver has its own +// !linux mount path — and it returns an error rather than nil so that +// whoever does wire it up finds out immediately, instead of getting a +// silent success on a data-loss-critical routine. +func unmountTarget(string, string) error { + return errors.New("mountsync: unmount is not supported on this platform") +}