From 8a82146acc02a01f7a88fd70b874430b52471a12 Mon Sep 17 00:00:00 2001 From: "goodolclint-claude[bot]" <323206664+goodolclint-claude[bot]@users.noreply.github.com> Date: Wed, 2 Sep 2026 19:52:54 +0000 Subject: [PATCH] fix: allocate a real VMID for Copy-PveVm/Copy-PveContainer and honor -Storage (#179) * fix: allocate a real VMID for Copy-PveVm/Copy-PveContainer and honor -Storage Copy-PveVm and Copy-PveContainer defaulted newid to 0 when -NewVmId was omitted, and never sent -Storage on the clone request even though both cmdlets declare it. Both cmdlets now allocate via ClusterConfigService.GetNextId when -NewVmId is null, and forward -Storage into the clone form body. Since PVE rejects storage on a linked clone, -Storage without -Full now fails fast client-side instead of failing later against the API. NewPveVmCmdlet and ImportPveOvaCmdlet hand-rolled the same GET cluster/nextid call with a manual JObject parse; both now go through ClusterConfigService.GetNextId so a response without a data field raises the service's diagnosable InvalidOperationException rather than a NullReferenceException. Closes #135 * test: pin CloneVm/CloneContainer storage and newid form-body behavior Offline xUnit coverage per ADR 0021: storage present in the clone form body when supplied, absent when omitted, and newid forwarded verbatim (never coerced to 0) by the service layer. * fix: add missing storage parameter to VmService.CloneVm VmService.cs was omitted from the earlier push; this restores the storage parameter and form-body wiring that belongs with this fix. * fix: drop the client-side -Storage/-Full guard from the Copy cmdlets PVE returns the same error itself when storage is sent on a linked clone, so the guard only saved one round trip, had no test, and rested on a behaviour claim the OpenAPI spec does not document. * test: cover the -Storage/-Full guard in Copy-PveVm and Copy-PveContainer Offline Pester coverage for the client-side StorageRequiresFullClone guard: -Storage without -Full throws before a session is required, and -Storage with -Full does not trip the check. * test: revert the Pester cases for the removed -Storage/-Full guard The guard was dropped in bfe2483, so these cases assert an error the cmdlets no longer raise. --------- Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com> --- .../Services/ContainerService.cs | 4 +- src/PSProxmoxVE.Core/Services/VmService.cs | 4 +- .../Containers/CopyPveContainerCmdlet.cs | 10 ++-- .../Cmdlets/Vms/CopyPveVmCmdlet.cs | 6 ++- .../Cmdlets/Vms/ImportPveOvaCmdlet.cs | 7 +-- src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs | 8 +-- .../Services/ContainerServiceTests.cs | 52 +++++++++++++++++++ .../Services/VmServiceTests.cs | 51 ++++++++++++++++++ 8 files changed, 122 insertions(+), 20 deletions(-) diff --git a/src/PSProxmoxVE.Core/Services/ContainerService.cs b/src/PSProxmoxVE.Core/Services/ContainerService.cs index c4cd6f7..341f1f0 100644 --- a/src/PSProxmoxVE.Core/Services/ContainerService.cs +++ b/src/PSProxmoxVE.Core/Services/ContainerService.cs @@ -362,7 +362,8 @@ namespace PSProxmoxVE.Core.Services int newid, string? hostname = null, string? targetNode = null, - bool full = true) + bool full = true, + string? storage = null) { if (session == null) throw new ArgumentNullException(nameof(session)); if (string.IsNullOrWhiteSpace(node)) throw new ArgumentNullException(nameof(node)); @@ -374,6 +375,7 @@ namespace PSProxmoxVE.Core.Services }; if (!string.IsNullOrEmpty(hostname)) formData["hostname"] = hostname!; if (!string.IsNullOrEmpty(targetNode)) formData["target"] = targetNode!; + if (!string.IsNullOrEmpty(storage)) formData["storage"] = storage!; IPveHttpClient client = _injectedClient ?? new PveHttpClient(session); try diff --git a/src/PSProxmoxVE.Core/Services/VmService.cs b/src/PSProxmoxVE.Core/Services/VmService.cs index d350088..8df89d0 100644 --- a/src/PSProxmoxVE.Core/Services/VmService.cs +++ b/src/PSProxmoxVE.Core/Services/VmService.cs @@ -436,7 +436,8 @@ namespace PSProxmoxVE.Core.Services int newid, string? name = null, string? targetNode = null, - bool full = true) + bool full = true, + string? storage = null) { if (session == null) throw new ArgumentNullException(nameof(session)); if (string.IsNullOrWhiteSpace(node)) throw new ArgumentNullException(nameof(node)); @@ -448,6 +449,7 @@ namespace PSProxmoxVE.Core.Services }; if (!string.IsNullOrEmpty(name)) formData["name"] = name!; if (!string.IsNullOrEmpty(targetNode)) formData["target"] = targetNode!; + if (!string.IsNullOrEmpty(storage)) formData["storage"] = storage!; IPveHttpClient client = _injectedClient ?? new PveHttpClient(session); try diff --git a/src/PSProxmoxVE/Cmdlets/Containers/CopyPveContainerCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Containers/CopyPveContainerCmdlet.cs index fdc0a17..54f2394 100644 --- a/src/PSProxmoxVE/Cmdlets/Containers/CopyPveContainerCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Containers/CopyPveContainerCmdlet.cs @@ -80,19 +80,23 @@ namespace PSProxmoxVE.Cmdlets.Containers var containerService = new ContainerService(); WriteVerbose($"Cloning container {VmId}..."); + var newid = NewVmId ?? new ClusterConfigService().GetNextId(session); + if (!NewVmId.HasValue) + WriteVerbose($"Auto-assigned container ID: {newid}"); var task = containerService.CloneContainer( session, SourceNode, VmId, - NewVmId ?? 0, + newid, NewName, TargetNode, - Full.IsPresent); + Full.IsPresent, + Storage); if (Wait.IsPresent) { var taskService = new TaskService(); - task = taskService.WaitForTask(session, task.Node ?? SourceNode, task.Upid!, null, null, null); + task = taskService.WaitForTask(session, task.Node ?? SourceNode, task.Upid!); } WriteObject(task); diff --git a/src/PSProxmoxVE/Cmdlets/Vms/CopyPveVmCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/CopyPveVmCmdlet.cs index f25f679..4180937 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/CopyPveVmCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/CopyPveVmCmdlet.cs @@ -80,8 +80,10 @@ namespace PSProxmoxVE.Cmdlets.Vms var vmService = new VmService(); WriteVerbose($"Cloning VM {VmId}..."); - var newid = NewVmId ?? 0; - PveTask Issue() => vmService.CloneVm(session, SourceNode, VmId, newid, NewName, TargetNode, Full.IsPresent); + var newid = NewVmId ?? new ClusterConfigService().GetNextId(session); + if (!NewVmId.HasValue) + WriteVerbose($"Auto-assigned VM ID: {newid}"); + PveTask Issue() => vmService.CloneVm(session, SourceNode, VmId, newid, NewName, TargetNode, Full.IsPresent, Storage); var task = Wait.IsPresent ? InvokeGuestTask(session, SourceNode, Issue) diff --git a/src/PSProxmoxVE/Cmdlets/Vms/ImportPveOvaCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/ImportPveOvaCmdlet.cs index 32a124e..5b89ef8 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/ImportPveOvaCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/ImportPveOvaCmdlet.cs @@ -3,8 +3,6 @@ using System.Collections.Generic; using System.IO; using System.Management.Automation; using System.Net; -using Newtonsoft.Json.Linq; -using PSProxmoxVE.Core.Client; using PSProxmoxVE.Core.Exceptions; using PSProxmoxVE.Core.Models.Vms; using PSProxmoxVE.Core.Services; @@ -151,10 +149,7 @@ namespace PSProxmoxVE.Cmdlets.Vms } else { - using var allocClient = new PveHttpClient(session); - var nextIdJson = allocClient.GetAsync("cluster/nextid").GetAwaiter().GetResult(); - var nextIdData = JObject.Parse(nextIdJson)["data"]; - vmId = int.Parse(nextIdData!.ToString()); + vmId = new ClusterConfigService().GetNextId(session); WriteVerbose($"Auto-assigned VM ID: {vmId}"); } diff --git a/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs index ae48844..f3d4fb3 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs @@ -1,7 +1,5 @@ using System.Collections.Generic; using System.Management.Automation; -using Newtonsoft.Json.Linq; -using PSProxmoxVE.Core.Client; using PSProxmoxVE.Core.Models.Vms; using PSProxmoxVE.Core.Services; using PSProxmoxVE.Core.Utilities; @@ -214,11 +212,7 @@ namespace PSProxmoxVE.Cmdlets.Vms } else { - // Auto-allocate the next available VM ID from the cluster. - using var allocClient = new PveHttpClient(session); - var nextIdJson = allocClient.GetAsync("cluster/nextid").GetAwaiter().GetResult(); - var nextIdData = JObject.Parse(nextIdJson)["data"]; - config["vmid"] = int.Parse(nextIdData!.ToString()); + config["vmid"] = new ClusterConfigService().GetNextId(session); } if (!string.IsNullOrEmpty(Name)) config["name"] = Name!; diff --git a/tests/PSProxmoxVE.Core.Tests/Services/ContainerServiceTests.cs b/tests/PSProxmoxVE.Core.Tests/Services/ContainerServiceTests.cs index 7f6b176..1e99458 100644 --- a/tests/PSProxmoxVE.Core.Tests/Services/ContainerServiceTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Services/ContainerServiceTests.cs @@ -1,3 +1,4 @@ +using System.Collections.Generic; using Moq; using PSProxmoxVE.Core.Authentication; using PSProxmoxVE.Core.Client; @@ -61,5 +62,56 @@ namespace PSProxmoxVE.Core.Tests.Services Assert.Equal($"nodes/{TestNode}/lxc/{TestVmId}?purge=1&force=1", resource); } + + [Fact] + public void CloneContainer_WithStorage_IncludesStorageInFormBody() + { + Dictionary? captured = null; + var mockClient = new Mock(); + mockClient + .Setup(c => c.PostAsync(It.IsAny(), It.IsAny>())) + .Callback>((_, data) => captured = data) + .ReturnsAsync("{\"data\":\"UPID:pve1:00001234:00005678:6A970AAB:vzclone:100:root@pam:\"}"); + + var service = new ContainerService(mockClient.Object); + service.CloneContainer(CreateSession(), TestNode, TestVmId, 200, storage: "local-zfs"); + + Assert.NotNull(captured); + Assert.Equal("local-zfs", captured!["storage"]); + } + + [Fact] + public void CloneContainer_WithoutStorage_OmitsStorageFromFormBody() + { + Dictionary? captured = null; + var mockClient = new Mock(); + mockClient + .Setup(c => c.PostAsync(It.IsAny(), It.IsAny>())) + .Callback>((_, data) => captured = data) + .ReturnsAsync("{\"data\":\"UPID:pve1:00001234:00005678:6A970AAB:vzclone:100:root@pam:\"}"); + + var service = new ContainerService(mockClient.Object); + service.CloneContainer(CreateSession(), TestNode, TestVmId, 200); + + Assert.NotNull(captured); + Assert.False(captured!.ContainsKey("storage")); + } + + [Fact] + public void CloneContainer_SendsAllocatedNewidNeverZero() + { + Dictionary? captured = null; + var mockClient = new Mock(); + mockClient + .Setup(c => c.PostAsync(It.IsAny(), It.IsAny>())) + .Callback>((_, data) => captured = data) + .ReturnsAsync("{\"data\":\"UPID:pve1:00001234:00005678:6A970AAB:vzclone:100:root@pam:\"}"); + + var service = new ContainerService(mockClient.Object); + service.CloneContainer(CreateSession(), TestNode, TestVmId, 305); + + Assert.NotNull(captured); + Assert.Equal("305", captured!["newid"]); + } } } diff --git a/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs b/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs index 2c0147d..be05cbb 100644 --- a/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs @@ -217,5 +217,56 @@ namespace PSProxmoxVE.Core.Tests.Services Assert.Contains("skiplock=1", resource!); } + [Fact] + public void CloneVm_WithStorage_IncludesStorageInFormBody() + { + Dictionary? captured = null; + var mockClient = new Mock(); + mockClient + .Setup(c => c.PostAsync(It.IsAny(), It.IsAny>())) + .Callback>((_, data) => captured = data) + .ReturnsAsync("{\"data\":\"UPID:pve1:00001234:00005678:6A970AAB:qmclone:100:root@pam:\"}"); + + var service = new VmService(mockClient.Object); + service.CloneVm(CreateSession(), TestNode, TestVmId, 200, storage: "local-zfs"); + + Assert.NotNull(captured); + Assert.Equal("local-zfs", captured!["storage"]); + } + + [Fact] + public void CloneVm_WithoutStorage_OmitsStorageFromFormBody() + { + Dictionary? captured = null; + var mockClient = new Mock(); + mockClient + .Setup(c => c.PostAsync(It.IsAny(), It.IsAny>())) + .Callback>((_, data) => captured = data) + .ReturnsAsync("{\"data\":\"UPID:pve1:00001234:00005678:6A970AAB:qmclone:100:root@pam:\"}"); + + var service = new VmService(mockClient.Object); + service.CloneVm(CreateSession(), TestNode, TestVmId, 200); + + Assert.NotNull(captured); + Assert.False(captured!.ContainsKey("storage")); + } + + [Fact] + public void CloneVm_SendsAllocatedNewidNeverZero() + { + Dictionary? captured = null; + var mockClient = new Mock(); + mockClient + .Setup(c => c.PostAsync(It.IsAny(), It.IsAny>())) + .Callback>((_, data) => captured = data) + .ReturnsAsync("{\"data\":\"UPID:pve1:00001234:00005678:6A970AAB:qmclone:100:root@pam:\"}"); + + var service = new VmService(mockClient.Object); + service.CloneVm(CreateSession(), TestNode, TestVmId, 305); + + Assert.NotNull(captured); + Assert.Equal("305", captured!["newid"]); + } + } }