From 1d86277a94f85be92f81c0df5694c6a456ba6e3e Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Mon, 17 Aug 2026 16:30:25 +0200 Subject: [PATCH] refactor(backup)!: run argv lists, never a shell The backup built command strings and handed them to shell=True, so four interpolated values per dump - user, password, container, database - were each a way out of the command. validate_database covered one of them since the previous commit; now there is nothing to cover: every command is an argv list, and a value can only ever be an argument. execute_to_file absorbs the atomic dump write. The shell redirect into .tmp and the separate mv process become a Python file handle and os.replace, and a failing dump deletes its partial file instead of leaving it. PGPASSWORD moves out of the command string into the child's environment, where a process listing does not show it. docker exec is built in one place, docker_exec_argv; db.py's three hand-built copies and the probe use it. The dead docker_volume_exists goes - never called, and the restore side owns the living twin. The rsync quoting in --link-dest falls away: inside an argv it would have become part of the path. The snapshot module's injected runner changes type with it, which the three e2e drivers implement - the first conversion missed them, btrfs ran with no arguments, and the e2e caught it. Marked breaking for that contract: any external runner injected into volume_snapshot must now accept a list. Co-Authored-By: Claude Opus 5 (1M context) --- src/baudolo/backup/db.py | 96 ++++++++++----------- src/baudolo/backup/docker.py | 61 ++++++++----- src/baudolo/backup/layout.py | 2 +- src/baudolo/backup/shell.py | 69 ++++++++++++--- src/baudolo/backup/snapshot.py | 20 +++-- src/baudolo/backup/volume.py | 15 ++-- tests/e2e/faithful_driver.py | 10 +-- tests/e2e/snapshot_db_driver.py | 8 +- tests/e2e/snapshot_driver.py | 8 +- tests/unit/backup/test_db_mariadb_dump.py | 71 +++++++++------ tests/unit/backup/test_docker_tool_probe.py | 4 +- tests/unit/backup/test_snapshot.py | 32 ++++--- tests/unit/backup/test_volume.py | 2 +- 13 files changed, 244 insertions(+), 154 deletions(-) diff --git a/src/baudolo/backup/db.py b/src/baudolo/backup/db.py index d6305d9..2045996 100644 --- a/src/baudolo/backup/db.py +++ b/src/baudolo/backup/db.py @@ -7,7 +7,10 @@ import re import pandas -from .shell import BackupException, execute_shell_command +from baudolo.databases import CLUSTER_ROW, validate_database + +from .docker import docker_exec_argv +from .shell import BackupException, execute_to_file log = logging.getLogger(__name__) @@ -21,47 +24,21 @@ def get_instance(container: str, database_containers: list[str]) -> str: return re.split(r"(_|-)(database|db|postgres)", container)[0] -def _validate_database_value(value: str | None, *, instance: str) -> str: - """ - Enforce explicit database semantics: - - - "*" => dump ALL databases (cluster dump for Postgres) - - "" => dump exactly this database - - "" => invalid configuration (would previously result in NaN / nan.backup.sql) - """ - v = (value or "").strip() - if v == "": - raise ValueError( - f"Invalid databases.csv entry for instance '{instance}': " - "column 'database' must be '*' or a concrete database name (not empty)." - ) - return v - - -def _atomic_write_cmd(cmd: str, out_file: str) -> None: - """ - Write dump output atomically: - - write to .tmp - - rename to only on success - - This prevents empty or partial dump files from being treated as valid backups. - """ - tmp = f"{out_file}.tmp" - execute_shell_command(f"{cmd} > {tmp}") - execute_shell_command(f"mv {tmp} {out_file}") - - def fallback_pg_dumpall( container: str, username: str, password: str, out_file: str ) -> None: """ Perform a full Postgres cluster dump using pg_dumpall. """ - cmd = ( - f"PGPASSWORD={password} docker exec -i {container} " - f"pg_dumpall -U {username} -h localhost" + execute_to_file( + docker_exec_argv( + container, + ["pg_dumpall", "-U", username, "-h", "localhost"], + interactive=True, + ), + out_file, + env={"PGPASSWORD": password}, ) - _atomic_write_cmd(cmd, out_file) def backup_database( @@ -99,13 +76,13 @@ def backup_database( user = (getattr(row, "username", "") or "").strip() password = (getattr(row, "password", "") or "").strip() - db_value = _validate_database_value(raw_db, instance=instance_name) + db_value = validate_database(raw_db, instance=instance_name) - if db_value == "*": + if db_value == CLUSTER_ROW: if db_type != "postgres": raise ValueError( f"databases.csv entry for instance '{instance_name}': " - "'*' is currently only supported for Postgres." + f"'{CLUSTER_ROW}' is currently only supported for Postgres." ) cluster_file = os.path.join(out_dir, f"{instance_name}.cluster.backup.sql") @@ -118,23 +95,46 @@ def backup_database( if db_type == "mariadb": # Force TCP so auth matches ''@'%' instead of socket -> 'localhost'. - cmd = ( - f"docker exec {container} {dump_tool} " - f"-h 127.0.0.1 --protocol=tcp " - f"-u {user} -p{password} {db_name}" + execute_to_file( + docker_exec_argv( + container, + [ + dump_tool, + "-h", + "127.0.0.1", + "--protocol=tcp", + "-u", + user, + f"-p{password}", + db_name, + ], + ), + dump_file, ) - _atomic_write_cmd(cmd, dump_file) produced = True continue if db_type == "postgres": try: - cmd = ( - f"PGPASSWORD={password} docker exec -i {container} " - f"pg_dump -U {user} -d {db_name} -h localhost " - f"--no-owner --no-privileges" + execute_to_file( + docker_exec_argv( + container, + [ + "pg_dump", + "-U", + user, + "-d", + db_name, + "-h", + "localhost", + "--no-owner", + "--no-privileges", + ], + interactive=True, + ), + dump_file, + env={"PGPASSWORD": password}, ) - _atomic_write_cmd(cmd, dump_file) produced = True except BackupException as e: raise BackupException( diff --git a/src/baudolo/backup/docker.py b/src/baudolo/backup/docker.py index 5b7cce5..825e35a 100644 --- a/src/baudolo/backup/docker.py +++ b/src/baudolo/backup/docker.py @@ -1,18 +1,27 @@ from __future__ import annotations +from collections.abc import Sequence + from .shell import BackupException, execute_shell_command +def docker_exec_argv( + container: str, argv: Sequence[str], *, interactive: bool = False +) -> list[str]: + """The argv that runs *argv* inside *container*.""" + return ["docker", "exec", *(["-i"] if interactive else []), container, *argv] + + def get_image_info(container: str) -> str: return execute_shell_command( - f"docker inspect --format '{{{{.Config.Image}}}}' {container}" + ["docker", "inspect", "--format", "{{.Config.Image}}", container] )[0] def image_id(container: str) -> str: """The container's image ID, identical for every replica of one image.""" return execute_shell_command( - f"docker inspect --format '{{{{.Image}}}}' {container}" + ["docker", "inspect", "--format", "{{.Image}}", container] )[0].strip() @@ -24,19 +33,26 @@ def has_tool(container: str, tool: str) -> bool: tool it ships. """ try: - execute_shell_command(f"docker exec {container} {tool} --version") + execute_shell_command(docker_exec_argv(container, [tool, "--version"])) except BackupException: return False return True def docker_volume_names() -> list[str]: - return execute_shell_command("docker volume ls --format '{{.Name}}'") + return execute_shell_command(["docker", "volume", "ls", "--format", "{{.Name}}"]) def containers_using_volume(volume_name: str) -> list[str]: return execute_shell_command( - f"docker ps --filter volume=\"{volume_name}\" --format '{{{{.Names}}}}'" + [ + "docker", + "ps", + "--filter", + f"volume={volume_name}", + "--format", + "{{.Names}}", + ] ) @@ -50,12 +66,25 @@ def is_swarm_task(container: str) -> bool: keeps failing the run loudly instead of silently skipping the stop.""" try: out = execute_shell_command( - "docker inspect --format " - f"'{{{{index .Config.Labels \"com.docker.swarm.task.id\"}}}}' {container}" + [ + "docker", + "inspect", + "--format", + '{{index .Config.Labels "com.docker.swarm.task.id"}}', + container, + ] ) except BackupException: still_listed = execute_shell_command( - f"docker ps -a --filter name=^{container}$ --format '{{{{.Names}}}}'" + [ + "docker", + "ps", + "-a", + "--filter", + f"name=^{container}$", + "--format", + "{{.Names}}", + ] ) if still_listed and still_listed[0].strip(): raise @@ -82,17 +111,5 @@ def change_containers_status(containers: list[str], status: str) -> None: if not containers: print(f"No containers to {status}.", flush=True) return - names = " ".join(containers) - print(f"{status.capitalize()} containers: {names}...", flush=True) - execute_shell_command(f"docker {status} {names}") - - -def docker_volume_exists(volume: str) -> bool: - # Avoid throwing exceptions for exists checks. - try: - execute_shell_command( - f"docker volume inspect {volume} >/dev/null 2>&1 && echo OK" - ) - return True - except BackupException: - return False + print(f"{status.capitalize()} containers: {' '.join(containers)}...", flush=True) + execute_shell_command(["docker", status, *containers]) diff --git a/src/baudolo/backup/layout.py b/src/baudolo/backup/layout.py index 6f99fbe..0374134 100644 --- a/src/baudolo/backup/layout.py +++ b/src/baudolo/backup/layout.py @@ -11,7 +11,7 @@ from .shell import BackupException, execute_shell_command def get_machine_id() -> str: - return execute_shell_command("sha256sum /etc/machine-id")[0][0:64] + return execute_shell_command(["sha256sum", "/etc/machine-id"])[0][0:64] def stamp_directory(version_dir: str) -> None: diff --git a/src/baudolo/backup/shell.py b/src/baudolo/backup/shell.py index 4ca650f..018eb6f 100644 --- a/src/baudolo/backup/shell.py +++ b/src/baudolo/backup/shell.py @@ -1,26 +1,71 @@ +"""Running external commands without a shell. + +Every command is an argv list. A database name, a password or a container name +therefore cannot close a quote and start a second command, which a formatted +string handed to ``shell=True`` allowed. +""" + from __future__ import annotations +import os import subprocess +from collections.abc import Mapping, Sequence class BackupException(Exception): """Generic exception for backup errors.""" -def execute_shell_command(command: str) -> list[str]: - """Execute a shell command and return its output lines.""" - print(command, flush=True) +def _child_env(env: Mapping[str, str] | None) -> dict[str, str] | None: + return None if env is None else {**os.environ, **env} + + +def _fail(command: Sequence[str], returncode: int, out: bytes, err: bytes) -> None: + raise BackupException( + f"Error in command: {' '.join(command)}\n" + f"Output: {out}\nError: {err}\n" + f"Exit code: {returncode}" + ) + + +def execute_shell_command( + command: Sequence[str], *, env: Mapping[str, str] | None = None +) -> list[str]: + """Run *command* and return its stdout lines. + + Args: + command: argv, the program first. + env: variables added to the child's environment, for values that must + not appear in the argv of a process listing. + """ + command = list(command) + print(" ".join(command), flush=True) process = subprocess.Popen( - [command], - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - shell=True, + command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=_child_env(env) ) out, err = process.communicate() if process.returncode != 0: - raise BackupException( - f"Error in command: {command}\n" - f"Output: {out}\nError: {err}\n" - f"Exit code: {process.returncode}" - ) + _fail(command, process.returncode, out, err) return [line.decode("utf-8") for line in out.splitlines()] + + +def execute_to_file( + command: Sequence[str], out_file: str, *, env: Mapping[str, str] | None = None +) -> None: + """Run *command*, writing its stdout to *out_file* only once it succeeded. + + The output goes to a sibling temporary file first, so a partial or empty + stream from a failing dump never takes the place of a valid backup. + """ + command = list(command) + print(" ".join(command), flush=True) + tmp = f"{out_file}.tmp" + with open(tmp, "wb") as handle: + process = subprocess.Popen( + command, stdout=handle, stderr=subprocess.PIPE, env=_child_env(env) + ) + _, err = process.communicate() + if process.returncode != 0: + os.unlink(tmp) + _fail(command, process.returncode, b"", err) + os.replace(tmp, out_file) diff --git a/src/baudolo/backup/snapshot.py b/src/baudolo/backup/snapshot.py index 8412d66..5a6c104 100644 --- a/src/baudolo/backup/snapshot.py +++ b/src/baudolo/backup/snapshot.py @@ -48,23 +48,27 @@ def _resolver(subject: str, root: str) -> Callable[[str], str]: return resolve -def _btrfs(subject: str, name: str, run: Callable[[str], list[str]]) -> tuple[str, str]: +def _btrfs( + subject: str, name: str, run: Callable[[list[str]], list[str]] +) -> tuple[str, 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 = os.path.join(os.path.abspath(subject), f".{name}") - run(f"btrfs subvolume snapshot -r {subject} {target}") - return target, f"btrfs subvolume delete {target}" + run(["btrfs", "subvolume", "snapshot", "-r", subject, target]) + return target, ["btrfs", "subvolume", "delete", target] -def _zfs(subject: str, name: str, run: Callable[[str], list[str]]) -> tuple[str, str]: - output = run(f"zfs list -H -o name {subject}") +def _zfs( + subject: str, name: str, run: Callable[[list[str]], list[str]] +) -> tuple[str, 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(f"zfs snapshot {dataset}@{name}") + run(["zfs", "snapshot", f"{dataset}@{name}"]) root = os.path.join(subject, ".zfs", "snapshot", name) - return root, f"zfs destroy {dataset}@{name}" + return root, ["zfs", "destroy", f"{dataset}@{name}"] _CREATE = {"btrfs": _btrfs, "zfs": _zfs} @@ -129,7 +133,7 @@ def volume_snapshot( kind: str, subject: str, tag: str, - run: Callable[[str], list[str]] = execute_shell_command, + run: Callable[[list[str]], list[str]] = execute_shell_command, ) -> Iterator[Callable[[str], str]]: """Yield a resolver mapping a path under ``subject`` into a snapshot of it. diff --git a/src/baudolo/backup/volume.py b/src/baudolo/backup/volume.py index fb7b5a2..dc72cac 100644 --- a/src/baudolo/backup/volume.py +++ b/src/baudolo/backup/volume.py @@ -30,7 +30,7 @@ class Backing: def inspect_backing(volume_name: str) -> Backing: reported = execute_shell_command( - f"docker volume inspect --format '{{{{json .}}}}' {volume_name}" + ["docker", "volume", "inspect", "--format", "{{json .}}", volume_name] )[0] data = json.loads(reported) return Backing( @@ -73,13 +73,12 @@ def backup_volume( pathlib.Path(dest).mkdir(parents=True, exist_ok=True) last = get_last_backup_dir(versions_dir, volume_name, dest) - link_dest = f"--link-dest='{last}'" if last else "" - verify = "--checksum " if authoritative else "" - - cmd = ( - f"rsync -aP --no-D --delete --delete-excluded " - f"{verify}{link_dest} {source} {dest}" - ) + cmd = ["rsync", "-aP", "--no-D", "--delete", "--delete-excluded"] + if authoritative: + cmd.append("--checksum") + if last: + cmd.append(f"--link-dest={last}") + cmd += [source, dest] try: execute_shell_command(cmd) diff --git a/tests/e2e/faithful_driver.py b/tests/e2e/faithful_driver.py index 55b3614..816ed0f 100644 --- a/tests/e2e/faithful_driver.py +++ b/tests/e2e/faithful_driver.py @@ -23,13 +23,11 @@ from baudolo.backup.volume import Backing SUBJECT = sys.argv[1] -def shell(command: str) -> list[str]: - proc = subprocess.run( - command, shell=True, capture_output=True, text=True, check=False - ) +def shell(command: list[str]) -> list[str]: + proc = subprocess.run(command, capture_output=True, text=True, check=False) if proc.returncode != 0: raise SnapshotError( - f"{command} exited {proc.returncode}: {proc.stderr.strip()}" + f"{' '.join(command)} exited {proc.returncode}: {proc.stderr.strip()}" ) return proc.stdout.splitlines() @@ -50,7 +48,7 @@ def volume(name: str, payload: str) -> Path: plain = volume("plain", "plain-payload") own = Path(SUBJECT) / "volumes" / "own" / "_data" own.mkdir(parents=True, exist_ok=True) -shell(f"mount -t tmpfs tmpfs {own}") +shell(["mount", "-t", "tmpfs", "tmpfs", own]) (own / "state").write_text("own-payload") check("a plain volume is captured", unsnapshotted(Backing(str(plain)), SUBJECT) is None) diff --git a/tests/e2e/snapshot_db_driver.py b/tests/e2e/snapshot_db_driver.py index cb11dea..0f19ff7 100644 --- a/tests/e2e/snapshot_db_driver.py +++ b/tests/e2e/snapshot_db_driver.py @@ -22,13 +22,11 @@ VERSIONS = "/backups" GENERATION = f"{VERSIONS}/20260731" -def shell(command: str) -> list[str]: - proc = subprocess.run( - command, shell=True, capture_output=True, text=True, check=False - ) +def shell(command: list[str]) -> list[str]: + proc = subprocess.run(command, capture_output=True, text=True, check=False) if proc.returncode != 0: raise SnapshotError( - f"{command} exited {proc.returncode}: {proc.stderr.strip()}" + f"{' '.join(command)} exited {proc.returncode}: {proc.stderr.strip()}" ) return proc.stdout.splitlines() diff --git a/tests/e2e/snapshot_driver.py b/tests/e2e/snapshot_driver.py index bfc9faf..0f12a6a 100644 --- a/tests/e2e/snapshot_driver.py +++ b/tests/e2e/snapshot_driver.py @@ -19,13 +19,11 @@ SUBJECT = sys.argv[2] EXPECT = sys.argv[3] -def shell(command: str) -> list[str]: - proc = subprocess.run( - command, shell=True, capture_output=True, text=True, check=False - ) +def shell(command: list[str]) -> list[str]: + proc = subprocess.run(command, capture_output=True, text=True, check=False) if proc.returncode != 0: raise SnapshotError( - f"{command} exited {proc.returncode}: {proc.stderr.strip()}" + f"{' '.join(command)} exited {proc.returncode}: {proc.stderr.strip()}" ) return proc.stdout.splitlines() diff --git a/tests/unit/backup/test_db_mariadb_dump.py b/tests/unit/backup/test_db_mariadb_dump.py index a6709d4..b2c6da5 100644 --- a/tests/unit/backup/test_db_mariadb_dump.py +++ b/tests/unit/backup/test_db_mariadb_dump.py @@ -13,16 +13,16 @@ def _df(rows): ) -def _capture_commands(*, db_type, rows, container, dump_tool="mariadb-dump"): +def _capture_dumps(*, db_type, rows, container, dump_tool="mariadb-dump"): + """Every (argv, env) the dump path would have run.""" captured = [] - def _capture(cmd): - captured.append(cmd) - return [] + def _capture(command, out_file, *, env=None): + captured.append((list(command), env)) with ( tempfile.TemporaryDirectory() as td, - patch.object(db_mod, "execute_shell_command", side_effect=_capture), + patch.object(db_mod, "execute_to_file", side_effect=_capture), ): db_mod.backup_database( container=container, @@ -41,45 +41,64 @@ class TestMariaDBDumpUsesTCP(unittest.TestCase): # the connection is auth-matched against '%' instead of socket->localhost. def test_mariadb_dump_forces_tcp_loopback(self): - captured = _capture_commands( + captured = _capture_dumps( db_type="mariadb", rows=[("mariadb", "appdb", "appuser", "s3cret")], container="mariadb", ) - dump_cmds = [c for c in captured if "mariadb-dump" in c] - self.assertEqual( - len(dump_cmds), 1, f"expected one dump command, got: {captured}" - ) + self.assertEqual(len(captured), 1, f"expected one dump, got: {captured}") - cmd = dump_cmds[0] - self.assertIn("-h 127.0.0.1", cmd) - self.assertIn("--protocol=tcp", cmd) - self.assertIn("-u appuser", cmd) - self.assertIn("-ps3cret", cmd) - self.assertIn(" appdb", cmd) + argv, env = captured[0] + self.assertEqual(argv[:3], ["docker", "exec", "mariadb"]) + self.assertIn("--protocol=tcp", argv) + self.assertEqual(argv[argv.index("-h") + 1], "127.0.0.1") + self.assertEqual(argv[argv.index("-u") + 1], "appuser") + self.assertIn("-ps3cret", argv) + self.assertEqual(argv[-1], "appdb") + self.assertIsNone(env) def test_the_probed_client_is_the_one_invoked(self): - captured = _capture_commands( + captured = _capture_dumps( db_type="mariadb", rows=[("mariadb", "appdb", "appuser", "s3cret")], container="mariadb", dump_tool="mysqldump", ) - dump_cmds = [c for c in captured if "mysqldump" in c] - self.assertEqual( - len(dump_cmds), 1, f"expected one dump command, got: {captured}" - ) - self.assertNotIn("mariadb-dump", dump_cmds[0]) + argv, _env = captured[0] + self.assertIn("mysqldump", argv) + self.assertNotIn("mariadb-dump", argv) def test_postgres_dump_unaffected(self): - captured = _capture_commands( + captured = _capture_dumps( db_type="postgres", rows=[("pg", "appdb", "appuser", "s3cret")], container="pg", ) - dump_cmds = [c for c in captured if "pg_dump" in c and "pg_dumpall" not in c] - self.assertEqual(len(dump_cmds), 1) - self.assertNotIn("--protocol=tcp", dump_cmds[0]) + argv, _env = captured[0] + self.assertIn("pg_dump", argv) + self.assertNotIn("--protocol=tcp", argv) + + def test_the_password_travels_in_the_environment_not_the_argv(self): + """A process listing shows argv; PGPASSWORD must not be in it.""" + captured = _capture_dumps( + db_type="postgres", + rows=[("pg", "appdb", "appuser", "s3cret")], + container="pg", + ) + argv, env = captured[0] + self.assertEqual(env, {"PGPASSWORD": "s3cret"}) + self.assertNotIn("s3cret", argv) + + +class TestNoShellReachesTheDump(unittest.TestCase): + def test_a_hostile_database_name_never_reaches_a_command(self): + """validate_database refuses it, so no argv is built at all.""" + with self.assertRaises(ValueError): + _capture_dumps( + db_type="postgres", + rows=[("pg", "app;rm -rf /", "appuser", "s3cret")], + container="pg", + ) if __name__ == "__main__": diff --git a/tests/unit/backup/test_docker_tool_probe.py b/tests/unit/backup/test_docker_tool_probe.py index b4f2dfb..53021a0 100644 --- a/tests/unit/backup/test_docker_tool_probe.py +++ b/tests/unit/backup/test_docker_tool_probe.py @@ -35,7 +35,9 @@ class TestHasTool(unittest.TestCase): with patch.object(docker_mod, "execute_shell_command", side_effect=_capture): docker_mod.has_tool("c1", "pg_dumpall") - self.assertEqual(captured, ["docker exec c1 pg_dumpall --version"]) + self.assertEqual( + captured, [["docker", "exec", "c1", "pg_dumpall", "--version"]] + ) if __name__ == "__main__": diff --git a/tests/unit/backup/test_snapshot.py b/tests/unit/backup/test_snapshot.py index 984e835..8a83513 100644 --- a/tests/unit/backup/test_snapshot.py +++ b/tests/unit/backup/test_snapshot.py @@ -10,13 +10,13 @@ from baudolo.backup.snapshot import SnapshotError, volume_snapshot class Runner: def __init__(self, replies: dict[str, list[str]] | None = None) -> None: - self.calls: list[str] = [] + self.calls: list[list[str]] = [] self.replies = replies or {} - def __call__(self, command: str) -> list[str]: - self.calls.append(command) + def __call__(self, command: list[str]) -> list[str]: + self.calls.append(list(command)) for prefix, reply in self.replies.items(): - if command.startswith(prefix): + if " ".join(command).startswith(prefix): return reply return [] @@ -28,7 +28,14 @@ class TestBtrfs(unittest.TestCase): pass self.assertEqual( run.calls[0], - "btrfs subvolume snapshot -r /var/lib/docker /var/lib/docker/.baudolo-20260731", + [ + "btrfs", + "subvolume", + "snapshot", + "-r", + "/var/lib/docker", + "/var/lib/docker/.baudolo-20260731", + ], ) def test_it_removes_the_snapshot_afterwards(self) -> None: @@ -36,7 +43,8 @@ class TestBtrfs(unittest.TestCase): with volume_snapshot("btrfs", "/var/lib/docker", "20260731", run=run): pass self.assertEqual( - run.calls[-1], "btrfs subvolume delete /var/lib/docker/.baudolo-20260731" + run.calls[-1], + ["btrfs", "subvolume", "delete", "/var/lib/docker/.baudolo-20260731"], ) def test_it_maps_a_volume_path_into_the_snapshot(self) -> None: @@ -66,7 +74,7 @@ class TestBtrfs(unittest.TestCase): volume_snapshot("btrfs", "/var/lib/docker", "20260731", run=run), ): raise ZeroDivisionError - self.assertTrue(run.calls[-1].startswith("btrfs subvolume delete")) + self.assertEqual(run.calls[-1][:3], ["btrfs", "subvolume", "delete"]) class TestZfs(unittest.TestCase): @@ -77,13 +85,15 @@ class TestZfs(unittest.TestCase): run = self._run() with volume_snapshot("zfs", "/var/lib/docker", "20260731", run=run): pass - self.assertIn("zfs snapshot tank/docker@baudolo-20260731", run.calls) + self.assertIn(["zfs", "snapshot", "tank/docker@baudolo-20260731"], run.calls) def test_it_destroys_the_snapshot_afterwards(self) -> None: run = self._run() with volume_snapshot("zfs", "/var/lib/docker", "20260731", run=run): pass - self.assertEqual(run.calls[-1], "zfs destroy tank/docker@baudolo-20260731") + self.assertEqual( + run.calls[-1], ["zfs", "destroy", "tank/docker@baudolo-20260731"] + ) def test_it_maps_a_volume_path_through_the_dot_zfs_directory(self) -> None: run = self._run() @@ -131,8 +141,8 @@ class TestRejections(unittest.TestCase): class Busy(Runner): - def __call__(self, command: str) -> list[str]: - if command.startswith("btrfs subvolume delete"): + def __call__(self, command: list[str]) -> list[str]: + if command[:3] == ["btrfs", "subvolume", "delete"]: raise BackupException("target is busy") return super().__call__(command) diff --git a/tests/unit/backup/test_volume.py b/tests/unit/backup/test_volume.py index 80bb7cd..4efe29b 100644 --- a/tests/unit/backup/test_volume.py +++ b/tests/unit/backup/test_volume.py @@ -48,7 +48,7 @@ class TestBackupVolume(unittest.TestCase): def test_it_keeps_no_twin_of_what_the_second_pass_replaces(self) -> None: command = self.copy(authoritative=True) - self.assertIn("rsync -aP ", command) + self.assertEqual(command[:2], ["rsync", "-aP"]) self.assertNotIn("--backup", command) def test_it_creates_the_destination(self) -> None: