feat: images from cofoundry, permissions and roles, and server migration - #168
Open
ericwang401 wants to merge 34 commits into
Open
ericwang401 wants to merge 34 commits into
ericwang401 wants to merge 34 commits into
Conversation
…hunk at a time Two sources for an image the panel did not build itself. A catalogue is a URL, not a concept. The panel reads the registry JSON, shows what it lists, and copies an entry into an ordinary image definition the operator then owns. Nothing subscribes, nothing syncs, and no registry entity exists: what an import leaves behind is the slug it came from, which is enough to recognise the same entry later and to say whether it has been rebuilt since. Identity is the set of disk hashes, not the version number, because the catalogue publishes no version at all. So re-importing an unchanged entry adds nothing, and a rebuild lands as a new version on the same definition rather than a second definition beside it. The number itself comes from the build date (2026.9.4), which orders correctly through the existing integer triple. Uploads are opened, appended to, and finished. One request was the wrong shape for a ten-gigabyte file: Cloudflare rejects a body over 100 MB on most plans, PHP has its own ceilings, and a connection that dropped at 90% cost the whole transfer. The server's offset is authoritative, a short write is rolled back rather than recorded, the assembled file is hashed streaming, and the response the version form consumes is unchanged. Disk `options` now survive both paths. `discard`/`ssd` and the varstore's `efitype`/`pre-enrolled-keys`/`ms-cert` describe the image that was built; the create call previously hardcoded `efitype=4m` and dropped the rest, which silently changed TRIM behaviour and Secure Boot state.
…nd resume The catalogue reads as the images list it feeds, because that is what an entry becomes: a row says what it costs to transfer and what it provisions, and importing writes an ordinary group, definition and version. A row the panel already holds says so, and one that has been rebuilt offers the update. Uploading gets the controls a multi-gigabyte transfer actually needs. The server's offset is authoritative, so Resume asks where the file got to and carries on from there rather than starting over, and Cancel gives the disk back instead of leaving the bytes for the sweep. `apiFetch` grew headers, an abort signal and upload progress, which is what the chunk endpoint needs and what the old uploader was reaching around it to get with a hand-rolled axios call.
`ServerPlacementService::rehome()` repointed `network_interface_id` at the bridge with the same name on the destination node and, finding one, cleared `flagged_at`. Name is all PVE needs; it is not what makes an address routable. Convoy records that in `address_block_group_to_network_interface`, and `vmbr0` exists on nearly every node, so the common case matched a bridge fronting a different pool, resolved the flag, and left the server holding addresses its new node cannot reach. `ServerNetworkService::syncSettings()` only refuses when the server is flagged, so the next sync would write one of those addresses into `ipconfig0` and PVE would re-bake it into a fresh cloud-init drive on the destination. The invariant existed only as validation closures in `UpdateAddressRequest` and `StoreServerRequest`, both creation-time and both driven by operator input, so nothing re-ran it on a placement change. It is now `AddressReachabilityService`: both requests call it, and so does `rehome()`, which flags naming the stranded pools instead of silently accepting the match. The service also carries `blocksReachableFrom()` and `interfacesCarrying()`, which migration preflight and adoption need next.
`$x?->y ?? $z` is redundant: the null-coalescing operator already suppresses a property read on null, so the nullsafe operator only hides that from the analyser. The rejected-value check in `diskOptions` was testing for a null the declared type says cannot arrive; `blank()` says what it meant.
…guest
Two features with the same hard part: an address is reachable from some nodes
and not others, and neither a placement change nor a guest arriving from
outside the panel has ever touched the address rows.
Migration. The address stays bound to its block and the server rebinds to a
node, which is Neutron's port/binding split; nothing gains a `node_id`. The
disposition is computed rather than asserted by the operator, because Convoy
already records which pools a bridge fronts and an operator ticking a
"preserve IP" box is asserting something they cannot see:
- same-named bridge attached to every pool the addresses live in: preserved,
and a running guest moves online.
- same-named bridge not attached to them: refused. PVE treats same-named
bridges as one network and Convoy does not, and the likeliest explanation
is an incomplete topology model. Attaching the pool is one click; an
unroutable address is an outage.
- no bridge of that name: the networks really are different, so the addresses
go back to their pool and the server gets new ones from the destination.
`GET /nodes/{node}/qemu/{vmid}/migrate` supplies PVE's own verdict, so storage
availability, passed-through devices and HA affinity are not re-derived. The
reallocated addresses are shown before the operator commits, computed by
running the allocator inside a transaction that is rolled back, and the change
needs an explicit acknowledgement. Writes are ordered the Neutron way: the
destination binding is reserved (`state_reason = migration`) before the task is
issued, the source is not released until PVE reports the guest landed, and a
failure frees the reservation.
Adoption. `/cluster/resources` minus the servers table by (cluster, vmid) is
the list, so nobody hand-types a node and a vmid the poll already sees. The
nine reconciliation cases for an address found on `ipconfig0` are implemented
against blocks scoped to what the guest's node can actually route. Adoption
never writes to the guest: no smbios stamp, no ipconfig rewrite, no NIC
rewrite, no disk resize, which is why `should_create_vm: false` is not reused.
An address it cannot resolve leaves the server flagged rather than rejecting
the whole guest, and `servers.ipconfig_managed` stops a later network sync from
overwriting an address the panel does not own.
Out of scope, deliberately: remote_migrate, SDN, DHCP/SLAAC leases, MTU, bulk
and HA migration, LXC, in-guest re-IP, auto-created blocks. Each is refused
with a reason at the point of use.
`ValidatePostSize` rejects any request whose `Content-Length` exceeds `post_max_size`, PUT included, and a stock php.ini sets that to 8M. A panel on default settings would have failed every upload with a 413 that names neither the limit nor the setting. The advertised chunk is now the configured size or what PHP will actually take, whichever is smaller.
The vocabulary listed `template-groups`, which lost its routes when templates became images, and omitted eleven resources that do have routes: version, audit-logs, clusters, storages, server-presets, backups, images, image-groups, isos, relays and settings. `ScopedTokenAbilities::requiredFor()` demands `*` for an unrecognised path segment, so each omission made that resource unreachable by any scoped token rather than merely unscopable -- a caller could not grant access to it at all. A coverage test now asserts the vocabulary and the route table agree in both directions.
Migrate sits in the server's PageToolbar and opens a destination picker whose rows carry the verdict rather than making the operator work it out: keeps its addresses, gets new addresses, or unavailable with the reason from Proxmox or from Convoy's own pool-to-bridge model. Picking a node that reallocates lists the exact addresses the server releases and the ones it gets, and the Migrate button stays disabled until the operator ticks that the guest's IP changes. Nothing is asserted by the operator that the panel can compute. The preview costs a second preflight call, so it is only fetched for a reallocation. The dialog also states, once, that it is cluster members only. Unmanaged Guests lists every QEMU guest on a registered node that no server owns, with a per-row Adopt. A guest that cannot be adopted shows the reason in place of the button rather than a disabled control with the reason hidden in a tooltip. A node that could not be asked gets its own line, because an empty list and an unanswered question read the same and mean opposite things. Adopting shows everything read off the guest before the operator commits: its resources, bridge and storage, and one row per address saying what the panel would do with it and why. The NICs past net0 are named as left alone, since a second interface silently ignored is worse than a second interface named. The only thing the operator supplies is the owner. Also carries the research doc the two features were designed from, which several docblocks cite by path.
…oles Three connected models, all shaped `<resource>.<action>` and all derived from the routes that exist rather than from a wishlist. Per-server sub-users. `server_subusers` holds a json list of `ServerPermission` values; `ServerPolicy`'s `__call` stops being a no-op and answers from that grant, with an unmapped ability denying so a new endpoint is closed rather than open. `Server::scopeOwnedBy` gains the share, which its own docblock nominated as the one place to change. `BackupPolicy` is deleted: every backup request calls `can(..., $server)`, so `ServerPolicy` was already the only policy consulted, and its three ambiguous abilities are renamed to `createBackup`/`restoreBackup`/`deleteBackup`. Sharing itself is owner-only -- a permission to grant permissions would be a permission to grant every one. Guest accounts. `users.type` separates accounts the operator provisioned from ones that exist only because a customer shared a server: filterable, sortable, counted apart on the overview, and on every payload. A guest cannot own a server, be signed in to over SSO, or hold an admin role -- the last enforced by a CHECK constraint, not only by the request layer. `PermissionSettings ::$allow_guest_accounts` is a kill switch in both directions: off means no guest is created and no existing guest signs in, with their shares kept so switching it back on restores what was there. Unknown invitees reuse `UserInviteService`; no second invite system. Admin roles. `admin_roles` plus `users.admin_role_id` replace `users.root_admin`, which is dropped; a `root_admin` accessor keeps `p:make-user`, the seeders and the Application API's own field meaning the Superadmin role. `AdminPermissions` derives the required permission from the request the way `ScopedTokenAbilities` derives a token ability, and both gates are enforced, so a token can never widen what its owner may do. `servers.power` and `users.impersonate` are carved out of their resources because support staff and billing staff are not the people who should hold `manage`. Four roles ship; a fifth is a duplicate-and-edit away. Existing root admins migrate onto Superadmin, so nobody is locked out and no operator has to do anything at upgrade time.
A poll landing between the PVE task finishing and the rebind job running sees the guest on the destination, re-homes the row itself, and — for a reallocating migration — flags it, because the destination bridge legitimately does not carry a pool the server has not given up yet. That flag is stale the moment it is written and would block the network sync the rebind is about to run, so the rebind clears it along with the bindings it knows are correct. Migration is also refused outright while a server is flagged. Every Proxmox URL a migration builds comes from `node_id`, and a flag is the panel saying it is not sure that column is true. And the adoptable-guest list no longer warns about a cluster whose first member was unreachable but whose second answered. The failure is recorded against the scope and dropped when any member of it answers: saying the list may be incomplete when it is not is how a warning gets ignored.
A single still cannot show a sub-second glitch, so this records the frames the compositor actually produced (CDP screencast, which emits only on visual change) and lays them out as a contact sheet composed in-browser -- no ffmpeg. Also lands the IPAM migration/import research that briefs the migration work, and documents the traps that produce false findings: page.goto() paints a real white frame, the collapsed sidebar has a hover flyout, "Add node" is a link to a page rather than a dialog, theme entries are menuitemradio, and dnd-kit needs priming past its activation constraint.
Client. A Sharing tab on a server the account owns: who it is shared with, what each of them can do, and a dialog of boxed permission checkboxes grouped the way the server's own sidebar is. An invitee with no account comes back with the link, shown rather than toasted, because that link is the whole handover on an install with no SMTP. Every other tab is now gated on the permission behind it, so a sub-user sees the parts of the server they can actually use; the nav reads `permissions` off the server payload, which the owner and an operator receive in full. Admin. A Roles screen, where a built-in role is duplicated and edited rather than rewritten -- that is what keeps one-role-per-account from costing anything when somebody needs Support plus Network. The user form swaps its administrator checkbox for a role select and an account type; the list gains a Guest badge, a type column and a type filter, and the detail page says which role an account holds instead of "Administrator" or "User". The admin sidebar, the account menu, the command palette and the admin route guard all read `adminPermissions` now, so a narrower role lands somewhere it can use rather than being bounced to the client area. Settings. A Sharing section with the guest-account switch, whose description carries what turning it off costs and how many accounts that is. The application-token ability picker picks up the eleven resources the vocabulary was missing.
ddev exports CACHE_STORE, QUEUE_CONNECTION and SESSION_DRIVER as real container environment variables, which Laravel's Env reads from $_SERVER before PHPUnit's <env> entries -- the same precedence problem tests/bootstrap.php already fixes for DB_DATABASE. The suite was therefore sharing one Redis with the development app, and `admin:overview`'s fifteen-second cache outlived the test that wrote it: the overview count assertions passed or failed depending on how recently the suite had last run. Also stops ServerSubuserData costing a query per row for `isPending`.
…laries The adoption routes and the permission/ability catalogues were written on separate branches, so the new routes reached integration in neither. Both coverage tests caught it, which is what they are for. Adopting an unmanaged guest is how a server comes into existence without a build, so it folds into `servers` rather than becoming a surface of its own.
The Role column fills, and under table-layout: auto its width: 100% beats a width declared on any other column, so Grants computed to 94px and wrapped a short summary over seven lines. Every row was 145-165px tall. Refusing to wrap makes this column's min-content width the thing the fill column yields to: Grants is now 355px and rows are a uniform 63px. Declaring w-[22rem] does not work here and was tried first.
… screens Covers what a happy-path click would not show: the chunked upload is driven with a deliberately short chunk so the server's offset is what the client resumes from, and a stale offset is asserted to be refused rather than silently appended.
The catalogue says a guest needs a TPM by carrying `tpm: v2.0` in its hardware and shipping no `tpmstate0` disk: the image is generalized, and one shipped state volume would give every guest built from it the same endorsement key. So the consumer allocates it. Both of cofoundry's own installers do exactly that (`src/registry/create.ts` marks `tpm` synthesized and emits `--tpmstate0 <storage>:0,version=`; `cf-cluster-templates.sh` the same). The panel was the odd one out: the import dropped the key, so windows-server-2022 and windows-server-2025 imported without a TPM. Server 2025 requires one to boot, and the failure lands at first power-on rather than at import. `tpm` now survives into the overlay as a synthesized key, is stripped before the create payload (Proxmox has no `tpm` parameter and would 400), and becomes a freshly allocated tpmstate0. Validation accepts only v1.2/v2.0, since the schema this rule normally consults has no `tpm` key to check against.
The catalogue numbers nothing, so the import derived a version from the build date: `2026.9.4`. Two builds of one recipe in a day then collided on a column that is unique per definition, and the collision was resolved by incrementing the patch. That produced `2026.9.5` for a build made on the 4th, and took the number a genuine build on the 5th would want, so the drift compounded. The identity was already published. `built_at` is second-resolution and unique per build; truncating it to a date is what manufactured the collision. So it is stored as itself, and the version goes back to being what it is for a catalogue image: which build the panel holds, counting up. Deliberately NOT fixed by adding a `build` field to the catalogue. That would need durable state its generator does not have (the registry is assembled by scanning per-build sidecars), would break on a fork or a clean republish, and would bump `schema_version`, forcing the panel, coport and every other consumer to land together -- all to replace an identifier already published for free. The build number is taken from the highest build currently held, so deleting the newest frees its number. That is safe rather than overlooked: a deployment records its version by foreign key and never by label, and a version cannot be deleted while a deployment points at it. Existing registry rows are renumbered in the order they already sort in. `built_at` is left null for them rather than guessed back out of the old label, since the collided ones encode the wrong day; a re-import repairs it, because an unchanged build is matched by content and would otherwise never create a row to carry it. The catalogue and the versions list now show the real build date, so the number is never the only thing on screen.
`qm migrate` is intra-cluster only and clustering standing nodes is not available to us: a node holding guests cannot join, joining replaces /etc/pve, and a two-member cluster drops to read-only whenever its peer is down. `qm remote-migrate` needs no cluster but Proxmox marks it `EXPERIMENTAL feature!` and documents it nowhere. So a destination outside the source's cluster takes a second transport: `vzdump` on the source, a direct node-to-node download, `qmrestore` on the destination, all driven over Anchor. It is deliberately offline; the guest is down for the whole transfer and nothing here pretends otherwise. The transport is derived, not chosen. The operator picks a destination and the panel decides how the guest gets there, exactly as it already decides what happens to the addresses. The address disposition logic is reused unchanged -- the only thing skipped for an Anchor destination is PVE's `allowed_nodes`, which is the source cluster's opinion of its own members and would block every node outside it. The ordering is the whole design: stop, export, install, verify, discard, destroy the source, rebind, start. The source guest is not destroyed until the destination guest is verified present with the disks it left with, so every failure before that point is undone by starting a guest nobody touched. AnchorMigrationRollbackService does exactly that, plus freeing the destination reservation and removing whatever a half-finished restore left behind -- which is what stops a retry finding the VMID taken and ending up with two guests. Two deltas from docs/migration-anchor-contract.md, both deliberate: - `Discard` runs before the destroy rather than after it. A step that runs once the source is gone has no recovery, and the artifact is redundant the moment the destination is verified, so moving it one step earlier keeps the point of no return in exactly one place. - An artifact that has expired or been swept by an Anchor restart is a re-export, not a failure. The source guest is still there, so the answer is another dump; it is capped at two so a node that loses every artifact stops the migration rather than exporting forever. Both nodes must be enrolled and must advertise `migration.export` / `migration.install`; an enrolled but older agent is refused by name rather than accepted, because the install pipeline finishes by running `qm template` on what it restored and an agent that does not know it is servicing a migration would turn the guest into a template.
… fails Almost all of these assert an order rather than a set. "The source was destroyed" is not the property that matters; "the source was destroyed after the destination was verified" is, so the suite reads Http::recorded() back as a labelled sequence and asserts on that. Nine injected failures -- export fails, export names no artifact, hash mismatch, install fails, cancellation on either side, no guest at the destination VMID, wrong disk size, discard refused -- share one set of assertions, because they all have to leave the same three things true: the source guest still on the source node and running if it was running, the destination reservation back in the pool, and no destroy of the source. The discard case is the reason the rollback boundary is `source_destroyed_at` and not `verified_at`: it gets past the verification and is still recoverable, because the source is still there to go back to. Runs the real chain on the sync queue rather than asserting on a list of jobs a test assembled itself, which is how the "dispatch inside the transaction" bug this fixes was found -- an inline failure was rolling back the rows the failure exists to leave behind. The one place that would prove less than driving the job directly is the poll loop, because the sync queue makes release() a no-op, so the phase-polling test drives ExportGuestJob a call at a time. The two plan tests that encoded the cluster-only world are updated rather than deleted: a standalone node now gets an Anchor candidate instead of an empty list, and "no other member" is now "no other node".
A destination outside the cluster means the guest is stopped while its whole disk is copied to another node, and that is not something to learn from a progress bar once it is running. The consequence goes on the control it belongs to, with the figure the panel actually has: "Stopped and copied across, up to 32 GiB", beside the "Moves without stopping" a cluster destination already showed. Drops the note claiming cluster members are the only destinations, which stopped being true one commit ago.
The agent's implementation diverged in ways a consumer has to know about: the enum is TemplateAction not Command, download auth reuses TemplateClaims with a new Fetch action rather than a separate token type, export status lives on its own route, there is a new `dumping` job status, multi-range returns 200 rather than 206, and an artifact is cleared on agent restart as well as by TTL -- so "artifact gone" has to be a recoverable re-export.
Anchor's install pipeline exists to import images, and Convoy offers a guest only when it reports `template: 1`, so a restore ends with `qm template` unless told otherwise. A migration restores the tenant's own machine, so the default would hand them back something that cannot be started, after a migration that reported success. The agent defaults `finalize_as_template` to true so every existing import caller stays correct, which is exactly why the panel has to send false explicitly. The agent advertises `migration.install` only when it honours the flag, so an agent too old to understand it is refused a guest rather than quietly templating one. The guard test reads the bearer token's `template` claim, because that is where the work order travels; asserting on the request body passes vacuously.
Exercises the panel's own HTTP API with a real session, so nothing in the path is mocked: discover the node's storages, adopt a running guest off Proxmox, read the migration plan, and commit it.
The agent serves GET and DELETE on one route, /artifacts/{artifact}. Discard
was addressed to /api/v1/templates/artifacts/{artifact}, which exists nowhere,
so the agent's router answered 404 -- and a 404 from discard reads exactly like
an artifact already swept by its TTL, so the failure looked like a race rather
than a wrong URL.
The tests pinned the invented path too, which is why every one of them passed
while a live migration lost its cleanup step. They now match on the real route
and on the method, so the download fake cannot answer the DELETE.
A live run addressed discard to a route the agent does not serve; the agent answered 404 and a migration whose destination had already been verified was marked failed, leaving the guest present on both nodes for an operator to reconcile by hand. Cleanup is surfaced but no longer fatal. The archive carries a TTL and is swept whether or not anyone asks, so carrying on costs at worst a temporary file that outlives its use by a day; aborting costs a duplicate guest and a manual reconciliation.
…behind Proxmox rejects a DELETE carrying a body with "Unexpected content for method 'DELETE'". `destroyGuest` sent `purge` as one, so the call always failed -- and the rollback swallows its failures as warnings, so nothing surfaced. A failed migration therefore left the guest on the destination as well as the source, stranding the same server on two nodes. Found by running a rollback against the live API with a real guest in place: the guest survived and no qmdestroy task was ever issued. Every other DELETE in the Proxmox namespace sends no body at all; this was the one that did. The test drives the client directly, because this path only runs during a rollback -- the happy path's DELETE is the source destroy, a different method that takes no purge, and asserting through a migration matches that one instead.
`MailConfigurator::isConfigured()` consults MailSettings, not the mail config. With `mail.default` on smtp, `managedElsewhere()` is false and the stored host decides -- so setting `mail.mailers.smtp.host` left it empty and no invitation was sent. The test passed anyway wherever the database already had mail configured, and failed on CI's fresh one. It now sets the setting it depends on, the same way it already sets the permission setting beside it.
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
1 participant
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.
Four workstreams, built in parallel and integrated here. Needs
ConvoyPanel/anchor#1 for the migration transport.
Images
Cofoundry replaced vzdump archives with disk images, and the panel had no way to
consume them.
convoy.registry.urldefaults to the publishedregistry. A browsable catalogue with one-click import, idempotent by content:
identity is the set of disk sha256s, so re-importing an unchanged entry adds
nothing and a rebuild lands as a new version on the same image.
breaks on Cloudflare's body limit and PHP's own ceilings. Now open →
PUTchunk at
Upload-Offset→ finalize, resumable, with the server's offsetauthoritative and a sweeper for abandoned uploads. Hand-rolled; tus-php,
laravel-chunk-upload and the tus clients were each evaluated and rejected in
the commit message.
tpm: v2.0and shipping notpmstate0disk, because the state volume must beunique per guest. The import dropped the key, so windows-server-2022 and 2025
imported without a TPM, which Server 2025 requires to boot. Now allocated.
one recipe in a day collided and the collision was resolved by naming the
wrong day.
built_atis stored as itself and the number is the panel's owncount of what it holds.
Permissions, sub-users and roles
the issue's wishlist.
users.type, separated everywhere an admin looks, andgated behind one global switch that also signs existing guests out.
users.root_adminis droppedbehind an accessor, with a migration that leaves existing root admins as
Superadmin and nobody locked out.
TokenAbilities::RESOURCESstill listed theretired
template-groupsand omitted 11 live admin resources, all of whichfell through to
*.Migration and VM import
Design notes in
docs/ipam-migration-research.mdanddocs/migration-anchor-contract.md.qm migrate;anything else goes over Anchor as an offline export/restore.
qm remote-migrateis deliberately not used: Proxmox marks itEXPERIMENTAL feature!in its own source and documents it nowhere.Every earlier failure rolls back by restarting a guest that was never touched.
with the address reconciliation cases the research doc enumerates.
ServerPlacementService::rehome()matched adestination bridge by name alone and cleared the flag, leaving a server with
addresses unroutable from its new node.
UI verification
A frame-by-frame harness (
.sbx/dev/uiverify) that records what the compositoractually painted, because a single still cannot show a sub-second glitch. Used
to check every admin route at desktop and phone width, in both themes.
Verification
1145 tests, PHPStan, Pint, typecheck and build all clean.
Driven end to end against two live Proxmox 9.2.2 nodes: a real guest adopted off
one node and migrated three times, both directions, each run leaving it present
on the destination, gone from the source, and no artifact behind.
Four bugs were found only by that live run, each of which every unit test had
passed over: an auth mismatch between the two repos, a discard URL the agent
does not serve, a failed cleanup aborting an already-successful migration, and a
destroyGuestthat always failed silently because Proxmox rejects a DELETEcarrying a body. That last one is why a failed migration left the same server on
two nodes.