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)