Repository navigation
Clear the engineering backlog: shared coordination, lifecycle races, durable failures, deps, jumpbox tooling - #71
Merged
Merged
Conversation
…n tests #48: document memory_free_gb as a deprecated alias of memory_available_gb (not MemFree) in the endpoint, README and LOAD_BALANCING.md; response now carries deprecated_fields; test pins the equality. #49: edge-case coverage for MemAvailable selection. Fixes found: clamp reported available to [0, total] and treat null as missing; negative request size can no longer admit an overcommitted node. Co-Authored-By: Claude <noreply@anthropic.com>
- requirements.txt is now generated from uv.lock (exact pins + hashes); both Dockerfiles install it with --require-hashes. CI runs uv lock --check and fails if requirements.txt drifts from the lock. Tests pin the same. - gunicorn 22.0.0 -> 23.0.0 (no gunicorn.conf.py/run_prod.py changes needed). - Replace datetime.utcnow() in tests; DB-column fixtures use naive local now() per the repo convention, the timebox skew test keeps explicit naive UTC. Replace deprecated db.get_engine() in /koth/api/_diag with db.engine. pytest now errors on utcnow(). Co-Authored-By: Claude <noreply@anthropic.com>
…le deleted guests Expiry vs extension: the expiry worker re-read the row under vm_lifecycle_lock but never re-checked expires_at, so an extension that committed after the batch was selected was destroyed anyway. The worker now skips a VM whose fresh expires_at is in the future, and /extend re-reads and commits each VM under the same per-VMID lock, so the two can no longer interleave. Same-named deployments: /extend matched every live VM whose template name started with the deployment group name, so a student's own lab and their group's lab of the same name were extended together. /list now emits one card per deployment with a deployment_scope (individual|group), the Extend button posts it, and /extend filters to that scope. A request without a scope (a page cached from before this change) is refused when the name is ambiguous and behaves as before otherwise. Reaper starvation: the orphan sweep ran before the expiry pass and had no bound, so a backlog of hung orphans (60 s stop + 120 s delete each) held up expiry. The expiry pass now runs first, and a sweep starts no new guest after ORPHAN_REAPER_MAX_PASS_SECONDS (default 120) or ORPHAN_REAPER_MAX_PASS_GUESTS (default 10); the rest wait for the next sweep. Manual deletion: a guest deleted by hand in Proxmox kept its record, and its admission commitment, until expiry unless someone loaded /list. Each expiry pass now retires a live record whose VMID fresh inventory has not listed for ORPHAN_CONFIRM_SECONDS (two sightings, the last via confirm_proxmox_absence under the lifecycle lock), and records a vm_record_reconciled admin alert. A failed or contradicting read restarts the count; KOTH records are marked destroyed, not deleted. Co-Authored-By: Claude <noreply@anthropic.com>
Survivor tracking (#47): when the database refuses the cleanup-tracking commit for a failed deploy's surviving guests, append one JSON line per guest (vmid, node, name, type, owner, reason; no credentials) to a 0600 fallback file under data/ (UNTRACKED_SURVIVORS_FILE overrides). The error still propagates so admission reservations stay held as before. Startup and every expiry pass import pending lines as already-expired VMRecords, skip vmids that already have a live record, and remove the file only after the commit. Corrupt lines are logged and moved to a .corrupt file. Alert outbox (#41): the deletion-failure alert is now queued in a new alert_outbox table in the same commit that sets deletion_alert_sent, instead of being sent after it. The expiry loop dispatches due rows, striking each delivered channel in its own commit and retrying failed channels with capped exponential backoff. The orphan-reaped alert goes through the same outbox (database channel only, as before). Adds scripts/migrate_add_alert_outbox.py. Co-Authored-By: Claude <noreply@anthropic.com>
The unit suite mocks proxmoxer, so clone parameters, task waiting and destroy semantics were never checked against a real cluster. tests/live/ exercises them through the app's own helpers (create_qemu_vm, wait_for_proxmox_task, get_node_resources, get_cluster_vm_index, inspect_vm_guest, confirm_proxmox_absence) and asserts the response shapes those helpers read. Every live test carries the `live` marker and is skipped unless LIVE_PROXMOX=1 and the required environment are set, so `pytest tests/` and CI skip it without opening a connection. All mutations go through ContractRun (tests/live/harness.py), which: - allocates only inside LIVE_VMID_RANGE and refuses to run while any guest in the range lacks the lab-contract-test- prefix; - names each guest lab-contract-test-<random> with an Expires: line 30 minutes out; - cleans up on exit even when the body raised, printing copy-paste qm/pvesh removal commands for any guest it could not delete; - starts, stops or destroys only guests it created in this run, and only while fresh inventory still shows the name and node it assigned. tests/test_live_contract_guards.py verifies those guards against an in-memory Proxmox double and runs in CI. Co-Authored-By: Claude <noreply@anthropic.com>
…database
Node-memory reservations, VMID reservations with their high-water mark,
VM lifecycle locks and the password-reset limiter lived in process memory,
so their guarantees held only with one gunicorn worker (ops #37).
- app/coordination.py: each check-and-reserve runs in one transaction that
locks a coordination_state row (SELECT ... FOR UPDATE on MySQL, a write
on SQLite). A process lock in front keeps greenlets of one worker from
each holding a pooled connection while they wait.
- select_best_node, get_next_available_vmid, renew/release and
is_vmid_reserved keep their signatures; reservations live in
node_memory_reservations / vmid_reservations, the high-water mark in
coordination_state. Leases use time.time() epoch seconds. A negative
memory request can no longer write a negative reservation.
- vm_lifecycle_lock holds GET_LOCK('vm-lifecycle-<vmid>') on a dedicated
MySQL connection, released (or the connection discarded) on every exit;
SQLite and DB outages keep the process lock only.
- The app's password-reset limiter counts failures in
password_reset_failures with the same limits and key cap.
- scripts/migrate_add_coordination_tables.py creates the tables and seeds
the lock rows; idempotent.
Workers stay at 1: APScheduler and the SSE bus are still in-process.
CLAUDE.md rule 10, gunicorn.conf.py and the deployment docs say which
state is shared now.
Co-Authored-By: Claude <noreply@anthropic.com>
The jumpbox was built by hand and the scripts no longer matched it: one NIC, no VLAN gateways, no DHCP, a manual Headscale join, and an ISO path whose installer gives the root LV half the disk (the jumpbox filled its disk and dnsmasq stopped writing leases). Replace the five obsolete scripts with three: - jumpbox_provision.sh (Proxmox node): cloud image resized before first boot, management NIC on its VLAN, untagged trunk NIC with the firewall, cloud-init key login. Dry run by default; refuses the wrong node or an existing VMID and never destroys a VM. - jumpbox_configure.sh (jumpbox): netplan vlan<N> gateways and dnsmasq DHCP for every VLAN the portal allocates (bands derived from app/vlan_allocator.py, enforced by a test), forwarding sysctls, LVM root growth. Dry run with diffs by default; netplan try (or explicit --no-try); validates before writing; refuses to duplicate a hand-built config; idempotent. Optional --tailscale (subnet router, no SNAT, key via a 0600 file, never argv), --ssh-keys-only (refuses without a usable authorized key), --monitoring (Zeek + hourly RITA, disk floor). - jumpbox_accounts.sh: optional accounts (random passwords), plus lock-stale with an undo list and unlock. docs/JUMPBOX.md replaces JUMPBOX_SETUP.md and JUMPBOX_SIMPLE.md and recommends a Headscale ACL that gives members 10.0.0.0/8 but not tag:jumpbox. Co-Authored-By: Claude <noreply@anthropic.com>
# Conflicts: # docker-compose.prod.yml # docs/PRODUCTION_DEPLOYMENT.md
Co-Authored-By: Claude <noreply@anthropic.com>
/destroy now selects one deployment the same way /extend does, through a shared _select_deployment: deployment_scope picks individual or group, a new group_id picks the owning group, and a request that still names more than one deployment is refused with a "reload" message. Before this, the split My Labs cards let a click on a student's own card destroy the group's same-named lab too. group_id covers a student who changed groups: membership is unique, but the old group's lab still matches through created_by_user_id, so a group-scoped request named two deployments and failed with a 403. /list now keys cards by owning group and returns group_id; both the Extend and Destroy buttons send data-scope and data-group-id. /list no longer deletes confirmed-missing records itself. It calls tasks.reconcile_missing_vm_record, the reconcile pass's single-record helper, which re-reads the row under vm_lifecycle_lock, re-confirms the absence, raises the vm_record_reconciled AdminAlert and keeps KOTH rows as destroyed. Co-Authored-By: Claude <noreply@anthropic.com>
…ve VMID range vm_lifecycle_lock took MySQL GET_LOCK on a second pool connection for every holder, which buys nothing while production runs one worker and doubled the pool cost of a class-wide destroy wave. DISTRIBUTED_VM_LOCKS (default false) restores the process-lock-only behaviour; it must be set to true before a second worker. The pool's max_overflow rises from 50 to 100 so a 45-student wave plus /list renders also fits with the flag on. - Re-entering a VMID lock this greenlet already holds raises RuntimeError instead of waiting for itself forever. - GET_LOCK names carry the database name, hashed when it would exceed MySQL's 64-character limit; mysql_named_lock refuses over-long names rather than mistaking MySQL's rejection for an outage. - The live contract harness refuses a VMID range above the portal's database high-water mark (LIVE_PORTAL_DATABASE_URL), or above every other cluster guest when it cannot read the mark, unless LIVE_SEPARATE_CLUSTER=1. tests/live/README.md now says the mark lives in the database and never goes down. Co-Authored-By: Claude <noreply@anthropic.com>
…ss processes I1: the expiry loop now delivers outbox alerts after expiry and reconcile (still before the reaper, and still when a Proxmox outage ends the pass early). dispatch_pending_alerts is time-boxed per pass (ALERT_DISPATCH_MAX_PASS_SECONDS, default 20; ALERT_DISPATCH_MAX_PASS_ALERTS, default 50); alerts not attempted stay due with their attempt counts unchanged. smtplib.SMTP gets timeout=10; the webhook already had one. M1: an unset, empty or non-numeric SMTP_PORT means email is not configured instead of raising, so alerts no longer retry email to 'abandoned'. M2: appends and the hand-off of data/untracked-survivors.jsonl hold an fcntl.flock on a .lock sidecar, and one importer at a time holds a non-blocking flock on .import.lock. CLAUDE.md rule 10 and gunicorn.conf.py now list exactly what still pins one worker. M5: the rollback section covers the survivor and .importing files. Co-Authored-By: Claude <noreply@anthropic.com>
# Conflicts: # CLAUDE.md # app/config.py
Temp names carry a timestamp, PID and random hex, so the substring check failed whenever one of them happened to contain 644. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes
Correctness and reliability
GET_LOCK(DISTRIBUTED_VM_LOCKS, default off). The single-worker requirement is still documented for APScheduler, SSE and the other in-process state that remains.expires_atunder the lifecycle lock, and extend takes the same lock./listuses the same path.memory_free_gbis documented as an alias of available memory.Dependencies
uv.lock(--require-hashes), and CI checks for drift.datetime.utcnow()uses are gone; the suite runs with 0 warnings.Tooling
scripts/jumpbox_provision.sh,jumpbox_configure.sh,jumpbox_netgen.py,jumpbox_accounts.sh,docs/JUMPBOX.md) reproduces a VLAN-gateway / Headscale-subnet-router jumpbox. Anything that rewrites network config is dry-run by default, and applies throughnetplan try.tests/live/, skipped unlessLIVE_PROXMOX=1) with VMID-range safety guards.How it was verified
GET_LOCKblocks across processes;netplan generateanddnsmasq --teston Ubuntu 24.04.Deploy notes
scripts/migrate_add_coordination_tables.pyandscripts/migrate_add_alert_outbox.py. They are idempotent, and startupcreate_allalso creates the tables.DISTRIBUTED_VM_LOCKS(false),ORPHAN_REAPER_MAX_PASS_SECONDS/_GUESTS,ALERT_DISPATCH_MAX_PASS_SECONDS/_ALERTS, andUNTRACKED_SURVIVORS_FILE.