diff --git a/alembic/versions/0002_proxmox_host_unique_name.py b/alembic/versions/0002_proxmox_host_unique_name.py new file mode 100644 index 0000000..d8cd291 --- /dev/null +++ b/alembic/versions/0002_proxmox_host_unique_name.py @@ -0,0 +1,101 @@ +"""dedupe proxmox_hosts, then enforce a unique name + +Until now nothing stopped a second host row under an existing name, and the +deploy bundle re-POSTs the same name on every scenario run — so field +databases carry one duplicate per re-run. The duplicates are not inert: +``deployments.target_host_id`` points at whichever one existed when the +deployment was created. + +Collapsing keys on ``added_at``: the earliest row under a name is the one the +first deploy registered, so it is the row most likely to be referenced and the +one whose id callers may have recorded. Deployments on the later duplicates are +repointed at it before those rows are deleted, so nothing is left dangling. + +Revision ID: 0002_proxmox_host_unique_name +Revises: 0001_v1_initial +Create Date: 2026-08-10 +""" +import logging + +import sqlalchemy as sa +from alembic import op + +revision = '0002_proxmox_host_unique_name' +down_revision = '0001_v1_initial' +branch_labels = None +depends_on = None + +log = logging.getLogger("alembic.runtime.migration") + + +def upgrade() -> None: + conn = op.get_bind() + + # added_at first, id as a deterministic tie-break for rows registered + # within the same clock tick. + rows = conn.execute( + sa.text("SELECT id, name FROM proxmox_hosts ORDER BY name, added_at, id") + ).fetchall() + + groups: dict[str, list[str]] = {} + for host_id, name in rows: + groups.setdefault(name, []).append(host_id) + + for name, ids in groups.items(): + if len(ids) == 1: + continue + keeper_id, *loser_ids = ids + + # The id is the oldest row's, because deployments reference it. The + # connection details are the NEWEST row's: duplicates accumulated one + # per scenario re-run, so the last registration holds the credentials + # in force. Keeping the first row wholesale would resurrect a token + # that may since have been rotated away, and every deployment + # repointed at it would start failing auth the moment this ran. + conn.execute( + sa.text( + "UPDATE proxmox_hosts SET" + " api_url = latest.api_url," + " node_name = latest.node_name," + " token_ref = latest.token_ref," + " token_scope = latest.token_scope," + " default_bridge = latest.default_bridge," + " protected_vmids_override_json =" + " latest.protected_vmids_override_json," + " last_health_check_json = latest.last_health_check_json" + " FROM (SELECT * FROM proxmox_hosts WHERE id = :newest) AS latest" + " WHERE proxmox_hosts.id = :keeper" + ), + {"newest": loser_ids[-1], "keeper": keeper_id}, + ) + + for loser_id in loser_ids: + moved = conn.execute( + sa.text( + "UPDATE deployments SET target_host_id = :keeper" + " WHERE target_host_id = :loser" + ), + {"keeper": keeper_id, "loser": loser_id}, + ).rowcount + conn.execute( + sa.text("DELETE FROM proxmox_hosts WHERE id = :loser"), + {"loser": loser_id}, + ) + log.info( + "proxmox_hosts: collapsed duplicate %r (%s) into %s, " + "repointed %d deployment(s)", + name, loser_id, keeper_id, moved, + ) + log.info( + "proxmox_hosts: %r kept id %s with the connection details from %s", + name, keeper_id, loser_ids[-1], + ) + + with op.batch_alter_table("proxmox_hosts") as batch: + batch.create_unique_constraint("uq_proxmox_host_name", ["name"]) + + +def downgrade() -> None: + # Only the constraint is reversible; the collapsed rows are gone for good. + with op.batch_alter_table("proxmox_hosts") as batch: + batch.drop_constraint("uq_proxmox_host_name", type_="unique") diff --git a/app/core/models.py b/app/core/models.py index da1ce88..ad3f0b6 100644 --- a/app/core/models.py +++ b/app/core/models.py @@ -57,6 +57,11 @@ class ProxmoxHost(Base): protected_vmids_override_json: Mapped[str | None] = mapped_column(Text) added_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), default=_utcnow) last_health_check_json: Mapped[str | None] = mapped_column(Text) + # A host is identified by its name: the deploy bundle re-POSTs the same + # name on every scenario run, and deployments.target_host_id is a FK here, + # so a second row per re-run would strand earlier deployments on a host + # nobody updates. Uniqueness turns those re-runs into in-place updates. + __table_args__ = (UniqueConstraint("name", name="uq_proxmox_host_name"),) class Project(Base): diff --git a/app/routes/v1/proxmox/hosts.py b/app/routes/v1/proxmox/hosts.py index 2d34b51..71819ba 100644 --- a/app/routes/v1/proxmox/hosts.py +++ b/app/routes/v1/proxmox/hosts.py @@ -7,8 +7,9 @@ from datetime import datetime, timezone import httpx -from fastapi import APIRouter, Depends, status +from fastapi import APIRouter, Depends, Response, status from sqlalchemy import select +from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession from app.core.errors import AuthFailedError, Range42Error @@ -47,6 +48,26 @@ def _row_to_out(row: ProxmoxHost) -> HostOut: ) +async def _find_host_by_name( + session: AsyncSession, name: str +) -> ProxmoxHost | None: + return ( + await session.execute( + select(ProxmoxHost).where(ProxmoxHost.name == name) + ) + ).scalar_one_or_none() + + +def _refresh_host(row: ProxmoxHost, payload: HostIn, overrides_json: str | None) -> None: + """Carry a re-registration onto an existing row, id and added_at intact.""" + row.api_url = str(payload.api_url) + row.node_name = payload.node_name + row.token_ref = payload.token_ref + row.token_scope = payload.token_scope + row.default_bridge = payload.default_bridge + row.protected_vmids_override_json = overrides_json + + @router.get("/hosts", response_model=Page[HostOut]) async def list_hosts( session: AsyncSession = Depends(_session), @@ -68,11 +89,49 @@ async def list_hosts( @router.post( - "/hosts", response_model=HostOut, status_code=status.HTTP_201_CREATED + "/hosts", + response_model=HostOut, + status_code=status.HTTP_201_CREATED, + responses={ + 200: { + "model": HostOut, + "description": "Host already registered under this name; updated in place.", + } + }, ) async def create_host( - payload: HostIn, session: AsyncSession = Depends(_session) + payload: HostIn, + response: Response, + session: AsyncSession = Depends(_session), ): + """Register a Proxmox host, or refresh the one already under that name. + + The deploy bundle POSTs this on every scenario run, so it has to be + idempotent. Re-registering keeps the existing row's id — ``deployments`` + reference it by FK — and returns 200 instead of 201. Credentials are part + of what gets refreshed: a rotated PVE token reaches the backend on the next + deploy rather than leaving it authenticating with a stale one. + """ + overrides_json = ( + json.dumps(payload.protected_vmids_override) + if payload.protected_vmids_override + else None + ) + + async def _update_in_place(row: ProxmoxHost) -> HostOut: + # added_at deliberately untouched: it records when this host was first + # registered, and the dedupe migration keys on it. + _refresh_host(row, payload, overrides_json) + await session.commit() + await session.refresh(row) + log.info("proxmox_host_reregistered", host_id=row.id, name=row.name) + response.status_code = status.HTTP_200_OK + return _row_to_out(row) + + existing = await _find_host_by_name(session, payload.name) + if existing is not None: + return await _update_in_place(existing) + row = ProxmoxHost( id=uuid.uuid4().hex[:16], name=payload.name, @@ -81,14 +140,22 @@ async def create_host( token_ref=payload.token_ref, token_scope=payload.token_scope, default_bridge=payload.default_bridge, - protected_vmids_override_json=( - json.dumps(payload.protected_vmids_override) - if payload.protected_vmids_override - else None - ), + protected_vmids_override_json=overrides_json, ) session.add(row) - await session.commit() + try: + await session.commit() + except IntegrityError: + # Lost the race: another request registered this name between the + # lookup above and this commit. Recover into the update path instead + # of surfacing a 500 — the caller asked for a registration and one + # now exists, which is the outcome they wanted. + await session.rollback() + winner = await _find_host_by_name(session, payload.name) + if winner is None: + raise + log.info("proxmox_host_register_race", name=payload.name, host_id=winner.id) + return await _update_in_place(winner) await session.refresh(row) return _row_to_out(row) diff --git a/openapi.json b/openapi.json index ee455fd..1191214 100644 --- a/openapi.json +++ b/openapi.json @@ -3507,6 +3507,7 @@ "v1 proxmox" ], "summary": "Create Host", + "description": "Register a Proxmox host, or refresh the one already under that name.\n\nThe deploy bundle POSTs this on every scenario run, so it has to be\nidempotent. Re-registering keeps the existing row's id \u2014 ``deployments``\nreference it by FK \u2014 and returns 200 instead of 201. Credentials are part\nof what gets refreshed: a rotated PVE token reaches the backend on the next\ndeploy rather than leaving it authenticating with a stale one.", "operationId": "create_host_v1_proxmox_hosts_post", "requestBody": { "required": true, @@ -3538,6 +3539,16 @@ } } } + }, + "200": { + "description": "Host already registered under this name; updated in place.", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/HostOut" + } + } + } } } } diff --git a/tests/routes/test_proxmox_hosts.py b/tests/routes/test_proxmox_hosts.py index 79b1c88..9ce09da 100644 --- a/tests/routes/test_proxmox_hosts.py +++ b/tests/routes/test_proxmox_hosts.py @@ -120,3 +120,184 @@ async def test_health_unreachable_host_returns_unreachable_status( assert body["status"] == "unreachable" finally: await dbmod.dispose_engine() + + +@pytest.mark.asyncio +async def test_reseeding_the_same_host_updates_it_in_place(tmp_path, monkeypatch): + """A scenario re-run POSTs the same name; it must not pile up duplicates. + + The id has to survive: deployments.target_host_id is a FK to it, so a new + row per re-run would strand every earlier deployment on a stale host. + """ + app, dbmod = await _boot(tmp_path, monkeypatch) + try: + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://t" + ) as c: + first = await c.post( + "/v1/proxmox/hosts", + json={ + "name": "pve01", + "api_url": "https://pve01:8006", + "node_name": "pve01", + "token_ref": "r42@pam!tok=abc", + }, + ) + assert first.status_code == 201, first.text + + second = await c.post( + "/v1/proxmox/hosts", + json={ + "name": "pve01", + "api_url": "https://pve01.lan:8006", + "node_name": "pve-node-2", + "token_ref": "r42@pam!tok=rotated", + }, + ) + assert second.status_code == 200, second.text + assert second.json()["id"] == first.json()["id"] + # pydantic HttpUrl normalises a bare-host URL with a trailing slash + assert second.json()["api_url"] == "https://pve01.lan:8006/" + assert second.json()["node_name"] == "pve-node-2" + + listing = await c.get("/v1/proxmox/hosts") + assert listing.json()["total"] == 1 + finally: + await dbmod.dispose_engine() + + +@pytest.mark.asyncio +async def test_reseeding_refreshes_a_rotated_token(tmp_path, monkeypatch): + """The stored PVE token must follow the re-seed, or deploys 401 forever.""" + app, dbmod = await _boot(tmp_path, monkeypatch) + try: + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://t" + ) as c: + await c.post( + "/v1/proxmox/hosts", + json={ + "name": "pve01", + "api_url": "https://pve01:8006", + "node_name": "pve01", + "token_ref": "r42@pam!tok=stale", + }, + ) + await c.post( + "/v1/proxmox/hosts", + json={ + "name": "pve01", + "api_url": "https://pve01:8006", + "node_name": "pve01", + "token_ref": "r42@pam!tok=fresh", + }, + ) + + from sqlalchemy import select + from app.core.models import ProxmoxHost + async with dbmod.get_session_factory()() as session: + stored = ( + await session.execute(select(ProxmoxHost.token_ref)) + ).scalars().all() + assert stored == ["r42@pam!tok=fresh"] + finally: + await dbmod.dispose_engine() + + +@pytest.mark.asyncio +async def test_a_second_host_under_a_different_name_is_still_created( + tmp_path, monkeypatch +): + """Upsert keys on name only — a genuinely new host must still register.""" + app, dbmod = await _boot(tmp_path, monkeypatch) + try: + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://t" + ) as c: + a = await c.post( + "/v1/proxmox/hosts", + json={ + "name": "pve01", + "api_url": "https://pve01:8006", + "node_name": "pve01", + "token_ref": "r42@pam!tok=a", + }, + ) + b = await c.post( + "/v1/proxmox/hosts", + json={ + "name": "pve02", + "api_url": "https://pve02:8006", + "node_name": "pve02", + "token_ref": "r42@pam!tok=b", + }, + ) + assert b.status_code == 201, b.text + assert b.json()["id"] != a.json()["id"] + + listing = await c.get("/v1/proxmox/hosts") + assert listing.json()["total"] == 2 + finally: + await dbmod.dispose_engine() + + +@pytest.mark.asyncio +async def test_losing_the_race_on_a_new_name_still_upserts(tmp_path, monkeypatch): + """Two concurrent registrations of the same new name must not 500. + + WEB_CONCURRENCY=1 keeps this to one process, but asyncio still interleaves + across the await between the duplicate check and the commit: both requests + can find nothing and both take the insert path. The loser of that race hits + uq_proxmox_host_name and has to recover into the update path rather than + surfacing an IntegrityError. + """ + app, dbmod = await _boot(tmp_path, monkeypatch) + try: + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://t" + ) as c: + first = await c.post( + "/v1/proxmox/hosts", + json={ + "name": "pve01", + "api_url": "https://pve01:8006", + "node_name": "pve01", + "token_ref": "r42@pam!tok=winner", + }, + ) + assert first.status_code == 201, first.text + + # Blind ONLY the pre-check, and only once: that is exactly what the + # racing request sees. The recovery path re-reads and must find the + # row the winner committed. + import app.routes.v1.proxmox.hosts as hosts_mod + + real_lookup = hosts_mod._find_host_by_name + seen = {"calls": 0} + + async def _blind_first_lookup(session, name): + seen["calls"] += 1 + if seen["calls"] == 1: + return None + return await real_lookup(session, name) + + monkeypatch.setattr( + hosts_mod, "_find_host_by_name", _blind_first_lookup + ) + + second = await c.post( + "/v1/proxmox/hosts", + json={ + "name": "pve01", + "api_url": "https://pve01:8006", + "node_name": "pve01", + "token_ref": "r42@pam!tok=loser", + }, + ) + assert second.status_code == 200, second.text + assert second.json()["id"] == first.json()["id"] + + listing = await c.get("/v1/proxmox/hosts") + assert listing.json()["total"] == 1 + finally: + await dbmod.dispose_engine() diff --git a/tests/test_migration_dedupe_proxmox_hosts.py b/tests/test_migration_dedupe_proxmox_hosts.py new file mode 100644 index 0000000..1fb455f --- /dev/null +++ b/tests/test_migration_dedupe_proxmox_hosts.py @@ -0,0 +1,161 @@ +"""Migration 0002: collapse duplicate proxmox_hosts before enforcing uniqueness. + +Every scenario re-run POSTed the same host name and got a brand-new row, so +databases in the field carry duplicates. The unique constraint cannot be added +on top of them, and the extra rows are not inert: deployments hold a FK to +whichever duplicate happened to exist at create time. +""" +import sqlite3 +from importlib import reload + +import pytest + + +def _alembic_config(): + from alembic.config import Config + + return Config("alembic.ini") + + +@pytest.fixture +def db_at_0001(tmp_path, monkeypatch): + """A database migrated to the revision that predates the constraint.""" + from alembic import command + + db = tmp_path / "m.db" + monkeypatch.setenv("RANGE42_DB_URL", f"sqlite+aiosqlite:///{db}") + monkeypatch.setenv("RANGE42_WORKSPACE_ROOT", str(tmp_path)) + from app.core import config as cfg + + reload(cfg) + command.upgrade(_alembic_config(), "0001_v1_initial") + return db + + +def _insert_host( + conn, host_id, name, added_at, token, api_url="https://pve:8006", node="pve" +): + conn.execute( + "INSERT INTO proxmox_hosts (id, name, api_url, node_name, token_ref," + " default_bridge, added_at) VALUES (?, ?, ?, ?, ?, ?, ?)", + (host_id, name, api_url, node, token, "vmbr0", added_at), + ) + + +def _insert_deployment(conn, dep_id, codename, target_host_id): + conn.execute( + "INSERT INTO deployments (id, codename, scenario_label, project_id," + " target_host_id, team_count, state, workspace_path, created_at," + " updated_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", + ( + dep_id, + codename, + "demo_lab", + "proj-1", + target_host_id, + 1, + "planned", + f"/tmp/{codename}", + "2026-07-01 00:00:00", + "2026-07-01 00:00:00", + ), + ) + + +def test_upgrade_keeps_the_oldest_duplicate_and_repoints_deployments(db_at_0001): + from alembic import command + + with sqlite3.connect(db_at_0001) as conn: + _insert_host(conn, "oldest", "pve01", "2026-06-01 00:00:00", "tok-1") + _insert_host(conn, "middle", "pve01", "2026-07-02 00:00:00", "tok-2") + _insert_host(conn, "newest", "pve01", "2026-08-01 00:00:00", "tok-3") + _insert_host(conn, "other", "pve02", "2026-06-15 00:00:00", "tok-4") + # Deployments created against the later duplicates must not be stranded. + _insert_deployment(conn, "dep-a", "alpha", "middle") + _insert_deployment(conn, "dep-b", "bravo", "newest") + _insert_deployment(conn, "dep-c", "charlie", "other") + conn.commit() + + command.upgrade(_alembic_config(), "head") + + with sqlite3.connect(db_at_0001) as conn: + hosts = dict(conn.execute("SELECT name, id FROM proxmox_hosts").fetchall()) + deployments = dict( + conn.execute("SELECT id, target_host_id FROM deployments").fetchall() + ) + + assert hosts == {"pve01": "oldest", "pve02": "other"} + assert deployments == {"dep-a": "oldest", "dep-b": "oldest", "dep-c": "other"} + + +def test_upgrade_rejects_a_second_host_under_an_existing_name(db_at_0001): + from alembic import command + + with sqlite3.connect(db_at_0001) as conn: + _insert_host(conn, "h1", "pve01", "2026-06-01 00:00:00", "tok-1") + conn.commit() + + command.upgrade(_alembic_config(), "head") + + with sqlite3.connect(db_at_0001) as conn: + with pytest.raises(sqlite3.IntegrityError): + _insert_host(conn, "h2", "pve01", "2026-06-02 00:00:00", "tok-2") + + +def test_upgrade_is_a_no_op_on_a_database_with_no_duplicates(db_at_0001): + from alembic import command + + with sqlite3.connect(db_at_0001) as conn: + _insert_host(conn, "h1", "pve01", "2026-06-01 00:00:00", "tok-1") + _insert_host(conn, "h2", "pve02", "2026-06-02 00:00:00", "tok-2") + _insert_deployment(conn, "dep-a", "alpha", "h1") + conn.commit() + + command.upgrade(_alembic_config(), "head") + + with sqlite3.connect(db_at_0001) as conn: + hosts = dict(conn.execute("SELECT name, id FROM proxmox_hosts").fetchall()) + deployments = dict( + conn.execute("SELECT id, target_host_id FROM deployments").fetchall() + ) + + assert hosts == {"pve01": "h1", "pve02": "h2"} + assert deployments == {"dep-a": "h1"} + + +def test_upgrade_carries_the_newest_credentials_onto_the_surviving_row(db_at_0001): + """Keep the oldest id, but not the oldest credentials. + + Duplicates accumulated one per scenario re-run, so the LAST row holds the + token the operator is actually using. Keeping the first row wholesale would + resurrect a token that may have been rotated away, and every deployment + repointed at it would start failing auth the moment the migration ran. + """ + from alembic import command + + with sqlite3.connect(db_at_0001) as conn: + _insert_host( + conn, "oldest", "pve01", "2026-06-01 00:00:00", "tok-revoked", + api_url="https://old-pve:8006", node="old-node", + ) + _insert_host( + conn, "newest", "pve01", "2026-08-01 00:00:00", "tok-current", + api_url="https://pve01.lan:8006", node="pve-node-2", + ) + conn.commit() + + command.upgrade(_alembic_config(), "head") + + with sqlite3.connect(db_at_0001) as conn: + row = conn.execute( + "SELECT id, added_at, api_url, node_name, token_ref FROM proxmox_hosts" + ).fetchone() + + host_id, added_at, api_url, node_name, token = row + # identity and registration date stay with the row deployments point at + assert host_id == "oldest" + assert added_at.startswith("2026-06-01") + # ...but the connection details come from the most recent registration + assert api_url == "https://pve01.lan:8006" + assert node_name == "pve-node-2" + assert token == "tok-current"