diff --git a/Dockerfile.test b/Dockerfile.test index e4b6682..f4f7445 100644 --- a/Dockerfile.test +++ b/Dockerfile.test @@ -25,7 +25,7 @@ COPY tests ./tests # docker-compose.yml.j2 is here because the compose-generation test renders the # REAL template — a stub would assert nothing about what actually ships. COPY requirements-api.txt requirements-mcpunifier.txt docker-compose.yml.example docker-compose.yml.j2 run.sh ./ -COPY scripts/config_helper.py scripts/start.bat scripts/check_health.py scripts/healthcheck.sh scripts/verify_binaries.py scripts/wickworks-healthcheck.py ./scripts/ +COPY scripts/config_helper.py scripts/start.bat scripts/check_health.py scripts/healthcheck.sh scripts/verify_binaries.py scripts/wickworks-healthcheck.py scripts/recreate-vm.sh ./scripts/ COPY assets/binaries.lock.json ./assets/ ENV PYTHONPATH=/app diff --git a/scripts/recreate-vm.sh b/scripts/recreate-vm.sh index e352afe..24abca1 100755 --- a/scripts/recreate-vm.sh +++ b/scripts/recreate-vm.sh @@ -33,10 +33,31 @@ # ./scripts/recreate-vm.sh mt5 # recreate mt5 + its sidecars # ./scripts/recreate-vm.sh mt5 mt5-b # recreate both VMs + their sidecars # +# STOPPING BEFORE RECREATING +# -------------------------- +# `docker compose up --force-recreate` stops each container using compose's OWN +# --timeout, which defaults to 10 SECONDS, not the service's stop_grace_period. +# A dockurr/windows VM needs far longer than that to shut down (ours declare +# `stop_grace_period: 2m`), so compose gave up waiting and went straight to +# removing a container that was still running: +# +# Error response from daemon: cannot remove container "...": +# container is running: stop the container before removing or force remove +# +# The watchdog then recorded a failed recreate and backed off, leaving the VM +# unhealthy and its sidecars stranded — the exact outcome this script exists to +# prevent. So the targets are stopped explicitly first, with a timeout that +# matches the grace period, and the same value is passed to `up` so its implicit +# stop can never fall back to 10s. +# # ENV # --- -# COMPOSE_FILE compose file to read services from -# (default: ./docker-compose.yml in the repo root) +# COMPOSE_FILE compose file to read services from +# (default: ./docker-compose.yml in the repo root) +# RECREATE_STOP_TIMEOUT seconds to allow each container to stop +# (default: 120, matching stop_grace_period: 2m). +# Keep this below WATCHDOG_RECREATE_TIMEOUT (300s) or +# the watchdog kills the script mid-recreate. # # The script never touches services it was not asked to recreate, and never # uses --no-deps in a way that skips the named sidecars. @@ -47,6 +68,7 @@ trap 'echo "[ERROR] ${BASH_SOURCE[0]}:${LINENO} - command failed (exit $?)" >&2' DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" COMPOSE_FILE="${COMPOSE_FILE:-${DIR}/docker-compose.yml}" +STOP_TIMEOUT="${RECREATE_STOP_TIMEOUT:-120}" DRY_RUN=0 usage() { @@ -117,12 +139,17 @@ main() { done if [ "$DRY_RUN" = "1" ]; then - log "DRY-RUN: would run 'docker compose up -d --force-recreate --no-deps ${targets[*]}'" + log "DRY-RUN: would run 'docker compose stop -t ${STOP_TIMEOUT} ${targets[*]}'" + log "DRY-RUN: would run 'docker compose up -d --force-recreate --no-deps -t ${STOP_TIMEOUT} ${targets[*]}'" return 0 fi + # Stop first, with the real grace period. See STOPPING BEFORE RECREATING. + log "stopping (timeout ${STOP_TIMEOUT}s): ${targets[*]}" + docker compose -f "$COMPOSE_FILE" stop -t "$STOP_TIMEOUT" "${targets[@]}" + log "recreating: ${targets[*]}" - docker compose -f "$COMPOSE_FILE" up -d --force-recreate --no-deps "${targets[@]}" + docker compose -f "$COMPOSE_FILE" up -d --force-recreate --no-deps -t "$STOP_TIMEOUT" "${targets[@]}" log "recreate done" } diff --git a/tests/test_recreate_vm_script.py b/tests/test_recreate_vm_script.py new file mode 100644 index 0000000..fe8a36f --- /dev/null +++ b/tests/test_recreate_vm_script.py @@ -0,0 +1,118 @@ +"""Tests for scripts/recreate-vm.sh. + +The script had no direct coverage, and the gap hid a production failure: it +recreated VMs with `docker compose up --force-recreate`, whose implicit stop +uses compose's own --timeout (10s by default) rather than the service's +stop_grace_period. A dockurr/windows VM cannot shut down in 10s, so compose +tried to remove a still-running container and the recreate failed: + + cannot remove container "...": container is running + +These exercise --dry-run, so they assert the planned command line without +needing a Docker daemon. +""" + +import os +import subprocess +from pathlib import Path + +_REPO = Path(__file__).resolve().parents[1] +_SCRIPT = _REPO / "scripts" / "recreate-vm.sh" + +COMPOSE = """\ +services: + mt5: + image: dockurr/windows:5.14 + stop_grace_period: 2m + wickworks: + image: psyb0t/wickworks + network_mode: "service:mt5" + mt5-b: + image: dockurr/windows:5.14 + wickworks-b: + image: psyb0t/wickworks + network_mode: "service:mt5-b" + unrelated: + image: nginx +""" + + +def run(tmp_path, *args, env=None): + compose = tmp_path / "docker-compose.yml" + compose.write_text(COMPOSE, encoding="utf-8") + # Inherit the real PATH: the script's sidecar discovery shells out to + # python3, which is not at a fixed location across host and test image. + full_env = {"PATH": os.environ.get("PATH", "/usr/bin:/bin"), "COMPOSE_FILE": str(compose)} + full_env.update(env or {}) + return subprocess.run( + [str(_SCRIPT), "--dry-run", *args], + capture_output=True, + text=True, + env=full_env, + timeout=60, + check=False, + ) + + +def test_stops_before_recreating(tmp_path): + """The stop must be explicit, or compose removes a running container.""" + res = run(tmp_path, "mt5") + assert res.returncode == 0, res.stderr + assert "docker compose stop" in res.stdout + stop_at = res.stdout.index("docker compose stop") + up_at = res.stdout.index("docker compose up") + assert stop_at < up_at, "stop must be planned before the recreate" + + +def test_stop_timeout_matches_the_grace_period_by_default(tmp_path): + """10s (compose's default) is far too short for a Windows VM.""" + res = run(tmp_path, "mt5") + assert "-t 120" in res.stdout + assert "-t 10 " not in res.stdout + + +def test_timeout_is_passed_to_up_as_well(tmp_path): + """`up --force-recreate` does its own stop; it must not use the 10s default.""" + res = run(tmp_path, "mt5") + up_line = next(ln for ln in res.stdout.splitlines() if "docker compose up" in ln) + assert "--force-recreate" in up_line + assert "--no-deps" in up_line + assert "-t 120" in up_line + + +def test_timeout_is_overridable(tmp_path): + res = run(tmp_path, "mt5", env={"RECREATE_STOP_TIMEOUT": "45"}) + assert "-t 45" in res.stdout + assert "-t 120" not in res.stdout + + +def test_sidecars_are_recreated_with_their_vm(tmp_path): + """The whole point: the sidecar must rejoin the VM's new netns.""" + res = run(tmp_path, "mt5") + up_line = next(ln for ln in res.stdout.splitlines() if "docker compose up" in ln) + assert "mt5" in up_line + assert "wickworks" in up_line + assert "unrelated" not in up_line + assert "mt5-b" not in up_line + + +def test_multiple_vms_expand_to_all_their_sidecars(tmp_path): + res = run(tmp_path, "mt5", "mt5-b") + up_line = next(ln for ln in res.stdout.splitlines() if "docker compose up" in ln) + for expected in ("mt5", "wickworks", "mt5-b", "wickworks-b"): + assert expected in up_line + assert "unrelated" not in up_line + + +def test_no_arguments_is_an_error(tmp_path): + compose = tmp_path / "docker-compose.yml" + compose.write_text(COMPOSE, encoding="utf-8") + res = subprocess.run( + [str(_SCRIPT)], + capture_output=True, + text=True, + env={"PATH": os.environ.get("PATH", "/usr/bin:/bin"), "COMPOSE_FILE": str(compose)}, + timeout=60, + check=False, + ) + assert res.returncode != 0