From f97efb10c44304bdf66b6fc3ae79ef030a6270a8 Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Tue, 18 Aug 2026 05:11:46 +0200 Subject: [PATCH] fix(backup)!: read the instance from one engine-name set, and trust it Two shapes fell through the inline regex, which knew `database`, `db` and `postgres` only. A container named exactly after its engine - what a compose file writes as `container_name: postgres` - carries no separator before the token, so it resolved to nothing and 6.0.0 stopped dumping it without saying so. And a swarm task of a central MariaDB reads `mariadb_mariadb.1.`, where `_mariadb` was no token at all, so that database has never been dumped under swarm at all. ENGINE_NAMES states the set once and serves both readings: carried as a suffix it makes the rest the instance, being one outright makes the container its own instance. backup_mariadb_or_postgres stops calling an application container a database. container_engine recognises an engine by its client tools, which an application image often ships, so refusing the dump alone would have recorded the volume as `database: true, dumped: false` - the exact shape a restore drill reads as a database that was missed. Without an instance there is no database to record. BREAKING CHANGE: `mariadb` and `mysql` join the suffix tokens, so a container named `-mariadb` resolves to the instance `` rather than to its own name. A databases.csv keyed on the full container name has to move to the application name, or name the container in --database-containers. Co-Authored-By: Claude Opus 5 (1M context) --- src/baudolo/backup/db.py | 23 ++++++++++++------- src/baudolo/backup/dumps.py | 4 +++- ...st_e2e_app_container_ships_client_tools.py | 23 ++++++++++++++++++- tests/unit/backup/test_get_instance.py | 19 ++++++++++++--- 4 files changed, 56 insertions(+), 13 deletions(-) diff --git a/src/baudolo/backup/db.py b/src/baudolo/backup/db.py index eb0829e..1e5027f 100644 --- a/src/baudolo/backup/db.py +++ b/src/baudolo/backup/db.py @@ -16,13 +16,18 @@ if TYPE_CHECKING: log = logging.getLogger(__name__) +ENGINE_NAMES = ("database", "postgres", "mariadb", "mysql", "db") +_SUFFIX_RE = re.compile(rf"(_|-)({'|'.join(ENGINE_NAMES)})") + def get_instance(container: str, database_containers: list[str]) -> str | None: """The databases.csv instance a container serves, or None for no database. - A declared container is its own instance. Every other name is normalised by - stripping a database suffix token, which maps both `-database` from - compose and `_database.1.` from swarm onto the same instance. + A declared container is its own instance. Every other name is read against + ENGINE_NAMES: carrying one as a suffix makes the rest the instance, which + maps `-database` from compose and `_database.1.` from swarm + onto the same one; being one outright makes the container its own instance, + the shape a compose file writes as `container_name: postgres`. Args: container: the running container's name. @@ -30,14 +35,16 @@ def get_instance(container: str, database_containers: list[str]) -> str | None: declared engines whatever they are called. Returns: - The instance name, or None when the name carries no database token: an - application container is not an engine, even when it ships the client - tools that would let a dump command start. + The instance name, or None when the name neither carries nor is an + engine name: an application container is not an engine, even when it + ships the client tools that would let a dump command start. """ if container in database_containers: return container - parts = re.split(r"(_|-)(database|db|postgres)", container) - return parts[0] if len(parts) > 1 else None + parts = _SUFFIX_RE.split(container) + if len(parts) > 1: + return parts[0] + return container if container in ENGINE_NAMES else None def fallback_pg_dumpall( diff --git a/src/baudolo/backup/dumps.py b/src/baudolo/backup/dumps.py index dabe38e..b7d66bb 100644 --- a/src/baudolo/backup/dumps.py +++ b/src/baudolo/backup/dumps.py @@ -10,7 +10,7 @@ from pandas.errors import EmptyDataError from baudolo.databases import COLUMNS, DELIMITER -from .db import backup_database +from .db import backup_database, get_instance from .docker import has_tool, image_id DUMP_TOOLS: tuple[tuple[str, str], ...] = ( @@ -76,6 +76,8 @@ def backup_mariadb_or_postgres( engine = container_engine(container) if engine is None: return VolumeOutcome(database=False, dumped=False) + if get_instance(container, database_containers) is None: + return VolumeOutcome(database=False, dumped=False) db_type, dump_tool = engine dumped = backup_database( container=container, diff --git a/tests/e2e/test_e2e_app_container_ships_client_tools.py b/tests/e2e/test_e2e_app_container_ships_client_tools.py index 2a39799..bdb9f54 100644 --- a/tests/e2e/test_e2e_app_container_ships_client_tools.py +++ b/tests/e2e/test_e2e_app_container_ships_client_tools.py @@ -13,10 +13,11 @@ by its own launcher and ships pg_dumpall, so a dump command starts there and writes a file that looks like a backup and holds none of the data. """ +import json import unittest from pathlib import Path -from baudolo.generation import DUMP_SUFFIX, FILES_DIR, SQL_DIR +from baudolo.generation import DUMP_SUFFIX, FILES_DIR, MANIFEST_FILE, SQL_DIR from .helpers import ( POSTGRES_DATA_DIR, @@ -158,6 +159,26 @@ class TestE2EAppContainerShipsClientTools(unittest.TestCase): self.assertTrue(marker.is_file(), f"expected a file backup at {marker}") self.assertIn(MARKER, marker.read_text(encoding="utf-8")) + def test_the_manifest_does_not_call_the_application_volume_a_database(self) -> None: + manifest = json.loads( + (self.volume_dir(self.app_volume).parent / MANIFEST_FILE).read_text( + encoding="utf-8" + ) + ) + entry = manifest["volumes"][self.app_volume] + self.assertFalse(entry["database"], entry) + self.assertFalse(entry["dumped"], entry) + + def test_the_manifest_records_the_engine_volume_as_dumped(self) -> None: + manifest = json.loads( + (self.volume_dir(self.engine_volume).parent / MANIFEST_FILE).read_text( + encoding="utf-8" + ) + ) + entry = manifest["volumes"][self.engine_volume] + self.assertTrue(entry["database"], entry) + self.assertTrue(entry["dumped"], entry) + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/backup/test_get_instance.py b/tests/unit/backup/test_get_instance.py index 8a99371..08dcfe8 100644 --- a/tests/unit/backup/test_get_instance.py +++ b/tests/unit/backup/test_get_instance.py @@ -24,12 +24,21 @@ class TestDeclaredContainers(unittest.TestCase): get_instance("shop-database", ["shop-database"]), "shop-database" ) - def test_an_undeclared_central_engine_resolves_to_nothing(self) -> None: - """`postgres-central` has no separator before its token, so nothing is - stripped - a central engine has to be declared to be found.""" + def test_a_qualified_central_name_still_has_to_be_declared(self) -> None: self.assertIsNone(get_instance("postgres-central", [])) +class TestContainersNamedAfterTheirEngine(unittest.TestCase): + def test_a_bare_engine_name_is_its_own_instance(self) -> None: + for name in ("postgres", "mariadb", "mysql", "db", "database"): + with self.subTest(container=name): + self.assertEqual(get_instance(name, []), name) + + def test_a_swarm_task_of_such_a_container_keeps_the_instance(self) -> None: + self.assertEqual(get_instance("postgres_postgres.1.k3f9x2", []), "postgres") + self.assertEqual(get_instance("mariadb_mariadb.1.k3f9x2", []), "mariadb") + + class TestDedicatedEngines(unittest.TestCase): def test_compose_names_the_container_with_a_hyphen(self) -> None: self.assertEqual(get_instance("discourse-database", []), "discourse") @@ -49,6 +58,10 @@ class TestDedicatedEngines(unittest.TestCase): def test_mariadb_uses_the_same_suffix(self) -> None: self.assertEqual(get_instance("matomo-database", []), "matomo") + def test_an_engine_named_suffix_is_stripped_too(self) -> None: + self.assertEqual(get_instance("shop-mariadb", []), "shop") + self.assertEqual(get_instance("shop-mysql", []), "shop") + class TestApplicationContainers(unittest.TestCase): def test_a_bare_application_name_is_not_a_database(self) -> None: