feat(agent): disk reclamation, retire_vm and volume retention (2.1, PR A) - #1155
feat(agent): disk reclamation, retire_vm and volume retention (2.1, PR A)#1155odesenfans wants to merge 35 commits into
Conversation
foxpatch-aleph
left a comment
foxpatch-aleph
left a comment
There was a problem hiding this comment.
A well-architected disk reclamation system with a unified retirement function, filesystem-as-truth markers, and a carefully reasoned reconciler. The TOCTOU protections (registry_is_live, creating() guard, mtime guard), startup refusal logic, and pool-scoped eviction accounting are all correct and thoroughly tested. Security is solid: erase authorization covers all states (registry record, DB record, marker owner, or 403), path traversal is defended against, and operator data under /mnt is protected. The only issue found is a minor inconsistency where _sweep_side_dirs does not use the is_live re-check function unlike the namespace pass, creating a small TOCTOU window for ephemeral side directories — low impact since they are rebuilt by the next create. Code quality and documentation are exceptional, with every design decision explained in terms of why, not just what. Test coverage is comprehensive across all new modules.
Minor inconsistency: _sweep_side_dirs receives the snapshot live set but not the is_live re-check function, unlike _reconcile_namespaces (line 269). A create that commits while _sweep_side_dirs is walking could have its side directories removed despite being live. This is low-impact (side dirs are ephemeral and rebuilt by the next create, and the creating() guard still applies) but inconsistent with the namespace pass. Consider passing is_live to _sweep_side_dirs for the same TOCTOU protection.
Nit: _creating is a module-level set. The tests manage this via context managers, but a ContextVar (as used in storage_pools._pinned_layouts) would be more robust against cross-test leakage and would be consistent with the existing pattern in this codebase.
c950544 to
29ada54
Compare
29ada54 to
5da6116
Compare
foxpatch-aleph
left a comment
foxpatch-aleph
left a comment
There was a problem hiding this comment.
Well-designed and carefully documented overall: the single retire path, the marker system, and the reconciler are coherent, and the live-set/creating-guard/mtime-guard layering shows real thought. However, two races remain that the PR's own guards exist to prevent: _evict() ignores the in-process creating() guard, so a retained VM re-created between the eviction listing and the eviction itself has its disks purged mid-create, and the in-place reinstall rebuild runs outside creating() so its .part downloads are exposed to the sweep. Both are small, targeted fixes.
src/aleph/vm/agent/vm/reconciler.py (line 514): _evict() checks is_live but never is_creating(namespace), even though the module's other two removal guards do: _is_orphan (line 291) and _is_stale_side_dir (line 412). Race: _reclaimable_on() lists retained VM X with its marker; the owner then re-creates X, whose creating() entry calls adopt() and clears the marker while X is still absent from the registry (the record is committed only at the end of the create), so is_live(X) is False; the evictor then reaches the stale listing, _still_on_disk(X) is True (adopt clears the marker, not the directory), and purge_vm_storage() deletes the volumes the create is actively writing. The window is the whole gap between listing and eviction, which in _enforce_retention_budget / make_room loops over multi-GB directories can be minutes. The creating() docstring (line 107) identifies exactly this race as the reason adoption happens on entry; _evict needs the same one-line check next to the is_live one.
src/aleph/vm/agent/views/operator.py (line 774): This rebuild is a create path but does not run under creating(), even though the creating() docstring says every create path must be wrapped. _part_roots (reconciler.py line 368) skips only namespaces that are creating, not live ones, so a .part file from this transfer whose mtime stalls longer than VOLUME_CREATE_GUARD (600 s) gets swept mid-transfer; the registry record does not help because the part sweep never consults it. A slow multi-GB rootfs download with one long write gap qualifies. Wrap the recreate_vm_volumes call in creating(str(vm_hash)) (adopt() on entry is a harmless no-op here) or make _part_roots skip live namespaces too.
src/aleph/vm/agent/vm/reconciler.py (line 555): total is decremented before _evict() has had a chance to decline the eviction, so an entry skipped because it is live, or because its purge was refused (dm-held volumes), still reduces total, and the loop can break while those bytes remain on disk. For the dm-held case the marker survives across passes, so the under-enforcement is stable rather than transient: every pass recomputes the same total, decrements the same un-evictable entry, and stops early. Move the decrement to where the eviction is actually counted.
src/aleph/vm/agent/expiry.py (line 54): Question, not a blocker: an idle-reaped on-demand program now keeps its host-port forwards indefinitely. For a program never requested again, what releases those mappings? If nothing sweeps them, the REUSE_TIMEOUT path trades a disk leak for a port-forward leak; if they are reclaimed elsewhere, a comment saying so would help.
5da6116 to
a9f68bb
Compare
|
Re: the 2026-09-02 review. All three findings were valid; fixes are on the branch (which is also rebased onto the current dev-2.1, 0d85bf4). _evict() ignores creating() — fixed in a9f68bb~3 ( Budget decremented before _evict can decline — fixed in Reinstall rebuild outside creating() — fixed in Port forwards of an idle-reaped program — intentional, and now said inline (expiry.py). RECREATE keeps the forwards so the program re-woken by the next request comes back on the same ports; the reservation lasts as long as the program does. When the program actually goes away (scheduler GONE, owner erase), retire_vm deletes the records, port forwards included. A program never requested again but still paying keeps its allocation on purpose. |
foxpatch-aleph
left a comment
foxpatch-aleph
left a comment
There was a problem hiding this comment.
Click Re-request review to retry.
…ason Every path that used to call supervisor.delete_vm and then some subset of registry.forget, delete_records_for_vm and remove_*_staging now goes through retire_vm with an explicit RetireReason (RECREATE, GONE, ERASE, FAILED_CREATE). RECREATE only quiesces the VM; the rest drop the registry record and DB records, purge the session and staging directories, and purge backups. GONE respects VOLUME_RETENTION=keep by marking the volumes reclaimable instead of deleting them; ERASE and FAILED_CREATE always purge. VmNotFoundError from delete_vm is swallowed so a double retire or a create that never committed is still a no-op cleanup. Splits purge_vm_side_dirs (session + staging) out of purge_vm_storage so the keep path can reuse it, and adds purge_vm_backups and sweep_expired_backups to backup.py as the backup module's side of storage reclamation.
check_payment used to delete_vm and hand-roll registry.forget plus delete_records_for_vm plus staging cleanup on its own. Route every stop through retire_vm(GONE): the terminal-status dealloc and the three payment-insufficiency sites (hold, credit, superfluid tiers).
…as GONE The allocation stop-loop and the migration cleanup endpoint each hand-rolled delete_vm plus registry.forget plus delete_records_for_vm plus staging cleanup. Route both through retire_vm(GONE): the scheduler saying a VM is gone, or a migration source that has handed off to its destination, both mean the record and the disks follow VOLUME_RETENTION.
…s RECREATE run.py had eleven delete_vm call sites, each hand-rolling its own subset of registry.forget, staging cleanup and keep_port_mappings. Split them by intent: a create that never committed (program, instance, v-program build or readiness failures, program-VM setup failure) retires FAILED_CREATE; a teardown where the same VM comes right back (program-VM recreate, empty-result and REUSE_TIMEOUT==0 teardowns, crash recovery) retires RECREATE, which keeps the port forwards and disks.
…ough retire_vm Idle-teardown, the update-triggered reap, the ephemeral stop/reboot cycle, erase and reinstall each called delete_vm with their own subset of registry.forget, DB cleanup and storage purge. Convert them to retire_vm: RECREATE for every delete+recreate cycle (idle reap, update reap, ephemeral stop/reboot, reinstall), ERASE for the owner-requested wipe. Add tests/supervisor/test_agent_no_direct_delete.py, a guard test that fails if any file under src/aleph/vm/agent/ other than retire.py calls supervisor.delete_vm directly.
A refused admission (InsufficientResourcesError from _admit) now retires the never-created VM as FAILED_CREATE, which calls delete_records_for_vm. These tests only check admission ordering and never set up an app-level DB session, so stub that call out; matches the fix already applied to test_supervisor_run_routing.py::test_firecracker_instance_rejected_via_spec_path.
…, not FAILED_CREATE run.py's create-path failure sites are also the re-create path for a host-persistent VM whose volumes already exist (downloader._make_writable_volume returns early when the destination is already there). Before this fix, a transient failure there (boot timeout, admission refusal on node restart, a base-image download error) retired FAILED_CREATE and purged the owner's existing rootfs and data volumes along with the DB record. Add purge.vm_has_volumes(vm_hash), snapshotted before any allocation at each of the six FAILED_CREATE call sites in run.py, and a small _retire_after_create_failure helper that picks RECREATE when the volumes already existed and FAILED_CREATE otherwise, always wrapped so a teardown error never masks the original failure (two of the six sites used to call retire_vm bare). RECREATE keeps the record and the disks; start_persistent_vm and update_allocations already re-create through the existing "supervisor doesn't know it, registry does" fallback, the same path the crash-recovery FAILED branch already exercises. Also convert the two test_snp_instance_run.py tests that patched the now-gone remove_snp_instance_staging attribute (could not run locally: this venv's aleph_message lacks SevSnpRegisters, same root cause as the pydantic environment failures elsewhere in this stack) and the two test_run_program_path.py tests that only asserted supervisor.delete_vm was awaited without pinning a reason.
…ention budget) One pass walks the pools and the per-VM side directories and applies VOLUME_RETENTION to everything no live VM owns: orphan namespace directories are purged under reap and marked reason=orphan under keep, stale .part downloads and side directories (session, staging, mount points) go, then the per-pool retention budget evicts reclaimable directories oldest first. creating() registers an in-flight create and adopts its retained directories on entry; make_room() is the admission pressure entry point.
…reate guard Review round 1: - make_room evicts until the pool's own free space covers the create, not until needed_bytes have been freed: a pool with 90G free and a 100G create was giving back 100G of retained user data. The freed-bytes bound stays as a terminating guard for an unreadable filesystem, and only the bytes the evicted VM held on that pool are counted, since the other pools' bytes do not help this create. - make_room takes an optional live set and never evicts a hash in it. A marker on a live directory is a bug the reconcile pass clears, but make_room runs on its own under admission pressure. - The side-directory sweep applies the same VOLUME_CREATE_GUARD age check as the namespace pass, so a create that has staged its bundle but has not reached the registry keeps it. The module docstring said this was already true; now it is, and creating() states that wrapping every create path is mandatory. - reconcile_now snapshots the live hashes on the event loop before handing the pass to a worker thread, instead of iterating the registry from the thread while the loop mutates it.
…and on placement pressure The reconciler, the create guard and the retention budget existed but nothing called them. Wire them into the app: - every create path (the three branches of create_vm_execution plus _ensure_program_vm) runs inside creating(), which adopts the retained volumes for that hash and keeps a concurrent pass off a half-built VM, - reconcile_at_startup runs after registry rehydration (a pass against an empty registry would call every running VM an orphan), followed by the periodic task, cancelled on cleanup, - a GONE under VOLUME_RETENTION=keep enforces the budget right away through retire.set_after_gone_hook, instead of waiting up to VOLUME_RECONCILE_INTERVAL, - placement asks storage_pools' room maker (wired to make_room, with the live set snapshotted so a running VM can never be evicted) before refusing a create, and admission counts reclaimable bytes as free: retention is a budgeted cache, so a retained disk must not shrink the capacity the node sells. The startup summary is logged from a dry run first, per pool, so an operator can explain why free space jumped after an upgrade.
… serialize passes
Review round 1 on the reconciler wiring.
The migration import was the hole: it streams multi-GB disks into
{pool}/{vm_hash}/ with no registry record and no create guard, and a
directory's mtime does not advance while a file inside it grows, so once
the transfer outlived VOLUME_CREATE_GUARD the reaper would take the
staging directory and its .part files. Worse, the import never recorded
the VM in the agent registry at all, so even a completed import left
disks nothing owned. run_import now holds creating() across the whole
body and records (and persists) the migrated VM once CreateVm returns;
a namespace under the guard is skipped whole by the .part sweep too.
"Free plus reclaimable" now applies to all three disk figures, which have
to agree: the aggregate, the largest-single-volume check (each pool's own
reclaimable bytes added to its free space) and the capacity the node
advertises. Advertising less than admission accepts just stops the
scheduler from sending work.
Also: the after-GONE hook is best effort, since the GONE sites sweep in a
loop with no local try; loop-triggered passes serialize on a lock, and
every removal tolerates a directory another pass took first (make_room
still runs concurrently, in a thread); and the four placement calls that
can trigger the evictor now run through asyncio.to_thread, so the walk
and the removals stay off the event loop.
The side-directory sweep matched any /mnt entry whose name prefix looked like an item hash and rmtree'd it, so an operator directory such as /mnt/externalstoragedisk_1 was one stale mtime away from being deleted with its contents. In prefix mode the only thing the agent ever puts under /mnt is a mount point: create_devmapper mkdirs it and unmounts after the resize, so an unmounted agent mount point is empty by construction. The sweep now skips a non-empty entry (debug log) and removes the empty ones with os.rmdir, which cannot take a directory that holds anything.
…runs The reconciler judged a directory an orphan against the agent registry alone. The registry is refilled at boot from the agent DB only, and that rehydration skips any record with an empty or unparseable message, so a fresh, lost or damaged DB meant the startup pass purged the disks of VMs the supervisor was still running. Every pass now acts on the union of the registry's hashes and the hashes the supervisor lists (mapped the way update_allocations maps them), built on the event loop before the walk goes to a worker thread. The startup pass is stricter still: it refuses to purge anything (runs dry, logs why) when list_vms did not answer, or when rehydration left the registry empty while the supervisor runs at least one VM. Liveness is also re-checked immediately before a namespace is marked, purged or evicted, through an is_live callable that asks the registry again: the snapshot is taken on the loop and the walk that follows can take minutes, so a create committing in between would otherwise look like an orphan (a directory's mtime does not move while a file inside it grows). The module docstring records that loop-triggered passes are serialized and not coalesced, and creating() records that adopt-on-entry resets a retained directory's eviction order when the create is refused.
_reconcile_namespaces counted a directory as purged before purge_vm_storage had run, so a directory the purge refuses (a device-mapper target still holds its volumes) was reported as freed space that is still on disk. The count and the purged list now follow the purge, and only when the directories are actually gone. A stale reclaimable marker found on a live VM's directory is a bug signal (the VM was one budget pass away from losing its disks), so clearing it logs at warning instead of debug.
sweep_expired_backups stat'd and unlinked archives bare, so an archive another sweep (or purge_vm_backups retiring a VM) removed between the listing and the unlink raised and took the rest of the pass with it. It also duplicated cleanup_expired_backups, which had the same weakness. The TTL logic now lives once, in archive.cleanup_expired_backups, which takes the timestamp to evaluate against (the reconciler threads one 'now' through a whole pass) and handles each file on its own. The agent's sweep is a call into it. purge_vm_backups interpolated an unvalidated vm_hash into a glob; it now goes through purge._checked_namespace like every other delete primitive.
A create that fails against volumes that already existed retires RECREATE and keeps the registry record on purpose (dropping it while keeping the disks would make the reconciler purge them as an orphan). The record then describes a VM that is not running and counts in CapacityManager's committed sums until the next allocation cycle. Both ends of that state are now documented, and a test pins the property that matters: the retry of the same create is still admitted, because _admit excludes the VM's own record. The record lifecycle itself is unchanged.
operate_erase asked the supervisor whether it knew the VM and 404'd when it did not. The supervisor forgets a VM on restart or after a delete while its disks stay on the node, and under VOLUME_RETENTION=keep a retired VM's volumes survive behind a .reclaimable marker with no registry record at all: in both cases the owner was told there was nothing there, with no way to have their data removed. The endpoint now answers whenever the node still holds something for the hash (a registry record, or a retained directory found through the new reclaimable.is_reclaimable), and 404s only when neither does. retire_vm already tolerates a supervisor that does not know the VM. Ownership is still proven against the message: the registry's record, or the agent DB's (the helper the logs endpoints use, renamed _owner_auth_message now that it serves both). A retained directory whose message the node no longer holds is refused with a 403 rather than wiped on request; that case belongs to the operator's storage CLI.
Bring the storage and VM-lifecycle documents in line with the reconciler: the invariant is now that it never removes a directory whose hash is live in the agent registry or listed by the supervisor, only empty mount-point directories are removed under /mnt, the startup pass refuses to purge when the live set cannot be trusted, and passes are serialized rather than coalesced. Also record that an erase is answered from a retained directory when the registry no longer knows the VM.
An erase of retained data resolved to 403 in exactly the state a GONE under keep produces: the reason had no message to authorize against, because retire_vm forgets the registry record and calls delete_records_for_vm for every non-RECREATE reason, and the marker carried nothing about the owner. ReclaimableMarker gains an optional 'owner' field (version stays 1; from_json tolerates its absence, so markers already on disk still parse). retire_vm(GONE) under keep copies the message's address into it, which is the last moment the node knows it. operate_erase authorizes a marker-only VM against that address, through the owner half of is_sender_authorized, split out as is_owner_address so there is one rule and no second signature check (the signature is already resolved to an address by require_jwk_authentication). Delegation needs the message and is not available in that branch. A marker with no owner still refuses with 403 and says why; 404 stays reserved for a hash neither the registry, the DB nor a marker knows.
The startup hook ran its preview non-strict and only then decided to refuse, so an operator could read 'will purge 3 orphans' immediately above 'purging nothing', and the two passes asked the supervisor for its VM list twice. reconcile_at_startup now takes the lock once, establishes the live set once, computes the refusal once, and passes it to the preview log, which states the same figures as what a trusted pass would have reclaimed. The real pass runs only when there is no refusal, so reconcile_now loses its strict flag.
_evict counted a directory as evicted before purge_vm_storage ran, the same pattern already fixed in the namespace pass: a directory a device-mapper target still holds was reported as bytes freed, and make_room would then tell placement it had made room it had not. The count now follows the purge and only when the directories are gone.
…emon The Python supervisor daemon removal deleted the four test files this stack was modifying: tests/supervisor/test_views.py, test_storage_pools.py, views/test_operator.py and views/test_migration.py. None of the reclamation cases in them needed the daemon: they build the app with setup_webapp and then replace app["supervisor"] with a mock of the Supervisor interface, so they are re-homed here rather than resurrected in place. - views/test_operator_retire.py: erase, stop, reboot and reinstall going through retire_vm, plus the erase of data the supervisor has forgotten (a retained .reclaimable directory, owner proven from the marker or the agent DB). - views/test_allocations_retire.py: the allocation stop loop retiring a deallocated VM as GONE. - views/test_migration_cleanup_retire.py: migration cleanup retiring the migrated-away source as GONE. - test_storage_pools_room_maker.py: placement asking the agent's evictor before refusing a create. setup_webapp(supervisor=LocalSupervisor(pool)) becomes a Supervisor double in every ported case; nothing else about the assertions changed.
_evict checked is_live but not is_creating, unlike the module's other two removal guards (_is_orphan, _is_stale_side_dir). A retained VM re-created between the eviction listing and the eviction itself has its marker cleared by adopt() on creating() entry, but its registry record only lands when the create commits, so is_live could not catch it and the budget pass would purge the disks the create was writing. The window is the whole gap between listing and eviction, minutes when the loop purges multi-GB directories. Found by review on #1155.
The reconciler's part sweep spares only namespaces inside creating(); the registry record the in-place reinstall keeps does not shield its downloads. A rebuild whose .part mtime stalled longer than VOLUME_CREATE_GUARD could be swept mid-transfer. Every create path must run under the guard; this one did not. Found by review on #1155.
…he disk _enforce_retention_budget decremented total before _evict could decline, so a declined eviction (a live re-create, a dm-held purge refusal) still reduced the excess and the loop could break with those bytes still on disk. The dm-held case made this stable, not transient: the marker survives, every pass recomputes the same total, spends the same un-evictable entry and stops early, forever over budget. Decrement only when the bytes are actually gone: a successful (or dry-run) eviction, or a directory that vanished under an overlapping pass. Younger entries now make up for a held one. Found by review on #1155.
Review question on #1155: an idle-reaped on-demand program keeps its host-port forwards, so what releases them for a program never requested again? Answer inline: the reservation is deliberate (stable ports across idle cycles) and lasts as long as the program does; retire_vm deletes the records, port forwards included, on GONE or owner erase.
…pend mypy (func-returns-value) rejects using list.append's return inside an or-expression; small defs say the same thing without the trick.
a9f68bb to
7c09706
Compare
On Python 3.12 Path.exists() goes through Path.stat, so the vanishing- backup test's stat patch recursed into itself and blew the stack (3.14 routes exists() through os.path, which is why it passed locally). unlink(missing_ok=True) needs no existence probe.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev-2.1 #1155 +/- ##
===========================================
+ Coverage 78.85% 81.76% +2.91%
===========================================
Files 146 160 +14
Lines 15884 17948 +2064
Branches 1000 1112 +112
===========================================
+ Hits 12525 14676 +2151
+ Misses 3185 3057 -128
- Partials 174 215 +41 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
foxpatch-aleph
left a comment
foxpatch-aleph
left a comment
There was a problem hiding this comment.
Well-built PR with a sound design (retire_vm reasons, filesystem-backed markers, careful reconciler guards) and extensive tests, but it contains one genuine capacity-accounting bug: under VOLUME_RETENTION=keep, the largest-single-volume admission check counts a pool's reclaimable bytes in its room figure, while the placement target chosen in _select_from is the free-max pool and only evicts that pool's retained directories — so a volume that fits on a non-free-max pool after eviction is admitted by the check and then repeatedly refused at placement, violating the PR's own invariant that the three disk figures agree.
src/aleph/vm/agent/capacity.py (line 254): Under VOLUME_RETENTION=keep this check can admit a volume the placement then refuses. roomiest_bytes is the max of (free + reclaimable_bytes(pool.path)) over all pools, but _select_from (storage_pools.py:330) picks the placement target by free bytes only and calls make_room on that single pool. Scenario: pool A free-max with no retained dirs, pool B with 100G free plus 300G retained within budget, and a 400G single-volume VM — _check_max_volume passes (roomiest = B, 400G), then make_room(A) frees nothing and select_pool raises InsufficientResourcesError. The scheduler retries, the create fails again, and a fresh create purges freshly allocated disks each retry (re-download). Either consult the room maker for candidate pools' room rather than only the free-max pool, or use the free-max pool's own room here. Note the new test (test_max_volume_check_counts_that_pools_reclaimable_bytes) only covers the case where the roomiest-by-room pool IS the free-max one, so the mismatch is not caught.
src/aleph/vm/agent/vm/reconciler.py (line 338): Narrow race with a GONE under keep: the retire drops the registry record, then a periodic pass walking this namespace sees not-live / no marker / old mtime and writes an "orphan" marker. If that write lands after the retire's "gone" marker (os.replace overwrites it), the marker loses its owner, so the retained VM can only leave via the budget and the owner's erase is refused with 403. Re-read the marker before writing the orphan marker (the window between _is_orphan and this write is where the retire's marker lands) to preserve the owner authorization.
src/aleph/vm/agent/tasks.py (line 425): ItemHash(last_info.vm_id) raises UnknownHashError if the supervisor lists an id that is not an item hash. The reconciler's supervisor_hashes explicitly tolerates such ids ("dropped rather than raised on"), so they are assumed possible; here the conversion sits inside the sweep loop with no guard, so one bad id breaks the whole check_payment pass. The old code (supervisor.delete_vm) accepted any string. Tolerate or guard it the way supervisor_hashes does (same at lines 449 and 507).
tests/supervisor/test_agent_no_direct_delete.py (line 21): Nit: the "migrations" exclusion silently removes the migration modules from the no-direct-delete pin. Nothing in agent/migration/ calls .delete_vm (verified), so the exclusion can be dropped to make the pin cover every agent module.
Admission's largest-single-volume check counts every pool's reclaimable bytes as free, but the placement fallback offered the room maker only the free-max pool: a volume that fits on another pool once its retained directories are evicted was admitted, then refused at placement on every retry. Offer every eligible candidate, most free bytes first (the pool needing the least eviction), so the two figures agree again. Found by review on #1155.
A GONE retire drops the registry record before its thread writes the gone marker, so a pass walking that namespace in the window could decide orphan and publish a marker with no owner and no parent pins over the real one: the owner's erase would 403 and the cache pass could evict parent images the retained volumes still need. The orphan write is now create-exclusive (os.link publishes the finished file only when nothing sits at the path), which is its intent anyway: the orphan pass only acts on unmarked directories. Found by review on #1155.
check_payment converted every listed vm_id with a bare ItemHash(), in the terminal-status loop and the per-tier grouping alike, so one id that is not an item hash killed the whole pass (a pre-existing fragility this stack's retire_vm conversions repeated). Filter the snapshot once at the top, drop-and-warn like the reconciler's supervisor_hashes: such an id cannot name a payment-checked VM anyway. Found by review on #1155.
The exclusion matches agent/migrations (alembic's generated DB-migration scripts), not the live-migration package agent/migration, which the pin covers.
|
Re: the 2026-09-07 review. Three of the four findings led to fixes on the branch; the fourth is a directory mixup worth recording. Admission/placement pool mismatch — confirmed, the sharpest finding of this round. Fixed on the placement side rather than in the check: Orphan marker vs the gone marker's owner — confirmed, and slightly worse than stated: the overwrite also loses ItemHash in the payment sweep — the crash is real but pre-existing: the pass's first loop and The "migrations" exclusion — two different directories: the exclusion matches |
foxpatch-aleph
left a comment
foxpatch-aleph
left a comment
There was a problem hiding this comment.
Well-designed and carefully defended implementation: the retire_vm single entry point with mandatory reasons, the .reclaimable marker protocol with its exclusive os.link write, and the layered create/reconcile race guards are all correct under scrutiny, path handling is consistently hardened, and test coverage of the new modules is thorough. No correctness bugs or security issues found. The comments below are non-blocking: an event-loop performance concern on the admission hot path, a narrow eviction race make_room leaves open, an error-propagation nit in the GONE sweep, and two design-plan docs the PR description says are included but are absent from the commit.
src/aleph/vm/agent/capacity.py (line 255): Performance: reclaimable_bytes(pool.path) still walks every namespace directory on every pool and filters them afterward (reclaimable.py:219-225), so _check_max_volume costs a full multi-pool walk, and _available_disk_bytes (line 276) immediately does a second one — all synchronous on the event loop inside every check_capacity call. On a node with many namespaces that's O(pools × namespaces) stat+read syscalls per allocation request, on the hot create path. Consider a short-TTL cached aggregate, or at least having the per-pool variant iterate only that pool's directories rather than filter the full iter_reclaimable() stream.
src/aleph/vm/agent/vm/reconciler.py (line 596): make_room protects evictions with only the frozen live snapshot, unlike reconcile_storage, which re-asks the registry at removal time via registry_is_live. A VM whose registry record commits between the supervisor wiring's live_hashes() snapshot and _evict isn't protected here. The window is small and adopt() clears markers on the create path, but since the registered room-maker lambda already closes over the registry, threading registry_is_live(registry, live) through as the default is_live would close the race for free, matching reconcile_storage's reasoning at line 260.
src/aleph/vm/agent/vm/retire.py (line 124): mark_reclaimable errors (e.g. an OSError from write_marker's temp-file write on a full or read-only filesystem) propagate out of retire_vm(GONE), and as the docstring at line 108 notes, the GONE call sites in check_payment sweep terminal messages with no local try — so one bad pool aborts the sweep for the remaining VMs. The VM is already quiesced and its records are gone at that point, so a failure here is safe to swallow with a logged exception, like the _after_gone() hook just below does.
docs/architecture/storage.md (line 1): The PR description says the design doc (docs/plans/2026-08-24-disk-reclamation-design.md) and its implementation companion are both included, but neither is in the commit — only docs/architecture/storage.md and vm-lifecycle.md are (docs/plans/ contains only the 2026-08-19 compose-runtime one). Consider adding them, or updating the description; the architecture docs that are included are good.
2 participants
Summary
PR A of the 2.1 disk reclamation work (design:
docs/plans/2026-08-24-disk-reclamation-design.md, plan:docs/plans/2026-08-24-disk-reclamation-implementation.md, both included). Spec sections 1, 3 and 6.retire_vm(vm_hash, RetireReason)withRECREATE/GONE/ERASE/FAILED_CREATE; every agent delete site (tasks.py,expiry.py,run.py,views/__init__.py,update_watcher.py,operator.py,migration.py) converted; a test pins that no agent module exceptretire.pycallssupervisor.delete_vm. A failed re-create of a VM whose volumes already existed retiresRECREATE(spec amended): owner data is never wiped by a boot timeout or an admission refusal.VOLUME_RETENTION = reap | keep(defaultreap),VOLUME_RETENTION_BUDGET(10% per pool). Underkeep, a GONE VM's directories get a.reclaimablemarker (reclaimable_since,reason,size_bytes,depends_on,owner); reclaimable space counts as free in all three disk figures; a create adopts a retained directory of the same hash;ERASEalways purges and works from the marker's owner after the record is gone.reconcile_storage()at startup, everyVOLUME_RECONCILE_INTERVAL, after every GONE and on placement pressure (storage_pools.set_room_maker, evicts oldest-first until the create fits, never a live hash). Passes: orphan namespaces (outsideVOLUME_CREATE_GUARDand thecreating()guard),.partfiles, side dirs (session, staging, empty/mntmount points viarmdironly), retention budget, backup TTL sweep. Live set is the registry unionsupervisor.list_vms(); the startup pass refuses to purge if the supervisor cannot be asked or the registry rehydrated empty while VMs are running. Startup logs a per-pool summary before purging.creating(); the migration import now records the imported VM in the registry.VOLUME_RETENTION,VOLUME_RETENTION_BUDGET,VOLUME_RECONCILE_INTERVAL,VOLUME_CREATE_GUARD,CACHE_BUDGET(consumed by PR D),MAX_RUNTIME_ARCHIVE_SIZE(PR D). Docs:docs/architecture/storage.md,vm-lifecycle.md.Follow-up PRs (stacked): B admission from the message, C device-mapper teardown, D cache bounds + in-stream size cap, E
aleph-vm storageCLI.Known limitations (deliberate)
GONEunderkeepqueues one serialized pass per VM; not coalesced.Test plan
tests/supervisor: 1320 passed, 48 pre-existing environment failures (identical set todev).test_storage_budget.py,test_reclaimable.py,test_retire.py,test_reconciler.py,test_agent_no_direct_delete.py; extended capacity, resources, operator, run, migration tests.reap; the log carries a per-pool count and byte total before it does.🤖 Generated with Claude Code
https://claude.ai/code/session_01RvDhr5NKq3AY17E1vwV3zk