diff --git a/src/baudolo/backup/app.py b/src/baudolo/backup/app.py index e9b478c..46c53a5 100644 --- a/src/baudolo/backup/app.py +++ b/src/baudolo/backup/app.py @@ -95,7 +95,16 @@ def main() -> int: ) if resolve_source is not None: - copy(authoritative=False, source=resolve_source(live_source)) + snapshot_source = resolve_source(live_source) + if os.path.isdir(snapshot_source): + copy(authoritative=True, source=snapshot_source) + else: + print( + f"WARNING: volume '{volume_name}' is not in the snapshot " + "(created after it was taken); copying it live instead.", + flush=True, + ) + copy(authoritative=False) continue if args.everything: diff --git a/src/baudolo/backup/cli.py b/src/baudolo/backup/cli.py index 81d6b38..c72acb8 100644 --- a/src/baudolo/backup/cli.py +++ b/src/baudolo/backup/cli.py @@ -94,4 +94,9 @@ def parse_args() -> argparse.Namespace: p.error("--snapshot and --snapshot-subject must be given together") if args.snapshot and args.shutdown: p.error("--shutdown is meaningless with --snapshot: containers are never stopped") + if args.snapshot and args.hard_restart_projects: + p.error( + "--hard-restart-projects is meaningless with --snapshot: the flag exists " + "for stacks whose database cannot be backed up hot, which a snapshot solves" + ) return args diff --git a/src/baudolo/backup/snapshot.py b/src/baudolo/backup/snapshot.py index c2a6bf1..1799c1b 100644 --- a/src/baudolo/backup/snapshot.py +++ b/src/baudolo/backup/snapshot.py @@ -17,12 +17,11 @@ import os from collections.abc import Callable, Iterator from contextlib import contextmanager -from .shell import execute_shell_command +from .shell import BackupException, execute_shell_command KINDS = ("btrfs", "zfs") - class SnapshotError(RuntimeError): """A snapshot could not be created, resolved or removed.""" @@ -32,7 +31,11 @@ def _resolver(subject: str, root: str) -> Callable[[str], str]: relative = os.path.relpath(os.path.abspath(path), os.path.abspath(subject)) if relative.startswith(".."): raise SnapshotError(f"{path} lies outside the snapshot subject {subject}") - return os.path.join(root, relative) if relative != "." else root + resolved = root if relative == "." else os.path.join(root, relative) + + # abspath drops a trailing separator, and rsync reads "dir/" as its + # contents where "dir" means the directory itself. + return resolved + os.sep if path.endswith(os.sep) else resolved return resolve @@ -74,6 +77,8 @@ def volume_snapshot( Raises: SnapshotError: the kind is unknown, or the snapshot cannot be created. + Removal failure is reported, not raised: a leftover snapshot is a + cleanup problem and must not discard a generation that is complete. """ create = _CREATE.get(kind) if create is None: @@ -83,4 +88,8 @@ def volume_snapshot( try: yield _resolver(subject, root) finally: - run(remove) + try: + run(remove) + except BackupException as error: + # Raising here would also mask whatever the body raised. + print(f"WARNING: {root} could not be removed: {error}", flush=True) diff --git a/tests/e2e/snapshot_db_driver.py b/tests/e2e/snapshot_db_driver.py index 1d7246f..0cf5431 100644 --- a/tests/e2e/snapshot_db_driver.py +++ b/tests/e2e/snapshot_db_driver.py @@ -34,8 +34,8 @@ with volume_snapshot("btrfs", SUBJECT, "dbtest", run=shell) as resolve: VERSIONS, VOLUME, f"{GENERATION}/{VOLUME}", - authoritative=False, - source=resolve(DATADIR) + "/", + authoritative=True, + source=resolve(f"{DATADIR}/"), ) print("SNAPSHOT COPY DONE", flush=True) diff --git a/tests/unit/backup/test_app_snapshot.py b/tests/unit/backup/test_app_snapshot.py new file mode 100644 index 0000000..44ab446 --- /dev/null +++ b/tests/unit/backup/test_app_snapshot.py @@ -0,0 +1,79 @@ +"""Contract of app.main's snapshot branch - the caller that runs in production.""" + +from __future__ import annotations + +import unittest +from unittest import mock + +from baudolo.backup import app +from baudolo.backup.snapshot import volume_snapshot + + +def stubbed_snapshot(kind: str, subject: str, tag: str): + return volume_snapshot(kind, subject, tag, run=lambda command: []) + +ARGV = [ + "baudolo", + "--compose-dir", + "/compose", + "--backups-dir", + "/backups", + "--snapshot", + "btrfs", + "--snapshot-subject", + "/var/lib/docker", +] + + +def drive(*, present: bool) -> list[dict]: + calls: list[dict] = [] + + def record(versions_dir, volume_name, volume_dir, *, authoritative, source): + calls.append( + {"volume": volume_name, "authoritative": authoritative, "source": source} + ) + + with ( + mock.patch("sys.argv", ARGV), + mock.patch.object(app, "get_machine_id", return_value="machine"), + mock.patch.object(app, "create_version_directory", return_value="/gen"), + mock.patch.object(app, "create_volume_directory", return_value="/gen/vol"), + mock.patch.object(app, "load_databases_df", return_value=None), + mock.patch.object(app, "docker_volume_names", return_value=["vol"]), + mock.patch.object(app, "containers_using_volume", return_value=[]), + mock.patch.object(app, "volume_is_fully_ignored", return_value=False), + mock.patch.object(app, "backup_dumps_for_volume", return_value=(False, False)), + mock.patch.object( + app, "get_storage_path", return_value="/var/lib/docker/volumes/vol/_data/" + ), + mock.patch.object(app, "stamp_directory"), + mock.patch.object(app, "handle_docker_compose_services"), + mock.patch.object(app.os.path, "isdir", return_value=present), + mock.patch.object(app, "backup_volume", side_effect=record), + mock.patch.object(app, "volume_snapshot", stubbed_snapshot), + ): + app.main() + return calls + + +class TestSnapshotBranch(unittest.TestCase): + def test_it_passes_a_path_ending_in_a_separator(self) -> None: + source = drive(present=True)[0]["source"] + self.assertTrue(source.endswith("/volumes/vol/_data/"), source) + self.assertNotIn("/var/lib/docker/volumes", source) + + def test_it_reads_from_the_snapshot_and_not_from_the_live_tree(self) -> None: + source = drive(present=True)[0]["source"] + self.assertTrue(source.startswith("/var/lib/.baudolo-"), source) + + def test_it_compares_by_content_against_the_previous_generation(self) -> None: + self.assertTrue(drive(present=True)[0]["authoritative"]) + + def test_a_volume_missing_from_the_snapshot_is_copied_live(self) -> None: + call = drive(present=False)[0] + self.assertEqual(call["source"], "/var/lib/docker/volumes/vol/_data/") + self.assertFalse(call["authoritative"]) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/unit/backup/test_cli.py b/tests/unit/backup/test_cli.py index e1ea17b..e13b949 100644 --- a/tests/unit/backup/test_cli.py +++ b/tests/unit/backup/test_cli.py @@ -48,6 +48,22 @@ class TestSnapshotFlags(unittest.TestCase): def test_shutdown_stays_available_without_a_snapshot(self) -> None: self.assertTrue(parse("--shutdown").shutdown) + def test_hard_restart_is_rejected_because_nothing_is_stopped(self) -> None: + with self.assertRaises(SystemExit): + parse( + "--snapshot", + "btrfs", + "--snapshot-subject", + "/d", + "--hard-restart-projects", + "mailu", + ) + + def test_hard_restart_stays_available_without_a_snapshot(self) -> None: + self.assertEqual( + parse("--hard-restart-projects", "mailu").hard_restart_projects, ["mailu"] + ) + class TestRequiredFlags(unittest.TestCase): def test_backups_dir_is_required(self) -> None: diff --git a/tests/unit/backup/test_snapshot.py b/tests/unit/backup/test_snapshot.py index f6219ac..d49d920 100644 --- a/tests/unit/backup/test_snapshot.py +++ b/tests/unit/backup/test_snapshot.py @@ -4,6 +4,7 @@ from __future__ import annotations import unittest +from baudolo.backup.shell import BackupException from baudolo.backup.snapshot import SnapshotError, volume_snapshot @@ -44,6 +45,14 @@ class TestBtrfs(unittest.TestCase): "/var/lib/.baudolo-20260731/volumes/postgres_data/_data", ) + def test_it_keeps_the_trailing_slash_rsync_reads_as_contents(self) -> None: + run = Runner() + with volume_snapshot("btrfs", "/var/lib/docker", "20260731", run=run) as resolve: + self.assertEqual( + resolve("/var/lib/docker/volumes/postgres_data/_data/"), + "/var/lib/.baudolo-20260731/volumes/postgres_data/_data/", + ) + def test_it_removes_the_snapshot_even_when_the_body_raises(self) -> None: run = Runner() with self.assertRaises(ZeroDivisionError): @@ -103,5 +112,23 @@ class TestRejections(unittest.TestCase): self.assertEqual(resolve("/var/lib/docker"), "/var/lib/.baudolo-20260731") +class Busy(Runner): + def __call__(self, command: str) -> list[str]: + if command.startswith("btrfs subvolume delete"): + raise BackupException("target is busy") + return super().__call__(command) + + +class TestRemovalFailure(unittest.TestCase): + def test_a_failed_removal_does_not_fail_a_completed_run(self) -> None: + with volume_snapshot("btrfs", "/var/lib/docker", "20260731", run=Busy()): + pass + + def test_a_failed_removal_does_not_mask_the_body(self) -> None: + with self.assertRaises(ZeroDivisionError): + with volume_snapshot("btrfs", "/var/lib/docker", "20260731", run=Busy()): + raise ZeroDivisionError + + if __name__ == "__main__": unittest.main()