From e9030e8443f93ef59c612bb438f0b1b809ba4d0b Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Sat, 11 Jul 2026 08:23:11 +0200 Subject: [PATCH] fix(backup,restore): make restore drills replayable and leave swarm tasks alone Restore fixes, both hit by the infinito svc-bkp e2e drill: - mariadb --empty dropped tables one docker exec at a time with SET FOREIGN_KEY_CHECKS=0 issued in a separate client session, so the session-scoped toggle never applied and any FK-referenced parent table (mailu.users) died with ERROR 1451. Issue the toggle and all DROPs in one session. - postgres --empty now drops only current_user-owned objects (extension members like pg_trgm's set_limit are superuser-owned) with IF EXISTS absorbing CASCADE fallout, and the replay skips superuser-only dump lines (COMMENT ON EXTENSION, ALTER DEFAULT PRIVILEGES) that abort an app-user psql run under ON_ERROR_STOP. Backup fixes: - pg_dump now runs with --no-owner --no-privileges so future dumps are replayable by the owning app user in the first place. - Swarm task containers are never stopped or started manually: the orchestrator replaces a stopped task and a later docker start fails on the detached overlay network. filter_stoppable skips them visibly and the whitelist stop check ignores them. Validated end to end against a live infinito compose stack: the full svc-bkp-volume-2-local drill (verify, restore cycle, sql replay for mailu, keycloak and one more db) passes with these patches applied. Co-Authored-By: Claude Fable 5 --- src/baudolo/backup/app.py | 18 +++++-- src/baudolo/backup/db.py | 3 +- src/baudolo/backup/docker.py | 25 +++++++++ src/baudolo/restore/db/mariadb.py | 37 ++++--------- src/baudolo/restore/db/postgres.py | 53 +++++++++++++++---- .../test_e2e_mariadb_anonymous_preemption.py | 45 +++++++++++----- tests/unit/backup/test_db_mariadb_dump.py | 4 +- 7 files changed, 129 insertions(+), 56 deletions(-) diff --git a/src/baudolo/backup/app.py b/src/baudolo/backup/app.py index 8f2a951..64c125d 100644 --- a/src/baudolo/backup/app.py +++ b/src/baudolo/backup/app.py @@ -16,8 +16,10 @@ from .docker import ( change_containers_status, containers_using_volume, docker_volume_names, + filter_stoppable, get_image_info, has_image, + is_swarm_task, ) from .shell import execute_shell_command from .volume import backup_volume @@ -66,9 +68,13 @@ def volume_is_fully_ignored( def requires_stop(containers: list[str], images_no_stop_required: list[str]) -> bool: """ - Stop is required if ANY container image is NOT in the whitelist patterns. + Stop is required if ANY stoppable container image is NOT in the + whitelist patterns. Swarm task containers never count: baudolo must + not cycle them (see docker.is_swarm_task). """ for c in containers: + if is_swarm_task(c): + continue img = get_image_info(c) if not any(pat in img for pat in images_no_stop_required): return True @@ -219,20 +225,22 @@ def main() -> int: if args.everything: # "everything": always do pre-rsync, then stop + rsync again + stoppable = filter_stoppable(containers) backup_volume(versions_dir, volume_name, vol_dir) - change_containers_status(containers, "stop") + change_containers_status(stoppable, "stop") backup_volume(versions_dir, volume_name, vol_dir) if not args.shutdown: - change_containers_status(containers, "start") + change_containers_status(stoppable, "start") continue # default: rsync, and if needed stop + rsync backup_volume(versions_dir, volume_name, vol_dir) if requires_stop(containers, args.images_no_stop_required): - change_containers_status(containers, "stop") + stoppable = filter_stoppable(containers) + change_containers_status(stoppable, "stop") backup_volume(versions_dir, volume_name, vol_dir) if not args.shutdown: - change_containers_status(containers, "start") + change_containers_status(stoppable, "start") # Stamp the backup version directory using dirval (python lib) stamp_directory(version_dir) diff --git a/src/baudolo/backup/db.py b/src/baudolo/backup/db.py index 3b732d2..732c308 100644 --- a/src/baudolo/backup/db.py +++ b/src/baudolo/backup/db.py @@ -129,7 +129,8 @@ def backup_database( try: cmd = ( f"PGPASSWORD={password} docker exec -i {container} " - f"pg_dump -U {user} -d {db_name} -h localhost" + f"pg_dump -U {user} -d {db_name} -h localhost " + f"--no-owner --no-privileges" ) _atomic_write_cmd(cmd, dump_file) produced = True diff --git a/src/baudolo/backup/docker.py b/src/baudolo/backup/docker.py index 23320fc..3997152 100644 --- a/src/baudolo/backup/docker.py +++ b/src/baudolo/backup/docker.py @@ -24,6 +24,31 @@ def containers_using_volume(volume_name: str) -> list[str]: ) +def is_swarm_task(container: str) -> bool: + """Swarm-managed task containers must never be stopped or started + manually: the orchestrator replaces the stopped task and a later + `docker start` fails on the detached overlay network.""" + out = execute_shell_command( + "docker inspect --format " + f"'{{{{index .Config.Labels \"com.docker.swarm.task.id\"}}}}' {container}" + ) + return bool(out and out[0].strip()) + + +def filter_stoppable(containers: list[str]) -> list[str]: + """Containers baudolo may stop/start itself (everything but swarm tasks).""" + stoppable = [] + for container in containers: + if is_swarm_task(container): + print( + f"Skipping stop/start for swarm task container '{container}'.", + flush=True, + ) + continue + stoppable.append(container) + return stoppable + + def change_containers_status(containers: list[str], status: str) -> None: """Stop or start a list of containers.""" if not containers: diff --git a/src/baudolo/restore/db/mariadb.py b/src/baudolo/restore/db/mariadb.py index 61acfde..5195069 100644 --- a/src/baudolo/restore/db/mariadb.py +++ b/src/baudolo/restore/db/mariadb.py @@ -47,18 +47,6 @@ def restore_mariadb_sql( # IMPORTANT: # Do NOT hardcode 'mysql' here. Use the detected client. # MariaDB 11 images may not contain the mysql binary at all. - docker_exec( - container, - [ - client, - "-u", - user, - f"--password={password}", - "-e", - "SET FOREIGN_KEY_CHECKS=0;", - ], - ) - result = docker_exec( container, [ @@ -74,7 +62,16 @@ def restore_mariadb_sql( ) tables = result.stdout.decode().split() - for tbl in tables: + if tables: + # SET FOREIGN_KEY_CHECKS is session-scoped, so it must share one + # client session with the DROPs or FK constraints still fire. + drop_sql = ( + "SET FOREIGN_KEY_CHECKS=0; " + + " ".join( + f"DROP TABLE IF EXISTS `{db_name}`.`{tbl}`;" for tbl in tables + ) + + " SET FOREIGN_KEY_CHECKS=1;" + ) docker_exec( container, [ @@ -83,22 +80,10 @@ def restore_mariadb_sql( user, f"--password={password}", "-e", - f"DROP TABLE IF EXISTS `{db_name}`.`{tbl}`;", + drop_sql, ], ) - docker_exec( - container, - [ - client, - "-u", - user, - f"--password={password}", - "-e", - "SET FOREIGN_KEY_CHECKS=1;", - ], - ) - with open(sql_path, "rb") as f: docker_exec( container, [client, "-u", user, f"--password={password}", db_name], stdin=f diff --git a/src/baudolo/restore/db/postgres.py b/src/baudolo/restore/db/postgres.py index 8b002d3..4132045 100644 --- a/src/baudolo/restore/db/postgres.py +++ b/src/baudolo/restore/db/postgres.py @@ -21,17 +21,43 @@ def restore_postgres_sql( docker_env = {"PGPASSWORD": password} if empty: + # Owner-filtered: extension members (pg_trgm's set_limit) are superuser-owned; IF EXISTS absorbs CASCADE fallout. drop_sql = r""" DO $$ DECLARE r RECORD; BEGIN FOR r IN ( - SELECT table_name AS name, 'TABLE' AS type FROM information_schema.tables WHERE table_schema='public' + SELECT c.relname AS name, + CASE c.relkind + WHEN 'v' THEN 'VIEW' + WHEN 'm' THEN 'MATERIALIZED VIEW' + WHEN 'f' THEN 'FOREIGN TABLE' + ELSE 'TABLE' + END AS type + FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = 'public' AND c.relkind IN ('r', 'p', 'v', 'm', 'f') + AND pg_get_userbyid(c.relowner) = current_user UNION ALL - SELECT routine_name AS name, 'FUNCTION' AS type FROM information_schema.routines WHERE specific_schema='public' + SELECT p.proname AS name, + CASE p.prokind WHEN 'p' THEN 'PROCEDURE' ELSE 'FUNCTION' END AS type + FROM pg_proc p JOIN pg_namespace n ON n.oid = p.pronamespace + WHERE n.nspname = 'public' AND p.prokind IN ('f', 'p', 'w') + AND pg_get_userbyid(p.proowner) = current_user UNION ALL - SELECT sequence_name AS name, 'SEQUENCE' AS type FROM information_schema.sequences WHERE sequence_schema='public' + SELECT c.relname AS name, 'SEQUENCE' AS type + FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = 'public' AND c.relkind = 'S' + AND pg_get_userbyid(c.relowner) = current_user + UNION ALL + SELECT t.typname AS name, 'TYPE' AS type + FROM pg_type t JOIN pg_namespace n ON n.oid = t.typnamespace + WHERE n.nspname = 'public' + AND pg_get_userbyid(t.typowner) = current_user + AND (t.typtype IN ('e', 'd') + OR (t.typtype = 'c' AND EXISTS ( + SELECT 1 FROM pg_class c2 + WHERE c2.oid = t.typrelid AND c2.relkind = 'c'))) ) LOOP - EXECUTE format('DROP %s public.%I CASCADE', r.type, r.name); + EXECUTE format('DROP %s IF EXISTS public.%I CASCADE', r.type, r.name); END LOOP; END $$; """ @@ -43,11 +69,18 @@ END $$; ) with open(sql_path, "rb") as f: - docker_exec( - container, - ["psql", "-v", "ON_ERROR_STOP=1", "-U", user, "-d", db_name], - stdin=f, - docker_env=docker_env, - ) + raw_sql = f.read() + # COMMENT ON EXTENSION and ALTER DEFAULT PRIVILEGES are superuser-only; + # app-level restores must skip them or ON_ERROR_STOP aborts the replay. + superuser_only = (b"COMMENT ON EXTENSION", b"ALTER DEFAULT PRIVILEGES") + sql = b"\n".join( + line for line in raw_sql.splitlines() if not line.startswith(superuser_only) + ) + docker_exec( + container, + ["psql", "-v", "ON_ERROR_STOP=1", "-U", user, "-d", db_name], + stdin=sql, + docker_env=docker_env, + ) print(f"PostgreSQL restore complete for db '{db_name}'.") diff --git a/tests/e2e/test_e2e_mariadb_anonymous_preemption.py b/tests/e2e/test_e2e_mariadb_anonymous_preemption.py index d22451c..c16f745 100644 --- a/tests/e2e/test_e2e_mariadb_anonymous_preemption.py +++ b/tests/e2e/test_e2e_mariadb_anonymous_preemption.py @@ -59,15 +59,24 @@ class TestE2EMariaDBAnonymousPreemption(unittest.TestCase): # Boot WITHOUT MARIADB_USER/MARIADB_PASSWORD/MARIADB_DATABASE so the # entrypoint does not auto-create ''@'%'. We provision the user # explicitly below to mirror the SQL path used by svc-db-mariadb. - run([ - "docker", "run", "-d", - "--name", cls.db_container, - "-e", f"MARIADB_ROOT_PASSWORD={cls.root_password}", - "-v", f"{cls.db_volume}:/var/lib/mysql", - "mariadb:12.2", - ]) + run( + [ + "docker", + "run", + "-d", + "--name", + cls.db_container, + "-e", + f"MARIADB_ROOT_PASSWORD={cls.root_password}", + "-v", + f"{cls.db_volume}:/var/lib/mysql", + "mariadb:12.2", + ] + ) - wait_for_mariadb(cls.db_container, root_password=cls.root_password, timeout_s=120) + wait_for_mariadb( + cls.db_container, root_password=cls.root_password, timeout_s=120 + ) # Provision: ''@'%' (the app/backup grant) + anonymous ''@'localhost' # (the preemption trigger). Mirrors the production state that produced @@ -81,10 +90,16 @@ class TestE2EMariaDBAnonymousPreemption(unittest.TestCase): f"CREATE TABLE {cls.db_name}.t (id INT PRIMARY KEY, v VARCHAR(50));" f"INSERT INTO {cls.db_name}.t VALUES (1,'ok');" ) - run([ - "docker", "exec", cls.db_container, "sh", "-lc", - f'mariadb -uroot --protocol=socket -e "{bootstrap_sql}"', - ]) + run( + [ + "docker", + "exec", + cls.db_container, + "sh", + "-lc", + f'mariadb -uroot --protocol=socket -e "{bootstrap_sql}"', + ] + ) # Sanity: '' can log in over TCP (matches '%'). If THIS fails, # the precondition for the fix to even apply is broken. @@ -104,7 +119,11 @@ class TestE2EMariaDBAnonymousPreemption(unittest.TestCase): # ability to discriminate "fix works" vs "bug never reproduced". p = run( [ - "docker", "exec", self.db_container, "sh", "-lc", + "docker", + "exec", + self.db_container, + "sh", + "-lc", f"mariadb-dump -u{self.db_user} -p{self.db_password} {self.db_name}", ], capture=True, diff --git a/tests/unit/backup/test_db_mariadb_dump.py b/tests/unit/backup/test_db_mariadb_dump.py index 2a9667b..8296be6 100644 --- a/tests/unit/backup/test_db_mariadb_dump.py +++ b/tests/unit/backup/test_db_mariadb_dump.py @@ -44,7 +44,9 @@ class TestMariaDBDumpUsesTCP(unittest.TestCase): 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(dump_cmds), 1, f"expected one dump command, got: {captured}" + ) cmd = dump_cmds[0] self.assertIn("-h 127.0.0.1", cmd)