From 34f132d421f8ad9973e10120ea5fcbfbca674e27 Mon Sep 17 00:00:00 2001 From: kacerr Date: Wed, 23 Sep 2026 15:32:31 +0000 Subject: [PATCH] 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) --- .../2026-09-18-1502-proxmox-provider.md | 134 +- docs/proxmox-provider-spec.md | 28 +- internal/provider/proxmox/create.go | 425 ++++++ internal/provider/proxmox/create_test.go | 1269 +++++++++++++++++ internal/provider/proxmox/proxmox.go | 10 +- internal/provider/proxmox/proxmox_test.go | 11 +- internal/provider/proxmox/seed.go | 22 + internal/provider/proxmox/task.go | 23 +- internal/provider/proxmox/task_test.go | 6 +- 9 files changed, 1900 insertions(+), 28 deletions(-) create mode 100644 internal/provider/proxmox/create.go create mode 100644 internal/provider/proxmox/create_test.go diff --git a/docs/plans-executions/2026-09-18-1502-proxmox-provider.md b/docs/plans-executions/2026-09-18-1502-proxmox-provider.md index d2e7308..2eac296 100644 --- a/docs/plans-executions/2026-09-18-1502-proxmox-provider.md +++ b/docs/plans-executions/2026-09-18-1502-proxmox-provider.md @@ -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. diff --git a/docs/proxmox-provider-spec.md b/docs/proxmox-provider-spec.md index 84712ab..b697917 100644 --- a/docs/proxmox-provider-spec.md +++ b/docs/proxmox-provider-spec.md @@ -72,7 +72,12 @@ lands on the same number: `vmidFor(uid, attempt)` is seed step (entry 10). It is **foreign** when the name differs: move to the next attempt. The same name with no mark, or with a different non-empty `uid`, is `ErrPermanent` naming the VMID and both UIDs; the - operator never takes over a VM it did not create. + operator never takes over a VM it did not create. A `suspended` lock (a + VM hibernated to disk) is not waited on: it is not a task and never + clears by itself. Ownership is judged at once, as `Get` does (entry 9): + a foreign hibernated VM is probed past, and ours is `ErrPermanent` + (resume or destroy it), like kvm2's paused domain. Container and + template entries at a VMID are foreign without a config read. The wait in step 3 is what closes the duplicate-VM bug Phase 0 found: after a crash mid-clone, the retry's by-name lookup misses the nameless VM, but @@ -113,7 +118,12 @@ template may be edited, replaced or deleted without affecting running VMs. Before cloning, Create reads the template's config and refuses it with `ErrPermanent` when it is not a template (`template` is not 1) or when any disk slot holds a Proxmox cloud-init drive (a value containing `cloudinit`), -which would compete with the provider's seed (entry 5). The template must +which would compete with the provider's seed (entry 5). Because the clone +copies the template's config, what the config or resize step would refuse +on the clone is refused on the template too, before any VM exists: no +`net0` while `bridge` or `vlanTag` is set, and no boot disk, or one +without a readable `size=`, while `diskGiB` is set. A Permanent error +after the clone would leave a VM that fixing the template cannot help. The template must boot from its first disk, run cloud-init with the NoCloud datasource, and either ship `qemu-guest-agent` or be able to install it (entry 7). @@ -320,11 +330,15 @@ waits for the clone task first. Steps, each idempotent, in order, under the per-name lock (entry 19): 1. Validate placement (entry 3). No backend call yet. -2. `GET /cluster/resources`. Find the template's node; a missing template - is `ErrPermanent`. Read the template's config and refuse it as entry 4 - says. -3. Find or allocate the VMID (entry 2). An adopted VM that is running - returns its ID at once. An adopted VM that is stopped continues at step 5. +2. `GET /cluster/resources` once, then look the name up (entry 2). +3. When the name finds nothing: find the template's node (a missing + template is `ErrPermanent`), read the template's config and refuse it as + entry 4 says, then allocate the VMID (entry 2). The template is checked + only here, because a VM that already exists no longer depends on it + (entry 4): a template deleted after its clone must not fail that + clone's adoption. An adopted VM, found by name or by the probe, gets one + `status/current` read. Running, it returns its ID at once. Stopped, it + continues at step 5. 4. Clone and wait (clone budget). 5. Build the seed and upload it to the VM's node, overwriting; wait (upload budget). diff --git a/internal/provider/proxmox/create.go b/internal/provider/proxmox/create.go new file mode 100644 index 0000000..df4c7ae --- /dev/null +++ b/internal/provider/proxmox/create.go @@ -0,0 +1,425 @@ +package proxmox + +import ( + "context" + "fmt" + "maps" + "regexp" + "slices" + "strconv" + "strings" + + "github.com/go-logr/logr" + + "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider" +) + +// maxProbes bounds the VMID probe of spec entry 2. +const maxProbes = 32 + +// diskKey is a Proxmox disk slot: the keys a boot disk, a CD-ROM or a +// cloud-init drive can sit on. +var diskKey = regexp.MustCompile(`^(ide|sata|scsi|virtio)[0-9]+$`) + +// ownedVM is a VM Create has cloned or adopted: where it lives and its +// config as last read, which the config and resize steps start from. +type ownedVM struct { + node string + vmid int + cfg vmConfig +} + +// Create is spec entry 10: validate placement, then, under the per-name +// lock, adopt the VM a previous attempt left or clone a new one, and carry +// it through seed, config, resize and start until it runs. +func (p *Provider) Create(ctx context.Context, req provider.CreateRequest) (string, error) { + node, mt, tmpl, err := p.placement(req) + if err != nil { + return "", p.wrapErr("create", "", err) + } + log := p.logger(ctx).WithValues("name", req.Name) + log.V(2).Info("proxmox create resolved", + "zone", node, "machineType", req.Placement.MachineType, "cores", mt.Cores, "memoryMiB", mt.MemoryMiB, + "diskGiB", mt.DiskGiB, "image", req.Placement.Image, "templateVMID", tmpl.VMID, "storage", p.cfg.Storage, + "isoStorage", p.cfg.ISOStorage, "bridge", p.cfg.Bridge, "vlanTag", p.cfg.VLANTag, "cloudInitBytes", len(req.CloudInit)) + + unlock, err := p.lockName(ctx, req.Name) + if err != nil { + return "", p.wrapErr("create", "", err) + } + defer unlock() + + vm, err := p.create(ctx, log, node, mt, tmpl, req) + if err != nil { + return "", p.wrapErr("create", "", err) + } + log.V(1).Info("proxmox vm ensured running", "vmid", vm.vmid, "node", vm.node) + return formatProviderID(vm.vmid, req.Name), nil +} + +// placement is step 1: every catalogue key is checked before any backend +// call, and an unknown one is Permanent naming the field and the value +// (spec entry 3). Region is ignored. +func (p *Provider) placement(req provider.CreateRequest) (string, provider.ProxmoxMachineType, provider.ProxmoxTemplate, error) { + var ( + mt provider.ProxmoxMachineType + tmpl provider.ProxmoxTemplate + ) + if req.Name == "" { + return "", mt, tmpl, permanentf("the request has no instance name") + } + node, err := p.node(req.Placement.Zone) + if err != nil { + return "", mt, tmpl, err + } + mt, ok := p.cfg.MachineTypes[req.Placement.MachineType] + if !ok { + return "", mt, tmpl, permanentf("placement.machineType %q is not in proxmox.machineTypes", req.Placement.MachineType) + } + tmpl, ok = p.cfg.Templates[req.Placement.Image] + if !ok { + return "", mt, tmpl, permanentf("placement.image %q is not in proxmox.templates", req.Placement.Image) + } + return node, mt, tmpl, nil +} + +// create is steps 2 to 9. The template is checked only when a clone is +// needed: a VM a previous attempt created no longer depends on it (spec +// entry 4), so a template removed since must not fail its adoption. +func (p *Provider) create(ctx context.Context, log logr.Logger, node string, mt provider.ProxmoxMachineType, tmpl provider.ProxmoxTemplate, req provider.CreateRequest) (ownedVM, error) { + rs, err := p.api.ClusterResources(ctx) + if err != nil { + return ownedVM{}, fmt.Errorf("listing cluster resources: %w", err) + } + vm, adopted, err := p.adoptByName(ctx, rs, req) + if err != nil { + return ownedVM{}, err + } + if !adopted { + src, err := p.template(ctx, rs, req.Placement.Image, tmpl.VMID, mt) + if err != nil { + return ownedVM{}, err + } + if vm, adopted, err = p.allocate(ctx, log, rs, node, src, req); err != nil { + return ownedVM{}, err + } + } + log = log.WithValues("vmid", vm.vmid, "node", vm.node) + if adopted { + st, err := p.api.VMStatus(ctx, vm.node, vm.vmid) + if err != nil { + return ownedVM{}, fmt.Errorf("reading status of VM %d: %w", vm.vmid, err) + } + log.V(1).Info("proxmox vm adopted", "status", st.Status, "qmpstatus", st.QMPStatus) + if st.Status == "running" { + return vm, nil + } + } + return vm, p.provision(ctx, log, vm, mt, req) +} + +// adoptByName is the by-name half of spec entry 2: two VMs with the name +// are for an admin to sort out, one goes through the ownership check. +func (p *Provider) adoptByName(ctx context.Context, rs []vmResource, req provider.CreateRequest) (ownedVM, bool, error) { + var found []vmResource + for _, r := range rs { + if r.Type == "qemu" && r.Template == 0 && r.Name == req.Name { + found = append(found, r) + } + } + switch len(found) { + case 0: + return ownedVM{}, false, nil + case 1: + return p.inspect(ctx, found[0], req) + } + vmids := make([]string, len(found)) + for i, r := range found { + vmids[i] = strconv.Itoa(r.VMID) + } + return ownedVM{}, false, permanentf("VMs %s are all named %s; remove all but one", strings.Join(vmids, ", "), req.Name) +} + +// inspect is the ownership check of spec entry 2 on a VMID that is taken. +// It waits out a task's lock (a clone in progress is also nameless), then +// reads the name and the mark: another name is someone else's VM, and our +// name without our mark is never taken over. +func (p *Provider) inspect(ctx context.Context, r vmResource, req provider.CreateRequest) (ownedVM, bool, error) { + cfg, err := p.waitUnlocked(ctx, r.Node, r.VMID, p.budgets.clone) + if err != nil { + return ownedVM{}, false, err + } + if cfg["name"] != req.Name { + return ownedVM{}, false, nil + } + mark := parseDescription(cfg["description"]) + switch { + case !mark.Managed: + return ownedVM{}, false, permanentf("VM %d is named %s but carries no proxy-operator mark; it was not created by this operator", r.VMID, req.Name) + case mark.UID != "" && mark.UID != req.UID: + return ownedVM{}, false, permanentf("VM %d named %s belongs to proxy uid %s, not %s", r.VMID, req.Name, mark.UID, req.UID) + case cfg["lock"] == lockSuspended: + return ownedVM{}, false, permanentf("VM %d named %s is hibernated (lock suspended); resume or destroy it in Proxmox", r.VMID, req.Name) + } + return ownedVM{node: r.Node, vmid: r.VMID, cfg: cfg}, true, nil +} + +// template finds the clone source and refuses one that is not a template +// or brings a Proxmox cloud-init drive, which would compete with the seed +// (spec entry 4). The clone copies the template's config, so what the +// config or resize step would refuse on the clone is refused here, while no +// VM exists that a fixed template could no longer help. +func (p *Provider) template(ctx context.Context, rs []vmResource, image string, vmid int, mt provider.ProxmoxMachineType) (vmResource, error) { + i := slices.IndexFunc(rs, func(r vmResource) bool { return r.VMID == vmid && r.Type == "qemu" }) + if i < 0 { + return vmResource{}, permanentf("placement.image %q: template VM %d does not exist", image, vmid) + } + src := rs[i] + cfg, err := p.api.VMConfig(ctx, src.Node, vmid) + if err != nil { + return vmResource{}, fmt.Errorf("reading config of template %d: %w", vmid, err) + } + if cfg["template"] != "1" { + return vmResource{}, permanentf("placement.image %q: VM %d is not a template", image, vmid) + } + for _, k := range slices.Sorted(maps.Keys(cfg)) { + if diskKey.MatchString(k) && strings.Contains(cfg[k], "cloudinit") { + return vmResource{}, permanentf("placement.image %q: template %d has a Proxmox cloud-init drive on %s; remove it, the provider attaches its own seed", image, vmid, k) + } + } + shape := ownedVM{node: src.Node, vmid: vmid, cfg: cfg} + if _, err := p.configParams(shape, mt, ""); err != nil { + return vmResource{}, err + } + if _, _, _, err := resizeTarget(shape, mt.DiskGiB); err != nil { + return vmResource{}, err + } + return src, nil +} + +// allocate is the probe of spec entry 2: the attempt-th VMID for the UID +// is cloned onto when the listing shows it free and put through the +// ownership check when it is taken, or when the clone finds it taken after +// all (F4). The boolean reports an adoption rather than a fresh clone. +func (p *Provider) allocate(ctx context.Context, log logr.Logger, rs []vmResource, node string, src vmResource, req provider.CreateRequest) (ownedVM, bool, error) { + taken := make(map[int]vmResource, len(rs)) + for _, r := range rs { + taken[r.VMID] = r + } + rng := p.cfg.VMIDRange + probes := min(maxProbes, rng.Max-rng.Min+1) + for attempt := range probes { + vmid := vmidFor(req.UID, rng.Min, rng.Max, attempt) + r, isTaken := taken[vmid] + if !isTaken { + vm, err := p.clone(ctx, log, node, src, vmid, req) + if !isAlreadyExists(err) { + return vm, false, err + } + var found bool + if r, found, err = p.resolve(ctx, vmid); err != nil { + return ownedVM{}, false, err + } + if !found { + log.V(1).Info("proxmox vmid taken by something other than a VM, probing on", "vmid", vmid, "attempt", attempt) + continue + } + } + if r.Type == "qemu" && r.Template == 0 { + vm, ours, err := p.inspect(ctx, r, req) + if err != nil || ours { + return vm, ours, err + } + } + log.V(1).Info("proxmox vmid taken by another vm, probing on", "vmid", vmid, "attempt", attempt) + } + return ownedVM{}, false, transientf("no free VMID for %s after %d probes in %d..%d", req.Name, probes, rng.Min, rng.Max) +} + +// clone is step 4: a full clone carrying the mark, so the VM is never +// unmarked (spec entry 8), then its unlocked config. A taken VMID comes +// back as the API's error for allocate to judge. +func (p *Provider) clone(ctx context.Context, log logr.Logger, node string, src vmResource, vmid int, req provider.CreateRequest) (ownedVM, error) { + params := cloneParams{ + NewID: vmid, + Name: req.Name, + Description: formatDescription(req.UID, p.now()), + Storage: p.cfg.Storage, + } + if node != src.Node { + params.Target = node + } + upid, err := p.api.Clone(ctx, src.Node, src.VMID, params) + if err != nil { + return ownedVM{}, fmt.Errorf("cloning template %d to VM %d: %w", src.VMID, vmid, err) + } + if err := p.waitTask(ctx, log, upid, p.budgets.clone); err != nil { + return ownedVM{}, fmt.Errorf("cloning template %d to VM %d: %w", src.VMID, vmid, err) + } + cfg, err := p.waitUnlocked(ctx, node, vmid, p.budgets.clone) + if err != nil { + return ownedVM{}, err + } + return ownedVM{node: node, vmid: vmid, cfg: cfg}, nil +} + +// provision is steps 5 to 9, each safe to repeat on a VM a previous attempt +// left stopped: the upload overwrites, the config write replaces, and the +// resize is skipped once the disk is large enough. Everything that can be +// refused from the config alone is checked before the first write. +func (p *Provider) provision(ctx context.Context, log logr.Logger, vm ownedVM, mt provider.ProxmoxMachineType, req provider.CreateRequest) error { + params, err := p.configParams(vm, mt, req.Name) + if err != nil { + return err + } + disk, size, grow, err := resizeTarget(vm, mt.DiskGiB) + if err != nil { + return err + } + if err := p.uploadSeed(ctx, log, vm.node, req); err != nil { + return err + } + if err := p.api.SetVMConfig(ctx, vm.node, vm.vmid, params); err != nil { + return fmt.Errorf("configuring VM %d: %w", vm.vmid, err) + } + if grow { + upid, err := p.api.Resize(ctx, vm.node, vm.vmid, disk, size) + if err != nil { + return fmt.Errorf("resizing %s of VM %d to %s: %w", disk, vm.vmid, size, err) + } + if err := p.waitTask(ctx, log, upid, p.budgets.resize); err != nil { + return fmt.Errorf("resizing %s of VM %d to %s: %w", disk, vm.vmid, size, err) + } + } + upid, err := p.api.Start(ctx, vm.node, vm.vmid) + if err != nil { + return fmt.Errorf("starting VM %d: %w", vm.vmid, err) + } + if err := p.waitTask(ctx, log, upid, p.budgets.start); err != nil { + return fmt.Errorf("starting VM %d: %w", vm.vmid, err) + } + return p.waitRunning(ctx, vm.node, vm.vmid) +} + +// configParams is step 6's write: size, agent, tag and seed, and net0 only +// when the config sets a bridge or a VLAN tag (spec entry 7). +func (p *Provider) configParams(vm ownedVM, mt provider.ProxmoxMachineType, name string) (map[string]string, error) { + params := map[string]string{ + "cores": strconv.Itoa(int(mt.Cores)), + "memory": strconv.FormatInt(mt.MemoryMiB, 10), + "agent": "1", + "tags": proxyTag, + "ide2": seedVolID(p.cfg.ISOStorage, name) + ",media=cdrom", + } + if p.cfg.Bridge == "" && p.cfg.VLANTag == 0 { + return params, nil + } + net0 := vm.cfg["net0"] + if net0 == "" { + return nil, permanentf("VM %d has no net0 to apply proxmox.bridge or proxmox.vlanTag to; the template needs a network device", vm.vmid) + } + params["net0"] = rewriteNet0(net0, p.cfg.Bridge, p.cfg.VLANTag) + return params, nil +} + +// rewriteNet0 replaces or appends bridge= and tag= in a net0 value, keeping +// every other option, the model and the MAC in place. An empty bridge or a +// zero tag leaves that option alone. +func rewriteNet0(net0, bridge string, tag int) string { + want := map[string]string{} + if bridge != "" { + want["bridge"] = bridge + } + if tag != 0 { + want["tag"] = strconv.Itoa(tag) + } + opts := strings.Split(net0, ",") + for i, opt := range opts { + key, _, _ := strings.Cut(opt, "=") + if v, ok := want[key]; ok { + opts[i] = key + "=" + v + delete(want, key) + } + } + for _, key := range []string{"bridge", "tag"} { + if v, ok := want[key]; ok { + opts = append(opts, key+"="+v) + } + } + return strings.Join(opts, ",") +} + +// resizeTarget decides step 7: grow the boot disk to diskGiB when that is +// larger than its size= (spec entry 6). A disk is never shrunk. +func resizeTarget(vm ownedVM, diskGiB int64) (disk, size string, grow bool, err error) { + if diskGiB == 0 { + return "", "", false, nil + } + disk, ok := bootDisk(vm.cfg) + if !ok { + return "", "", false, permanentf("VM %d has no boot disk to grow to %dG", vm.vmid, diskGiB) + } + cur, ok := diskBytes(vm.cfg[disk]) + if !ok { + return "", "", false, permanentf("boot disk %s of VM %d has no readable size in %q", disk, vm.vmid, vm.cfg[disk]) + } + if cur >= diskGiB<<30 { + return "", "", false, nil + } + return disk, strconv.FormatInt(diskGiB, 10) + "G", true, nil +} + +// bootDisk is the first disk of boot: order=… that is not a CD-ROM, else +// the legacy bootdisk key, else the first present of scsi0, virtio0, +// sata0 and ide0. Proxmox appends the seed's ide2 to the order (F19), so +// CD-ROMs are skipped. +func bootDisk(cfg vmConfig) (string, bool) { + isDisk := func(k string) bool { + v, ok := cfg[k] + return ok && diskKey.MatchString(k) && diskOption(v, "media") != "cdrom" + } + for dev := range strings.SplitSeq(diskOption(cfg["boot"], "order"), ";") { + if isDisk(dev) { + return dev, true + } + } + if k := cfg["bootdisk"]; isDisk(k) { + return k, true + } + for _, k := range []string{"scsi0", "virtio0", "sata0", "ide0"} { + if isDisk(k) { + return k, true + } + } + return "", false +} + +// diskOption returns the value of key= among a config value's +// comma-separated options. +func diskOption(value, key string) string { + for opt := range strings.SplitSeq(value, ",") { + if v, ok := strings.CutPrefix(opt, key+"="); ok { + return v + } + } + return "" +} + +// diskBytes reads a disk value's size=, in Proxmox's [KMGT] notation +// (bytes without a suffix). +func diskBytes(disk string) (int64, bool) { + s := diskOption(disk, "size") + if s == "" { + return 0, false + } + shift := map[byte]uint{'K': 10, 'M': 20, 'G': 30, 'T': 40} + unit, ok := shift[s[len(s)-1]] + if ok { + s = s[:len(s)-1] + } + n, err := strconv.ParseFloat(s, 64) + if err != nil || n < 0 { + return 0, false + } + return int64(n * float64(int64(1)<= n { + if vm, ok := f.vms[vmid]; ok { + vm.lock = "" + } + } + }) +} + +func TestCreate_repeatReturnsSameIDAndCreatesNothing(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + first := fx.create(fx.req()) + if first != formatProviderID(slot(0), testName) { + t.Errorf("Create = %q, want the attempt-0 VMID %d", first, slot(0)) + } + before := len(fx.fake.callLog()) + + if second := fx.create(fx.req()); second != first { + t.Errorf("second Create = %q, want %q", second, first) + } + repeat := fx.fake.callLog()[before:] + if !slices.Equal(repeat, []string{"ClusterResources", "VMConfig", "VMStatus"}) { + t.Errorf("repeat Create made %v, want only the listing, the config and the status reads", repeat) + } + if got := fx.fake.callCount("Clone"); got != 1 { + t.Errorf("Clone calls = %d, want 1", got) + } + if len(fx.logs.matching(`"msg"="proxmox vm adopted"`)) != 1 { + t.Errorf("no adoption line:\n%s", strings.Join(fx.logs.all(), "\n")) + } +} + +func TestCreate_resumesAfterEachPartialStep(t *testing.T) { + t.Parallel() + tests := []struct { + name string + machineType string + setup func(fx *fixture, vm *fakeVM) + wantCalls map[string]int + }{ + { + name: "clone finished", + setup: func(*fixture, *fakeVM) {}, + wantCalls: map[string]int{"UploadISO": 1, "SetVMConfig": 1, "Start": 1}, + }, + { + name: "seed uploaded", + setup: func(fx *fixture, _ *fakeVM) { + upid, _ := fx.fake.UploadISO(context.Background(), fakeNode, fakeISOStorage, seedISOName(testName), strings.NewReader("cut"), 3) + pollTask(fx.t, fx.fake, upid) + }, + wantCalls: map[string]int{"UploadISO": 2, "SetVMConfig": 1, "Start": 1}, + }, + { + name: "config written", + setup: func(_ *fixture, vm *fakeVM) { + vm.config["ide2"] = seedVolID(fakeISOStorage, testName) + ",media=cdrom" + vm.config["boot"] = "order=scsi0;ide2" + vm.config["tags"] = proxyTag + }, + wantCalls: map[string]int{"UploadISO": 1, "SetVMConfig": 1, "Start": 1}, + }, + { + name: "disk resized", + machineType: "big", + setup: func(_ *fixture, vm *fakeVM) { + vm.config["scsi0"] = "local-lvm:vm-1-disk-1,size=8G" + }, + wantCalls: map[string]int{"UploadISO": 1, "SetVMConfig": 1, "Resize": 0, "Start": 1}, + }, + { + name: "start submitted, still booting", + setup: func(_ *fixture, vm *fakeVM) { + vm.status, vm.qmpstatus = "running", "prelaunch" + }, + wantCalls: map[string]int{"UploadISO": 0, "SetVMConfig": 0, "Start": 0}, + }, + { + name: "running", + setup: func(_ *fixture, vm *fakeVM) { + vm.status, vm.qmpstatus = "running", "running" + }, + wantCalls: map[string]int{"UploadISO": 0, "SetVMConfig": 0, "Start": 0}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.p.cfg.MachineTypes["big"] = provider.ProxmoxMachineType{Cores: 1, MemoryMiB: 1024, DiskGiB: 8} + vm := fx.addOurVM(slot(0), testName, "stopped") + delete(vm.config, "tags") + tc.setup(fx, vm) + req := fx.req() + if tc.machineType != "" { + req.Placement.MachineType = tc.machineType + } + if id := fx.create(req); id != formatProviderID(slot(0), testName) { + t.Errorf("Create = %q, want the adopted VM", id) + } + if got := fx.fake.callCount("Clone"); got != 0 { + t.Errorf("Clone calls = %d, want 0", got) + } + for method, want := range tc.wantCalls { + if got := fx.fake.callCount(method); got != want { + t.Errorf("%s calls = %d, want %d", method, got, want) + } + } + if vm, _ := fx.fake.vmSnapshot(slot(0)); vm.status != "running" { + t.Errorf("VM status = %s, want running", vm.status) + } + if b, _ := fx.fake.iso(fakeISOStorage, seedISOName(testName)); tc.wantCalls["UploadISO"] > 0 && !bytes.Contains(b, []byte(nocloud.SeedLabel)) { + t.Errorf("seed on the storage is not a rebuilt ISO (%d bytes)", len(b)) + } + }) + } +} + +// TestCreate_resumesMidCloneVMAtDeterministicVMID is the crash Phase 0 +// found: the clone was submitted, the listing shows the VM nameless and +// locked, and the retry must wait for it and adopt it rather than probe +// past it and clone a duplicate. +func TestCreate_resumesMidCloneVMAtDeterministicVMID(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + vm := fx.addOurVM(slot(0), testName, "stopped") + delete(vm.config, "tags") + vm.lock = "clone" + clearLockOnVMConfig(fx.fake, slot(0), 4) + + if id := fx.create(fx.req()); id != formatProviderID(slot(0), testName) { + t.Errorf("Create = %q, want the mid-clone VM at %d", id, slot(0)) + } + if got := fx.fake.callCount("Clone"); got != 0 { + t.Errorf("Clone calls = %d, want 0: the retry cloned a duplicate", got) + } + if got := fx.fake.callCount("VMConfig"); got < 4 { + t.Errorf("VMConfig calls = %d, want the locked VM polled until unlocked", got) + } + fx.assertRunning(slot(0)) +} + +func TestCreate_takenVMIDInspectedBeforeProbing(t *testing.T) { + t.Parallel() + tests := []struct { + name string + setup func(fx *fixture) + // wantVMID is the attempt the VM ends up on; -1 is an error. + wantAttempt int + wantClass error + // wantConfigReads is how many VMConfig calls precede the first + // Clone: the template, plus one per inspected VM. + wantConfigReads int + }{ + { + name: "foreign VM probes on after its config is read", + setup: func(fx *fixture) { fx.fake.addVM(slot(0), "someone-else", "running") }, + wantAttempt: 1, + wantConfigReads: 2, + }, + { + name: "container probes on without a read", + setup: func(fx *fixture) { + fx.fake.addVM(slot(0), "ct", "running").kind = "lxc" + }, + wantAttempt: 1, + wantConfigReads: 1, + }, + { + name: "template probes on without a read", + setup: func(fx *fixture) { fx.fake.addTemplate(slot(0), "other-template") }, + wantAttempt: 1, + wantConfigReads: 1, + }, + { + name: "ours, nameless in the listing, is adopted", + setup: func(fx *fixture) { + fx.addOurVM(slot(0), testName, "stopped").lock = "clone" + clearLockOnVMConfig(fx.fake, slot(0), 1) + }, + wantAttempt: 0, + wantConfigReads: 2, + }, + { + name: "someone else's clone with our name is permanent", + setup: func(fx *fixture) { + vm := fx.fake.addVM(slot(0), testName, "stopped") + vm.config["description"] = formatDescription("another-uid", testCreatedAt) + vm.lock = "clone" + clearLockOnVMConfig(fx.fake, slot(0), 1) + }, + wantAttempt: -1, + wantClass: provider.ErrPermanent, + wantConfigReads: 2, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + tc.setup(fx) + if tc.wantAttempt < 0 { + fx.createFails(fx.req(), tc.wantClass) + } else if id := fx.create(fx.req()); id != formatProviderID(slot(tc.wantAttempt), testName) { + t.Errorf("Create = %q, want attempt %d (VMID %d)", id, tc.wantAttempt, slot(tc.wantAttempt)) + } + if got := callsBefore(fx.fake.callLog(), "VMConfig", "Clone"); got != tc.wantConfigReads { + t.Errorf("VMConfig calls before the clone = %d, want %d", got, tc.wantConfigReads) + } + }) + } +} + +// TestCreate_cloneAlreadyExistsRaceRunsOwnershipCheck: the listing shows +// the VMID free, but another Create takes it before our clone lands (F4). +func TestCreate_cloneAlreadyExistsRaceRunsOwnershipCheck(t *testing.T) { + t.Parallel() + tests := []struct { + name string + racerName string + racerUID string + wantAttempt int + wantClones int + }{ + {name: "another proxy's clone is waited on and probed past", racerName: "proxy-racer", racerUID: "racer-uid", wantAttempt: 1, wantClones: 2}, + {name: "our own clone is adopted", racerName: testName, racerUID: testUID, wantAttempt: 0, wantClones: 1}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + var raced bool + fx.fake.hookOn("Clone", func() error { + if !raced { + raced = true + vm := fx.fake.addVM(slot(0), tc.racerName, "stopped") + fx.fake.mu.Lock() + vm.config["description"] = formatDescription(tc.racerUID, testCreatedAt) + vm.lock = "clone" + fx.fake.mu.Unlock() + } + return nil + }) + clearLockOnVMConfig(fx.fake, slot(0), 3) + + if id := fx.create(fx.req()); id != formatProviderID(slot(tc.wantAttempt), testName) { + t.Errorf("Create = %q, want attempt %d", id, tc.wantAttempt) + } + if got := fx.fake.callCount("Clone"); got != tc.wantClones { + t.Errorf("Clone calls = %d, want %d", got, tc.wantClones) + } + if vm, _ := fx.fake.vmSnapshot(slot(0)); vm.name != tc.racerName { + t.Errorf("VM %d is named %q, want the racer's %q left alone", slot(0), vm.name, tc.racerName) + } + }) + } +} + +func TestCreate_returnsOnlyWhenRunning(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + var polls int + mutateOn(fx.fake, "VMStatus", func() { + polls++ + if vm, ok := fx.fake.vms[slot(0)]; ok && polls < 3 { + vm.qmpstatus = "prelaunch" + } else if ok { + vm.qmpstatus = "running" + } + }) + fx.create(fx.req()) + if got := fx.fake.callCount("VMStatus"); got != 3 { + t.Errorf("VMStatus calls = %d, want 3: Create must wait through prelaunch", got) + } + fx.assertRunning(slot(0)) +} + +func TestCreate_vmThatStopsAfterStartIsTransientAtOnce(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.p.budgets.running = time.Hour + mutateOn(fx.fake, "VMStatus", func() { + fx.fake.vms[slot(0)].status = "stopped" + }) + msg := fx.createFails(fx.req(), provider.ErrTransient) + if !strings.Contains(msg, "stopped right after its start") { + t.Errorf("error = %s, want it to say the VM stopped", msg) + } + if got := fx.fake.callCount("VMStatus"); got != 1 { + t.Errorf("VMStatus calls = %d, want 1", got) + } +} + +func TestCreate_runningWaitCapIsTransient(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.p.budgets.running = 30 * time.Millisecond + mutateOn(fx.fake, "VMStatus", func() { + fx.fake.vms[slot(0)].qmpstatus = "prelaunch" + }) + msg := fx.createFails(fx.req(), provider.ErrTransient) + if !strings.Contains(msg, "VM "+strconv.Itoa(slot(0))+" to run") { + t.Errorf("error = %s, want it to name the running wait", msg) + } +} + +func TestCreate_rejectsUnknownPlacement(t *testing.T) { + t.Parallel() + tests := []struct { + name string + nodes []string + mutate func(*provider.CreateRequest) + want string + }{ + {name: "unknown zone", mutate: func(r *provider.CreateRequest) { r.Placement.Zone = "pv9-9" }, want: `placement.zone "pv9-9" is not in proxmox.nodes`}, + {name: "empty zone with two nodes", nodes: []string{"pv1-1", "pv1-2"}, mutate: func(*provider.CreateRequest) {}, want: "placement.zone is required"}, + {name: "unknown machine type", mutate: func(r *provider.CreateRequest) { r.Placement.MachineType = "huge" }, want: `placement.machineType "huge" is not in proxmox.machineTypes`}, + {name: "empty machine type", mutate: func(r *provider.CreateRequest) { r.Placement.MachineType = "" }, want: `placement.machineType "" is not in proxmox.machineTypes`}, + {name: "unknown image", mutate: func(r *provider.CreateRequest) { r.Placement.Image = "arch" }, want: `placement.image "arch" is not in proxmox.templates`}, + {name: "no name", mutate: func(r *provider.CreateRequest) { r.Name = "" }, want: "no instance name"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + if tc.nodes != nil { + fx.p.cfg.Nodes = tc.nodes + } + req := fx.req() + tc.mutate(&req) + if msg := fx.createFails(req, provider.ErrPermanent); !strings.Contains(msg, tc.want) { + t.Errorf("error = %s, want it to contain %s", msg, tc.want) + } + if got := fx.fake.callLog(); len(got) != 0 { + t.Errorf("calls = %v, want none before placement is valid", got) + } + }) + } +} + +func TestCreate_zoneOptionalWithOneNode(t *testing.T) { + t.Parallel() + for _, zone := range []string{"", fakeNode} { + t.Run("zone "+strconv.Quote(zone), func(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + req := fx.req() + req.Placement.Zone = zone + fx.create(req) + if vm := fx.assertRunning(slot(0)); vm.node != fakeNode { + t.Errorf("VM node = %s, want %s", vm.node, fakeNode) + } + }) + } +} + +func TestCreate_regionIgnored(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + req := fx.req() + req.Placement.Region = "eu-west-1" + if id := fx.create(req); id != formatProviderID(slot(0), testName) { + t.Errorf("Create = %q, want the same VM as without a region", id) + } +} + +func TestCreate_adoptionWithForeignUIDIsPermanent(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.fake.addVM(testVMID, testName, "running").config["description"] = formatDescription("another-uid", testCreatedAt) + msg := fx.createFails(fx.req(), provider.ErrPermanent) + for _, want := range []string{"150042", "another-uid", testUID} { + if !strings.Contains(msg, want) { + t.Errorf("error = %s, want it to name %s", msg, want) + } + } + if got := fx.fake.callCount("Clone") + fx.fake.callCount("Start"); got != 0 { + t.Errorf("Clone and Start calls = %d, want 0", got) + } +} + +func TestCreate_adoptsOnlyMarkedVMs(t *testing.T) { + t.Parallel() + tests := []struct { + name string + description string + wantClass error + }{ + {name: "no description is refused", description: "", wantClass: provider.ErrPermanent}, + {name: "admin notes only are refused", description: "owner=alice\nticket=OPS-1", wantClass: provider.ErrPermanent}, + {name: "managed false is refused", description: provider.LabelManaged + "=false\n" + provider.LabelUID + "=" + testUID, wantClass: provider.ErrPermanent}, + {name: "mark with an empty uid is adopted", description: provider.LabelManaged + "=true"}, + {name: "mark with our uid is adopted", description: formatDescription(testUID, testCreatedAt)}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + if tc.description != "" { + fx.fake.addVM(testVMID, testName, "stopped").config["description"] = tc.description + } else { + fx.fake.addVM(testVMID, testName, "stopped") + } + if tc.wantClass != nil { + if msg := fx.createFails(fx.req(), tc.wantClass); !strings.Contains(msg, "no proxy-operator mark") { + t.Errorf("error = %s, want it to say the VM is unmarked", msg) + } + if vm, _ := fx.fake.vmSnapshot(testVMID); vm.status != "stopped" || fx.fake.callCount("SetVMConfig") != 0 { + t.Error("an unmarked VM was touched") + } + return + } + if id := fx.create(fx.req()); id != fx.id() { + t.Errorf("Create = %q, want the adopted %q", id, fx.id()) + } + }) + } +} + +// TestCreate_hibernatedVMIsNotWaitedOn: a suspended lock is not a task and +// never clears, so the ownership check judges the VM at once instead of +// waiting out the clone budget. +func TestCreate_hibernatedVMIsNotWaitedOn(t *testing.T) { + t.Parallel() + t.Run("ours is permanent", func(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.p.budgets.clone = time.Hour + fx.addOurVM(testVMID, testName, "stopped").lock = lockSuspended + if msg := fx.createFails(fx.req(), provider.ErrPermanent); !strings.Contains(msg, "hibernated") { + t.Errorf("error = %s, want it to say the VM is hibernated", msg) + } + if got := fx.fake.callCount("VMConfig"); got != 1 { + t.Errorf("VMConfig calls = %d, want 1", got) + } + }) + t.Run("foreign is probed past", func(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.p.budgets.clone = time.Hour + fx.fake.addVM(slot(0), "someone-else", "stopped").lock = lockSuspended + if id := fx.create(fx.req()); id != formatProviderID(slot(1), testName) { + t.Errorf("Create = %q, want attempt 1", id) + } + }) +} + +func TestCreate_duplicateNameIsPermanent(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.addOurVM(testVMID, testName, "running") + fx.addOurVM(testVMID+7, testName, "stopped") + msg := fx.createFails(fx.req(), provider.ErrPermanent) + if !strings.Contains(msg, "150042, 150049") { + t.Errorf("error = %s, want it to name both VMIDs", msg) + } + if got := fx.fake.callLog(); !slices.Equal(got, []string{"ClusterResources"}) { + t.Errorf("calls = %v, want only the listing", got) + } +} + +func TestCreate_vmidCollisionProbesForward(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.fake.addVM(slot(0), "someone-else", "running") + fx.fake.addVM(slot(1), "someone-else-too", "stopped") + if id := fx.create(fx.req()); id != formatProviderID(slot(2), testName) { + t.Errorf("Create = %q, want attempt 2 (VMID %d)", id, slot(2)) + } + if got := len(fx.logs.matching("probing on")); got != 2 { + t.Errorf("probe lines = %d, want 2", got) + } +} + +func TestCreate_vmidRangeExhaustedIsTransient(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.p.cfg.VMIDRange = &provider.ProxmoxVMIDRange{Min: 100000, Max: 100003} + for vmid := 100000; vmid <= 100003; vmid++ { + fx.fake.addVM(vmid, "someone-"+strconv.Itoa(vmid), "running") + } + msg := fx.createFails(fx.req(), provider.ErrTransient) + if !strings.Contains(msg, "no free VMID") || !strings.Contains(msg, "100000..100003") { + t.Errorf("error = %s, want it to name the range", msg) + } + if got := fx.fake.callCount("Clone"); got != 0 { + t.Errorf("Clone calls = %d, want 0", got) + } + if got := fx.fake.callCount("VMConfig"); got != 5 { + t.Errorf("VMConfig calls = %d, want the template and each of the 4 VMIDs once", got) + } +} + +func TestCreate_waitsForCloneLockOnRetry(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.p.budgets.clone = 30 * time.Millisecond + fx.fake.cloneLeavesLock = true + if msg := fx.createFails(fx.req(), provider.ErrTransient); !strings.Contains(msg, "the lock on VM") { + t.Errorf("first error = %s, want the lock wait to run out", msg) + } + fx.fake.mu.Lock() + fx.fake.cloneLeavesLock = false + fx.fake.mu.Unlock() + fx.p.budgets.clone = time.Second + clearLockOnVMConfig(fx.fake, slot(0), 4) + + if id := fx.create(fx.req()); id != formatProviderID(slot(0), testName) { + t.Errorf("retry Create = %q, want the locked VM adopted", id) + } + if got := fx.fake.callCount("Clone"); got != 1 { + t.Errorf("Clone calls = %d, want 1", got) + } + fx.assertRunning(slot(0)) +} + +func TestCreate_cloneTaskFailureIsTransient(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.fake.cloneFailsWith = "clone failed: storage 'local-lvm' is full" + msg := fx.createFails(fx.req(), provider.ErrTransient) + if !strings.Contains(msg, "clone failed: storage 'local-lvm' is full") { + t.Errorf("error = %s, want the task's exitstatus", msg) + } + if len(fx.logs.matching(`"msg"="proxmox task failed"`)) != 1 { + t.Errorf("no task-failed line:\n%s", strings.Join(fx.logs.all(), "\n")) + } + fx.fake.mu.Lock() + fx.fake.cloneFailsWith = "" + fx.fake.mu.Unlock() + if id := fx.create(fx.req()); id != formatProviderID(slot(0), testName) { + t.Errorf("retry Create = %q, want attempt 0 again", id) + } +} + +func TestCreate_cloneTaskTimeoutIsTransient(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.p.budgets.clone = 30 * time.Millisecond + fx.fake.taskPolls = 1 << 30 + msg := fx.createFails(fx.req(), provider.ErrTransient) + if !strings.Contains(msg, "waiting for task UPID:"+fakeNode) || !strings.Contains(msg, "not done after") { + t.Errorf("error = %s, want it to name the task and the budget", msg) + } +} + +// TestCreate_markWrittenInCloneCall reads the VM when the seed upload +// starts, between the clone and the config write: the mark is already +// there, so it came with the clone. +func TestCreate_markWrittenInCloneCall(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + now := time.Date(2026, 9, 23, 12, 30, 0, 0, time.UTC) + fx.p.now = func() time.Time { return now } + var atUpload string + mutateOn(fx.fake, "UploadISO", func() { atUpload = fx.fake.vms[slot(0)].config["description"] }) + fx.create(fx.req()) + want := formatDescription(testUID, now) + if atUpload != want { + t.Errorf("description after the clone = %q, want %q", atUpload, want) + } + vm, _ := fx.fake.vmSnapshot(slot(0)) + if m := parseDescription(vm.config["description"]); !m.Managed || m.UID != testUID || !m.CreatedAt.Equal(now) { + t.Errorf("mark read back = %+v", m) + } +} + +func TestCreate_tagSetInConfigStep(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + atUpload := "unset" + mutateOn(fx.fake, "UploadISO", func() { atUpload = fx.fake.vms[slot(0)].config["tags"] }) + fx.create(fx.req()) + if atUpload != "" { + t.Errorf("tags after the clone = %q, want none: tags are not a clone parameter", atUpload) + } + if vm, _ := fx.fake.vmSnapshot(slot(0)); vm.config["tags"] != proxyTag { + t.Errorf("tags = %q, want %q", vm.config["tags"], proxyTag) + } +} + +func TestCreate_seedAlwaysRebuilt(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.create(fx.req()) + fx.fake.mu.Lock() + fx.fake.vms[slot(0)].status = "stopped" + fx.fake.isos[fakeISOStorage][seedISOName(testName)] = []byte("cut") + fx.fake.mu.Unlock() + + fx.create(fx.req()) + if got := fx.fake.callCount("UploadISO"); got != 2 { + t.Errorf("UploadISO calls = %d, want 2", got) + } + if again, _ := fx.fake.iso(fakeISOStorage, seedISOName(testName)); !bytes.Contains(again, []byte(nocloud.SeedLabel)) { + t.Errorf("seed after the retry has %d bytes and no %s label, want it rebuilt", len(again), nocloud.SeedLabel) + } +} + +func TestCreate_seedUploadOverwritesWithoutDelete(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.fake.mu.Lock() + fx.fake.isos[fakeISOStorage] = map[string][]byte{seedISOName(testName): []byte("stale")} + fx.fake.mu.Unlock() + fx.create(fx.req()) + if got := fx.fake.callCount("DeleteVolume"); got != 0 { + t.Errorf("DeleteVolume calls = %d, want 0: an upload overwrites (F12)", got) + } + b, _ := fx.fake.iso(fakeISOStorage, seedISOName(testName)) + if !bytes.Contains(b, []byte(nocloud.SeedLabel)) { + t.Errorf("seed has %d bytes and no %s label, want the new ISO", len(b), nocloud.SeedLabel) + } +} + +func TestCreate_seedUploadedBeforeConfig(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.fake.taskPolls = 2 + fx.create(fx.req()) + log := fx.fake.callLog() + up, cfg := slices.Index(log, "UploadISO"), slices.Index(log, "SetVMConfig") + if up < 0 || cfg < up { + t.Fatalf("call log = %v, want UploadISO before SetVMConfig", log) + } + if got := callsBefore(log[up:], "TaskStatus", "SetVMConfig"); got != 3 { + t.Errorf("upload task polls before the config write = %d, want 3", got) + } +} + +func TestCreate_configSetsCoresMemoryAgentAndSeed(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.fake.mu.Lock() + tmpl := fx.fake.vms[testTemplateVMID].config + tmpl["agent"], tmpl["cores"], tmpl["memory"] = "0", "4", "8192" + fx.fake.mu.Unlock() + fx.create(fx.req()) + vm, _ := fx.fake.vmSnapshot(slot(0)) + want := map[string]string{ + "cores": "1", + "memory": "1024", + "agent": "1", + "ide2": "proxy-seeds:iso/" + testName + "-seed.iso,media=cdrom", + "tags": proxyTag, + } + for k, v := range want { + if vm.config[k] != v { + t.Errorf("%s = %q, want %q", k, vm.config[k], v) + } + } +} + +func TestCreate_bridgeOverrideKeepsMAC(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.p.cfg.Bridge = "vmbr4" + fx.create(fx.req()) + if vm, _ := fx.fake.vmSnapshot(slot(0)); vm.config["net0"] != "virtio=BC:24:11:48:11:46,bridge=vmbr4" { + t.Errorf("net0 = %q, want the MAC kept and the bridge replaced", vm.config["net0"]) + } +} + +// TestCreate_noBridgeOverrideLeavesNet0 changes net0 just before the config +// write: a write that carried net0 would put the cloned value back. +func TestCreate_noBridgeOverrideLeavesNet0(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + const changed = "virtio=BC:24:11:48:11:46,bridge=vmbr9" + mutateOn(fx.fake, "SetVMConfig", func() { fx.fake.vms[slot(0)].config["net0"] = changed }) + fx.create(fx.req()) + if vm, _ := fx.fake.vmSnapshot(slot(0)); vm.config["net0"] != changed { + t.Errorf("net0 = %q, want it left out of the config write", vm.config["net0"]) + } +} + +func TestCreate_vlanTagAppendedKeepsMACAndOptions(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.p.cfg.VLANTag = 4 + fx.fake.mu.Lock() + fx.fake.vms[testTemplateVMID].config["net0"] = "virtio=BC:24:11:48:11:46,bridge=vmbr0,firewall=1,mtu=1500" + fx.fake.mu.Unlock() + fx.create(fx.req()) + if vm, _ := fx.fake.vmSnapshot(slot(0)); vm.config["net0"] != "virtio=BC:24:11:48:11:46,bridge=vmbr0,firewall=1,mtu=1500,tag=4" { + t.Errorf("net0 = %q, want tag=4 appended and everything else in order", vm.config["net0"]) + } +} + +func TestRewriteNet0_table(t *testing.T) { + t.Parallel() + const mac = "virtio=BC:24:11:48:11:46" + tests := []struct { + name string + net0 string + bridge string + tag int + want string + }{ + {name: "nothing configured", net0: mac + ",bridge=vmbr0", want: mac + ",bridge=vmbr0"}, + {name: "bridge replaced in place", net0: mac + ",bridge=vmbr0,firewall=1", bridge: "vmbr4", want: mac + ",bridge=vmbr4,firewall=1"}, + {name: "tag appended", net0: mac + ",bridge=vmbr0", tag: 4, want: mac + ",bridge=vmbr0,tag=4"}, + {name: "tag replaced in place", net0: mac + ",bridge=vmbr0,tag=2,firewall=1", tag: 4, want: mac + ",bridge=vmbr0,tag=4,firewall=1"}, + {name: "both appended", net0: mac, bridge: "vmbr4", tag: 4, want: mac + ",bridge=vmbr4,tag=4"}, + {name: "the captured VLAN template", net0: "virtio=BC:24:11:2F:97:A0,bridge=vmbr0,tag=4", bridge: "vmbr1", tag: 7, want: "virtio=BC:24:11:2F:97:A0,bridge=vmbr1,tag=7"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if got := rewriteNet0(tc.net0, tc.bridge, tc.tag); got != tc.want { + t.Errorf("rewriteNet0(%q, %q, %d) = %q, want %q", tc.net0, tc.bridge, tc.tag, got, tc.want) + } + }) + } +} + +// TestCreate_templateShapeRefusedBeforeClone: the clone copies the +// template's config, so what the config or resize step would refuse on the +// clone is refused on the template, before a VM exists that a fixed +// template could no longer help. +func TestCreate_templateShapeRefusedBeforeClone(t *testing.T) { + t.Parallel() + tests := []struct { + name string + setup func(fx *fixture, tmpl map[string]string) + want string + bigger bool + }{ + { + name: "no net0 with a bridge configured", + setup: func(fx *fixture, tmpl map[string]string) { + fx.p.cfg.Bridge = "vmbr4" + delete(tmpl, "net0") + }, + want: "no net0", + }, + { + name: "no boot disk with diskGiB set", + setup: func(_ *fixture, tmpl map[string]string) { + delete(tmpl, "scsi0") + }, + want: "no boot disk", + bigger: true, + }, + { + name: "boot disk without a size", + setup: func(_ *fixture, tmpl map[string]string) { + tmpl["scsi0"] = "local-lvm:base-9001-disk-1" + }, + want: "no readable size", + bigger: true, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.p.cfg.MachineTypes["big"] = provider.ProxmoxMachineType{Cores: 1, MemoryMiB: 1024, DiskGiB: 8} + fx.fake.mu.Lock() + tc.setup(fx, fx.fake.vms[testTemplateVMID].config) + fx.fake.mu.Unlock() + req := fx.req() + if tc.bigger { + req.Placement.MachineType = "big" + } + if msg := fx.createFails(req, provider.ErrPermanent); !strings.Contains(msg, tc.want) { + t.Errorf("error = %s, want %s", msg, tc.want) + } + if got := fx.fake.callCount("Clone"); got != 0 { + t.Errorf("Clone calls = %d, want 0", got) + } + }) + } +} + +func TestCreate_resizeWaitsForTask(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.p.cfg.MachineTypes["big"] = provider.ProxmoxMachineType{Cores: 1, MemoryMiB: 1024, DiskGiB: 8} + fx.fake.taskPolls = 2 + req := fx.req() + req.Placement.MachineType = "big" + fx.create(req) + log := fx.fake.callLog() + rs := slices.Index(log, "Resize") + if rs < 0 { + t.Fatalf("call log = %v, want a Resize", log) + } + if got := callsBefore(log[rs:], "TaskStatus", "Start"); got != 3 { + t.Errorf("resize task polls before the start = %d, want 3", got) + } + if vm, _ := fx.fake.vmSnapshot(slot(0)); vm.config["scsi0"] != "local-lvm:vm-"+strconv.Itoa(slot(0))+"-disk-1,size=8G" { + t.Errorf("scsi0 = %q, want size=8G", vm.config["scsi0"]) + } +} + +func TestCreate_resizeSkippedWhenNotLarger(t *testing.T) { + t.Parallel() + for _, diskGiB := range []int64{0, 2, 3} { + t.Run("diskGiB "+strconv.FormatInt(diskGiB, 10), func(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.p.cfg.MachineTypes["sized"] = provider.ProxmoxMachineType{Cores: 1, MemoryMiB: 1024, DiskGiB: diskGiB} + req := fx.req() + req.Placement.MachineType = "sized" + fx.create(req) + if got := fx.fake.callCount("Resize"); got != 0 { + t.Errorf("Resize calls = %d, want 0 for a 3G disk", got) + } + if vm, _ := fx.fake.vmSnapshot(slot(0)); diskOption(vm.config["scsi0"], "size") != "3G" { + t.Errorf("scsi0 = %q, want 3G kept", vm.config["scsi0"]) + } + }) + } +} + +func TestBootDisk_table(t *testing.T) { + t.Parallel() + const ( + disk = "local-lvm:vm-1-disk-1,size=3G" + seed = "proxy-seeds:iso/proxy-x-seed.iso,media=cdrom" + cdrom = "none,media=cdrom" + ) + tests := []struct { + name string + cfg vmConfig + want string + wantOK bool + }{ + {name: "order names the disk", cfg: vmConfig{"boot": "order=scsi0", "scsi0": disk}, want: "scsi0", wantOK: true}, + {name: "seed appended by Proxmox", cfg: vmConfig{"boot": "order=scsi0;ide2", "scsi0": disk, "ide2": seed}, want: "scsi0", wantOK: true}, + {name: "cdrom first in order", cfg: vmConfig{"boot": "order=ide2;scsi0", "scsi0": disk, "ide2": cdrom}, want: "scsi0", wantOK: true}, + {name: "network first in order", cfg: vmConfig{"boot": "order=net0;virtio0", "net0": "virtio=BC:24:11:48:11:46", "virtio0": disk}, want: "virtio0", wantOK: true}, + {name: "second disk first in order", cfg: vmConfig{"boot": "order=scsi1;scsi0", "scsi0": disk, "scsi1": disk}, want: "scsi1", wantOK: true}, + {name: "order names a missing disk", cfg: vmConfig{"boot": "order=scsi3;sata0", "sata0": disk}, want: "sata0", wantOK: true}, + {name: "legacy boot with bootdisk", cfg: vmConfig{"boot": "cdn", "bootdisk": "virtio0", "virtio0": disk, "scsi0": disk}, want: "virtio0", wantOK: true}, + {name: "no boot line, first known slot", cfg: vmConfig{"sata0": disk, "ide0": disk}, want: "sata0", wantOK: true}, + {name: "ide0 disk is a disk", cfg: vmConfig{"ide0": disk, "ide2": seed}, want: "ide0", wantOK: true}, + {name: "efidisk is not a boot disk", cfg: vmConfig{"efidisk0": "local-lvm:vm-1-disk-0,size=4M", "ide2": seed}}, + {name: "cdrom only", cfg: vmConfig{"boot": "order=ide0", "ide0": cdrom}}, + {name: "empty", cfg: vmConfig{}}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, ok := bootDisk(tc.cfg) + if got != tc.want || ok != tc.wantOK { + t.Errorf("bootDisk = %q, %v; want %q, %v", got, ok, tc.want, tc.wantOK) + } + }) + } +} + +func TestBootDisk_capturedClone(t *testing.T) { + t.Parallel() + cfg := capturedConfig(t, "vm_config_clone") + if got, ok := bootDisk(cfg); got != "scsi0" || !ok { + t.Errorf("bootDisk of the captured clone = %q, %v; want scsi0", got, ok) + } + if got, ok := diskBytes(cfg["scsi0"]); got != 3<<30 || !ok { + t.Errorf("diskBytes of the captured scsi0 = %d, %v; want 3 GiB", got, ok) + } +} + +func TestDiskBytes_table(t *testing.T) { + t.Parallel() + tests := []struct { + disk string + want int64 + wantOK bool + }{ + {disk: "local-lvm:vm-1-disk-1,size=3G", want: 3 << 30, wantOK: true}, + {disk: "local-lvm:vm-1-disk-0,efitype=4m,size=4M", want: 4 << 20, wantOK: true}, + {disk: "local:1/vm-1-disk-0.qcow2,size=512K", want: 512 << 10, wantOK: true}, + {disk: "big:vm-1-disk-1,size=2T", want: 2 << 40, wantOK: true}, + {disk: "raw:vm-1-disk-1,size=1073741824", want: 1 << 30, wantOK: true}, + {disk: "odd:vm-1-disk-1,size=1.5G", want: 3 << 29, wantOK: true}, + {disk: "local-lvm:vm-1-disk-1"}, + {disk: "local-lvm:vm-1-disk-1,size="}, + {disk: "local-lvm:vm-1-disk-1,size=lots"}, + {disk: "local-lvm:vm-1-disk-1,size=-1G"}, + } + for _, tc := range tests { + t.Run(tc.disk, func(t *testing.T) { + t.Parallel() + got, ok := diskBytes(tc.disk) + if got != tc.want || ok != tc.wantOK { + t.Errorf("diskBytes(%q) = %d, %v; want %d, %v", tc.disk, got, ok, tc.want, tc.wantOK) + } + }) + } +} + +func TestCreate_startTaskFailureIsTransient(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.fake.startFailsWith = "start failed: QEMU exited with code 1" + if msg := fx.createFails(fx.req(), provider.ErrTransient); !strings.Contains(msg, "start failed: QEMU exited with code 1") { + t.Errorf("error = %s, want the task's exitstatus", msg) + } +} + +func TestCreate_templateMissingIsPermanent(t *testing.T) { + t.Parallel() + fx := newFixture(t, 1) + msg := fx.createFails(fx.req(), provider.ErrPermanent) + if !strings.Contains(msg, `placement.image "debian-13": template VM 9001 does not exist`) { + t.Errorf("error = %s, want it to name the image and the VMID", msg) + } + if got := fx.fake.callCount("Clone"); got != 0 { + t.Errorf("Clone calls = %d, want 0", got) + } +} + +// capturedConfig loads a config capture the way the client decodes it. +func capturedConfig(t *testing.T, name string) vmConfig { + t.Helper() + cfg, err := decodeConfig(decodeData[map[string]json.RawMessage](t, name)) + if err != nil { + t.Fatalf("decoding %s: %v", name, err) + } + return cfg +} + +func TestCreate_templateWithCloudInitDriveIsPermanent(t *testing.T) { + t.Parallel() + tests := []struct { + name string + setup func(vm *fakeVM) + want string + }{ + { + name: "the captured Ubuntu template", + setup: func(vm *fakeVM) { + vm.config = capturedConfig(t, "vm_config_template_ubuntu") + }, + want: "cloud-init drive on ide2", + }, + { + name: "cloud-init drive on another slot", + setup: func(vm *fakeVM) { vm.config["sata1"] = "local-lvm:vm-9001-cloudinit,media=cdrom" }, + want: "cloud-init drive on sata1", + }, + { + name: "not a template", + setup: func(vm *fakeVM) { vm.template = false }, + want: "VM 9001 is not a template", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.fake.mu.Lock() + tc.setup(fx.fake.vms[testTemplateVMID]) + fx.fake.mu.Unlock() + if msg := fx.createFails(fx.req(), provider.ErrPermanent); !strings.Contains(msg, tc.want) { + t.Errorf("error = %s, want %s", msg, tc.want) + } + if got := fx.fake.callCount("Clone"); got != 0 { + t.Errorf("Clone calls = %d, want 0", got) + } + }) + } +} + +// TestCreate_adoptionDoesNotNeedTheTemplate: a template may be deleted once +// its clones exist (spec entry 4), so a retry that finds its VM must not +// fail on the template. +func TestCreate_adoptionDoesNotNeedTheTemplate(t *testing.T) { + t.Parallel() + for _, status := range []string{"running", "stopped"} { + t.Run(status, func(t *testing.T) { + t.Parallel() + fx := newFixture(t, 1) + fx.addOurVM(testVMID, testName, status) + if id := fx.create(fx.req()); id != fx.id() { + t.Errorf("Create = %q, want %q", id, fx.id()) + } + fx.assertRunning(testVMID) + }) + } +} + +func TestCreate_cloneTargetAndStorage(t *testing.T) { + t.Parallel() + tests := []struct { + name string + zone string + storage string + wantTarget string + wantStorage string + wantNode string + }{ + {name: "template's node, template's storage", zone: fakeNode, wantNode: fakeNode}, + {name: "another node, configured storage", zone: "pv1-2", storage: "fast-lvm", wantTarget: "pv1-2", wantStorage: "fast-lvm", wantNode: "pv1-2"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 2) + fx.p.api = observe(fx.fake, testProvider) + fx.p.cfg.Nodes = []string{fakeNode, "pv1-2"} + fx.p.cfg.Storage = tc.storage + req := fx.req() + req.Placement.Zone = tc.zone + fx.create(req) + lines := fx.logs.matching(`"call"="Clone"`) + if len(lines) != 1 { + t.Fatalf("clone call lines = %d, want 1", len(lines)) + } + for _, want := range []string{`"target"="` + tc.wantTarget + `"`, `"storage"="` + tc.wantStorage + `"`, `"node"="` + fakeNode + `"`} { + if !strings.Contains(lines[0], want) { + t.Errorf("clone call line lacks %s: %s", want, lines[0]) + } + } + if vm := fx.assertRunning(slot(0)); vm.node != tc.wantNode { + t.Errorf("VM node = %s, want %s", vm.node, tc.wantNode) + } + }) + } +} + +// TestCreate_adoptedVMStaysOnItsNode: the ID is location-free, so a VM that +// migrated is carried on where it is, not where placement.zone points. +func TestCreate_adoptedVMStaysOnItsNode(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.addOurVM(testVMID, testName, "stopped").node = "pv1-2" + if id := fx.create(fx.req()); id != fx.id() { + t.Errorf("Create = %q, want %q", id, fx.id()) + } + if vm := fx.assertRunning(testVMID); vm.node != "pv1-2" { + t.Errorf("VM node = %s, want pv1-2", vm.node) + } + if lines := fx.logs.matching(`"msg"="proxmox vm ensured running"`); len(lines) != 1 || !strings.Contains(lines[0], `"node"="pv1-2"`) { + t.Errorf("outcome lines = %v, want one naming pv1-2", lines) + } +} + +func TestCreate_notExistInsideCreateIsTransientNotNotFound(t *testing.T) { + t.Parallel() + for _, method := range []string{"SetVMConfig", "Start", "VMStatus"} { + t.Run(method, func(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.fake.failWith(method, errVMMissing(fakeNode, slot(0))) + fx.createFails(fx.req(), provider.ErrTransient) + }) + } +} + +func TestCreate_cloudInitSentinelNeverLoggedOrInErrors(t *testing.T) { + t.Parallel() + const sentinel = "SENTINEL-s3cr3t-passw0rd-7f3a" + tests := []struct { + name string + setup func(fx *fixture) + }{ + {name: "success", setup: func(*fixture) {}}, + {name: "upload refused", setup: func(fx *fixture) { fx.fake.failWith("UploadISO", fakeErr(500, "fake: upload failed")) }}, + {name: "upload task failed", setup: func(fx *fixture) { + fx.fake.hookOn("TaskStatus", func() error { + if fx.fake.callCount("SetVMConfig") == 0 && fx.fake.callCount("UploadISO") > 0 { + return fakeErr(596, "fake: broken pipe") + } + return nil + }) + }}, + {name: "config refused", setup: func(fx *fixture) { fx.fake.failWith("SetVMConfig", errLocked("backup")) }}, + {name: "start failed", setup: func(fx *fixture) { fx.fake.startFailsWith = "start failed" }}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 2) + fx.p.api = observe(fx.fake, testProvider) + tc.setup(fx) + req := fx.req() + req.CloudInit = "#cloud-config\nwrite_files:\n - content: " + sentinel + "\n" + _, err := fx.p.Create(fx.ctx, req) + if err != nil && strings.Contains(err.Error(), sentinel) { + t.Errorf("error carries the cloud-init: %v", err) + } + lines := fx.logs.all() + for _, l := range lines { + if strings.Contains(l, sentinel) { + t.Errorf("log line carries the cloud-init: %s", l) + } + } + if got := fx.logs.matching(`"cloudInitBytes"=` + strconv.Itoa(len(req.CloudInit))); len(got) != 1 { + t.Errorf("cloudInitBytes lines = %d, want 1:\n%s", len(got), strings.Join(lines, "\n")) + } + }) + } +} + +func TestCreate_concurrentSameNameSerialised(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + release := fx.fake.blockOn("Clone") + ids := make([]string, 2) + errs := make([]error, 2) + var wg sync.WaitGroup + for i := range ids { + wg.Go(func() { ids[i], errs[i] = fx.p.Create(fx.ctx, fx.req()) }) + if i == 0 { + for fx.fake.callCount("Clone") == 0 { + time.Sleep(time.Millisecond) + } + } + } + close(release) + wg.Wait() + for i, err := range errs { + if err != nil { + t.Fatalf("Create %d error = %v", i, err) + } + } + if ids[0] != ids[1] { + t.Errorf("ids = %v, want the same VM for both", ids) + } + if got := fx.fake.callCount("Clone"); got != 1 { + t.Errorf("Clone calls = %d, want 1", got) + } +} + +func TestCreate_cancelledMidWaitReturnsPromptly(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.p.budgets = defaultBudgets() + fx.p.budgets.poll = time.Millisecond + fx.fake.taskPolls = 1 << 30 + ctx, cancel := context.WithCancel(fx.ctx) + defer cancel() + fx.fake.hookOn("TaskStatus", func() error { + cancel() + return nil + }) + start := time.Now() + _, err := fx.p.Create(ctx, fx.req()) + if elapsed := time.Since(start); elapsed > 5*time.Second { + t.Errorf("Create took %v after cancellation", elapsed) + } + assertProviderError(t, err, "create", provider.ErrTransient) + if !errors.Is(err, context.Canceled) { + t.Errorf("error = %v, want it to wrap context.Canceled", err) + } +} + +func TestCreate_outcomeLine(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 1) + fx.create(fx.req()) + lines := fx.logs.matching(`"msg"="proxmox vm ensured running"`) + if len(lines) != 1 { + t.Fatalf("outcome lines = %d, want 1:\n%s", len(lines), strings.Join(fx.logs.all(), "\n")) + } + for _, want := range []string{`"vmid"=` + strconv.Itoa(slot(0)), `"name"="` + testName + `"`, `"node"="` + fakeNode + `"`, `"provider"="` + testProvider + `"`} { + if !strings.Contains(lines[0], want) { + t.Errorf("outcome line lacks %s: %s", want, lines[0]) + } + } +} + +func TestCreate_quietAtV0(t *testing.T) { + t.Parallel() + fx := newCreateFixture(t, 0) + fx.p.api = observe(fx.fake, testProvider) + fx.create(fx.req()) + if lines := fx.logs.all(); len(lines) != 0 { + t.Errorf("V(0) lines = %v, want none", lines) + } +} diff --git a/internal/provider/proxmox/proxmox.go b/internal/provider/proxmox/proxmox.go index 1be34e8..5843857 100644 --- a/internal/provider/proxmox/proxmox.go +++ b/internal/provider/proxmox/proxmox.go @@ -173,14 +173,8 @@ func (p *Provider) lockName(ctx context.Context, name string) (func(), error) { return release, nil } -// Create and Delete are stubs until the phases that implement them (Create -// in 6, Delete in 7). Each answers Transient, so a Proxy on this provider -// retries rather than latching. - -func (p *Provider) Create(_ context.Context, req provider.CreateRequest) (string, error) { - return "", p.wrapErr("create", "", transientf("proxmox Create is not implemented yet (request %s)", req.Name)) -} - +// Delete is a stub until Phase 7. It answers Transient, so a Proxy on this +// provider retries rather than latching. func (p *Provider) Delete(_ context.Context, id string) error { return p.wrapErr("delete", id, transientf("proxmox Delete is not implemented yet")) } diff --git a/internal/provider/proxmox/proxmox_test.go b/internal/provider/proxmox/proxmox_test.go index 42d540e..502ff9d 100644 --- a/internal/provider/proxmox/proxmox_test.go +++ b/internal/provider/proxmox/proxmox_test.go @@ -216,17 +216,12 @@ func TestWrapErr_carriesProviderNameOpAndID(t *testing.T) { } // TestStubs_answerTransient keeps a Proxy on this provider retrying, not -// latched, until Create and Delete are implemented in later phases. +// latched, until Delete is implemented in Phase 7. func TestStubs_answerTransient(t *testing.T) { t.Parallel() - ctx := context.Background() p := newProvider("proxmox-home", testConfig(), nil) - _, createErr := p.Create(ctx, provider.CreateRequest{Name: "proxy-x"}) - deleteErr := p.Delete(ctx, "vms/150042/proxy-x") - for op, err := range map[string]error{"create": createErr, "delete": deleteErr} { - if !errors.Is(err, provider.ErrTransient) { - t.Errorf("%s error = %v, want ErrTransient", op, err) - } + if err := p.Delete(context.Background(), "vms/150042/proxy-x"); !errors.Is(err, provider.ErrTransient) { + t.Errorf("delete error = %v, want ErrTransient", err) } } diff --git a/internal/provider/proxmox/seed.go b/internal/provider/proxmox/seed.go index cb7dc20..dfdf9a0 100644 --- a/internal/provider/proxmox/seed.go +++ b/internal/provider/proxmox/seed.go @@ -2,9 +2,12 @@ package proxmox import ( "bytes" + "context" "errors" "fmt" + "github.com/go-logr/logr" + "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider" "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider/nocloud" ) @@ -41,3 +44,22 @@ func (p *Provider) buildSeed(req provider.CreateRequest) (*bytes.Buffer, error) } return &iso, nil } + +// uploadSeed puts a freshly built seed on the VM's node and waits for the +// copy (spec entry 5). An upload overwrites a file of the same name (F12), +// so what a cut-off attempt left is replaced without a delete first. +func (p *Provider) uploadSeed(ctx context.Context, log logr.Logger, node string, req provider.CreateRequest) error { + iso, err := p.buildSeed(req) + if err != nil { + return err + } + volid := seedVolID(p.cfg.ISOStorage, req.Name) + upid, err := p.api.UploadISO(ctx, node, p.cfg.ISOStorage, seedISOName(req.Name), iso, int64(iso.Len())) + if err != nil { + return fmt.Errorf("uploading seed %s: %w", volid, err) + } + if err := p.waitTask(ctx, log, upid, p.budgets.upload); err != nil { + return fmt.Errorf("uploading seed %s: %w", volid, err) + } + return nil +} diff --git a/internal/provider/proxmox/task.go b/internal/provider/proxmox/task.go index 3ebe972..542423e 100644 --- a/internal/provider/proxmox/task.go +++ b/internal/provider/proxmox/task.go @@ -89,7 +89,9 @@ func (p *Provider) waitTask(ctx context.Context, log logr.Logger, upid string, b } // waitUnlocked polls a VM's config until no task holds its lock and returns -// that config, for Create's ownership check (spec entry 2). A VM that +// that config, for Create's ownership check (spec entry 2). A suspended lock +// is returned at once: it marks a hibernated VM, not a task, and never +// clears on its own, so waiting would only burn the clone budget. A VM that // disappears meanwhile, as a failed clone does, returns the does-not-exist // error for the caller to judge. func (p *Provider) waitUnlocked(ctx context.Context, node string, vmid int, budget time.Duration) (vmConfig, error) { @@ -100,10 +102,27 @@ func (p *Provider) waitUnlocked(ctx context.Context, node string, vmid int, budg return false, fmt.Errorf("reading config of VM %d: %w", vmid, err) } cfg = c - return c["lock"] == "", nil + return c["lock"] == "" || c["lock"] == lockSuspended, nil }) if err != nil { return nil, err } return cfg, nil } + +// waitRunning polls status/current after the start task until QEMU runs +// the guest (spec entry 10, step 9). A VM that is stopped again is +// Transient at once: it will not come up on its own, and the next attempt +// adopts and starts it. +func (p *Provider) waitRunning(ctx context.Context, node string, vmid int) error { + return pollUntil(ctx, p.budgets.poll, p.budgets.running, fmt.Sprintf("VM %d to run", vmid), func() (bool, error) { + st, err := p.api.VMStatus(ctx, node, vmid) + if err != nil { + return false, fmt.Errorf("reading status of VM %d: %w", vmid, err) + } + if st.Status == "stopped" { + return false, transientf("VM %d stopped right after its start", vmid) + } + return st.Status == "running" && st.QMPStatus == "running", nil + }) +} diff --git a/internal/provider/proxmox/task_test.go b/internal/provider/proxmox/task_test.go index f25d401..a5c3b0b 100644 --- a/internal/provider/proxmox/task_test.go +++ b/internal/provider/proxmox/task_test.go @@ -160,8 +160,10 @@ func TestWaitUnlocked_table(t *testing.T) { cancelled bool wantErr func(error) bool wantCalls int + wantLock string }{ {name: "unlocked answers at once", wantCalls: 1}, + {name: "suspended lock is not waited on", lock: lockSuspended, budget: time.Hour, wantCalls: 1, wantLock: lockSuspended}, { name: "waits until the lock clears", lock: "clone", wantCalls: 3, onCall: func(f *fakeAPI, n int) { @@ -219,8 +221,8 @@ func TestWaitUnlocked_table(t *testing.T) { } cfg, err := fx.p.waitUnlocked(ctx, fakeNode, testVMID, budget) if tc.wantErr == nil { - if err != nil || cfg["name"] != testName || cfg["lock"] != "" { - t.Errorf("waitUnlocked = %v, %v; want the unlocked config", cfg, err) + if err != nil || cfg["name"] != testName || cfg["lock"] != tc.wantLock { + t.Errorf("waitUnlocked = %v, %v; want the config with lock %q", cfg, err, tc.wantLock) } } else if !tc.wantErr(err) || cfg != nil { t.Errorf("waitUnlocked = %v, %v; want no config and the expected error", cfg, err)