diff --git a/bot_bottle/backend/firecracker/infra_vm.py b/bot_bottle/backend/firecracker/infra_vm.py index d4d642a..c031994 100644 --- a/bot_bottle/backend/firecracker/infra_vm.py +++ b/bot_bottle/backend/firecracker/infra_vm.py @@ -161,20 +161,23 @@ def ensure_running() -> InfraVm: slot = netpool.orch_slot() url = f"http://{slot.guest_ip}:{CONTROL_PLANE_PORT}" key = _infra_dir() / "id_ed25519" - if key.exists() and _health_ok(url): + want = _expected_version() + if _adoptable(key, url, want): info(f"adopting running infra VM at {url}") return InfraVm(guest_ip=slot.guest_ip, private_key=key) with _singleton_lock(): # Re-check under the lock: another launcher may have booted it while # we waited for the lock (double-checked, so we adopt not re-boot). - if key.exists() and _health_ok(url): + if _adoptable(key, url, want): info(f"adopting running infra VM at {url}") return InfraVm(guest_ip=slot.guest_ip, private_key=key) - stop() # clear a stale/hung VM holding the link before booting fresh + # Clear a stale/hung/OUTDATED VM holding the link before booting fresh. + stop() ensure_built() infra = boot() wait_for_health(infra) + _record_booted_version(want) return infra @@ -193,9 +196,15 @@ def _singleton_lock() -> Generator[None, None, None]: def stop() -> None: - """Stop the infra VM singleton (idempotent — absent is success).""" + """Stop the infra VM singleton (idempotent — absent is success). Reaps the + recorded VMM AND any orphaned firecracker still bound to the infra config — + the PID file drifts after crashes / out-of-band kills, and a survivor would + hold the orchestrator TAP so the next boot dies with "tap … Resource busy". + Drops the version marker so a stopped VM is never treated as adoptable.""" _kill_pidfile() + _kill_infra_firecrackers() _pid_file().unlink(missing_ok=True) + _version_file().unlink(missing_ok=True) def boot() -> InfraVm: @@ -238,6 +247,36 @@ def _pid_file() -> Path: return _infra_dir() / "vm.pid" +def _version_file() -> Path: + """Records the infra-artifact version the *running* VM booted from, so a + later launcher can tell whether the singleton it found is the current code. + Without it, a healthy VM built from an older image gets adopted forever and + the new code never boots — every infra change would need an out-of-band + kill to dislodge the stale VM (and races whatever launched next).""" + return _infra_dir() / "booted-version" + + +def _expected_version() -> str: + return infra_artifact.infra_artifact_version(_infra_init()) + + +def _adoptable(key: Path, url: str, want: str) -> bool: + """Adopt a running infra VM only if it booted from the CURRENT version and + its control plane is healthy. A missing/mismatched marker means a prior + launcher booted an older infra image — reboot rather than reuse stale code.""" + if not key.exists(): + return False + try: + booted = _version_file().read_text(encoding="utf-8").strip() + except OSError: + return False + return booted == want and _health_ok(url) + + +def _record_booted_version(version: str) -> None: + _version_file().write_text(version + "\n", encoding="utf-8") + + # The registry "volume": a host-side ext4 file attached to the infra VM as a # second virtio-block device (guest /dev/vdb), mounted at the control plane's # DB dir. It outlives the ephemeral rootfs, so the bottle registry survives an @@ -309,6 +348,28 @@ def _kill_pidfile() -> None: pass +def _kill_infra_firecrackers(proc_root: Path = Path("/proc")) -> None: + """SIGKILL any firecracker VMM whose `--config-file` is this host's infra + config, independent of the PID file — reaps orphans it lost track of so the + orchestrator TAP is free to rebind. Scoped to the infra config path, so the + interactive pool's agent/infra VMs (other config paths) are untouched.""" + cfg = str(_infra_dir() / "config.json") + for entry in proc_root.iterdir(): + if not entry.name.isdigit(): + continue + try: + if (entry / "comm").read_text().strip() != "firecracker": + continue + args = (entry / "cmdline").read_bytes().split(b"\0") + except OSError: + continue # process vanished / not ours + if any(a.decode("utf-8", "replace") == cfg for a in args): + try: + os.kill(int(entry.name), signal.SIGKILL) + except (OSError, ValueError): + pass + + def _health_ok(url: str) -> bool: try: with urllib.request.urlopen(f"{url}/health", timeout=1.0) as resp: diff --git a/tests/unit/test_firecracker_infra_vm.py b/tests/unit/test_firecracker_infra_vm.py index ea740d5..0521ecd 100644 --- a/tests/unit/test_firecracker_infra_vm.py +++ b/tests/unit/test_firecracker_infra_vm.py @@ -140,27 +140,58 @@ class TestWaitForHealth(unittest.TestCase): class TestEnsureRunningSingleton(unittest.TestCase): - def test_adopts_when_healthy(self): - # A healthy control plane + existing key -> adopt (no boot), vm=None. - with patch.object(infra_vm, "_health_ok", return_value=True), \ - patch.object(infra_vm, "_infra_dir") as d, \ - patch.object(infra_vm, "boot") as boot: - keydir = MagicMock() - (keydir / "id_ed25519").exists.return_value = True - d.return_value = keydir - infra = infra_vm.ensure_running() + def test_adopts_when_healthy_and_version_matches(self): + # Healthy control plane + existing key + matching version marker + # -> adopt (no boot), vm=None. + import tempfile + with tempfile.TemporaryDirectory() as td: + d = Path(td) + (d / "id_ed25519").write_text("k") + (d / "booted-version").write_text("v-current\n") + with patch.object(infra_vm, "_infra_dir", return_value=d), \ + patch.object(infra_vm, "_expected_version", return_value="v-current"), \ + patch.object(infra_vm, "_health_ok", return_value=True), \ + patch.object(infra_vm, "boot") as boot: + infra = infra_vm.ensure_running() boot.assert_not_called() self.assertIsNone(infra.vm) + def test_reboots_when_version_stale(self): + # Healthy control plane but the running VM booted an OLDER image + # (marker mismatch) -> reboot rather than adopt stale code. + import tempfile + with tempfile.TemporaryDirectory() as td: + d = Path(td) + (d / "id_ed25519").write_text("k") + (d / "booted-version").write_text("v-old\n") + with patch.object(infra_vm, "_infra_dir", return_value=d), \ + patch.object(infra_vm, "_expected_version", return_value="v-current"), \ + patch.object(infra_vm, "_health_ok", return_value=True), \ + patch.object(infra_vm, "stop") as stop, \ + patch.object(infra_vm, "ensure_built"), \ + patch.object(infra_vm, "wait_for_health"), \ + patch.object(infra_vm, "boot") as boot: + boot.return_value = infra_vm.InfraVm( + guest_ip="10.243.255.1", private_key=Path("/k"), vm=MagicMock()) + infra_vm.ensure_running() + stop.assert_called_once() # dislodge the outdated VM + boot.assert_called_once() + # The fresh boot records the current version for the next launcher. + self.assertEqual("v-current\n", (d / "booted-version").read_text()) + def test_boots_when_unhealthy(self): - with patch.object(infra_vm, "_health_ok", return_value=False), \ - patch.object(infra_vm, "stop") as stop, \ - patch.object(infra_vm, "ensure_built") as built, \ - patch.object(infra_vm, "boot") as boot, \ - patch.object(infra_vm, "wait_for_health") as wait: - boot.return_value = infra_vm.InfraVm( - guest_ip="10.243.255.1", private_key=Path("/k"), vm=MagicMock()) - infra_vm.ensure_running() + import tempfile + with tempfile.TemporaryDirectory() as td: + with patch.object(infra_vm, "_infra_dir", return_value=Path(td)), \ + patch.object(infra_vm, "_expected_version", return_value="v-current"), \ + patch.object(infra_vm, "_health_ok", return_value=False), \ + patch.object(infra_vm, "stop") as stop, \ + patch.object(infra_vm, "ensure_built") as built, \ + patch.object(infra_vm, "boot") as boot, \ + patch.object(infra_vm, "wait_for_health") as wait: + boot.return_value = infra_vm.InfraVm( + guest_ip="10.243.255.1", private_key=Path("/k"), vm=MagicMock()) + infra_vm.ensure_running() stop.assert_called_once() # clear a stale VM first built.assert_called_once() boot.assert_called_once() @@ -187,5 +218,74 @@ class TestKillPidfile(unittest.TestCase): kill.assert_not_called() +class TestAdoptable(unittest.TestCase): + def _dir(self, td, *, key=True, version=None): + d = Path(td) + if key: + (d / "id_ed25519").write_text("k") + if version is not None: + (d / "booted-version").write_text(version + "\n") + return d + + def test_true_when_key_version_and_health(self): + import tempfile + with tempfile.TemporaryDirectory() as td: + d = self._dir(td, version="v1") + with patch.object(infra_vm, "_infra_dir", return_value=d), \ + patch.object(infra_vm, "_health_ok", return_value=True): + self.assertTrue(infra_vm._adoptable(d / "id_ed25519", "u", "v1")) + + def test_false_when_key_missing(self): + import tempfile + with tempfile.TemporaryDirectory() as td: + d = self._dir(td, key=False, version="v1") + with patch.object(infra_vm, "_infra_dir", return_value=d): + self.assertFalse(infra_vm._adoptable(d / "id_ed25519", "u", "v1")) + + def test_false_when_no_version_marker(self): + import tempfile + with tempfile.TemporaryDirectory() as td: + d = self._dir(td) # key present, no booted-version + with patch.object(infra_vm, "_infra_dir", return_value=d): + self.assertFalse(infra_vm._adoptable(d / "id_ed25519", "u", "v1")) + + def test_false_when_version_mismatch(self): + import tempfile + with tempfile.TemporaryDirectory() as td: + d = self._dir(td, version="v-old") + with patch.object(infra_vm, "_infra_dir", return_value=d), \ + patch.object(infra_vm, "_health_ok", return_value=True): + self.assertFalse(infra_vm._adoptable(d / "id_ed25519", "u", "v1")) + + +class TestKillInfraFirecrackers(unittest.TestCase): + def _fake_proc(self, root, pid, comm, cmdline): + p = root / str(pid) + p.mkdir() + (p / "comm").write_text(comm + "\n") + (p / "cmdline").write_bytes(b"\0".join(a.encode() for a in cmdline) + b"\0") + + def test_kills_only_matching_infra_firecracker(self): + import tempfile + with tempfile.TemporaryDirectory() as td, \ + tempfile.TemporaryDirectory() as proc: + infra_dir = Path(td) + cfg = str(infra_dir / "config.json") + root = Path(proc) + # target: firecracker bound to the infra config -> killed + self._fake_proc(root, 111, "firecracker", + ["firecracker", "--no-api", "--config-file", cfg]) + # a firecracker for a different (interactive) VM -> spared + self._fake_proc(root, 222, "firecracker", + ["firecracker", "--config-file", "/home/u/other.json"]) + # a non-firecracker process on the same config path -> spared + self._fake_proc(root, 333, "python3", ["python3", cfg]) + (root / "not-a-pid").mkdir() + with patch.object(infra_vm, "_infra_dir", return_value=infra_dir), \ + patch.object(infra_vm.os, "kill") as kill: + infra_vm._kill_infra_firecrackers(proc_root=root) + kill.assert_called_once_with(111, infra_vm.signal.SIGKILL) + + if __name__ == "__main__": unittest.main()