From e942878aa249d87891a57fb8913d04ec09f3f07f Mon Sep 17 00:00:00 2001 From: "goodolclint-claude[bot]" <323206664+goodolclint-claude[bot]@users.noreply.github.com> Date: Wed, 2 Sep 2026 13:20:04 -0500 Subject: [PATCH] fix: plumb skiplock/force parameters through Remove-PveVm/Container (#164) * fix: plumb skiplock parameter through Remove-PveVm and Remove-PveContainer When -Force is specified, both cmdlets now pass skiplock=1 to PVE, which bypasses locks. PVE honours the skiplock parameter for root@pam only. Also updated help text on both cmdlets to clarify the limitation and behaviour. Fixes #136. * fix: plumb skiplock parameter for VMs, force for containers VM removal: add skiplock=1 parameter when -Force is specified. PVE honors it for root@pam only; non-root callers receive 403 errors. Updated help text. Container removal: map -Force to force=1 (LXC-specific parameter for forcing removal of running containers). Containers do not support skiplock. Also: clarified class and parameter documentation to remove false claims about -Force suppressing confirmation (it does not). Fixes #136. Addresses correctness reviewer findings. * fix: revert unrelated formatting churn, add container force test The em-dash-to-hyphen sweep in commit 75f4bbf touched VmService.cs and VmServiceTests.cs outside the skiplock/force change; restore the original text there, including the <vmid> XML-doc escape that had been un-escaped and was malforming the generated documentation. Add ContainerServiceTests.cs, covering the untested force=1 query-string plumbing in ContainerService.RemoveContainer. --------- Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com> --- .../Services/ContainerService.cs | 12 +++- src/PSProxmoxVE.Core/Services/VmService.cs | 12 +++- .../Containers/RemovePveContainerCmdlet.cs | 8 +-- .../Cmdlets/Vms/RemovePveVmCmdlet.cs | 8 +-- .../Services/ContainerServiceTests.cs | 65 +++++++++++++++++++ .../Services/VmServiceTests.cs | 52 +++++++++++++++ 6 files changed, 143 insertions(+), 14 deletions(-) create mode 100644 tests/PSProxmoxVE.Core.Tests/Services/ContainerServiceTests.cs diff --git a/src/PSProxmoxVE.Core/Services/ContainerService.cs b/src/PSProxmoxVE.Core/Services/ContainerService.cs index 374b8ba..c4cd6f7 100644 --- a/src/PSProxmoxVE.Core/Services/ContainerService.cs +++ b/src/PSProxmoxVE.Core/Services/ContainerService.cs @@ -329,16 +329,22 @@ namespace PSProxmoxVE.Core.Services PveSession session, string node, int vmid, - bool purge = false) + bool purge = false, + bool force = false) { if (session == null) throw new ArgumentNullException(nameof(session)); if (string.IsNullOrWhiteSpace(node)) throw new ArgumentNullException(nameof(node)); - var purgeParam = purge ? "?purge=1" : "?purge=0"; + var queryParams = new List(); + queryParams.Add(purge ? "purge=1" : "purge=0"); + if (force) + queryParams.Add("force=1"); + var queryString = "?" + string.Join("&", queryParams); + IPveHttpClient client = _injectedClient ?? new PveHttpClient(session); try { - var response = client.DeleteAsync($"nodes/{Uri.EscapeDataString(node)}/lxc/{vmid}{purgeParam}") + var response = client.DeleteAsync($"nodes/{Uri.EscapeDataString(node)}/lxc/{vmid}{queryString}") .GetAwaiter().GetResult(); return ParseTask(response, node); } diff --git a/src/PSProxmoxVE.Core/Services/VmService.cs b/src/PSProxmoxVE.Core/Services/VmService.cs index bc409e7..d350088 100644 --- a/src/PSProxmoxVE.Core/Services/VmService.cs +++ b/src/PSProxmoxVE.Core/Services/VmService.cs @@ -394,16 +394,22 @@ namespace PSProxmoxVE.Core.Services /// The cluster node name. /// The VM ID. /// If true, also removes all associated backup files and jobs. - public PveTask RemoveVm(PveSession session, string node, int vmid, bool purge = false) + /// If true, bypasses locks (PVE honours this for root@pam only). + public PveTask RemoveVm(PveSession session, string node, int vmid, bool purge = false, bool skipLock = false) { if (session == null) throw new ArgumentNullException(nameof(session)); if (string.IsNullOrWhiteSpace(node)) throw new ArgumentNullException(nameof(node)); - var purgeParam = purge ? "?purge=1" : "?purge=0"; + var queryParams = new List(); + queryParams.Add(purge ? "purge=1" : "purge=0"); + if (skipLock) + queryParams.Add("skiplock=1"); + var queryString = "?" + string.Join("&", queryParams); + IPveHttpClient client = _injectedClient ?? new PveHttpClient(session); try { - var response = client.DeleteAsync($"nodes/{Uri.EscapeDataString(node)}/qemu/{vmid}{purgeParam}") + var response = client.DeleteAsync($"nodes/{Uri.EscapeDataString(node)}/qemu/{vmid}{queryString}") .GetAwaiter().GetResult(); return ParseTask(response, node); } diff --git a/src/PSProxmoxVE/Cmdlets/Containers/RemovePveContainerCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Containers/RemovePveContainerCmdlet.cs index 58fdfcd..7c6e5a3 100644 --- a/src/PSProxmoxVE/Cmdlets/Containers/RemovePveContainerCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Containers/RemovePveContainerCmdlet.cs @@ -8,7 +8,7 @@ namespace PSProxmoxVE.Cmdlets.Containers /// Removes an LXC container from a Proxmox VE node. /// /// Deletes an LXC container and, optionally, all associated storage. - /// This operation is destructive and requires confirmation unless -Force is specified. + /// This operation is destructive and requires confirmation. /// /// [Cmdlet(VerbsCommon.Remove, "PveContainer", @@ -42,10 +42,10 @@ namespace PSProxmoxVE.Cmdlets.Containers /// /// - /// When specified, bypasses locks and forces removal even if a lock is set on the container. + /// When specified, sends force=1 to PVE, allowing removal of running containers. /// /// - [Parameter(Mandatory = false, HelpMessage = "Force the operation without additional checks.")] + [Parameter(Mandatory = false, HelpMessage = "Force destroy, even if running.")] public SwitchParameter Force { get; set; } /// @@ -63,7 +63,7 @@ namespace PSProxmoxVE.Cmdlets.Containers var containerService = new ContainerService(); WriteVerbose($"Removing container {VmId} from node '{Node}'..."); - var task = containerService.RemoveContainer(session, Node, VmId, Purge.IsPresent); + var task = containerService.RemoveContainer(session, Node, VmId, Purge.IsPresent, Force.IsPresent); if (Wait.IsPresent) { diff --git a/src/PSProxmoxVE/Cmdlets/Vms/RemovePveVmCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/RemovePveVmCmdlet.cs index 785433b..e5901ed 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/RemovePveVmCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/RemovePveVmCmdlet.cs @@ -8,7 +8,7 @@ namespace PSProxmoxVE.Cmdlets.Vms /// Removes a QEMU/KVM virtual machine from a Proxmox VE node. /// /// Deletes a virtual machine and, optionally, all associated disk images. - /// This operation is destructive and requires confirmation unless -Force is specified. + /// This operation is destructive and requires confirmation. /// /// [Cmdlet(VerbsCommon.Remove, "PveVm", @@ -42,10 +42,10 @@ namespace PSProxmoxVE.Cmdlets.Vms /// /// - /// When specified, bypasses locks and forces removal even if a lock is set on the VM. + /// When specified, sends skiplock=1 to PVE to bypass locks. PVE honours this parameter for root@pam only; non-root callers will receive a 403 permission error. /// /// - [Parameter(Mandatory = false, HelpMessage = "Force the operation without additional checks.")] + [Parameter(Mandatory = false, HelpMessage = "Bypass locks via skiplock=1 (root@pam only).")] public SwitchParameter Force { get; set; } /// @@ -63,7 +63,7 @@ namespace PSProxmoxVE.Cmdlets.Vms var vmService = new VmService(); WriteVerbose($"Removing VM {VmId} from node '{Node}'..."); - var task = vmService.RemoveVm(session, Node, VmId, Purge.IsPresent); + var task = vmService.RemoveVm(session, Node, VmId, Purge.IsPresent, Force.IsPresent); if (Wait.IsPresent) { diff --git a/tests/PSProxmoxVE.Core.Tests/Services/ContainerServiceTests.cs b/tests/PSProxmoxVE.Core.Tests/Services/ContainerServiceTests.cs new file mode 100644 index 0000000..7f6b176 --- /dev/null +++ b/tests/PSProxmoxVE.Core.Tests/Services/ContainerServiceTests.cs @@ -0,0 +1,65 @@ +using Moq; +using PSProxmoxVE.Core.Authentication; +using PSProxmoxVE.Core.Client; +using PSProxmoxVE.Core.Services; +using Xunit; + +namespace PSProxmoxVE.Core.Tests.Services +{ + public class ContainerServiceTests + { + private const string TestNode = "pve1"; + private const int TestVmId = 100; + + private static PveSession CreateSession() => + new PveSession("pve1.example.com", 8006, true, "PVE:root@pam:TEST_TOKEN"); + + [Fact] + public void RemoveContainer_WithForceTrue_IncludesForceInQueryString() + { + string? resource = null; + var mockClient = new Mock(); + mockClient + .Setup(c => c.DeleteAsync(It.IsAny())) + .Callback(r => resource = r) + .ReturnsAsync("{\"data\":\"UPID:pve1:00001234:00005678:6A970AAB:vzdestroy:100:root@pam:\"}"); + + var service = new ContainerService(mockClient.Object); + service.RemoveContainer(CreateSession(), TestNode, TestVmId, purge: false, force: true); + + Assert.Equal($"nodes/{TestNode}/lxc/{TestVmId}?purge=0&force=1", resource); + } + + [Fact] + public void RemoveContainer_WithForceFalse_OmitsForceFromQueryString() + { + string? resource = null; + var mockClient = new Mock(); + mockClient + .Setup(c => c.DeleteAsync(It.IsAny())) + .Callback(r => resource = r) + .ReturnsAsync("{\"data\":\"UPID:pve1:00001234:00005678:6A970AAB:vzdestroy:100:root@pam:\"}"); + + var service = new ContainerService(mockClient.Object); + service.RemoveContainer(CreateSession(), TestNode, TestVmId, purge: false, force: false); + + Assert.Equal($"nodes/{TestNode}/lxc/{TestVmId}?purge=0", resource); + } + + [Fact] + public void RemoveContainer_WithPurgeAndForce_IncludesBothInQueryString() + { + string? resource = null; + var mockClient = new Mock(); + mockClient + .Setup(c => c.DeleteAsync(It.IsAny())) + .Callback(r => resource = r) + .ReturnsAsync("{\"data\":\"UPID:pve1:00001234:00005678:6A970AAB:vzdestroy:100:root@pam:\"}"); + + var service = new ContainerService(mockClient.Object); + service.RemoveContainer(CreateSession(), TestNode, TestVmId, purge: true, force: true); + + Assert.Equal($"nodes/{TestNode}/lxc/{TestVmId}?purge=1&force=1", resource); + } + } +} diff --git a/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs b/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs index 230ce2e..2c0147d 100644 --- a/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs @@ -165,5 +165,57 @@ namespace PSProxmoxVE.Core.Tests.Services Assert.Empty(captured!); } + [Fact] + public void RemoveVm_WithSkipLockTrue_IncludesSkiplockInQueryString() + { + string? resource = null; + var mockClient = new Mock(); + mockClient + .Setup(c => c.DeleteAsync(It.IsAny())) + .Callback(r => resource = r) + .ReturnsAsync("{\"data\":\"UPID:pve1:00001234:00005678:6A970AAB:qmremove:100:root@pam:\"}"); + + var service = new VmService(mockClient.Object); + service.RemoveVm(CreateSession(), TestNode, TestVmId, purge: false, skipLock: true); + + Assert.NotNull(resource); + Assert.Contains("skiplock=1", resource!); + } + + [Fact] + public void RemoveVm_WithSkipLockFalse_OmitsSkiplockFromQueryString() + { + string? resource = null; + var mockClient = new Mock(); + mockClient + .Setup(c => c.DeleteAsync(It.IsAny())) + .Callback(r => resource = r) + .ReturnsAsync("{\"data\":\"UPID:pve1:00001234:00005678:6A970AAB:qmremove:100:root@pam:\"}"); + + var service = new VmService(mockClient.Object); + service.RemoveVm(CreateSession(), TestNode, TestVmId, purge: false, skipLock: false); + + Assert.NotNull(resource); + Assert.DoesNotContain("skiplock", resource!); + } + + [Fact] + public void RemoveVm_WithPurgeAndSkipLock_IncludesBothInQueryString() + { + string? resource = null; + var mockClient = new Mock(); + mockClient + .Setup(c => c.DeleteAsync(It.IsAny())) + .Callback(r => resource = r) + .ReturnsAsync("{\"data\":\"UPID:pve1:00001234:00005678:6A970AAB:qmremove:100:root@pam:\"}"); + + var service = new VmService(mockClient.Object); + service.RemoveVm(CreateSession(), TestNode, TestVmId, purge: true, skipLock: true); + + Assert.NotNull(resource); + Assert.Contains("purge=1", resource!); + Assert.Contains("skiplock=1", resource!); + } + } }