From f0d856b644d16a6f94501347f4439f7b5b3fa839 Mon Sep 17 00:00:00 2001 From: Simone Date: Thu, 23 Jul 2026 17:15:23 +0200 Subject: [PATCH] =?UTF-8?q?fix(ip-pool):=20reconcile=20stale=20leases=20ag?= =?UTF-8?q?ainst=20live=20clones=20(=C2=A72.8,=20W1)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A job killed mid-flight never released its IP lease; with only 4 pool addresses a single leak exhausted the pool ("IP pool exhausted after 60s"). IpSlotPool.reconcile() releases leases whose owner clone-dir is gone; vm cleanup runs it only against the real build_vms base. Closes TODO §2.8. --- TODO.md | 12 +++- plans/web-ui-management-plan.md | 4 +- src/ci_orchestrator/commands/vm.py | 19 +++++++ src/ci_orchestrator/ip_pool.py | 26 +++++++++ src/ci_orchestrator/operations/vms.py | 23 ++++++++ tests/python/test_commands_vm.py | 79 +++++++++++++++++++++++++++ tests/python/test_ip_pool.py | 42 ++++++++++++++ tests/python/test_operations_vms.py | 61 +++++++++++++++++++++ 8 files changed, 263 insertions(+), 3 deletions(-) diff --git a/TODO.md b/TODO.md index 2d68587..db4c31f 100644 --- a/TODO.md +++ b/TODO.md @@ -299,7 +299,7 @@ Cooldown: max 3 restart/h via JSON state file `F:\CI\State\runner-restart-log.js Oltre il limite: EventId 1004 (Error) + webhook `:sos:` senza restart. Webhook opzionale Discord/Gitea identico a `Watch-DiskSpace.ps1`. -### 2.8 [P2] [ ] IP-pool auto-riconciliazione vs clone reali — APERTO (2026-06-07) +### 2.8 [P2] [x] IP-pool auto-riconciliazione vs clone reali — CHIUSO (2026-07-23) `ip-pool.json` non viene riconciliato contro i clone vivi all'avvio. Un job killato mid-flight (SIGKILL/crash) **leaka il lease in modo permanente**; con @@ -312,6 +312,16 @@ mai rilasciati. Workaround manuale: reset a `{}` con `build-vms/` vuoto. cui clone-dir non esiste più in `CloneBaseDir`. Opzionale: allargare il pool (es. 201–210) per dare margine oltre parallelism. Aggiungere test. +**Risolto 2026-07-23 (W1 del web-UI plan)**: `IpSlotPool.reconcile(live_owners)` +rilascia i lease il cui owner (== nome clone-dir) non è più fra le sotto-dir di +`CloneBaseDir`; `operations.vms.reconcile_ip_leases(clone_base, pool)` calcola i +`live_owners` e delega. `vm cleanup` lo invoca **solo** quando opera sul clone +base reale (`base_path == config.paths.build_vms`) — un `--clone-base-dir` +custom NON riconcilia (i lease appartengono al build-vms reale). Supporta +`--what-if` (dry-run, nessuna scrittura). Test: `test_ip_pool.py` (reconcile), +`test_operations_vms.py` (reconcile_ip_leases), `test_commands_vm.py` (CLI, real +vs custom base). Allargamento del pool oltre 201–204: non fatto (opzionale). + ### 2.9 [P2] [ ] `retention run` non copre backup/template/ISO — APERTO (2026-07-22) `ci-retention-policy` agisce solo su **artifact, log e lease IP**. Su questo host diff --git a/plans/web-ui-management-plan.md b/plans/web-ui-management-plan.md index 75550ef..d7f35a0 100644 --- a/plans/web-ui-management-plan.md +++ b/plans/web-ui-management-plan.md @@ -18,8 +18,8 @@ Stato passi (spunta man mano): -- [ ] **W0** — Scaffolding: extra `[web]` in `pyproject.toml`, dir package `operations/` + `web/`, skeleton test. Nessuna logica. -- [ ] **W1** — Layer `operations/vms.py`: estrai la logica da `commands/vm.py` in funzioni tipate che ritornano dataclass. Refactor dei comandi CLI per chiamarle. **Comportamento CLI invariato.** +- [x] **W0** — Scaffolding: extra `[web]` in `pyproject.toml`, dir package `operations/` + `web/`, skeleton test. Nessuna logica. +- [x] **W1** — Layer `operations/vms.py`: estrai la logica da `commands/vm.py` in funzioni tipate che ritornano dataclass. Refactor dei comandi CLI per chiamarle. **Comportamento CLI invariato.** (Incluso: seam `VmBackend.list_running()` + fold del fix §2.8 IP-pool leak via `IpSlotPool.reconcile` / `operations.vms.reconcile_ip_leases`.) - [ ] **W2** — Layer `operations/cache.py` + `operations/host.py`: stato cache cargo, purge, disco, runner, backup template. Refactor `retention.py` / `monitor.py`. - [ ] **W3** — `operations/audit.py`: append-only JSONL + lettura. Config `[web]` in `config.py`. - [ ] **W4** — App web core: `web/app.py` factory + `commands/web.py` (`ci web serve`). Dashboard **read-only** (VM, cache, disco, runner). Ancora **senza auth**, bind localhost. diff --git a/src/ci_orchestrator/commands/vm.py b/src/ci_orchestrator/commands/vm.py index e8913a8..127a81f 100644 --- a/src/ci_orchestrator/commands/vm.py +++ b/src/ci_orchestrator/commands/vm.py @@ -27,6 +27,7 @@ from ci_orchestrator.backends import load_backend from ci_orchestrator.backends.errors import BackendError, BackendNotAvailable from ci_orchestrator.backends.protocol import VmHandle from ci_orchestrator.config import load_config +from ci_orchestrator.ip_pool import IpSlotPool from ci_orchestrator.operations import vms as ops_vms if TYPE_CHECKING: # pragma: no cover @@ -307,6 +308,24 @@ def vm_cleanup( f"[vm cleanup] removed stale lock file (age: {lock_age_min} min): {lock_path}" ) + # Reconcile the IP pool against surviving clones (TODO §2.8). Only when + # cleaning the *real* clone base — the lease owners are clone names created + # under config.paths.build_vms, so reconciling a custom --clone-base-dir + # would wrongly release every live lease. + if config.ip_pool is not None and base_path.resolve() == config.paths.build_vms.resolve(): + pool = IpSlotPool( + addresses=config.ip_pool.addresses, + netmask=config.ip_pool.netmask, + gateway=config.ip_pool.gateway, + lease_file=config.ip_pool.lease_file, + ) + released = ops_vms.reconcile_ip_leases(base_path, pool, dry_run=what_if) + if released: + verb = "would release" if what_if else "released" + click.echo( + f"[vm cleanup] {verb} {len(released)} stale IP lease(s): {', '.join(released)}" + ) + click.echo("[vm cleanup] done.") diff --git a/src/ci_orchestrator/ip_pool.py b/src/ci_orchestrator/ip_pool.py index fb3b99a..63a5d72 100644 --- a/src/ci_orchestrator/ip_pool.py +++ b/src/ci_orchestrator/ip_pool.py @@ -93,6 +93,32 @@ class IpSlotPool: leases[ip] = None self._write_leases(leases) + def reconcile(self, live_owners: set[str], *, dry_run: bool = False) -> list[str]: + """Release leases whose owner is not among *live_owners*. + + Owners are clone names — identical to the clone directory names under + ``CloneBaseDir`` (``acquire`` is called with the clone name). A lease + whose owner clone no longer exists on disk is a permanent leak: a job + killed mid-flight (SIGKILL/crash) never calls :meth:`release`, and with + only four pool addresses a single leak exhausts the pool (TODO §2.8). + + Returns the IPs released — or, when *dry_run* is set, the IPs that + *would* be released (no write performed). + """ + with _file_lock(self._lock_file): + leases = self._read_leases() + stale = [ + ip + for ip, owner in leases.items() + if owner is not None and owner not in live_owners + ] + if not stale or dry_run: + return stale + for ip in stale: + leases[ip] = None + self._write_leases(leases) + return stale + # ── internals ────────────────────────────────────────────────────────── def _read_leases(self) -> dict[str, str | None]: diff --git a/src/ci_orchestrator/operations/vms.py b/src/ci_orchestrator/operations/vms.py index d31d4ea..db1c392 100644 --- a/src/ci_orchestrator/operations/vms.py +++ b/src/ci_orchestrator/operations/vms.py @@ -21,6 +21,7 @@ from ci_orchestrator.backends.protocol import VmHandle if TYPE_CHECKING: # pragma: no cover from ci_orchestrator.backends.protocol import VmBackend from ci_orchestrator.config import Config + from ci_orchestrator.ip_pool import IpSlotPool # Matches the default ``vm cleanup --max-age-hours`` threshold; used to flag # a live VM as an orphan candidate in ``list_live_vms``. @@ -302,6 +303,27 @@ def cleanup_orphans( return results +def reconcile_ip_leases( + clone_base: Path, pool: IpSlotPool, *, dry_run: bool = False +) -> list[str]: + """Release IP-pool leases whose owner clone directory no longer exists (TODO §2.8). + + ``live_owners`` is the set of sub-directory names directly under + ``clone_base`` (each clone dir name equals the lease owner recorded at + ``acquire`` time). Any lease pointing at a clone that is gone is released, + closing the permanent-leak hole left by a job killed mid-flight. + + Returns the IPs released (or, with *dry_run*, those that would be). If + ``clone_base`` cannot be enumerated, returns ``[]`` without touching the + lease file — we must not release leases we cannot prove are stale. + """ + try: + live_owners = {entry.name for entry in clone_base.iterdir() if entry.is_dir()} + except OSError: + return [] + return pool.reconcile(live_owners, dry_run=dry_run) + + __all__ = [ "LiveVm", "Orphan", @@ -309,5 +331,6 @@ __all__ = [ "cleanup_orphans", "list_live_vms", "list_orphans", + "reconcile_ip_leases", "remove_vm", ] diff --git a/tests/python/test_commands_vm.py b/tests/python/test_commands_vm.py index 4bd5197..769e920 100644 --- a/tests/python/test_commands_vm.py +++ b/tests/python/test_commands_vm.py @@ -395,6 +395,85 @@ def test_vm_cleanup_backend_unavailable(monkeypatch: pytest.MonkeyPatch, tmp_pat assert "vmrun not available" in result.output +def _config_with_pool(base: Path, lease_file: Path) -> Any: + from ci_orchestrator.config import BackendConfig, Config, IpPoolConfig, Paths + + return Config( + paths=Paths( + root=base.parent, + templates=base.parent / "templates", + build_vms=base, + artifacts=base.parent / "artifacts", + keys=base.parent / "keys", + ), + backend=BackendConfig(), + ip_pool=IpPoolConfig( + addresses=["10.0.0.1", "10.0.0.2"], + netmask="255.255.255.0", + gateway="10.0.0.254", + lease_file=lease_file, + ), + ) + + +def test_vm_cleanup_reconciles_ip_pool_on_real_base( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """§2.8: cleaning the real clone base releases leases of vanished clones.""" + from ci_orchestrator.ip_pool import IpSlotPool + + base = tmp_path / "build-vms" + base.mkdir() + (base / "Clone_live").mkdir() # still on disk + + lease = tmp_path / "ip-pool.json" + pool = IpSlotPool(["10.0.0.1", "10.0.0.2"], "255.255.255.0", "10.0.0.254", lease) + pool.acquire("Clone_live") # 10.0.0.1 — kept + pool.acquire("Clone_dead") # 10.0.0.2 — dir gone, must be released + + monkeypatch.setattr(vm_module, "_make_backend", lambda _v: None) + monkeypatch.setattr(vm_module, "load_config", lambda: _config_with_pool(base, lease)) + + result = CliRunner().invoke(cli, ["vm", "cleanup", "--clone-base-dir", str(base)]) + assert result.exit_code == 0, result.output + assert "released 1 stale IP lease" in result.output + assert "10.0.0.2" in result.output + + import json + + leases = json.loads(lease.read_text()) + assert leases["10.0.0.1"] == "Clone_live" + assert leases["10.0.0.2"] is None + + +def test_vm_cleanup_skips_reconcile_on_custom_base( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """A custom --clone-base-dir must NOT reconcile the real pool (leases stay).""" + from ci_orchestrator.ip_pool import IpSlotPool + + real_base = tmp_path / "build-vms" + real_base.mkdir() + custom = tmp_path / "elsewhere" + custom.mkdir() + + lease = tmp_path / "ip-pool.json" + pool = IpSlotPool(["10.0.0.1"], "255.255.255.0", "10.0.0.254", lease) + pool.acquire("Clone_dead") # would be released if the guard misfired + + monkeypatch.setattr(vm_module, "_make_backend", lambda _v: None) + monkeypatch.setattr(vm_module, "load_config", lambda: _config_with_pool(real_base, lease)) + + result = CliRunner().invoke(cli, ["vm", "cleanup", "--clone-base-dir", str(custom)]) + assert result.exit_code == 0, result.output + assert "stale IP lease" not in result.output + + import json + + leases = json.loads(lease.read_text()) + assert leases["10.0.0.1"] == "Clone_dead" # untouched + + # ── vmware ─────────────────────────────────────────────────────────────────── diff --git a/tests/python/test_ip_pool.py b/tests/python/test_ip_pool.py index 5e918ce..c953f28 100644 --- a/tests/python/test_ip_pool.py +++ b/tests/python/test_ip_pool.py @@ -115,3 +115,45 @@ def test_non_dict_lease_json_treated_as_empty(tmp_path: Path) -> None: pool = make_pool(tmp_path) ip = pool.acquire("owner-1") assert ip == "10.0.0.1" + + +# ── reconcile (TODO §2.8) ───────────────────────────────────────────────────── + + +def test_reconcile_releases_leases_with_dead_owners(tmp_path: Path) -> None: + pool = make_pool(tmp_path) + pool.acquire("clone-alive") # 10.0.0.1 + pool.acquire("clone-dead") # 10.0.0.2 + + released = pool.reconcile({"clone-alive"}) + + assert released == ["10.0.0.2"] + leases = json.loads((tmp_path / "ip-pool.json").read_text()) + assert leases["10.0.0.1"] == "clone-alive" # kept + assert leases["10.0.0.2"] is None # released + + +def test_reconcile_keeps_all_when_all_owners_live(tmp_path: Path) -> None: + pool = make_pool(tmp_path) + pool.acquire("clone-a") + pool.acquire("clone-b") + assert pool.reconcile({"clone-a", "clone-b"}) == [] + leases = json.loads((tmp_path / "ip-pool.json").read_text()) + assert leases["10.0.0.1"] == "clone-a" + assert leases["10.0.0.2"] == "clone-b" + + +def test_reconcile_dry_run_reports_without_writing(tmp_path: Path) -> None: + pool = make_pool(tmp_path) + pool.acquire("clone-dead") # 10.0.0.1 + + released = pool.reconcile(set(), dry_run=True) + + assert released == ["10.0.0.1"] + leases = json.loads((tmp_path / "ip-pool.json").read_text()) + assert leases["10.0.0.1"] == "clone-dead" # untouched + + +def test_reconcile_empty_pool_returns_empty(tmp_path: Path) -> None: + pool = make_pool(tmp_path) # no acquires → all leases None + assert pool.reconcile(set()) == [] diff --git a/tests/python/test_operations_vms.py b/tests/python/test_operations_vms.py index 1a3725a..95bfc6a 100644 --- a/tests/python/test_operations_vms.py +++ b/tests/python/test_operations_vms.py @@ -445,3 +445,64 @@ def test_remove_vm_dir_removal_failure(monkeypatch: pytest.MonkeyPatch, tmp_path monkeypatch.setattr(vms_module, "_try_remove_dir", lambda _p: False) result = remove_vm(backend, vmx) # type: ignore[arg-type] assert result.dir_removed is False + + +# ── reconcile_ip_leases (TODO §2.8) ─────────────────────────────────────────── + + +def _make_pool(tmp_path: Path) -> Any: + from ci_orchestrator.ip_pool import IpSlotPool + + return IpSlotPool( + addresses=["10.0.0.1", "10.0.0.2"], + netmask="255.255.255.0", + gateway="10.0.0.254", + lease_file=tmp_path / "ip-pool.json", + ) + + +def test_reconcile_ip_leases_releases_dead_clone(tmp_path: Path) -> None: + from ci_orchestrator.operations.vms import reconcile_ip_leases + + base = tmp_path / "build-vms" + base.mkdir() + (base / "clone-alive").mkdir() # survives on disk + pool = _make_pool(tmp_path) + pool.acquire("clone-alive") # 10.0.0.1 + pool.acquire("clone-dead") # 10.0.0.2 — no dir on disk + + released = reconcile_ip_leases(base, pool) + + assert released == ["10.0.0.2"] + + +def test_reconcile_ip_leases_dry_run_no_write(tmp_path: Path) -> None: + from ci_orchestrator.operations.vms import reconcile_ip_leases + + base = tmp_path / "build-vms" + base.mkdir() + pool = _make_pool(tmp_path) + pool.acquire("clone-dead") # 10.0.0.1, dir absent + + released = reconcile_ip_leases(base, pool, dry_run=True) + + assert released == ["10.0.0.1"] + import json + + leases = json.loads((tmp_path / "ip-pool.json").read_text()) + assert leases["10.0.0.1"] == "clone-dead" # not written + + +def test_reconcile_ip_leases_unreadable_base_is_noop(tmp_path: Path) -> None: + from ci_orchestrator.operations.vms import reconcile_ip_leases + + missing = tmp_path / "does-not-exist" + pool = _make_pool(tmp_path) + pool.acquire("clone-dead") + + # clone_base cannot be enumerated → must NOT release anything. + assert reconcile_ip_leases(missing, pool) == [] + import json + + leases = json.loads((tmp_path / "ip-pool.json").read_text()) + assert leases["10.0.0.1"] == "clone-dead"