diff --git a/backend/docker_client.py b/backend/docker_client.py index d9cf19d..e7ed9d9 100644 --- a/backend/docker_client.py +++ b/backend/docker_client.py @@ -6,11 +6,11 @@ to the frontend. from __future__ import annotations import functools -from typing import Any +from typing import Any, Optional import docker import requests -from docker.errors import APIError, DockerException, NotFound +from docker.errors import APIError, DockerException, ImageNotFound, NotFound from config import settings @@ -57,6 +57,70 @@ def reset_client() -> None: get_long_client.cache_clear() +#: Label on every throwaway container StackPilot starts (backup helpers, NFS +#: I/O, the scanner), so leftovers can be found and removed. +HELPER_LABEL = "stackpilot.helper" + + +def run_container( + client: docker.DockerClient, + image: str, + command: Any, + *, + volumes: Optional[dict] = None, + timeout: Optional[float] = None, +) -> bytes: + """``docker run --rm`` that also cleans up a container that cannot start. + + docker-py's ``containers.run(remove=True)`` only removes a container after + it ran. One whose start fails — a volume that cannot be mounted, say an + NFS share that refuses the connection — stays behind in "Created" and + keeps its volumes in use; every retry adds another. Returns stdout; a + non-zero exit raises ``DockerError("container_failed", )``. + """ + kwargs = {"command": command, "volumes": volumes or {}, "labels": {HELPER_LABEL: "1"}} + try: + container = client.containers.create(image, **kwargs) + except ImageNotFound: + safe_call(client.images.pull, image) + container = safe_call(client.containers.create, image, **kwargs) + except DockerException as exc: + raise DockerError("docker_api_error", str(exc)) from exc + try: + safe_call(container.start) + code = safe_call(container.wait, timeout=timeout).get("StatusCode", 1) + out = container.logs(stdout=True, stderr=False) or b"" + if code != 0: + err = (container.logs(stdout=False, stderr=True) or b"").decode("utf-8", "replace") + raise DockerError("container_failed", err.strip() or f"exit status {code}") + return out + finally: + try: + container.remove(force=True) + except Exception: # noqa: BLE001 - best effort, never mask the real error + pass + + +def remove_stale_helpers(client: docker.DockerClient) -> int: + """Remove StackPilot's helper containers that are not running. + + Called at startup, when nothing of ours can be in flight. Returns how many + were removed. + """ + removed = 0 + for container in safe_call( + client.containers.list, all=True, filters={"label": HELPER_LABEL} + ): + if container.status == "running": + continue + try: + container.remove(force=True) + removed += 1 + except Exception: # noqa: BLE001 + pass + return removed + + def safe_call(fn, *args, **kwargs) -> Any: """Run a docker-py call, converting exceptions into DockerError.""" try: diff --git a/backend/main.py b/backend/main.py index 90d9a1b..2ad56d5 100644 --- a/backend/main.py +++ b/backend/main.py @@ -13,6 +13,7 @@ from sqlmodel import Session from config import settings from version import APP_VERSION from database import engine, init_db +import docker_client from docker_client import DockerError from routers import ( audit, @@ -79,6 +80,17 @@ async def lifespan(app: FastAPI): logger.info("Migrated %d custom template(s) from the database to folders", moved) except Exception as exc: # noqa: BLE001 logger.warning("Legacy template migration failed: %s", exc) + # Throwaway containers a previous run left behind (a helper whose start + # failed, e.g. on an NFS mount, used to stay in "Created" and keep its + # volume in use). Nothing of ours is in flight yet, so all can go. + try: + client = docker_client.get_client() + cleared = docker_client.remove_stale_helpers(client) + cleared += backup_destination_service.clear_nfs_leftovers(client) + if cleared: + logger.info("Removed %d leftover helper container(s)", cleared) + except Exception as exc: # noqa: BLE001 - Docker may not be up yet + logger.warning("Could not clean up helper containers: %s", exc) # Runtime state that used to live in module dicts and was lost on restart. try: with Session(engine) as session: diff --git a/backend/services/backup_destination_service.py b/backend/services/backup_destination_service.py index a964d6c..bcf7569 100644 --- a/backend/services/backup_destination_service.py +++ b/backend/services/backup_destination_service.py @@ -16,7 +16,7 @@ import posixpath import stat import tarfile import tempfile -from typing import Any +from typing import Any, Optional from models.backup_destination import BackupDestination from services import crypto_service @@ -309,10 +309,21 @@ def _nfs_volume(dest: BackupDestination, cfg: dict) -> str: client = get_client() try: vol = safe_call(client.volumes.get, name) - if (vol.attrs.get("Options") or {}) != driver_opts: - safe_call(vol.remove) - raise DockerError("recreate", "options changed") except DockerError: + vol = None + if vol is not None and (vol.attrs.get("Options") or {}) != driver_opts: + # Stopped containers still hold the volume; before 0.66.1 every failed + # mount left one behind, and then new settings never took effect. + clear_nfs_leftovers(client, name) + try: + safe_call(vol.remove) + except DockerError as exc: + raise DestinationError( + f"The NFS settings changed, but the Docker volume '{name}' is still in use " + f"and cannot be recreated with them: {exc.detail or exc.error}" + ) from exc + vol = None + if vol is None: safe_call( client.volumes.create, name=name, @@ -323,35 +334,81 @@ def _nfs_volume(dest: BackupDestination, cfg: dict) -> str: return name +def clear_nfs_leftovers(client, volume: Optional[str] = None) -> int: + """Remove stopped containers holding an NFS destination volume. + + Only StackPilot's helpers ever mount these volumes, so any container on + one that is not running is a leftover (older versions left one behind per + failed mount). ``volume=None`` covers every NFS destination. + """ + from docker_client import DockerError, safe_call + + if volume: + names = [volume] + else: + try: + vols = safe_call( + client.volumes.list, filters={"label": "stackpilot.nfs-destination"} + ) + except DockerError: + return 0 + names = [v.name for v in vols] + removed = 0 + for name in names: + try: + containers = safe_call(client.containers.list, all=True, filters={"volume": name}) + except DockerError: + continue + for container in containers: + if container.status == "running": + continue + try: + container.remove(force=True) + removed += 1 + except Exception: # noqa: BLE001 + pass + return removed + + def _nfs_target(cfg: dict) -> str: sub = _nfs_subdir(cfg) return f"/nfs/{sub}" if sub else "/nfs" +def _mount_error(exc: Exception) -> DestinationError: + """A mount failure, with what to do about the common one.""" + text = str(exc) + message = f"NFS mount failed: {text}" + if "connection refused" in text.lower(): + message += ( + " — the NFS server refused the Docker host's connection. Check that NFS is " + "enabled on the server and reachable from the Docker host. NFSv3 also needs " + "rpcbind (port 111) and mountd; mount options 'rw,nfsvers=4' need only port 2049." + ) + return DestinationError(message) + + def _nfs_run(volume: str, command: list[str]) -> str: """Run a helper container with the NFS volume at /nfs; return stdout.""" - import docker.errors - from config import settings - from docker_client import DockerError, get_client + from docker_client import DockerError, get_long_client, run_container from services.stack_assets_service import ensure_helper_image - client = get_client() + client = get_long_client() ensure_helper_image(client) try: - out = client.containers.run( + out = run_container( + client, settings.BACKUP_HELPER_IMAGE, command, volumes={volume: {"bind": "/nfs", "mode": "rw"}}, - remove=True, ) - return (out or b"").decode("utf-8", "replace") - except docker.errors.ContainerError as exc: - stderr = (exc.stderr or b"").decode("utf-8", "replace").strip() - raise DestinationError(f"NFS operation failed: {stderr or exc}") from exc - except (docker.errors.APIError, DockerError) as exc: + return out.decode("utf-8", "replace") + except DockerError as exc: + if exc.error == "container_failed": + raise DestinationError(f"NFS operation failed: {exc.detail}") from exc # Mount errors surface here (unreachable server, bad export, ...). - raise DestinationError(f"NFS mount failed: {exc}") from exc + raise _mount_error(exc) from exc def _nfs_helper(volume: str, command: list[str] | str = "true"): @@ -359,7 +416,7 @@ def _nfs_helper(volume: str, command: list[str] | str = "true"): import docker.errors from config import settings - from docker_client import DockerError, get_client, safe_call + from docker_client import HELPER_LABEL, DockerError, get_client, safe_call from services.stack_assets_service import ensure_helper_image client = get_client() @@ -370,9 +427,10 @@ def _nfs_helper(volume: str, command: list[str] | str = "true"): settings.BACKUP_HELPER_IMAGE, command=command, volumes={volume: {"bind": "/nfs", "mode": "rw"}}, + labels={HELPER_LABEL: "1"}, ) except (docker.errors.APIError, DockerError) as exc: - raise DestinationError(f"NFS mount failed: {exc}") from exc + raise _mount_error(exc) from exc def _nfs_upload(dest: BackupDestination, cfg: dict, local_path: str, filename: str) -> str: diff --git a/backend/services/scan_service.py b/backend/services/scan_service.py index 6c089e5..360f945 100644 --- a/backend/services/scan_service.py +++ b/backend/services/scan_service.py @@ -26,7 +26,7 @@ from typing import Optional from sqlmodel import Session, select from config import settings -from docker_client import DockerError, get_client, safe_call +from docker_client import DockerError, get_client, get_long_client, run_container, safe_call from models.image_scan import SEVERITIES, ImageScan logger = logging.getLogger("stackpilot.scan") @@ -55,9 +55,15 @@ class ScanError(Exception): # --------------------------------------------------------------------------- # +#: Wait for the scanner a little longer than its own --timeout of 10m. +SCAN_WAIT_SECONDS = 720 + + def _run_trivy(image: str) -> str: """Run Trivy against a local image and return its JSON report.""" - client = get_client() + # Trivy may take its full --timeout (the first run also downloads its + # vulnerability database); the 60s default client gave up long before. + client = get_long_client() socket_path = settings.DOCKER_SOCKET command = [ "image", @@ -70,21 +76,19 @@ def _run_trivy(image: str) -> str: image, ] try: - output = safe_call( - client.containers.run, + output = run_container( + client, settings.SCANNER_IMAGE, command, volumes={ socket_path: {"bind": "/var/run/docker.sock", "mode": "ro"}, CACHE_VOLUME: {"bind": "/root/.cache/trivy", "mode": "rw"}, }, - remove=True, - stdout=True, - stderr=False, + timeout=SCAN_WAIT_SECONDS, ) except DockerError as exc: - raise ScanError(str(exc)) from exc - except Exception as exc: # noqa: BLE001 - docker-py raises ContainerError etc. + raise ScanError(_readable(Exception(exc.detail or exc.error))) from exc + except Exception as exc: # noqa: BLE001 raise ScanError(_readable(exc)) from exc return (output or b"").decode("utf-8", "replace") diff --git a/backend/services/stack_assets_service.py b/backend/services/stack_assets_service.py index c24860a..8d7d5b6 100644 --- a/backend/services/stack_assets_service.py +++ b/backend/services/stack_assets_service.py @@ -24,7 +24,14 @@ from typing import Optional import yaml from config import settings -from docker_client import DockerError, get_client, get_long_client, safe_call +from docker_client import ( + HELPER_LABEL, + DockerError, + get_client, + get_long_client, + run_container, + safe_call, +) from services import compose_service logger = logging.getLogger("stackpilot.assets") @@ -97,6 +104,7 @@ def _create_helper(client, volumes: dict): settings.BACKUP_HELPER_IMAGE, command="true", volumes=volumes, + labels={HELPER_LABEL: "1"}, ) @@ -157,6 +165,7 @@ def _create_helper_with(client, volumes: dict, command: list[str]): settings.BACKUP_HELPER_IMAGE, command=command, volumes=volumes, + labels={HELPER_LABEL: "1"}, ) @@ -192,14 +201,8 @@ def inspect_paths(paths: list[str]) -> dict[str, dict]: ) script = "; ".join(script_parts) try: - out = safe_call( - client.containers.run, - settings.BACKUP_HELPER_IMAGE, - ["sh", "-c", script], - volumes=mounts, - remove=True, - stdout=True, - stderr=False, + out = run_container( + client, settings.BACKUP_HELPER_IMAGE, ["sh", "-c", script], volumes=mounts ) except DockerError as exc: logger.warning("Path inspection failed: %s", exc) @@ -341,12 +344,11 @@ def import_volume( ) if existed and wipe: # Restore means "back to the snapshot": drop files created since. - safe_call( - client.containers.run, + run_container( + client, settings.BACKUP_HELPER_IMAGE, ["sh", "-c", "find /v -mindepth 1 -delete"], volumes={full_name: {"bind": "/v", "mode": "rw"}}, - remove=True, ) mounts = {full_name: {"bind": "/v", "mode": "rw"}} container = _create_helper(client, mounts) diff --git a/backend/tests/test_backup_restore.py b/backend/tests/test_backup_restore.py index 2a95378..a86d8bd 100644 --- a/backend/tests/test_backup_restore.py +++ b/backend/tests/test_backup_restore.py @@ -648,6 +648,8 @@ def test_download_removes_its_temp_file(stack, as_admin, db): class _HelperClient: + """Stands in for the helper container that runs the inspection script.""" + def __init__(self, output: str): self.output = output self.images = self @@ -656,8 +658,20 @@ class _HelperClient: def get(self, image): return object() - def run(self, image, command, **kwargs): - return self.output.encode() + def create(self, image, **kwargs): + return self + + def start(self): + pass + + def wait(self, timeout=None): + return {"StatusCode": 0} + + def logs(self, stdout=True, stderr=True): + return self.output.encode() if stdout else b"" + + def remove(self, force=False): + pass def test_inspect_paths_flags_remote_and_unmeasured(monkeypatch): diff --git a/backend/tests/test_helper_containers.py b/backend/tests/test_helper_containers.py new file mode 100644 index 0000000..4b84bc9 --- /dev/null +++ b/backend/tests/test_helper_containers.py @@ -0,0 +1,208 @@ +"""Throwaway containers must not outlive their job. + +docker-py's ``containers.run(remove=True)`` only removes a container after it +ran. A helper whose start failed — an NFS destination that refuses the mount — +stayed behind in "Created", holding the destination's volume. Every retry of +"Test" or of a scheduled backup added one (a user found 18), and because the +volume stayed in use, changing the destination's mount options never took +effect: the volume could not be recreated with them. +""" +from __future__ import annotations + +import pytest + +from docker_client import HELPER_LABEL, DockerError + + +class FakeContainer: + def __init__(self, client, image, command=None, volumes=None, labels=None, status="created"): + self.client, self.image, self.command = client, image, command + self.volumes, self.labels, self.status = volumes or {}, labels or {}, status + self.removed = False + + def start(self): + if self.client.start_error: + raise DockerError("docker_api_error", self.client.start_error) + self.status = "exited" + + def wait(self, timeout=None): + self.client.wait_timeouts.append(timeout) + return {"StatusCode": self.client.exit_code} + + def logs(self, stdout=True, stderr=True): + return b"out" if stdout else b"boom" + + def remove(self, force=False): + self.removed = True + self.client.all.remove(self) + + +class FakeClient: + def __init__(self, start_error=None, exit_code=0): + self.start_error, self.exit_code = start_error, exit_code + self.all: list[FakeContainer] = [] + self.wait_timeouts: list = [] + self.containers = self + self.volumes = FakeVolumes(self) + self.images = self + + def create(self, image, command=None, volumes=None, labels=None): + c = FakeContainer(self, image, command, volumes, labels) + self.all.append(c) + return c + + def get(self, image): + return object() + + def list(self, all=False, filters=None): + filters = filters or {} + out = list(self.all) + if "label" in filters: + out = [c for c in out if filters["label"] in c.labels] + if "volume" in filters: + out = [c for c in out if filters["volume"] in c.volumes] + return out + + +class FakeVolume: + def __init__(self, client, name, options, labels): + self.client, self.name = client, name + self.attrs = {"Options": options, "Labels": labels} + + def remove(self): + if any(self.name in c.volumes for c in self.client.all): + raise DockerError("docker_api_error", "volume is in use") + del self.client.volumes.items[self.name] + + +class FakeVolumes: + def __init__(self, client): + self.client = client + self.items: dict[str, FakeVolume] = {} + + def get(self, name): + if name not in self.items: + raise DockerError("not_found", name) + return self.items[name] + + def create(self, name, driver, driver_opts, labels): + self.items[name] = FakeVolume(self.client, name, driver_opts, labels) + return self.items[name] + + def list(self, filters=None): + label = (filters or {}).get("label") + return [v for v in self.items.values() if not label or label in v.attrs["Labels"]] + + +def test_container_that_cannot_start_is_removed(): + from docker_client import run_container + + client = FakeClient(start_error="failed to mount local volume: connection refused") + with pytest.raises(DockerError, match="connection refused"): + run_container(client, "alpine", ["true"], volumes={"nfs": {"bind": "/nfs"}}) + assert client.all == [] + + +def test_failed_command_is_removed_and_reports_stderr(): + from docker_client import run_container + + client = FakeClient(exit_code=2) + with pytest.raises(DockerError) as info: + run_container(client, "alpine", ["false"]) + assert info.value.error == "container_failed" and info.value.detail == "boom" + assert client.all == [] + + +def test_successful_run_returns_stdout_and_is_labelled(): + from docker_client import run_container + + client = FakeClient() + seen = [] + original = client.create + client.create = lambda *a, **k: seen.append(k["labels"]) or original(*a, **k) + assert run_container(client, "alpine", ["echo"], timeout=900) == b"out" + assert seen == [{HELPER_LABEL: "1"}] and client.all == [] + assert client.wait_timeouts == [900] + + +def test_startup_removes_stopped_helpers_only(): + from docker_client import remove_stale_helpers + + client = FakeClient() + stale = client.create("alpine", labels={HELPER_LABEL: "1"}) + busy = client.create("alpine", labels={HELPER_LABEL: "1"}) + busy.status = "running" + foreign = client.create("nginx", labels={}) + + assert remove_stale_helpers(client) == 1 + assert stale.removed and not busy.removed and not foreign.removed + + +def _nfs_dest(options="rw"): + from models.backup_destination import BackupDestination + + return BackupDestination(id=1, name="nas", type="nfs", config="{}"), { + "server": "10.10.1.80", "path": "/mnt/user/Share", "options": options, + } + + +def test_changed_nfs_options_take_effect_despite_leftovers(monkeypatch): + from services import backup_destination_service as svc + + client = FakeClient() + monkeypatch.setattr("docker_client.get_client", lambda: client) + dest, cfg = _nfs_dest("rw") + name = svc._nfs_volume(dest, cfg) + # 18 helpers left behind by failed mounts under the old version. + for _ in range(18): + client.create("alpine", volumes={name: {"bind": "/nfs"}}) + + dest, cfg = _nfs_dest("rw,nfsvers=4") + assert svc._nfs_volume(dest, cfg) == name + assert client.volumes.items[name].attrs["Options"]["o"] == "addr=10.10.1.80,rw,nfsvers=4" + assert client.all == [] + + +def test_volume_held_by_a_running_container_is_a_clear_error(monkeypatch): + from services import backup_destination_service as svc + + client = FakeClient() + monkeypatch.setattr("docker_client.get_client", lambda: client) + dest, cfg = _nfs_dest("rw") + name = svc._nfs_volume(dest, cfg) + client.create("alpine", volumes={name: {"bind": "/nfs"}}).status = "running" + + dest, cfg = _nfs_dest("rw,nfsvers=4") + with pytest.raises(svc.DestinationError, match="still in use"): + svc._nfs_volume(dest, cfg) + + +def test_clear_nfs_leftovers_covers_every_destination(): + from services import backup_destination_service as svc + + client = FakeClient() + client.volumes.create("stackpilot-nfs-dest-1", "local", {}, {"stackpilot.nfs-destination": "1"}) + client.volumes.create("stackpilot-nfs-dest-2", "local", {}, {"stackpilot.nfs-destination": "2"}) + client.volumes.create("app_data", "local", {}, {}) + client.create("alpine", volumes={"stackpilot-nfs-dest-1": {}}) + client.create("alpine", volumes={"stackpilot-nfs-dest-2": {}}) + keep = client.create("postgres", volumes={"app_data": {}}) + + assert svc.clear_nfs_leftovers(client) == 2 + assert client.all == [keep] + + +def test_refused_mount_says_what_to_do(monkeypatch): + from services import backup_destination_service as svc + + client = FakeClient(start_error=( + "error while mounting volume: failed to mount local volume: mount " + ":/mnt/user/Share: data: addr=10.10.1.80: connection refused" + )) + monkeypatch.setattr("docker_client.get_long_client", lambda: client) + monkeypatch.setattr("services.stack_assets_service.ensure_helper_image", lambda c: None) + + with pytest.raises(svc.DestinationError) as info: + svc._nfs_run("stackpilot-nfs-dest-1", ["mkdir", "-p", "/nfs"]) + assert "nfsvers=4" in str(info.value) and "port 2049" in str(info.value) + assert client.all == [] # and nothing is left behind diff --git a/backend/version.py b/backend/version.py index 734a2e5..094951e 100644 --- a/backend/version.py +++ b/backend/version.py @@ -1,3 +1,3 @@ """Single source of truth for the StackPilot release version.""" -APP_VERSION = "0.66.0" +APP_VERSION = "0.66.1" diff --git a/frontend/package.json b/frontend/package.json index a59dbf9..c20156e 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,7 +1,7 @@ { "name": "stackpilot-frontend", "private": true, - "version": "0.66.0", + "version": "0.66.1", "type": "module", "scripts": { "dev": "vite",