fix(proxmox): make host registration idempotent on name - #137
Conversation
The deploy bundle POSTs /v1/proxmox/hosts on every scenario run, and nothing stopped a second row under an existing name: create_host never checked for a clash and proxmox_hosts.name had no constraint, so the documented "idempotent via 409" was never reachable. Each re-run added a host, and the UI picker filled up with duplicates of the same hypervisor. Delete-and-recreate was not an option for the fix: deployments.target_host_id is a FK to proxmox_hosts.id, so a new row per re-run strands every earlier deployment on a host nobody updates. POST now upserts on name and keeps the id, returning 200 instead of 201. Credentials are refreshed too, so a rotated PVE token reaches the backend on the next deploy instead of leaving it authenticating with a stale one -- there is no update route to fix it by hand. Migration 0002 collapses duplicates already in the field before adding the constraint: for each name it keeps the earliest added_at row, repoints any deployments at it, deletes the rest, and logs every collapse.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83d279487c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| rows = conn.execute( | ||
| sa.text("SELECT id, name FROM proxmox_hosts ORDER BY name, added_at, id") | ||
| ).fetchall() |
There was a problem hiding this comment.
When duplicate registrations contain a rotated token or updated URL/node, sorting oldest-first and retaining that entire row permanently deletes the newest configuration while repointing every deployment to the stale credentials. This can make existing deployments and health checks fail immediately after upgrade, before another deploy bundle happens to re-register the host. Keep the oldest ID if identity preservation is required, but copy the newest duplicate's mutable host fields onto it before deleting the later rows.
Useful? React with 👍 / 👎.
| existing = ( | ||
| await session.execute( | ||
| select(ProxmoxHost).where(ProxmoxHost.name == payload.name) | ||
| ) | ||
| ).scalar_one_or_none() |
There was a problem hiding this comment.
When two scenario runs register the same previously unseen name concurrently, both requests can complete this query before either inserts; each then takes the create path, and the second commit violates uq_proxmox_host_name, producing an unhandled database error instead of the promised idempotent 200 response. Use an atomic database upsert or recover from the unique-conflict by rolling back, loading the winning row, and applying the update.
Useful? React with 👍 / 👎.
…om a lost race Two problems found in review of the previous commit. The migration kept the oldest duplicate wholesale. Duplicates accumulated one per scenario re-run, so the newest row holds the credentials in force -- and keeping the first row resurrected a token that may have been rotated away, with every deployment repointed at it. The keeper now takes the newest duplicate's api_url / node / token / bridge / overrides while keeping its own id and added_at. The earlier test asserted on ids only, which is why it did not catch this. The upsert's duplicate check and its commit are separated by an await, so two concurrent registrations of the same new name could both take the insert path -- WEB_CONCURRENCY=1 bounds this to one process but not to one task. The loser now rolls back, re-reads the winner and applies its update, matching how create_deployment already handles the same shape.
| 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 |
There was a problem hiding this comment.
This assigns all six columns unconditionally, so the upsert has PUT semantics against a payload where three fields carry schema defaults. Anything the caller omits is reset rather than left alone.
Confirmed against the schema on this branch:
>>> p = HostIn(name='x', api_url='https://h:8006', node_name='n', token_ref='t')
>>> sorted(p.model_fields_set)
['api_url', 'name', 'node_name', 'token_ref']
>>> p.default_bridge, p.protected_vmids_override
('vmbr0', None)
Those four are exactly what the deploy bundle sends (range42-playbooks bundles/admin/software.install.deployer_api_backend/main.yml:495). So every scenario re-run now writes token_scope = NULL, default_bridge = 'vmbr0', protected_vmids_override_json = NULL.
Concrete case: a host registered by hand with default_bridge: vmbr142 and protected_vmids_override: [[200, 250]]. The next scenario run silently resets the bridge to vmbr0 and drops the 200–250 guard, and filter_safe_vmids will then hand 200–250 to a mass delete as safe. With no PUT/PATCH on hosts (only this POST and the DELETE at line 163) and the UI read-only, re-POSTing by hand is the only recovery — and the following deploy wipes it again.
Worth being explicit that this is not the VMID 100/101 case: _effective_ranges() appends overrides onto DEFAULT_PROTECTED_RANGES (app/core/vmid_guard.py:40-45), so clearing an override narrows protection back to the defaults and can never unprotect pmg01/zbx01. That is what keeps this non-blocking.
Pydantic 2.13.4 is already in use, so restricting the write to what was actually supplied is a small change:
| row.protected_vmids_override_json = overrides_json | |
| 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. | |
| Only fields the caller actually sent are written: the deploy bundle posts | |
| four of them, and a blind assign would reset default_bridge, token_scope | |
| and the protected-VMID override to their schema defaults on every re-run. | |
| """ | |
| sent = payload.model_fields_set | |
| if "api_url" in sent: | |
| row.api_url = str(payload.api_url) | |
| if "node_name" in sent: | |
| row.node_name = payload.node_name | |
| if "token_ref" in sent: | |
| row.token_ref = payload.token_ref | |
| if "token_scope" in sent: | |
| row.token_scope = payload.token_scope | |
| if "default_bridge" in sent: | |
| row.default_bridge = payload.default_bridge | |
| if "protected_vmids_override" in sent: | |
| row.protected_vmids_override_json = overrides_json |
If you'd rather keep full-replace semantics, that works too — but then the bundle should send the full record, otherwise the re-seed is the thing destroying it.
Generated by Claude Code
Review — approve with commentsTurns a POST that silently accumulated a row per scenario re-run into a name-keyed upsert, plus a migration that collapses the duplicates already in the field. The reasoning for upsert-over-409 holds up: Migration 0002 checks out on the details that usually go wrong here. Keeper selection is Blocking
Non-blocking
Cross-repo
Verification
Not reviewed
Generated by Claude Code |
2 participants
The bug
The deploy bundle POSTs
/v1/proxmox/hostson every scenario run, and nothing stopped a second row under an existing name —create_hostnever checked for a clash andproxmox_hosts.namehad no constraint. The documented "idempotent via 409" was unreachable: 409 could never be returned. Each re-run added a host, and the UI picker filled up with duplicates of the same hypervisor.Why upsert rather than 409
Delete-and-recreate is not a viable re-seed path:
deployments.target_host_idis a FK toproxmox_hosts.id, so a new row per re-run strands every earlier deployment on a host nobody updates.So POST now upserts on
nameand keeps the id, returning 200 instead of 201.added_atis deliberately untouched — it records first registration, and the migration keys on it.Credentials are part of what gets refreshed, which closes a second trap: there is no PUT/PATCH on hosts and the UI is read-only on them, so a rotated PVE token was previously unfixable short of a manual DB edit. It now reaches the backend on the next deploy.
Migration 0002
Databases in the field already carry duplicates, and the constraint cannot be added on top of them. For each name the migration keeps the earliest
added_atrow, repoints any deployments at it, deletes the rest, and logs every collapse:Verified
DID NOT RAISE IntegrityErrorand un-repointed FKs.alembic upgrade, not by inspection: a 0001-era DB seeded with threepve01duplicates plus deployments pointing at the later two, then asserted on the survivor and the repointing. Plus a no-op case on clean data.openapi.jsonregenerated for the drift gate.Pairs with
range42/range42-playbooks#142, which drops 409 from the bundle's accepted status codes — keeping it would only mask a real conflict.