diff --git a/CHANGELOG.md b/CHANGELOG.md index 10041be..a251d46 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,8 @@ and this project adheres to [Conventional Commits](https://www.conventionalcommi ### Fixed -- Lifecycle cmdlets with `-Wait` (`Start`/`Stop`/`Restart`/`Reset`/`Resume` for VMs and containers) now wait for the guest's config lock to clear, not just for the status to change. PVE reports the target status before the operation releases the lock, so a caller acting immediately afterwards could fail with `can't lock file '/var/lock/qemu-server/lock-.conf' - got timeout`. Observed as a cascade of 4 integration failures in run 183. If the lock outlasts `-Timeout` the cmdlet still returns success, as before. See `DECISIONS.md` D015. +- `Restart-PveVm` now uses PVE's native reboot endpoint (`POST {vmid}/status/reboot`) instead of composing a shutdown followed by a start. The two-call form raced Proxmox's own post-stop cleanup: the start won the guest's config lock, `qm cleanup` then held that lock for 30 seconds waiting on the newly started process, and the caller's next operation failed with `can't lock file '/var/lock/qemu-server/lock-.conf' - got timeout`. Reproduced in integration runs 183, 185 and 186 as a cascade of 4 failures. `Restart-PveContainer` is unchanged — LXC has no reboot endpoint. See `DECISIONS.md` D016. +- Lifecycle cmdlets with `-Wait` (`Start`/`Stop`/`Restart`/`Reset`/`Resume` for VMs and containers) also wait for the guest's config lock (the `lock:` property, e.g. `backup` or `migrate`) to clear before returning, and the post-timeout fallback tests the most recent poll rather than whether a match was ever seen. See `DECISIONS.md` D015 — note that entry misdiagnosed the failure above and is superseded by D016. - `New-PveCluster -Wait` now blocks until the cluster reports quorum, not merely until the creation task finishes. PVE's create task returns before corosync converges (~6s earlier in testing), so the natural `New-PveCluster -Wait` → `Add-PveClusterMember` sequence failed with `cluster not ready - no quorum?`. Adds `-Timeout` (seconds, default 60, range 1-3600) following the `-Wait` timeout convention used by `Stop-PveContainer` and `Reset-PveVm`. See `DECISIONS.md` D014. ## [0.2.0] - 2026-05-22 diff --git a/DECISIONS.md b/DECISIONS.md index 9587b50..a11fb78 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -466,10 +466,20 @@ Add-PveClusterMember ... ## D015 — Lifecycle -Wait blocks until the guest config lock clears -**Status**: Active +**Status**: Superseded by D016 (2026-09-01) — the mechanism below is wrong **Finding refs**: (none — found via integration runs 183/184, 2026-09-01) **Resolved in scan**: n/a +> **This entry misdiagnosed the failure it was written for.** Two different things in PVE are +> called "lock": the **config lock** (the `lock:` property — `migrate`, `backup`, `clone`, +> `snapshot` — a persisted config field, exposed as `lock` in `status/current`) and the +> **flock** on `/var/lock/qemu-server/lock-.conf` taken by `PVE::QemuConfig->lock_config`, +> which is not exposed through the API at all. The integration failures were the flock; this +> entry guards the config lock, which an ordinary start/stop never sets. Run 186 confirmed the +> check never fired — `Restart-PveVm` took 4.11 s, unchanged. The real cause and fix are in +> **D016**. The waiting behaviour described below is harmless and still applies when a genuine +> config lock is present, so the code stays. + ### Decision `WaitForStatusTransition` returns only when the guest reports the expected status **and** its config lock has cleared. Reaching the status is not enough: PVE publishes the new @@ -517,3 +527,66 @@ var snapshot = GuestStatusSnapshot.Evaluate(json, expectedStatus); if (snapshot.StatusMatched && !snapshot.Locked) return task; ``` + +--- + +## D016 — Restart-PveVm uses PVE's native reboot endpoint + +**Status**: Active +**Finding refs**: (none — found via integration runs 183/185/186, root-caused on a live PVE 9.2.2 node 2026-09-01) +**Resolved in scan**: n/a + +### Decision +`Restart-PveVm` calls `POST /nodes/{node}/qemu/{vmid}/status/reboot` (`VmService.RebootVm`). +It must **not** compose a restart client-side as `status/shutdown` followed by `status/start`. + +### Rationale +Composing the restart races Proxmox's own post-stop cleanup for the guest's config flock: + +1. The shutdown completes and the QEMU process exits. +2. `qmeventd` forks `/usr/sbin/qm cleanup ...`. +3. The client sees `status == stopped` and immediately posts `status/start`. `vm_start` takes + the flock, wins the race, starts a **new** QEMU, releases. +4. `qm cleanup` then takes the flock with a **60 s** timeout and polls `vm_running_locally` + for up to **30 s**, holding it the whole time, because it sees the new PID as the old one + failing to exit. PVE's own warning names this: `"QEMU process $pid for VM $vmid still + running (or newly started)"`. +5. Every subsequent call fails: `lock_config` defaults to **10 s**, so the client gets + `can't lock file '/var/lock/qemu-server/lock-.conf' - got timeout`. + +Measured on a reproduction (integration run 187), three distinct source constants matching: + +``` +qmstart ends t+3 <- qm cleanup takes the flock, sees the NEW pid +qmstop #1 FAIL t+14 10 s = lock_config default +qmstop #2 FAIL t+24 10 s = lock_config default +qmclone FAIL t+25 1 s = qmclone's separate source-VM lock timeout +qmstop #3 OK t+33 <- released; hold was t+3..t+33 = 30 s = cleanup's wait loop +``` + +`vm_reboot` avoids all of it by holding the config lock across the entire shutdown and letting +`qm cleanup` perform the restart while it already holds that same lock — there is no window for +a client call to interleave. + +This surfaced on PVE 9.2 and not 9.1 because of two May 2026 qemu-server changes (cleanup +deduplication, shipped for 9.1.13, and the 30 s cleanup wait). Neither touches the REST surface, +so the API changelog showed nothing — "the API did not change, therefore behaviour did not" is +not a valid inference for this class of bug. + +**Containers are not affected by this decision**: `/nodes/{node}/lxc/{vmid}/status/reboot` does +not exist, so `Restart-PveContainer` necessarily keeps shutdown + start. + +### Anti-pattern (do not reintroduce) +```csharp +// NEVER compose a VM restart from two client calls — it races qmeventd's cleanup +var shutdownTask = vmService.ShutdownVm(session, node, vmid, timeout); +WaitForStatusTransition(session, node, shutdownTask, vmid, "stopped", timeout); +var startTask = vmService.StartVm(session, node, vmid); +``` + +### Correct pattern +```csharp +var task = vmService.RebootVm(session, node, vmid, timeout); +if (Wait.IsPresent) + task = WaitForStatusTransition(session, node, task, vmid, "running", timeout); +``` diff --git a/src/PSProxmoxVE.Core/Services/VmService.cs b/src/PSProxmoxVE.Core/Services/VmService.cs index 5b7764a..bc409e7 100644 --- a/src/PSProxmoxVE.Core/Services/VmService.cs +++ b/src/PSProxmoxVE.Core/Services/VmService.cs @@ -326,6 +326,42 @@ namespace PSProxmoxVE.Core.Services } } + /// + /// Reboots a VM through PVE's native reboot endpoint. Returns the task UPID. + /// + /// + /// PVE holds the guest's config lock across the whole shutdown and restarts the VM from + /// its own post-stop cleanup, so nothing can interleave between the two halves. Composing + /// a reboot client-side as shutdown + start instead races that cleanup: the start wins the + /// lock, cleanup then holds it for 30 s waiting on the newly started process, and the next + /// call fails with "can't lock file '/var/lock/qemu-server/lock-<vmid>.conf' - got timeout". + /// + /// The authenticated PVE session. + /// The cluster node name. + /// The VM ID. + /// Optional maximum seconds to wait for the shutdown half. + public PveTask RebootVm(PveSession session, string node, int vmid, int? timeoutSeconds = null) + { + if (session == null) throw new ArgumentNullException(nameof(session)); + if (string.IsNullOrWhiteSpace(node)) throw new ArgumentNullException(nameof(node)); + + var formData = new Dictionary(); + if (timeoutSeconds.HasValue) + formData["timeout"] = timeoutSeconds.Value.ToString(); + + IPveHttpClient client = _injectedClient ?? new PveHttpClient(session); + try + { + var response = client.PostAsync($"nodes/{Uri.EscapeDataString(node)}/qemu/{vmid}/status/reboot", formData) + .GetAwaiter().GetResult(); + return ParseTask(response, node); + } + finally + { + if (_injectedClient == null) client.Dispose(); + } + } + /// Resets a VM (hard reset). Returns the task UPID. /// The authenticated PVE session. /// The cluster node name. diff --git a/src/PSProxmoxVE/Cmdlets/Vms/RestartPveVmCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/RestartPveVmCmdlet.cs index 5b3bdae..8c1b049 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/RestartPveVmCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/RestartPveVmCmdlet.cs @@ -7,9 +7,10 @@ namespace PSProxmoxVE.Cmdlets.Vms /// /// Gracefully restarts a QEMU/KVM virtual machine on a Proxmox VE node. /// - /// Performs a graceful shutdown of the VM followed by a start, via the Proxmox VE API. - /// A configurable timeout controls how long to wait for the guest to shut down cleanly - /// before the operation is considered failed. Use -Wait to block until both tasks complete. + /// Reboots the VM through Proxmox VE's native reboot endpoint, which shuts the guest down + /// and starts it again as a single server-side operation. A configurable timeout controls + /// how long to wait for the guest to shut down cleanly. Use -Wait to block until the VM is + /// running again. /// /// [Cmdlet(VerbsLifecycle.Restart, "PveVm", SupportsShouldProcess = true, ConfirmImpact = ConfirmImpact.High)] @@ -41,7 +42,7 @@ namespace PSProxmoxVE.Cmdlets.Vms public int Timeout { get; set; } = 60; /// - /// When specified, waits for both shutdown and start tasks to complete before returning. + /// When specified, waits until the VM is running again before returning. /// [Parameter(Mandatory = false, HelpMessage = "Wait for the task to complete before returning.")] public SwitchParameter Wait { get; set; } @@ -56,19 +57,12 @@ namespace PSProxmoxVE.Cmdlets.Vms WriteVerbose($"Restarting VM {VmId} on node '{Node}'..."); - // Graceful shutdown - var shutdownTask = vmService.ShutdownVm(session, Node, VmId, Timeout); + var task = vmService.RebootVm(session, Node, VmId, Timeout); if (Wait.IsPresent) - WaitForStatusTransition(session, Node, shutdownTask, VmId, "stopped", Timeout); + task = WaitForStatusTransition(session, Node, task, VmId, "running", Timeout); - // Start - var startTask = vmService.StartVm(session, Node, VmId); - - if (Wait.IsPresent) - startTask = WaitForStatusTransition(session, Node, startTask, VmId, "running", Timeout); - - WriteObject(startTask); + WriteObject(task); } } } diff --git a/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs b/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs index 136b064..230ce2e 100644 --- a/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs @@ -109,5 +109,61 @@ namespace PSProxmoxVE.Core.Tests.Services "cmd.exe", new[] { "/c", null!, "echo" })); Assert.Equal("args", ex.ParamName); } + + [Fact] + public void RebootVm_PostsToTheNativeRebootEndpoint() + { + string? resource = null; + var mockClient = new Mock(); + mockClient + .Setup(c => c.PostAsync(It.IsAny(), It.IsAny>())) + .Callback>((r, _) => resource = r) + .ReturnsAsync("{\"data\":\"UPID:pve1:00001234:00005678:6A970AAB:qmreboot:100:root@pam:\"}"); + + var service = new VmService(mockClient.Object); + var task = service.RebootVm(CreateSession(), TestNode, TestVmId); + + // Composing a reboot as shutdown + start races PVE's post-stop cleanup for the + // config lock; the native endpoint keeps the whole restart server-side. + Assert.Equal($"nodes/{TestNode}/qemu/{TestVmId}/status/reboot", resource); + Assert.Contains("qmreboot", task.Upid); + } + + [Fact] + public void RebootVm_SendsTimeoutWhenSupplied() + { + List>? captured = null; + var mockClient = new Mock(); + mockClient + .Setup(c => c.PostAsync(It.IsAny(), It.IsAny>())) + .Callback>((_, data) => captured = data.ToList()) + .ReturnsAsync("{\"data\":\"UPID:pve1:00001234:00005678:6A970AAB:qmreboot:100:root@pam:\"}"); + + var service = new VmService(mockClient.Object); + service.RebootVm(CreateSession(), TestNode, TestVmId, 45); + + Assert.NotNull(captured); + Assert.Single(captured!); + Assert.Equal("timeout", captured![0].Key); + Assert.Equal("45", captured![0].Value); + } + + [Fact] + public void RebootVm_OmitsTimeoutWhenNotSupplied() + { + List>? captured = null; + var mockClient = new Mock(); + mockClient + .Setup(c => c.PostAsync(It.IsAny(), It.IsAny>())) + .Callback>((_, data) => captured = data.ToList()) + .ReturnsAsync("{\"data\":\"UPID:pve1:00001234:00005678:6A970AAB:qmreboot:100:root@pam:\"}"); + + var service = new VmService(mockClient.Object); + service.RebootVm(CreateSession(), TestNode, TestVmId); + + Assert.NotNull(captured); + Assert.Empty(captured!); + } + } }