diff --git a/docs/content/Deploying/Docker-Deploying.mdx b/docs/content/Deploying/Docker-Deploying.mdx index 847c7453..ac835442 100644 --- a/docs/content/Deploying/Docker-Deploying.mdx +++ b/docs/content/Deploying/Docker-Deploying.mdx @@ -89,6 +89,12 @@ docsgpt backup # into /backups docsgpt backup --out /mnt/backups # somewhere else, e.g. a mounted disk ``` +While the archive is made, the backend and the worker stop and start again, so +the database dump and the files in the volumes describe the same moment; Postgres +itself keeps running. On a small install that pause is seconds, but count on it +if you run `docsgpt backup` from cron. The archive is written readable only by +the user who took it. + The archive does **not** include `.env`, because that file holds the install's secrets. `docsgpt backup --with-settings` puts it in, for when the archive itself is stored somewhere private. Keep `.env` safe separately otherwise: the diff --git a/docs/content/changelog.mdx b/docs/content/changelog.mdx index 2c5f9501..056fb101 100644 --- a/docs/content/changelog.mdx +++ b/docs/content/changelog.mdx @@ -18,7 +18,8 @@ request, and [Upgrading](/upgrading) covers the steps an existing deployment has `docsgpt backup` writes a dump of the database and a tar of each data volume into one archive, and `docsgpt restore ` puts them back. The settings file is left out unless `--with-settings` asks for it, since it holds the install's secrets, and a backup from a newer -DocsGPT is refused unless you pass `--force`. See +DocsGPT is refused unless you pass `--force`. The archive is written readable only by its owner, +and the backend and the worker pause while it is made so the dump and the volume tars match. See [Backups](/Deploying/Docker-Deploying#backups). ## 0.21.0 diff --git a/docsgpt/deploy/backup.py b/docsgpt/deploy/backup.py index 5b041c23..eb9dbfac 100644 --- a/docsgpt/deploy/backup.py +++ b/docsgpt/deploy/backup.py @@ -13,6 +13,7 @@ from __future__ import annotations import io import json +import os import tarfile import time from collections.abc import Mapping @@ -50,7 +51,10 @@ def write_archive( ) -> None: """Write the backup archive; the manifest is added last so a truncated file has none.""" path.parent.mkdir(parents=True, exist_ok=True) - with tarfile.open(path, "w:gz") as archive: + # 0600 from the start: the dump is the install's data, and --with-settings adds its secrets. + descriptor = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + os.fchmod(descriptor, 0o600) + with os.fdopen(descriptor, "wb") as handle, tarfile.open(fileobj=handle, mode="w:gz") as archive: archive.add(dump, arcname=DUMP) for name, tar_path in sorted(volume_tars.items()): archive.add(tar_path, arcname=volume_member(name)) @@ -81,6 +85,34 @@ def read_manifest(path: Path) -> dict: return manifest +def validate(path: Path, manifest: Mapping[str, object]) -> list[str]: + """The volumes to restore, once the archive is known to hold everything it declares. + + Checked before the stack is stopped: a damaged or hand-made archive must fail while DocsGPT is + still running, not after ``docker compose down``. Only the volumes a backup is made of are + accepted, so a manifest cannot name ``postgres_data`` and have it emptied on the way in. + """ + volumes = manifest.get("volumes") + if not isinstance(volumes, list) or not all(isinstance(name, str) for name in volumes): + raise DeployError(f"{path} is not a DocsGPT backup: its manifest does not list the volumes it holds") + unsupported = sorted(set(volumes) - set(DATA_VOLUMES)) + if unsupported: + raise DeployError( + f"{path} names volumes that are not part of a backup: {', '.join(unsupported)}. " + f"A DocsGPT backup holds {', '.join(DATA_VOLUMES)}." + ) + required = [DUMP, *(volume_member(name) for name in volumes)] + try: + with tarfile.open(path, "r:gz") as archive: + present = {member.name for member in archive.getmembers() if member.isfile()} + except (tarfile.TarError, OSError) as exc: + raise DeployError(f"{path} is not a DocsGPT backup: {exc}") from exc + missing = [name for name in required if name not in present] + if missing: + raise DeployError(f"{path} is missing {', '.join(missing)}, so there is nothing to restore from") + return list(volumes) + + def extract(path: Path, destination: Path) -> None: """Unpack the archive into ``destination`` (data filter: no paths outside it, no devices).""" with tarfile.open(path, "r:gz") as archive: diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index fb9c5e25..b3bf7b28 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -371,6 +371,26 @@ def backup(args, context: Optional[Context] = None) -> int: target = out_dir / backup_format.archive_name(taken_at) image = _stack_image(env) + print("Pausing the backend and the worker so the database and the files match ...") + context.docker.compose(directory, "stop", "backend", "worker") + try: + _write_backup(args, context, directory, env, target, image, taken_at) + finally: + print("Starting the backend and the worker again ...") + context.docker.compose(directory, "up", "-d", "backend", "worker") + + size = target.stat().st_size / 1_000_000 + print(f"\nBackup written to {target} ({size:.1f} MB)") + if args.with_settings: + print("It contains .env, so it holds this install's secrets: keep it somewhere private.") + else: + print("Settings are not in it; `docsgpt backup --with-settings` includes .env, secrets and all.") + print(f"Restore it with: docsgpt restore {target}") + return 0 + + +def _write_backup(args, context: Context, directory: Path, env, target: Path, image: str, taken_at) -> None: + """Dump the database and the data volumes into ``target``, with the writers stopped.""" with tempfile.TemporaryDirectory() as workspace: work = Path(workspace) dump = work / backup_format.DUMP @@ -398,15 +418,6 @@ def backup(args, context: Optional[Context] = None) -> int: settings = directory / ".env" if args.with_settings else None backup_format.write_archive(target, dump=dump, volume_tars=volume_tars, manifest=manifest, settings=settings) - size = target.stat().st_size / 1_000_000 - print(f"\nBackup written to {target} ({size:.1f} MB)") - if args.with_settings: - print("It contains .env, so it holds this install's secrets: keep it somewhere private.") - else: - print("Settings are not in it; `docsgpt backup --with-settings` includes .env, secrets and all.") - print(f"Restore it with: docsgpt restore {target}") - return 0 - def restore(args, context: Optional[Context] = None) -> int: """Put a backup's database and data volumes back over this install.""" @@ -414,6 +425,8 @@ def restore(args, context: Optional[Context] = None) -> int: archive = Path(args.archive).expanduser() manifest = backup_format.read_manifest(archive) backup_format.check_version(manifest, context.version, args.force) + # Everything the archive declares is checked here, while DocsGPT is still up. + volumes = backup_format.validate(archive, manifest) directory = stack.stack_dir(args.dir) env = _installed(directory) @@ -436,7 +449,7 @@ def restore(args, context: Optional[Context] = None) -> int: with tempfile.TemporaryDirectory() as workspace: work = Path(workspace) backup_format.extract(archive, work) - for name in manifest.get("volumes", []): + for name in volumes: tar_path = work / backup_format.volume_member(name) if not tar_path.is_file(): raise DeployError(f"{archive} is missing the {name} volume it says it contains") @@ -452,7 +465,7 @@ def restore(args, context: Optional[Context] = None) -> int: with dump.open("r", encoding="utf-8") as handle: context.docker.compose( directory, "exec", "-T", "postgres", - "psql", "--quiet", "-U", "docsgpt", "-d", "docsgpt", + "psql", "--quiet", "--set", "ON_ERROR_STOP=on", "-U", "docsgpt", "-d", "docsgpt", stdin=handle, ) diff --git a/docsgpt/deploy/docker.py b/docsgpt/deploy/docker.py index 69cbe187..c44eb241 100644 --- a/docsgpt/deploy/docker.py +++ b/docsgpt/deploy/docker.py @@ -119,8 +119,11 @@ class Docker: """Replace ``volume``'s contents with the tar at ``source``; the volume is created when missing.""" with source.open("rb") as handle: self._run( - ["docker", "run", "--rm", "-i", "-v", f"{volume}:/data", image, - "sh", "-c", "find /data -mindepth 1 -delete && tar xf - -C /data"], + ["docker", "run", "--rm", "-i", "-v", f"{volume}:/data", image, "sh", "-c", + # Unpack into the container's own filesystem first: a truncated or corrupt tar must + # fail before the live volume is touched, not halfway through emptying it. + "set -e; rm -rf /stage; mkdir /stage; tar xf - -C /stage; " + "find /data -mindepth 1 -delete; tar cf - -C /stage . | tar xf - -C /data"], stdin=handle, ) diff --git a/tests/deploy/test_backup.py b/tests/deploy/test_backup.py index be43bfcf..371023a5 100644 --- a/tests/deploy/test_backup.py +++ b/tests/deploy/test_backup.py @@ -2,6 +2,7 @@ import io import json +import stat import tarfile import pytest @@ -26,6 +27,22 @@ def _manifest(path): return json.loads(archive.extractfile(backup_module.MANIFEST).read()) +def _rewrite(archive, dest, *, volumes=None, drop=()): + """The same archive with other volumes in its manifest, or with members left out.""" + with tarfile.open(archive, "r:gz") as source, tarfile.open(dest, "w:gz") as out: + for member in source.getmembers(): + if member.name in drop: + continue + body = source.extractfile(member).read() if member.isfile() else None + if member.name == backup_module.MANIFEST and volumes is not None: + manifest = json.loads(body.decode()) + manifest["volumes"] = volumes + body = json.dumps(manifest).encode() + member.size = len(body) + out.addfile(member, io.BytesIO(body) if body is not None else None) + return dest + + def _copy_with_version(archive, version, dest): """The same archive, with another DocsGPT version written into its manifest.""" with tarfile.open(archive, "r:gz") as source, tarfile.open(dest, "w:gz") as out: @@ -80,6 +97,40 @@ class TestBackup: assert dumps, docker.calls assert dumps[0][:3] == ["exec", "-T", "postgres"] + def test_the_archive_is_private_to_whoever_took_it(self, tmp_path): + """It can hold .env, and the dump is the install's data either way.""" + _installed(tmp_path) + out = tmp_path / "backups" + argv = ["backup", "--dir", str(tmp_path), "--out", str(out), "--with-settings"] + assert _run(argv, _context()) == 0 + archive = next(out.glob("*.tar.gz")) + assert stat.S_IMODE(archive.stat().st_mode) == 0o600 + + def test_the_writers_are_stopped_while_the_archive_is_made(self, tmp_path): + """Ingestion writes files and rows; a dump taken alongside live writes would not match them.""" + _installed(tmp_path) + docker = FakeDocker() + assert _run(["backup", "--dir", str(tmp_path), "--out", str(tmp_path / "b")], _context(docker)) == 0 + joined = [" ".join(args) for _, args in docker.calls] + stopped = next(index for index, call in enumerate(joined) if call.startswith("stop backend worker")) + dumped = next(index for index, call in enumerate(joined) if "pg_dump" in call) + started = next(index for index, call in enumerate(joined) if call.startswith("up -d backend worker")) + assert stopped < dumped < started, joined + + def test_the_writers_come_back_even_when_the_dump_fails(self, tmp_path): + _installed(tmp_path) + + class FailingDump(FakeDocker): + def compose(self, directory, *args, **kwargs): + if "pg_dump" in args: + raise DeployError("pg_dump exploded") + return super().compose(directory, *args, **kwargs) + + docker = FailingDump() + with pytest.raises(DeployError, match="pg_dump exploded"): + _run(["backup", "--dir", str(tmp_path), "--out", str(tmp_path / "b")], _context(docker)) + assert any(" ".join(args).startswith("up -d backend worker") for _, args in docker.calls), docker.calls + def test_without_an_install(self, tmp_path, capsys): assert _run(["backup", "--dir", str(tmp_path)], _context()) == 1 assert "docsgpt up" in capsys.readouterr().err @@ -122,6 +173,31 @@ class TestRestore: with pytest.raises(DeployError, match="does not exist"): _run(["restore", str(tmp_path / "nope.tar.gz"), "--dir", str(tmp_path), "--yes"], _context()) + def test_a_manifest_naming_another_volume_is_refused(self, tmp_path): + """import_volume empties what it is given, so a hand-made manifest must not name postgres_data.""" + crafted = _rewrite(self._backup(tmp_path), tmp_path / "crafted.tar.gz", volumes=["postgres_data"]) + docker = FakeDocker(volumes={"docsgpt_postgres_data"}) + with pytest.raises(DeployError, match="not part of a backup"): + _run(["restore", str(crafted), "--dir", str(tmp_path), "--yes"], _context(docker)) + assert docker.calls == [], "nothing was stopped" + assert docker.volume_ops == [], "and nothing was touched" + + def test_a_damaged_archive_fails_before_the_stack_is_stopped(self, tmp_path): + """Discovering a missing member after `compose down` would leave DocsGPT down for nothing.""" + without_dump = _rewrite(self._backup(tmp_path), tmp_path / "nodump.tar.gz", drop=(backup_module.DUMP,)) + docker = FakeDocker(volumes={"docsgpt_postgres_data"}) + with pytest.raises(DeployError, match="nothing to restore"): + _run(["restore", str(without_dump), "--dir", str(tmp_path), "--yes"], _context(docker)) + assert docker.calls == [], "DocsGPT is still running" + + def test_psql_stops_at_the_first_failing_statement(self, tmp_path): + """Without it psql runs on after an error and a half-restored database looks like success.""" + archive = self._backup(tmp_path) + docker = FakeDocker(volumes={"docsgpt_postgres_data"}) + assert _run(["restore", str(archive), "--dir", str(tmp_path), "--yes"], _context(docker)) == 0 + psql = next(args for _, args in docker.calls if "psql" in args) + assert "ON_ERROR_STOP=on" in psql + def test_a_newer_backup_is_refused_without_force(self, tmp_path): """Restoring a 0.22 backup into 0.21 would hand an older schema newer data.""" newer = _copy_with_version(self._backup(tmp_path), "0.22.0", tmp_path / "newer.tar.gz") diff --git a/tests/deploy/test_docker.py b/tests/deploy/test_docker.py index 0ae2dfe4..61c15dd8 100644 --- a/tests/deploy/test_docker.py +++ b/tests/deploy/test_docker.py @@ -129,7 +129,10 @@ class TestVolumes: assert "docsgpt_inputs:/data" in args assert args[-2] == "-c" assert "tar xf - -C /data" in args[-1] - assert "find /data -mindepth 1 -delete" in args[-1], "old contents go first" + command = args[-1] + assert command.index("tar xf - -C /stage") < command.index("find /data -mindepth 1 -delete"), ( + "the incoming tar is unpacked outside the volume first, so a corrupt one leaves it alone" + ) assert runner.streams[0][1] is not None, "the tar is fed in on stdin"