proxmox: Create by cloning a template, seed upload, config, start
Create validates placement, then under the per-name lock adopts the VM a previous attempt left (by name, or at a taken VMID after the ownership check of spec entry 2) or full-clones the template onto the UID's deterministic VMID with the mark in the clone call. It then uploads the seed over any existing file, writes cores, memory, agent, tag, ide2 and net0 when configured, grows the boot disk when diskGiB is larger, starts, and waits for running/running. waitUnlocked no longer waits on a suspended lock: a hibernated VM is judged at once, so ours is Permanent and a foreign one is probed past. The template is checked only on the allocation path, and its shape (net0, boot disk size) is refused before any clone exists. Spec entries 2, 4 and 10 updated to match. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -10,7 +10,7 @@ Pairs with [docs/plans/2026-09-18-1502-proxmox-provider.md](../plans/2026-09-18-
|
||||
- [x] Phase 3 — Extract `nocloud` and `keyedlock` from kvm2
|
||||
- [x] Phase 4 — Seam, HTTP client, decorator, fake
|
||||
- [x] Phase 5 — Get, ListByTag, task helpers
|
||||
- [ ] Phase 6 — Create
|
||||
- [x] Phase 6 — Create
|
||||
- [ ] Phase 7 — Delete, Checker, CRD comment
|
||||
- [ ] Phase 8 — Contract and observability sweep, integration test, testing.md
|
||||
- [ ] Phase 9 — Deployment and documentation
|
||||
@@ -657,3 +657,135 @@ Worth noting:
|
||||
- Nothing in this phase talked to the host. Every Proxmox message a test
|
||||
relies on comes from `testdata/` or from fake errors that
|
||||
`TestFakeAPI_errorsMatchCaptures` ties to the captures.
|
||||
|
||||
## Phase 6 — Create
|
||||
|
||||
`create.go` holds `Create` and its steps: placement, the by-name lookup,
|
||||
the template check, the VMID probe with its ownership check (`inspect`),
|
||||
the clone, and `provision` (seed, config, resize, start). `configParams`,
|
||||
`rewriteNet0`, `resizeTarget`, `bootDisk` and `diskBytes` are pure helpers
|
||||
with their own table tests. `seed.go` gains `uploadSeed`, which uploads
|
||||
over any existing file with no delete first (F12). `task.go` gains
|
||||
`waitRunning`, built on `pollUntil` with `budgets.running`. It finishes
|
||||
only at `running`/`running`, and a `stopped` status after the start is
|
||||
Transient at once. The Create stub is gone, and `TestStubs_answerTransient`
|
||||
now covers only Delete.
|
||||
|
||||
The spec won over the plan wherever the Phase 6 notes said it would. The
|
||||
plan's allocation loop is replaced by entry 2: a taken VMID and a clone
|
||||
answering F4 both go through the ownership check before any probing, and a
|
||||
locked or nameless entry is waited on with `waitUnlocked` and the clone
|
||||
budget. The seed is uploaded without a delete. The resize is a task. `net0`
|
||||
changes only when `bridge` or `vlanTag` is set, and `tag=` is replaced or
|
||||
appended in place.
|
||||
|
||||
Judgment calls, with spec entries 2, 4 and 10 updated to match:
|
||||
|
||||
- **The Phase 5 open question: a `suspended` lock is not waited on.**
|
||||
`waitUnlocked` now returns at once when it sees `suspended`, because a
|
||||
hibernated VM is not a task and would otherwise burn the 10-minute clone
|
||||
budget on every attempt. The ownership check then judges the VM the way
|
||||
`Get` does. A foreign hibernated VM is probed past. Ours is Permanent
|
||||
("resume or destroy it"), following kvm2's precedent for a paused or
|
||||
pmsuspended domain in Create. Transient was the alternative: it would
|
||||
self-heal once an admin acts, but it would retry indefinitely on
|
||||
something only an admin can fix. `TestWaitUnlocked_table` has a new row,
|
||||
and `TestCreate_hibernatedVMIsNotWaitedOn` covers both cases.
|
||||
- **The template is checked only when a clone is needed.** Spec entry 10
|
||||
read the template's config in step 2, before the by-name lookup. A retry
|
||||
for a VM that already exists would then fail Permanent if the template
|
||||
had been deleted since, which entry 4 explicitly allows. The template
|
||||
check now runs after the by-name lookup misses, so a repeat Create costs
|
||||
three calls: resources, config and status
|
||||
(`TestCreate_adoptionDoesNotNeedTheTemplate`).
|
||||
- **The template's shape is checked before the clone.** No `net0` while
|
||||
`bridge` or `vlanTag` is set, or no boot disk (or none with a readable
|
||||
`size=`) while `diskGiB` is set, is Permanent. Found after the clone, the
|
||||
VM would already carry the bad config, so fixing the template could not
|
||||
help that Proxy's next attempt. `template()` runs the same
|
||||
`configParams` and `resizeTarget` over the template's config that
|
||||
`provision` later runs over the clone's.
|
||||
- **An empty `req.Name` is Permanent at placement.** A nameless request
|
||||
would match nameless mid-clone entries in the by-name lookup.
|
||||
- **Adopted VMs get one `status/current` read** rather than trusting the
|
||||
listing's status, which may be stale after a lock wait. A running VM,
|
||||
whatever its `qmpstatus`, returns its ID at once, as entry 10 says. `Get`
|
||||
then reports prelaunch as Provisioning and paused as Stopped.
|
||||
- **After the clone task, Create reads the config through
|
||||
`waitUnlocked`.** That read supplies the `net0` and boot disk the later
|
||||
steps need, and it turns a clone that ended but kept its lock (F15) into
|
||||
a Transient lock wait rather than a refused config write.
|
||||
- **An F4 whose VMID then resolves to no QEMU VM probes on.** Either the
|
||||
racer's clone failed and removed it, or a container took it.
|
||||
- **Errors carry no ID** (`wrapErr("create", "", …)`), since the VMID is
|
||||
known only after allocation. The messages name the VMID wherever one
|
||||
exists.
|
||||
|
||||
Test names: every plan and entry 21 name exists except
|
||||
`TestCreate_resizeOnlyWhenLarger`. It is covered by the spec's pair:
|
||||
`TestCreate_resizeWaitsForTask` for the larger case and
|
||||
`TestCreate_resizeSkippedWhenNotLarger` for equal, smaller and unset. Tests
|
||||
beyond both lists: `TestCreate_vmThatStopsAfterStartIsTransientAtOnce`,
|
||||
`TestCreate_hibernatedVMIsNotWaitedOn`,
|
||||
`TestCreate_templateShapeRefusedBeforeClone`,
|
||||
`TestCreate_adoptionDoesNotNeedTheTemplate`,
|
||||
`TestCreate_cloneTargetAndStorage` (read from the decorator's V(2) clone
|
||||
line), `TestCreate_adoptedVMStaysOnItsNode`, `TestCreate_outcomeLine`,
|
||||
`TestCreate_quietAtV0`, `TestRewriteNet0_table`, `TestBootDisk_capturedClone`
|
||||
and `TestDiskBytes_table`. The only new test helpers are the small ones in
|
||||
`create_test.go` (`slot`, `newCreateFixture`, `req`/`create`/`createFails`,
|
||||
`assertRunning`, `callsBefore`, `clearLockOnVMConfig`). Everything else
|
||||
reuses the Phase 5 fixture and fake. Two tests check config-write contents
|
||||
without a recording wrapper. `TestCreate_markWrittenInCloneCall` reads the
|
||||
VM at the start of `UploadISO`, which is after the clone and before the
|
||||
config write. `TestCreate_noBridgeOverrideLeavesNet0` changes `net0` at the
|
||||
start of `SetVMConfig`, so a write that carried `net0` would put the old
|
||||
value back.
|
||||
|
||||
A mutation check applied 18 mutations, and each failed at least one test.
|
||||
They were: no lock wait in the ownership check; taken VMIDs never
|
||||
inspected; the F4 race skipping the ownership check; no cloud-init drive
|
||||
check; upload or resize task not waited on; resize at equal size; a
|
||||
CD-ROM counted as a boot disk; prelaunch counted as running; the suspended
|
||||
lock waited on; `net0` always written; `bridge=` appended instead of
|
||||
replaced; duplicate names ignored; a running VM re-provisioned; `target`
|
||||
always set; a foreign UID adopted; the tag dropped; probes not capped by
|
||||
the range. The last one was first **missed**. A 4-VMID range with 32
|
||||
probes wraps, re-inspects each VM eight times and still ends Transient, so
|
||||
`TestCreate_vmidRangeExhaustedIsTransient` now pins five config reads.
|
||||
Five mutations did not compile on the first try and were redone. Every
|
||||
mutation was reverted, and the files were compared identical to backups.
|
||||
|
||||
```bash
|
||||
go test -race -count=2 ./internal/provider/proxmox/ # ok
|
||||
grep -n 'CloudInit' internal/provider/proxmox/create.go internal/provider/proxmox/seed.go
|
||||
# seed.go: the pass-through into nocloud.BuildSeedISO; create.go: len(req.CloudInit)
|
||||
grep -n 'not implemented' internal/provider/proxmox/*.go | grep -v _test # Delete only
|
||||
grep -n 'context.Background()' internal/provider/proxmox/*.go | grep -v _test.go # nothing
|
||||
go build ./... && go vet ./... && go vet -tags=integration ./...
|
||||
go test -short -count=1 ./... # all ok but the parked kvm2 flake, below
|
||||
go test -count=1 -v ./internal/provider/proxmox/ | grep -c '^=== RUN' # 393 before, 519 after
|
||||
bin/golangci-lint run --disable staticcheck ./internal/provider/proxmox/...
|
||||
```
|
||||
|
||||
Worth noting:
|
||||
|
||||
- `go test -short ./...` failed once on the parked kvm2 flake
|
||||
`TestCheck_hostsCheckedConcurrently` (measured 199.98 ms against a
|
||||
200 ms floor). Three reruns passed. This phase does not touch kvm2.
|
||||
- The new lint findings follow the baseline's shape. There are logcheck
|
||||
hits on `create`, `allocate` and `clone` (ctx plus logger, as in kvm2),
|
||||
and goconst on test strings. One more unparam than the notes list:
|
||||
`(*fakeAPI).iso`'s `storage` always receives `fakeISOStorage`, because
|
||||
this phase is its first caller. `addTemplate`'s hit changed from a
|
||||
constant VMID to an unused result.
|
||||
- The config write sets `agent=1` and `tags=proxy_operator` as values. A
|
||||
template's agent options (such as `fstrim_cloned_disks=1`) and its own
|
||||
tags are replaced, not merged. That is what entry 10 specifies, and the
|
||||
guide should say so.
|
||||
- In the fake, a cross-node clone puts the config on the target at once.
|
||||
Real Proxmox may move it at the end of the task, in which case a lock
|
||||
wait on the target node would see `does not exist` and the next attempt
|
||||
would find the VM. This stays untested, like every cross-node path
|
||||
(entry 24).
|
||||
- Nothing in this phase talked to the host.
|
||||
|
||||
Reference in New Issue
Block a user