From f791046c02987d61119d6fbd32a7b92f046e8d5f Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Mon, 17 Aug 2026 13:51:26 +0200 Subject: [PATCH] fix(backup): detect the engine by its dump tool, not by the image name 36b2336 matched postgres and mariadb against the image's repository path. That name is not a property of the software: a dedicated Postgres inside an app's own stack is tagged -database or postgis/postgis and carries no engine token at all, so it was never recognised, never dumped, and its data directory was copied as files without a single warning - the fallback notice hangs on found_db, which stayed false. The container is now asked what it can run. pg_dumpall, mariadb-dump and mysqldump are probed by executing them, not by asking a shell for them, because a distroless image has no shell and would deny every tool it ships. The verdict is cached per image ID rather than per container, so replicas of one image cost a single probe. Because the probe names the tool it found, the dump uses it instead of the hardcoded /usr/bin/mariadb-dump, which makes an image that ships only mysqldump dumpable rather than silently file-copied. image_name and has_image are gone with their registry-host and tag stripping; the trap they worked around cannot occur when nothing reads the name. get_image_info stays, since the --images-* lists match exact references. Co-Authored-By: Claude Opus 5 (1M context) --- src/baudolo/backup/db.py | 7 +- src/baudolo/backup/docker.py | 38 ++-- src/baudolo/backup/dumps.py | 64 +++++-- .../e2e/test_e2e_engine_detection_by_tool.py | 177 ++++++++++++++++++ .../test_e2e_mariadb_anonymous_preemption.py | 1 + tests/unit/backup/test_db_mariadb_dump.py | 16 +- tests/unit/backup/test_docker_image_name.py | 50 ----- tests/unit/backup/test_docker_tool_probe.py | 42 +++++ .../backup/test_dumps_engine_detection.py | 138 ++++++++++++++ 9 files changed, 448 insertions(+), 85 deletions(-) create mode 100644 tests/e2e/test_e2e_engine_detection_by_tool.py delete mode 100644 tests/unit/backup/test_docker_image_name.py create mode 100644 tests/unit/backup/test_docker_tool_probe.py create mode 100644 tests/unit/backup/test_dumps_engine_detection.py diff --git a/src/baudolo/backup/db.py b/src/baudolo/backup/db.py index 862aec9..d6305d9 100644 --- a/src/baudolo/backup/db.py +++ b/src/baudolo/backup/db.py @@ -69,12 +69,17 @@ def backup_database( container: str, volume_dir: str, db_type: str, + dump_tool: str, databases_df: pandas.DataFrame, database_containers: list[str], ) -> bool: """ Backup databases for a given DB container. + Args: + dump_tool: the MariaDB client found in the container, so an image + that ships only mysqldump is dumped with the tool it has. + Returns True if at least one dump was produced. """ instance_name = get_instance(container, database_containers) @@ -114,7 +119,7 @@ def backup_database( if db_type == "mariadb": # Force TCP so auth matches ''@'%' instead of socket -> 'localhost'. cmd = ( - f"docker exec {container} /usr/bin/mariadb-dump " + f"docker exec {container} {dump_tool} " f"-h 127.0.0.1 --protocol=tcp " f"-u {user} -p{password} {db_name}" ) diff --git a/src/baudolo/backup/docker.py b/src/baudolo/backup/docker.py index c93a1a5..5b7cce5 100644 --- a/src/baudolo/backup/docker.py +++ b/src/baudolo/backup/docker.py @@ -9,29 +9,25 @@ def get_image_info(container: str) -> str: )[0] -def image_name(container: str) -> str: - """The image's repository path, without registry host, tag or digest. +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}" + )[0].strip() - A swarm node that hosts the local registry puts its own hostname in front - of every pull, so the raw reference of a Postgres container can read - `svc-db-mariadb-swarm-mgr-01:5000/postgres_custom:17-3.5`. Matching the - whole reference finds "mariadb" there and dumps the database with - mariadb-dump, which the Postgres image does not ship (exit 127). Tags bite - the same way: `xwiki_custom:lts-postgres-tomcat`. + +def has_tool(container: str, tool: str) -> bool: + """Whether *tool* runs inside the container. + + Executes the binary rather than asking a shell for it: a distroless image + has no shell, and `sh -c 'command -v'` would answer "absent" for every + tool it ships. """ - reference = get_image_info(container).strip().split("@", 1)[0] - head, _, tail = reference.rpartition("/") - tail = tail.split(":", 1)[0] - if head: - registry = head.split("/", 1)[0] - if "." in registry or ":" in registry or registry == "localhost": - head = head.partition("/")[2] - return f"{head}/{tail}" if head else tail - - -def has_image(container: str, pattern: str) -> bool: - """Return True if the container's image name contains the pattern.""" - return pattern in image_name(container) + try: + execute_shell_command(f"docker exec {container} {tool} --version") + except BackupException: + return False + return True def docker_volume_names() -> list[str]: diff --git a/src/baudolo/backup/dumps.py b/src/baudolo/backup/dumps.py index fe6be3e..e373425 100644 --- a/src/baudolo/backup/dumps.py +++ b/src/baudolo/backup/dumps.py @@ -8,7 +8,45 @@ import pandas from pandas.errors import EmptyDataError from .db import backup_database -from .docker import has_image +from .docker import has_tool, image_id + +DUMP_TOOLS: tuple[tuple[str, str], ...] = ( + ("postgres", "pg_dumpall"), + ("mariadb", "mariadb-dump"), + ("mariadb", "mysqldump"), +) + +_ENGINE_BY_IMAGE: dict[str, tuple[str, str] | None] = {} + + +def container_engine(container: str) -> tuple[str, str] | None: + """The (engine, dump tool) a container can serve, or None for neither. + + Asks the container what it can run instead of reading its image name. A + dedicated Postgres is tagged `-database` or `postgis/postgis` and + carries no engine token at all, while a swarm registry host such as + `svc-db-mariadb-swarm-mgr-01:5000` carries the wrong one. + + Args: + container: must be running - `docker exec` is the probe, and a + stopped container would be cached as "no engine" for its whole + image. The only caller feeds it `docker ps` output. + + Returns: + The engine and the tool that dumps it, cached per image ID so that + replicas of one image are probed once. + """ + image = image_id(container) + if image not in _ENGINE_BY_IMAGE: + _ENGINE_BY_IMAGE[image] = next( + ( + (engine, tool) + for engine, tool in DUMP_TOOLS + if has_tool(container, tool) + ), + None, + ) + return _ENGINE_BY_IMAGE[image] def backup_mariadb_or_postgres( @@ -21,17 +59,19 @@ def backup_mariadb_or_postgres( """ Returns (is_db_container, dumped_any) """ - for img in ["mariadb", "postgres"]: - if has_image(container, img): - dumped = backup_database( - container=container, - volume_dir=volume_dir, - db_type=img, - databases_df=databases_df, - database_containers=database_containers, - ) - return True, dumped - return False, False + engine = container_engine(container) + if engine is None: + return False, False + db_type, dump_tool = engine + dumped = backup_database( + container=container, + volume_dir=volume_dir, + db_type=db_type, + dump_tool=dump_tool, + databases_df=databases_df, + database_containers=database_containers, + ) + return True, dumped def _empty_databases_df() -> pandas.DataFrame: diff --git a/tests/e2e/test_e2e_engine_detection_by_tool.py b/tests/e2e/test_e2e_engine_detection_by_tool.py new file mode 100644 index 0000000..cf00830 --- /dev/null +++ b/tests/e2e/test_e2e_engine_detection_by_tool.py @@ -0,0 +1,177 @@ +"""The engine comes from the tools a container ships, not from its image name. + +Two containers in one backup run, each lying in one direction: + +* a real Postgres tagged `-database`, the way a dedicated database is + built inside an app's own stack - no engine token anywhere in the name; +* an Alpine tagged `postgres:`, carrying the token without shipping a + single Postgres binary. + +Reading the name gets both wrong, and the second one fatally: pg_dump exits 127 +inside Alpine and takes the whole run with it. +""" + +import unittest + +from .helpers import ( + POSTGRES_DATA_DIR, + POSTGRES_IMAGE, + backup_path, + backup_run, + cleanup_docker, + create_minimal_compose_dir, + ensure_empty_dir, + latest_version_dir, + require_docker, + run, + unique, + wait_for_postgres, + write_databases_csv, +) + +IMPOSTOR_BASE_IMAGE = "alpine:3.20" +MARKER = "engine-detection-by-tool" + + +class TestE2EEngineDetectionByTool(unittest.TestCase): + @classmethod + def setUpClass(cls) -> None: + require_docker() + cls.prefix = unique("baudolo-e2e-engine-by-tool") + cls.backups_dir = f"/tmp/{cls.prefix}/Backups" + ensure_empty_dir(cls.backups_dir) + cls.compose_dir = create_minimal_compose_dir(f"/tmp/{cls.prefix}") + cls.repo_name = cls.prefix + + cls.engine_image = f"{cls.prefix}-database:17" + cls.impostor_image = f"postgres:{cls.prefix}" + + cls.engine_container = f"{cls.prefix}-engine" + cls.impostor_container = f"{cls.prefix}-impostor" + cls.engine_volume = f"{cls.prefix}-engine-vol" + cls.impostor_volume = f"{cls.prefix}-impostor-vol" + + cls.containers = [cls.engine_container, cls.impostor_container] + cls.volumes = [cls.engine_volume, cls.impostor_volume] + + run(["docker", "pull", POSTGRES_IMAGE]) + run(["docker", "pull", IMPOSTOR_BASE_IMAGE]) + run(["docker", "tag", POSTGRES_IMAGE, cls.engine_image]) + run(["docker", "tag", IMPOSTOR_BASE_IMAGE, cls.impostor_image]) + run(["docker", "volume", "create", cls.engine_volume]) + run(["docker", "volume", "create", cls.impostor_volume]) + + run( + [ + "docker", + "run", + "-d", + "--name", + cls.engine_container, + "-e", + "POSTGRES_PASSWORD=pgpw", + "-e", + "POSTGRES_DB=appdb", + "-e", + "POSTGRES_USER=postgres", + "-v", + f"{cls.engine_volume}:{POSTGRES_DATA_DIR}", + cls.engine_image, + ] + ) + wait_for_postgres(cls.engine_container, user="postgres", timeout_s=90) + run( + [ + "docker", + "exec", + cls.engine_container, + "sh", + "-lc", + ( + "psql -U postgres -d appdb -c " + '"CREATE TABLE t (id int primary key, v text); ' + "INSERT INTO t VALUES (1,'ok');\"" + ), + ] + ) + + run( + [ + "docker", + "run", + "-d", + "--name", + cls.impostor_container, + "-v", + f"{cls.impostor_volume}:/data", + cls.impostor_image, + "sh", + "-lc", + f"echo '{MARKER}' > /data/marker.txt && sleep 3600", + ] + ) + + cls.databases_csv = f"/tmp/{cls.prefix}/databases.csv" + write_databases_csv( + cls.databases_csv, + [ + (cls.engine_container, "appdb", "postgres", "pgpw"), + (cls.impostor_container, "appdb", "postgres", "pgpw"), + ], + ) + + backup_run( + backups_dir=cls.backups_dir, + repo_name=cls.repo_name, + compose_dir=cls.compose_dir, + databases_csv=cls.databases_csv, + database_containers=[cls.engine_container, cls.impostor_container], + images_no_stop_required=[cls.engine_image, cls.impostor_image], + only_sql=True, + ) + + cls.hash, cls.version = latest_version_dir(cls.backups_dir, cls.repo_name) + + @classmethod + def tearDownClass(cls) -> None: + cleanup_docker(containers=cls.containers, volumes=cls.volumes) + run(["docker", "rmi", cls.engine_image], check=False) + run(["docker", "rmi", cls.impostor_image], check=False) + + def _volume_dir(self, volume: str): + return backup_path(self.backups_dir, self.repo_name, self.version, volume) + + def test_an_engine_without_an_engine_name_is_still_dumped(self) -> None: + dump = self._volume_dir(self.engine_volume) / "sql" / "appdb.backup.sql" + self.assertTrue( + dump.is_file(), + f"a Postgres tagged '{self.engine_image}' produced no dump at {dump}", + ) + self.assertIn("Dumped by pg_dump", dump.read_text(encoding="utf-8")) + + def test_an_engine_name_without_an_engine_is_not_dumped(self) -> None: + sql_dir = self._volume_dir(self.impostor_volume) / "sql" + dumps = list(sql_dir.glob("*.sql")) if sql_dir.exists() else [] + self.assertEqual( + dumps, + [], + f"'{self.impostor_image}' ships no Postgres yet was dumped: {dumps}", + ) + + def test_the_recognised_engine_is_dumped_instead_of_copied(self) -> None: + files = self._volume_dir(self.engine_volume) / "files" + self.assertFalse( + files.exists(), + f"--only-sql still copied the engine's files to {files}", + ) + + def test_the_impostor_falls_through_to_a_file_backup(self) -> None: + files = self._volume_dir(self.impostor_volume) / "files" + self.assertTrue(files.is_dir(), f"expected a file backup at {files}") + self.assertEqual( + (files / "marker.txt").read_text(encoding="utf-8").strip(), MARKER + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/e2e/test_e2e_mariadb_anonymous_preemption.py b/tests/e2e/test_e2e_mariadb_anonymous_preemption.py index c0ba244..75641d9 100644 --- a/tests/e2e/test_e2e_mariadb_anonymous_preemption.py +++ b/tests/e2e/test_e2e_mariadb_anonymous_preemption.py @@ -148,6 +148,7 @@ class TestE2EMariaDBAnonymousPreemption(unittest.TestCase): container=self.db_container, volume_dir=volume_dir, db_type="mariadb", + dump_tool="mariadb-dump", databases_df=df, database_containers=[self.db_container], ) diff --git a/tests/unit/backup/test_db_mariadb_dump.py b/tests/unit/backup/test_db_mariadb_dump.py index c123244..a6709d4 100644 --- a/tests/unit/backup/test_db_mariadb_dump.py +++ b/tests/unit/backup/test_db_mariadb_dump.py @@ -13,7 +13,7 @@ def _df(rows): ) -def _capture_commands(*, db_type, rows, container): +def _capture_commands(*, db_type, rows, container, dump_tool="mariadb-dump"): captured = [] def _capture(cmd): @@ -28,6 +28,7 @@ def _capture_commands(*, db_type, rows, container): container=container, volume_dir=td, db_type=db_type, + dump_tool=dump_tool, databases_df=_df(rows), database_containers=[container], ) @@ -57,6 +58,19 @@ class TestMariaDBDumpUsesTCP(unittest.TestCase): self.assertIn("-ps3cret", cmd) self.assertIn(" appdb", cmd) + def test_the_probed_client_is_the_one_invoked(self): + captured = _capture_commands( + 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]) + def test_postgres_dump_unaffected(self): captured = _capture_commands( db_type="postgres", diff --git a/tests/unit/backup/test_docker_image_name.py b/tests/unit/backup/test_docker_image_name.py deleted file mode 100644 index b857f30..0000000 --- a/tests/unit/backup/test_docker_image_name.py +++ /dev/null @@ -1,50 +0,0 @@ -import unittest -from unittest.mock import patch - -from baudolo.backup import docker as docker_mod - - -def _with_image(reference: str): - return patch.object(docker_mod, "execute_shell_command", return_value=[reference]) - - -class TestImageName(unittest.TestCase): - def test_plain_reference(self) -> None: - with _with_image("postgres:16"): - self.assertEqual(docker_mod.image_name("c1"), "postgres") - - def test_registry_host_is_dropped(self) -> None: - with _with_image("svc-db-mariadb-swarm-mgr-01:5000/postgres_custom:17-3.5"): - self.assertEqual(docker_mod.image_name("c1"), "postgres_custom") - - def test_pull_through_path_is_kept(self) -> None: - with _with_image( - "svc-db-mariadb-swarm-mgr-01:5000/ghcr.io/x/mirror/docker.io/postgres:16" - ): - self.assertEqual( - docker_mod.image_name("c1"), "ghcr.io/x/mirror/docker.io/postgres" - ) - - def test_digest_is_dropped(self) -> None: - with _with_image("registry:5000/postgres@sha256:" + "0" * 64): - self.assertEqual(docker_mod.image_name("c1"), "postgres") - - -class TestHasImage(unittest.TestCase): - def test_registry_hostname_does_not_decide_the_engine(self) -> None: - with _with_image("svc-db-mariadb-swarm-mgr-01:5000/postgres_custom:17-3.5"): - self.assertFalse(docker_mod.has_image("c1", "mariadb")) - with _with_image("svc-db-mariadb-swarm-mgr-01:5000/postgres_custom:17-3.5"): - self.assertTrue(docker_mod.has_image("c1", "postgres")) - - def test_tag_does_not_decide_the_engine(self) -> None: - with _with_image("registry:5000/xwiki_custom:lts-postgres-tomcat"): - self.assertFalse(docker_mod.has_image("c1", "postgres")) - - def test_mirrored_mariadb_still_matches(self) -> None: - with _with_image("registry:5000/ghcr.io/x/mirror/docker.io/mariadb:11"): - self.assertTrue(docker_mod.has_image("c1", "mariadb")) - - -if __name__ == "__main__": - unittest.main() diff --git a/tests/unit/backup/test_docker_tool_probe.py b/tests/unit/backup/test_docker_tool_probe.py new file mode 100644 index 0000000..b4f2dfb --- /dev/null +++ b/tests/unit/backup/test_docker_tool_probe.py @@ -0,0 +1,42 @@ +import unittest +from unittest.mock import patch + +from baudolo.backup import docker as docker_mod +from baudolo.backup.shell import BackupException + + +class TestImageId(unittest.TestCase): + def test_the_id_is_returned_without_surrounding_whitespace(self) -> None: + with patch.object( + docker_mod, "execute_shell_command", return_value=["sha256:abc \n"] + ): + self.assertEqual(docker_mod.image_id("c1"), "sha256:abc") + + +class TestHasTool(unittest.TestCase): + def test_a_tool_that_runs_is_present(self) -> None: + with patch.object(docker_mod, "execute_shell_command", return_value=[]): + self.assertTrue(docker_mod.has_tool("c1", "pg_dumpall")) + + def test_a_tool_that_exits_non_zero_is_absent(self) -> None: + with patch.object( + docker_mod, "execute_shell_command", side_effect=BackupException("127") + ): + self.assertFalse(docker_mod.has_tool("c1", "mariadb-dump")) + + def test_the_probe_needs_no_shell_in_the_image(self) -> None: + """A distroless database ships no shell; `sh -c` would deny every tool.""" + captured = [] + + def _capture(cmd): + captured.append(cmd) + return [] + + 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"]) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/unit/backup/test_dumps_engine_detection.py b/tests/unit/backup/test_dumps_engine_detection.py new file mode 100644 index 0000000..1e479ff --- /dev/null +++ b/tests/unit/backup/test_dumps_engine_detection.py @@ -0,0 +1,138 @@ +import unittest +from unittest.mock import patch + +import pandas + +from baudolo.backup import dumps as dumps_mod + + +def _df(rows): + return pandas.DataFrame( + rows, columns=["instance", "database", "username", "password"] + ) + + +class _Probe: + def __init__(self, available, image="sha256:aaa"): + self.available = set(available) + self.image = image + self.calls = [] + + def has_tool(self, container, tool): + self.calls.append((container, tool)) + return tool in self.available + + def image_id(self, container): + return self.image if isinstance(self.image, str) else self.image[container] + + +def _detect(probe, container="c1"): + dumps_mod._ENGINE_BY_IMAGE.clear() + with ( + patch.object(dumps_mod, "has_tool", probe.has_tool), + patch.object(dumps_mod, "image_id", probe.image_id), + ): + return dumps_mod.container_engine(container) + + +class TestContainerEngine(unittest.TestCase): + def test_a_postgres_is_found_by_its_dump_tool(self): + self.assertEqual(_detect(_Probe(["pg_dumpall"])), ("postgres", "pg_dumpall")) + + def test_a_mariadb_is_found_by_its_dump_tool(self): + self.assertEqual(_detect(_Probe(["mariadb-dump"])), ("mariadb", "mariadb-dump")) + + def test_an_image_with_only_mysqldump_is_dumped_with_mysqldump(self): + self.assertEqual(_detect(_Probe(["mysqldump"])), ("mariadb", "mysqldump")) + + def test_a_container_without_either_tool_is_no_database(self): + self.assertIsNone(_detect(_Probe([]))) + + def test_the_probe_stops_at_the_first_tool_it_finds(self): + probe = _Probe(["pg_dumpall", "mariadb-dump"]) + _detect(probe) + self.assertEqual(probe.calls, [("c1", "pg_dumpall")]) + + def test_the_image_name_does_not_decide_the_engine(self): + """The trap the old substring test fell into, from both directions.""" + probe = _Probe(["pg_dumpall"], image="svc-db-mariadb-mgr-01:5000/pg_custom") + self.assertEqual(_detect(probe), ("postgres", "pg_dumpall")) + + probe = _Probe(["mariadb-dump"], image="discourse-database:17") + self.assertEqual(_detect(probe), ("mariadb", "mariadb-dump")) + + +class TestProbeCache(unittest.TestCase): + def test_replicas_of_one_image_are_probed_once(self): + probe = _Probe(["pg_dumpall"]) + dumps_mod._ENGINE_BY_IMAGE.clear() + with ( + patch.object(dumps_mod, "has_tool", probe.has_tool), + patch.object(dumps_mod, "image_id", probe.image_id), + ): + first = dumps_mod.container_engine("replica-1") + second = dumps_mod.container_engine("replica-2") + self.assertEqual(first, second) + self.assertEqual(len(probe.calls), 1) + + def test_a_second_image_is_probed_separately(self): + probe = _Probe(["pg_dumpall"], image={"pg": "sha256:aaa", "app": "sha256:bbb"}) + dumps_mod._ENGINE_BY_IMAGE.clear() + with ( + patch.object(dumps_mod, "has_tool", probe.has_tool), + patch.object(dumps_mod, "image_id", probe.image_id), + ): + self.assertEqual( + dumps_mod.container_engine("pg"), ("postgres", "pg_dumpall") + ) + probe.available = set() + self.assertIsNone(dumps_mod.container_engine("app")) + + +class TestBackupDispatch(unittest.TestCase): + def test_the_probed_tool_reaches_the_dump(self): + probe = _Probe(["mysqldump"]) + seen = {} + + def _fake_backup_database(**kwargs): + seen.update(kwargs) + return True + + dumps_mod._ENGINE_BY_IMAGE.clear() + with ( + patch.object(dumps_mod, "has_tool", probe.has_tool), + patch.object(dumps_mod, "image_id", probe.image_id), + patch.object(dumps_mod, "backup_database", _fake_backup_database), + ): + is_db, dumped = dumps_mod.backup_mariadb_or_postgres( + container="c1", + volume_dir="/tmp", + databases_df=_df([("c1", "appdb", "u", "p")]), + database_containers=["c1"], + ) + + self.assertTrue(is_db) + self.assertTrue(dumped) + self.assertEqual(seen["db_type"], "mariadb") + self.assertEqual(seen["dump_tool"], "mysqldump") + + def test_a_non_database_container_is_left_to_the_file_backup(self): + probe = _Probe([]) + dumps_mod._ENGINE_BY_IMAGE.clear() + with ( + patch.object(dumps_mod, "has_tool", probe.has_tool), + patch.object(dumps_mod, "image_id", probe.image_id), + ): + self.assertEqual( + dumps_mod.backup_mariadb_or_postgres( + container="c1", + volume_dir="/tmp", + databases_df=_df([]), + database_containers=[], + ), + (False, False), + ) + + +if __name__ == "__main__": + unittest.main()