diff --git a/docs/review/findings.json b/docs/review/findings.json index 4db20e6..57dee1e 100644 --- a/docs/review/findings.json +++ b/docs/review/findings.json @@ -2881,6 +2881,66 @@ "evidence": "Added PveSession.Timeout (default 100s) and a TimeSpan? override on PveHttpClient. Connect-PveServer exposes -TimeoutSeconds to set the session-default; Send-PveFile and Invoke-PveStorageDownload expose -TimeoutSeconds with a 30-minute implicit default so large uploads/downloads do not trip the 100s HttpClient default. -TimeoutSeconds 0 means Timeout.InfiniteTimeSpan. PveHttpClient.SendAsync now catches the TimeoutException-wrapped TaskCanceledException and rethrows it as PveApiException(RequestTimeout) with the resource path.", "verified_by": "dotnet build + dotnet test (passed including new SendAsync_TimeoutFires xUnit test) + Pester (-TimeoutSeconds coverage on all three cmdlets)" } + }, + { + "id": "F089", + "title": "New-PveVm cannot set disk controller or IO options at create time", + "category": "enhancement", + "severity": "medium", + "status": "resolved", + "first_detected": "2026-05-22", + "github_issue": 65, + "files": [ + "src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs" + ], + "description": "New-PveVm only emitted a plain virtio0 from -DiskStorage/-DiskSize/-DiskFormat. There was no way to choose the controller (virtio-scsi-single) or set performance/IO options (iothread, aio, ssd, discard, cache) at create time, forcing callers to create the VM diskless and then hand-build a raw scsi0 string via Set-PveVmConfig.", + "scan_history": [ + { + "scan_date": "2026-05-22", + "local_id": null, + "status": "new" + }, + { + "scan_date": "2026-05-22", + "local_id": null, + "status": "fixed" + } + ], + "resolution": { + "scan_date": "2026-05-22", + "evidence": "Added -DiskBus (virtio/scsi/sata/ide, default virtio), -ScsiHardware (scsihw), -DiskIoThread, -DiskAio, -DiskSsd, -DiskDiscard, -DiskCache. The disk spec is built via a BuildDiskSpec helper; ValidateDiskOptions enforces (before ShouldProcess) that ssd is not used on virtio and that iothread is only used on virtio or scsi+virtio-scsi-single, surfacing clear errors at create time instead of at VM start.", + "verified_by": "dotnet build (0 warnings) + Pester (47 New-PveVm tests incl. disk-option validation matrix)" + } + }, + { + "id": "F090", + "title": "Get-PveVmConfig silently drops config keys not in the typed allow-list", + "category": "completeness", + "severity": "medium", + "status": "resolved", + "first_detected": "2026-05-22", + "github_issue": 65, + "files": [ + "src/PSProxmoxVE.Core/Models/Vms/PveVmConfig.cs" + ], + "description": "PveVmConfig was a fixed allow-list of [JsonProperty] fields with no catch-all, so keys like scsihw, efidisk0, tpmstate0, hostpci0, and additional disk buses were silently dropped on deserialize — making the F089 workaround unverifiable by reading the config back.", + "scan_history": [ + { + "scan_date": "2026-05-22", + "local_id": null, + "status": "new" + }, + { + "scan_date": "2026-05-22", + "local_id": null, + "status": "fixed" + } + ], + "resolution": { + "scan_date": "2026-05-22", + "evidence": "Added typed scsihw/efidisk0/tpmstate0 properties plus a private [JsonExtensionData] landing field exposed as AdditionalProperties (Dictionary via JsonHelper.ToNative, so values are native .NET types per D013 — no JToken leakage). Typed keys do not duplicate into the catch-all.", + "verified_by": "dotnet build + dotnet test (586 passed, new VmModelTests for scsihw/efidisk/tpm, native-typed AdditionalProperties, and no typed-key leakage)" + } } ] } diff --git a/src/PSProxmoxVE.Core/Models/Vms/PveVmConfig.cs b/src/PSProxmoxVE.Core/Models/Vms/PveVmConfig.cs index 8fbf688..1dd5ba3 100644 --- a/src/PSProxmoxVE.Core/Models/Vms/PveVmConfig.cs +++ b/src/PSProxmoxVE.Core/Models/Vms/PveVmConfig.cs @@ -1,4 +1,8 @@ +using System.Collections.Generic; +using System.Linq; using Newtonsoft.Json; +using Newtonsoft.Json.Linq; +using PSProxmoxVE.Core.Utilities; namespace PSProxmoxVE.Core.Models.Vms; @@ -52,6 +56,24 @@ public class PveVmConfig [JsonProperty("machine")] public string? Machine { get; set; } + /// + /// SCSI controller hardware model (e.g., "virtio-scsi-single", "lsi"). + /// + [JsonProperty("scsihw")] + public string? ScsiHardware { get; set; } + + /// + /// EFI vars disk spec (present on OVMF/UEFI VMs), e.g. "local-lvm:vm-100-disk-1,...". + /// + [JsonProperty("efidisk0")] + public string? EfiDisk0 { get; set; } + + /// + /// TPM state disk spec (present on VMs with a virtual TPM). + /// + [JsonProperty("tpmstate0")] + public string? TpmState0 { get; set; } + // ------------------------------------------------------------------------- // Boot / Args // ------------------------------------------------------------------------- @@ -288,6 +310,29 @@ public class PveVmConfig [JsonProperty("searchdomain")] public string? Searchdomain { get; set; } + // ------------------------------------------------------------------------- + // Catch-all for unmapped config keys + // ------------------------------------------------------------------------- + + /// + /// Raw landing spot for any config key not mapped to a typed property above. + /// Populated by Newtonsoft during deserialization; exposed natively via + /// . + /// + [JsonExtensionData] + private IDictionary? ExtensionData { get; set; } + + /// + /// Any VM config keys not surfaced as a typed property above (e.g. hostpci0, + /// usb0, numa0, additional disk buses). Keys map to native .NET values so the + /// dictionary works naturally in PowerShell pipelines. + /// + [JsonIgnore] + public Dictionary AdditionalProperties => + ExtensionData == null + ? new Dictionary() + : ExtensionData.ToDictionary(kvp => kvp.Key, kvp => JsonHelper.ToNative(kvp.Value)); + /// public override string ToString() { diff --git a/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs index 0c90225..2f8f15e 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs @@ -96,6 +96,67 @@ namespace PSProxmoxVE.Cmdlets.Vms [Parameter(Mandatory = false, HelpMessage = "Disk format (e.g. raw, qcow2).")] public string? DiskFormat { get; set; } + /// + /// + /// Bus/controller for the primary disk: "virtio" (default), "scsi", "sata", or "ide". + /// Determines the device key (virtio0, scsi0, sata0, ide0). + /// + /// + [Parameter(Mandatory = false, HelpMessage = "Primary disk bus: virtio (default), scsi, sata, ide.")] + [ValidateSet("virtio", "scsi", "sata", "ide", IgnoreCase = true)] + public string? DiskBus { get; set; } + + /// + /// + /// SCSI controller hardware model (sets the VM-level "scsihw" key), e.g. + /// "virtio-scsi-single", "virtio-scsi-pci", "lsi". Required as + /// "virtio-scsi-single" when combining -DiskBus scsi with -DiskIoThread. + /// + /// + [Parameter(Mandatory = false, HelpMessage = "SCSI controller model (e.g. virtio-scsi-single).")] + public string? ScsiHardware { get; set; } + + /// + /// + /// Enable a dedicated IO thread for the primary disk (iothread=1). Valid only for + /// the virtio bus or for the scsi bus with -ScsiHardware virtio-scsi-single. + /// + /// + [Parameter(Mandatory = false, HelpMessage = "Enable a dedicated IO thread (iothread=1).")] + public SwitchParameter DiskIoThread { get; set; } + + /// + /// Async IO mode for the primary disk: "native", "threads", or "io_uring". + /// + [Parameter(Mandatory = false, HelpMessage = "Async IO mode: native, threads, io_uring.")] + [ValidateSet("native", "threads", "io_uring", IgnoreCase = true)] + public string? DiskAio { get; set; } + + /// + /// + /// Mark the primary disk as SSD (ssd=1). Not supported on the virtio bus — use + /// -DiskBus scsi, sata, or ide. + /// + /// + [Parameter(Mandatory = false, HelpMessage = "Mark the primary disk as SSD (ssd=1).")] + public SwitchParameter DiskSsd { get; set; } + + /// + /// Enable discard/TRIM passthrough on the primary disk (discard=on). + /// + [Parameter(Mandatory = false, HelpMessage = "Enable discard/TRIM passthrough (discard=on).")] + public SwitchParameter DiskDiscard { get; set; } + + /// + /// + /// Cache mode for the primary disk: "none", "writethrough", "writeback", + /// "directsync", or "unsafe". + /// + /// + [Parameter(Mandatory = false, HelpMessage = "Disk cache mode: none, writethrough, writeback, directsync, unsafe.")] + [ValidateSet("none", "writethrough", "writeback", "directsync", "unsafe", IgnoreCase = true)] + public string? DiskCache { get; set; } + /// /// Network interface model (e.g., "virtio", "e1000"). /// @@ -128,13 +189,16 @@ namespace PSProxmoxVE.Cmdlets.Vms protected override void ProcessRecord() { - // Validate -DiskSize before ShouldProcess so typos like "512M" are rejected - // even with -WhatIf, and so the error is raised regardless of whether - // -DiskStorage is also supplied. + // Validate -DiskSize and disk-option combinations before ShouldProcess so + // typos and invalid combos are rejected even with -WhatIf, and so the error + // is raised regardless of whether -DiskStorage is also supplied. string? diskSizeGib = null; if (!string.IsNullOrEmpty(DiskSize)) diskSizeGib = SizeParser.NormalizeToGibibytes(DiskSize!, nameof(DiskSize)); + var diskBus = string.IsNullOrEmpty(DiskBus) ? "virtio" : DiskBus!.ToLowerInvariant(); + ValidateDiskOptions(diskBus); + if (!ShouldProcess($"VM on node '{Node}'", "New-PveVm")) return; @@ -172,14 +236,15 @@ namespace PSProxmoxVE.Cmdlets.Vms config["machine"] = Machine!; if (!string.IsNullOrEmpty(OsType)) config["ostype"] = OsType!; + if (!string.IsNullOrEmpty(ScsiHardware)) + config["scsihw"] = ScsiHardware!; if (!string.IsNullOrEmpty(DiskStorage) && diskSizeGib != null) - { - var diskValue = $"{DiskStorage}:{diskSizeGib}"; - if (!string.IsNullOrEmpty(DiskFormat)) - diskValue += $",format={DiskFormat}"; - config["virtio0"] = diskValue; - } + config[$"{diskBus}0"] = BuildDiskSpec(DiskStorage!, diskSizeGib); + else if (HasDiskOptions()) + WriteWarning("Disk IO options (-DiskBus/-DiskIoThread/-DiskAio/-DiskSsd/-DiskDiscard/-DiskCache) " + + "were specified but no disk is being created (-DiskStorage and -DiskSize are both required). " + + "The options were ignored."); if (!string.IsNullOrEmpty(Bridge)) { @@ -200,5 +265,61 @@ namespace PSProxmoxVE.Cmdlets.Vms WriteObject(task); } + + private bool HasDiskOptions() => + !string.IsNullOrEmpty(DiskBus) + || DiskIoThread.IsPresent + || !string.IsNullOrEmpty(DiskAio) + || DiskSsd.IsPresent + || DiskDiscard.IsPresent + || !string.IsNullOrEmpty(DiskCache); + + /// + /// Validates disk option combinations that PVE would otherwise reject at VM start + /// rather than at create time, surfacing a clear error up front. + /// + private void ValidateDiskOptions(string diskBus) + { + if (DiskSsd.IsPresent && diskBus == "virtio") + throw new PSArgumentException( + "-DiskSsd is not supported on the virtio bus. Use -DiskBus scsi, sata, or ide.", + nameof(DiskSsd)); + + if (DiskIoThread.IsPresent) + { + if (diskBus == "sata" || diskBus == "ide") + throw new PSArgumentException( + "-DiskIoThread requires -DiskBus virtio or scsi.", + nameof(DiskIoThread)); + + if (diskBus == "scsi" + && !string.Equals(ScsiHardware, "virtio-scsi-single", System.StringComparison.OrdinalIgnoreCase)) + throw new PSArgumentException( + "-DiskIoThread on a scsi disk requires -ScsiHardware virtio-scsi-single.", + nameof(DiskIoThread)); + } + } + + /// + /// Builds the disk volume spec ("<storage>:<sizeGiB>[,opt=val...]") for the + /// primary disk, appending format and any requested IO options in a stable order. + /// + private string BuildDiskSpec(string storage, string sizeGib) + { + var sb = new System.Text.StringBuilder($"{storage}:{sizeGib}"); + if (!string.IsNullOrEmpty(DiskFormat)) + sb.Append($",format={DiskFormat}"); + if (!string.IsNullOrEmpty(DiskCache)) + sb.Append($",cache={DiskCache!.ToLowerInvariant()}"); + if (!string.IsNullOrEmpty(DiskAio)) + sb.Append($",aio={DiskAio!.ToLowerInvariant()}"); + if (DiskSsd.IsPresent) + sb.Append(",ssd=1"); + if (DiskDiscard.IsPresent) + sb.Append(",discard=on"); + if (DiskIoThread.IsPresent) + sb.Append(",iothread=1"); + return sb.ToString(); + } } } diff --git a/tests/PSProxmoxVE.Core.Tests/Models/VmModelTests.cs b/tests/PSProxmoxVE.Core.Tests/Models/VmModelTests.cs index 51fb673..b92baae 100644 --- a/tests/PSProxmoxVE.Core.Tests/Models/VmModelTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Models/VmModelTests.cs @@ -173,5 +173,57 @@ namespace PSProxmoxVE.Core.Tests.Models Assert.Equal("8.8.8.8", config.Nameserver); Assert.Equal("example.com", config.Searchdomain); } + + [Fact] + public void PveVmConfig_Deserialize_SurfacesScsiHardware() + { + var json = TestHelper.LoadFixture("pve9_vm_config.json"); + var data = JObject.Parse(json)["data"]; + Assert.NotNull(data); + var config = data.ToObject(); + Assert.NotNull(config); + Assert.Equal("virtio-scsi-single", config.ScsiHardware); + } + + [Fact] + public void PveVmConfig_Deserialize_SurfacesEfiDiskAndTpm() + { + var json = @"{ ""efidisk0"": ""local-lvm:vm-100-disk-1,efitype=4m,size=528K"", + ""tpmstate0"": ""local-lvm:vm-100-disk-2,size=4M,version=v2.0"" }"; + var config = JObject.Parse(json).ToObject(); + Assert.NotNull(config); + Assert.Contains("efitype=4m", config!.EfiDisk0); + Assert.Contains("version=v2.0", config.TpmState0); + } + + [Fact] + public void PveVmConfig_UnmappedKeys_LandInAdditionalProperties_AsNativeTypes() + { + // hostpci0 and numa0 are not typed properties; they must not be dropped. + var json = @"{ ""cores"": 2, + ""hostpci0"": ""0000:01:00.0,pcie=1"", + ""numa0"": ""cpus=0-1,memory=2048"" }"; + var config = JObject.Parse(json).ToObject(); + Assert.NotNull(config); + + Assert.Equal(2, config!.Cores); // typed property still works + Assert.True(config.AdditionalProperties.ContainsKey("hostpci0")); + Assert.Equal("0000:01:00.0,pcie=1", config.AdditionalProperties["hostpci0"]); + // Value must be a native type (string), never a Newtonsoft JToken (D013). + Assert.IsType(config.AdditionalProperties["hostpci0"]); + Assert.DoesNotContain("Newtonsoft", config.AdditionalProperties["hostpci0"]!.GetType().FullName); + } + + [Fact] + public void PveVmConfig_TypedKeys_DoNotLeakIntoAdditionalProperties() + { + var json = TestHelper.LoadFixture("pve9_vm_config.json"); + var data = JObject.Parse(json)["data"]; + var config = data!.ToObject(); + Assert.NotNull(config); + // scsihw and cores are typed → they must not also appear in the catch-all. + Assert.False(config!.AdditionalProperties.ContainsKey("scsihw")); + Assert.False(config.AdditionalProperties.ContainsKey("cores")); + } } } diff --git a/tests/PSProxmoxVE.Tests/Vms/New-PveVm.Tests.ps1 b/tests/PSProxmoxVE.Tests/Vms/New-PveVm.Tests.ps1 index e26116e..f8d2cbe 100644 --- a/tests/PSProxmoxVE.Tests/Vms/New-PveVm.Tests.ps1 +++ b/tests/PSProxmoxVE.Tests/Vms/New-PveVm.Tests.ps1 @@ -168,4 +168,75 @@ Describe 'New-PveVm' { Should -Not -Throw } } + + Context 'Disk controller / IO parameter metadata' { + BeforeAll { $script:Cmd = Get-Command 'New-PveVm' } + + It 'Should have <_> parameter' -ForEach @( + 'DiskBus', 'ScsiHardware', 'DiskIoThread', 'DiskAio', 'DiskSsd', 'DiskDiscard', 'DiskCache' + ) { + $script:Cmd.Parameters.ContainsKey($_) | Should -BeTrue + } + + It 'DiskBus should have a ValidateSet of virtio, scsi, sata, ide' { + $vs = $script:Cmd.Parameters['DiskBus'].Attributes | + Where-Object { $_ -is [System.Management.Automation.ValidateSetAttribute] } | + Select-Object -First 1 + $vs.ValidValues | Should -Contain 'virtio' + $vs.ValidValues | Should -Contain 'scsi' + $vs.ValidValues | Should -Contain 'sata' + $vs.ValidValues | Should -Contain 'ide' + } + + It 'DiskIoThread and DiskSsd should be switch parameters' { + $script:Cmd.Parameters['DiskIoThread'].ParameterType | Should -Be ([System.Management.Automation.SwitchParameter]) + $script:Cmd.Parameters['DiskSsd'].ParameterType | Should -Be ([System.Management.Automation.SwitchParameter]) + } + } + + Context 'Disk option validation' { + # Validation runs before ShouldProcess, so -WhatIf exercises it offline. + + It 'Should reject -DiskSsd on the virtio bus' { + { New-PveVm -Node 'pve-node1' -DiskStorage 'local-lvm' -DiskSize '32' -DiskSsd -WhatIf -ErrorAction Stop } | + Should -Throw '*virtio bus*' + } + + It 'Should reject -DiskSsd on the default (virtio) bus when bus omitted' { + { New-PveVm -Node 'pve-node1' -DiskStorage 'local-lvm' -DiskSize '32' -DiskSsd -WhatIf -ErrorAction Stop } | + Should -Throw '*virtio bus*' + } + + It 'Should accept -DiskSsd on the scsi bus' { + { New-PveVm -Node 'pve-node1' -DiskStorage 'local-lvm' -DiskSize '32' -DiskBus scsi -DiskSsd -WhatIf -ErrorAction Stop } | + Should -Not -Throw + } + + It 'Should reject -DiskIoThread on the sata bus' { + { New-PveVm -Node 'pve-node1' -DiskStorage 'local-lvm' -DiskSize '32' -DiskBus sata -DiskIoThread -WhatIf -ErrorAction Stop } | + Should -Throw '*requires -DiskBus virtio or scsi*' + } + + It 'Should reject -DiskIoThread on scsi without -ScsiHardware virtio-scsi-single' { + { New-PveVm -Node 'pve-node1' -DiskStorage 'local-lvm' -DiskSize '32' -DiskBus scsi -DiskIoThread -WhatIf -ErrorAction Stop } | + Should -Throw '*virtio-scsi-single*' + } + + It 'Should accept -DiskIoThread on scsi with -ScsiHardware virtio-scsi-single' { + { New-PveVm -Node 'pve-node1' -DiskStorage 'local-lvm' -DiskSize '32' -DiskBus scsi -ScsiHardware 'virtio-scsi-single' -DiskIoThread -WhatIf -ErrorAction Stop } | + Should -Not -Throw + } + + It 'Should accept -DiskIoThread on the default virtio bus' { + { New-PveVm -Node 'pve-node1' -DiskStorage 'local-lvm' -DiskSize '32' -DiskIoThread -WhatIf -ErrorAction Stop } | + Should -Not -Throw + } + + It 'Should accept a fully tuned scsi disk spec' { + { New-PveVm -Node 'pve-node1' -DiskStorage 'local-lvm' -DiskSize '60' -DiskBus scsi ` + -ScsiHardware 'virtio-scsi-single' -DiskIoThread -DiskAio native -DiskSsd -DiskDiscard ` + -DiskCache none -WhatIf -ErrorAction Stop } | + Should -Not -Throw + } + } }