From 4fc4523384590daece9f4923e1589144fd7d9a53 Mon Sep 17 00:00:00 2001 From: Marinski Date: Fri, 21 Aug 2026 15:26:48 +0300 Subject: [PATCH] fix(ops): stop VMs before recreating them, with the real grace period recreate-vm.sh recreates with `docker compose up -d --force-recreate`, whose implicit stop uses compose's own --timeout -- 10 SECONDS by default -- rather than the service's declared stop_grace_period. A dockurr/windows guest cannot shut down in ten seconds, so compose stops waiting and goes straight to removing a container that is still running: Error response from daemon: cannot remove container "1d5e3c2f...": container is running: stop the container before removing or force remove The script then exits 1 and the VM is left unhealthy with its network_mode sidecar stranded on a dead netns -- the exact outcome recreate-vm.sh exists to prevent. It is timing-dependent, which is why it can look fine for a while. On the deployment where this was found the script recreated two VMs successfully three times inside one hour, then failed on the fourth attempt when the guest took longer than ten seconds to go down. The targets are now stopped explicitly first with a timeout that matches the grace period, and the same value is passed to `up` so its implicit stop cannot fall back to 10s. RECREATE_STOP_TIMEOUT overrides the 120s default; anything calling this script on a timeout of its own should stay above it. The script had no direct test coverage. tests/test_recreate_vm_script.py covers it through --dry-run, so it needs no Docker daemon: stop-before-up ordering, the timeout default and its override, and the sidecar expansion that is the reason the script exists. Four of the seven fail against the current version. Worth noting for #15: that watchdog delegates recovery to this script, so merging it without this fix ships an automated recovery path that hits the failure above. Dockerfile.test copies a named subset of scripts/ and recreate-vm.sh was not in it, so the new tests could not see the script. It is added to that COPY line; nothing else about the image changes. --- Dockerfile.test | 2 +- scripts/recreate-vm.sh | 35 +++++++-- tests/test_recreate_vm_script.py | 118 +++++++++++++++++++++++++++++++ 3 files changed, 150 insertions(+), 5 deletions(-) create mode 100644 tests/test_recreate_vm_script.py 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