From 0735e5fa7b2e0d81de409a10c68053023aa9601d Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Sun, 18 Oct 2026 00:22:42 +0200 Subject: [PATCH] fix(backup): mount the zfs snapshot itself so a container can read it The zfs snapshot was read through /.zfs/snapshot/. The kernel automounts that directory through a usermode helper that runs in the host's mount namespace, so a containerised baudolo never sees the mount and finds no snapshot. The snapshot is now mounted explicitly at /.baudolo-, beside where btrfs carves its own, and removed in order: umount, rmdir, zfs destroy. A failed mount destroys the snapshot at once instead of leaking one per run. The e2e suite could not reach this code at all: zpool create on the /img file failed with "no such pool or dataset", because the kernel opens vdevs from a taskq thread that resolves paths in the host's root, where the container's /img does not exist. The pool now sits on a loop device, whose /dev/loopN node the kernel creates on the host as well, and it is destroyed on exit once this script created it, since a pool is kernel-wide. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/baudolo/backup/snapshot.py | 41 ++++++++++++++++------ tests/e2e/test_e2e_snapshot.py | 9 +++-- tests/unit/backup/test_snapshot.py | 55 ++++++++++++++++++++++++++++-- 3 files changed, 90 insertions(+), 15 deletions(-) diff --git a/src/baudolo/backup/snapshot.py b/src/baudolo/backup/snapshot.py index 401e224..b608819 100644 --- a/src/baudolo/backup/snapshot.py +++ b/src/baudolo/backup/snapshot.py @@ -58,27 +58,50 @@ def _resolver(subject: str, root: str) -> Callable[[str], str]: return resolve +def _remove( + root: str, commands: list[list[str]], run: Callable[[list[str]], list[str]] +) -> None: + try: + for command in commands: + run(command) + except BackupError as error: + # Raising here would also mask whatever the body raised. + print(f"WARNING: {root} could not be removed: {error}", flush=True) + + def _btrfs( subject: str, name: str, run: Callable[[list[str]], list[str]] -) -> tuple[str, list[str]]: +) -> tuple[str, list[list[str]]]: # The snapshot goes inside the subject, never beside it: the kernel rejects # a snapshot whose destination is on another filesystem, which is exactly # what the parent directory is when the subject is a mountpoint of its own. target = str(Path(os.path.abspath(subject)) / f".{name}") # noqa: PTH100 - see _resolver run(["btrfs", "subvolume", "snapshot", "-r", subject, target]) - return target, ["btrfs", "subvolume", "delete", target] + return target, [["btrfs", "subvolume", "delete", target]] def _zfs( subject: str, name: str, run: Callable[[list[str]], list[str]] -) -> tuple[str, list[str]]: +) -> tuple[str, list[list[str]]]: output = run(["zfs", "list", "-H", "-o", "name", subject]) dataset = (output[0] if output else "").strip() if not dataset: raise SnapshotError(f"no zfs dataset is mounted at {subject}") - run(["zfs", "snapshot", f"{dataset}@{name}"]) - root = str(Path(subject) / ".zfs" / "snapshot" / name) - return root, ["zfs", "destroy", f"{dataset}@{name}"] + snapshot = f"{dataset}@{name}" + # Exception: mounted here rather than read through .zfs/snapshot, which the + # kernel automounts via a usermode helper in the host's mount namespace, + # so a containerised run never sees it. + root = str(Path(os.path.abspath(subject)) / f".{name}") # noqa: PTH100 - see _resolver + run(["zfs", "snapshot", snapshot]) + remove = [["zfs", "destroy", snapshot]] + try: + run(["mkdir", root]) + remove.insert(0, ["rmdir", root]) + run(["mount", "-t", "zfs", snapshot, root]) + except BaseException: + _remove(root, remove, run) + raise + return root, [["umount", root], *remove] _CREATE = {"btrfs": _btrfs, "zfs": _zfs} @@ -169,8 +192,4 @@ def volume_snapshot( try: yield _resolver(subject, root) finally: - try: - run(remove) - except BackupError as error: - # Raising here would also mask whatever the body raised. - print(f"WARNING: {root} could not be removed: {error}", flush=True) + _remove(root, remove, run) diff --git a/tests/e2e/test_e2e_snapshot.py b/tests/e2e/test_e2e_snapshot.py index ef9be4a..5c73e4d 100644 --- a/tests/e2e/test_e2e_snapshot.py +++ b/tests/e2e/test_e2e_snapshot.py @@ -36,9 +36,14 @@ ZFS_READY = "{ [ -c /dev/zfs ] || modprobe zfs 2>/dev/null; }; [ -c /dev/zfs ]" def mount_script(fstype: str) -> str: """Build a filesystem on a loop device and carve out the snapshot subject.""" if fstype == "zfs": + # Exception: a loop device, not the /img file, because the kernel opens + # vdevs from a taskq thread that resolves paths in the host's root. + # The pool is kernel-wide, so it is destroyed on exit, but only once + # this script created it. return ( - f"{PACKAGES} && {ZFS_READY} && truncate -s 400M /img " - "&& zpool create -m none baudolo /img " + f"{PACKAGES} && {ZFS_READY} && truncate -s 400M /img && {ATTACH} " + '&& zpool create -m none baudolo "$LOOP" ' + "&& trap 'zpool destroy -f baudolo; losetup -d \"$LOOP\"' EXIT " "&& zfs create -o mountpoint=/subject/docker baudolo/docker" ) return ( diff --git a/tests/unit/backup/test_snapshot.py b/tests/unit/backup/test_snapshot.py index ffd6797..49e0c9d 100644 --- a/tests/unit/backup/test_snapshot.py +++ b/tests/unit/backup/test_snapshot.py @@ -21,6 +21,14 @@ class Runner: return [] +class Unmountable(Runner): + def __call__(self, command: list[str]) -> list[str]: + result = super().__call__(command) + if command[0] == "mount": + raise BackupError("permission denied") + return result + + class TestBtrfs(unittest.TestCase): def test_it_creates_a_read_only_snapshot_inside_the_subject(self) -> None: run = Runner() @@ -95,14 +103,57 @@ class TestZfs(unittest.TestCase): run.calls[-1], ["zfs", "destroy", "tank/docker@baudolo-20260731"] ) - def test_it_maps_a_volume_path_through_the_dot_zfs_directory(self) -> None: + def test_it_mounts_the_snapshot_inside_the_subject(self) -> None: + run = self._run() + with volume_snapshot("zfs", "/var/lib/docker", "20260731", run=run): + pass + self.assertIn( + [ + "mount", + "-t", + "zfs", + "tank/docker@baudolo-20260731", + "/var/lib/docker/.baudolo-20260731", + ], + run.calls, + ) + + def test_it_maps_a_volume_path_into_the_mounted_snapshot(self) -> None: run = self._run() with volume_snapshot("zfs", "/var/lib/docker", "20260731", run=run) as resolve: self.assertEqual( resolve("/var/lib/docker/volumes/postgres_data/_data"), - "/var/lib/docker/.zfs/snapshot/baudolo-20260731/volumes/postgres_data/_data", + "/var/lib/docker/.baudolo-20260731/volumes/postgres_data/_data", ) + def test_it_unmounts_before_it_destroys(self) -> None: + run = self._run() + with volume_snapshot("zfs", "/var/lib/docker", "20260731", run=run): + pass + self.assertEqual( + run.calls[-3:], + [ + ["umount", "/var/lib/docker/.baudolo-20260731"], + ["rmdir", "/var/lib/docker/.baudolo-20260731"], + ["zfs", "destroy", "tank/docker@baudolo-20260731"], + ], + ) + + def test_a_failed_mount_leaves_no_snapshot_behind(self) -> None: + run = Unmountable({"zfs list": ["tank/docker"]}) + with ( + self.assertRaises(BackupError), + volume_snapshot("zfs", "/var/lib/docker", "20260731", run=run), + ): + pass + self.assertEqual( + run.calls[-2:], + [ + ["rmdir", "/var/lib/docker/.baudolo-20260731"], + ["zfs", "destroy", "tank/docker@baudolo-20260731"], + ], + ) + def test_an_unmounted_dataset_is_an_error(self) -> None: run = Runner({"zfs list": [""]}) with (