From 6181c8ce779ab5aa4cae164e12eb7edc09a584db Mon Sep 17 00:00:00 2001 From: Clint Branham Date: Wed, 20 May 2026 17:52:44 -0500 Subject: [PATCH] fix: normalize -DiskSize and -RootFsSize before sending to PVE Disk and rootfs size strings were interpolated directly into the disk spec as ":", so "60G" produced "local-lvm:60G". On LVM/LVM-thin storages PVE parses the value after the colon as a volume name unless it is a bare integer, returning "unable to parse lvm volume name '60G'". File-backed storages mask this by accepting either form. SizeParser.NormalizeToGibibytes() now strips G/GB/T/TB suffixes and returns a bare GiB integer string, so the documented "32G" call shape works on every storage type. Sub-GB units are rejected with a clear error rather than being silently truncated. Tracked as F086. Closes #58. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/review/findings.json | 31 +++++++ src/PSProxmoxVE.Core/Utilities/SizeParser.cs | 73 +++++++++++++++++ .../Containers/NewPveContainerCmdlet.cs | 14 +++- src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs | 12 ++- .../Utilities/SizeParserTests.cs | 80 +++++++++++++++++++ 5 files changed, 204 insertions(+), 6 deletions(-) create mode 100644 src/PSProxmoxVE.Core/Utilities/SizeParser.cs create mode 100644 tests/PSProxmoxVE.Core.Tests/Utilities/SizeParserTests.cs diff --git a/docs/review/findings.json b/docs/review/findings.json index bec7f9b..aaa8c77 100644 --- a/docs/review/findings.json +++ b/docs/review/findings.json @@ -2815,6 +2815,37 @@ "evidence": "Replaced JArray? Nodelist with List>? + NativeListConverter, and JObject? Totem with Dictionary? + NativeDictionaryConverter. Removed Newtonsoft.Json.Linq dependency.", "verified_by": "dotnet build + dotnet test (382 passed)" } + }, + { + "id": "F086", + "title": "New-PveVm -DiskSize and New-PveContainer -RootFsSize pass unit suffix verbatim, LVM rejects 'NG'", + "category": "api_contract", + "severity": "high", + "status": "resolved", + "first_detected": "2026-05-20", + "github_issue": 58, + "files": [ + "src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs", + "src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerCmdlet.cs" + ], + "description": "Disk and rootfs sizes are interpolated directly into the disk spec as ':'. The parameter docstring advertises 'e.g. 32G' but on LVM/LVM-thin storages PVE parses the value after the colon as a volume name unless it is a bare integer, failing with 'unable to parse lvm volume name \"32G\"'. File-backed storages (NFS, directory) accept either form, masking the bug in mixed environments.", + "scan_history": [ + { + "scan_date": "2026-05-20", + "local_id": null, + "status": "new" + }, + { + "scan_date": "2026-05-20", + "local_id": null, + "status": "fixed" + } + ], + "resolution": { + "scan_date": "2026-05-20", + "evidence": "Added SizeParser.NormalizeToGibibytes() which strips G/GB/T/TB suffixes (and rejects sub-GB units with a clear error). New-PveVm and New-PveContainer normalize -DiskSize and -RootFsSize through it before constructing the disk spec, so '32G' becomes ':32' on every storage type.", + "verified_by": "dotnet build + dotnet test (576 passed, 31 new SizeParserTests)" + } } ] } diff --git a/src/PSProxmoxVE.Core/Utilities/SizeParser.cs b/src/PSProxmoxVE.Core/Utilities/SizeParser.cs new file mode 100644 index 0000000..307f569 --- /dev/null +++ b/src/PSProxmoxVE.Core/Utilities/SizeParser.cs @@ -0,0 +1,73 @@ +using System; +using System.Globalization; +using System.Text.RegularExpressions; + +namespace PSProxmoxVE.Core.Utilities +{ + /// + /// Parses storage size strings (e.g. "32G", "1T", "60") and normalizes them + /// to a bare integer count of gibibytes for use in PVE disk specs. + /// + /// + /// PVE accepts a size suffix on file-backed storages (NFS, directory) but parses + /// the value after the colon as a volume name on LVM-backed storages — so + /// local-lvm:32G fails with "unable to parse lvm volume name '32G'" while + /// local-lvm:32 works on every storage type. Cmdlets that build disk specs + /// must normalize size inputs through this helper before joining with the storage. + /// + public static class SizeParser + { + private static readonly Regex Pattern = new Regex( + @"^\s*(?\d+)\s*(?[A-Za-z]*)\s*$", + RegexOptions.Compiled); + + /// + /// Parses a size string and returns the value as a bare integer count of GiB. + /// Accepts values like "60", "60G", "60GB" (= 60), "1T", "1TB" (= 1024). + /// Sub-GB units are rejected because PVE disk allocation is GB-granular. + /// + /// The size string supplied by the user. + /// Parameter name used in the error message. + /// The size in whole GiB as a string, suitable for direct use in disk specs. + /// The input cannot be parsed or uses an unsupported unit. + public static string NormalizeToGibibytes(string value, string parameterName = "size") + { + if (string.IsNullOrWhiteSpace(value)) + throw new ArgumentException($"{parameterName} must not be null or empty.", parameterName); + + var match = Pattern.Match(value); + if (!match.Success) + throw new ArgumentException( + $"{parameterName} '{value}' is not a valid size. Expected a positive integer optionally suffixed with G, GB, T, or TB (e.g. '32G', '1T', '60').", + parameterName); + + if (!long.TryParse(match.Groups["num"].Value, NumberStyles.Integer, CultureInfo.InvariantCulture, out var num) || num <= 0) + throw new ArgumentException( + $"{parameterName} '{value}' must be a positive integer.", + parameterName); + + var unit = match.Groups["unit"].Value.ToUpperInvariant(); + long gib; + switch (unit) + { + case "": + case "G": + case "GB": + case "GIB": + gib = num; + break; + case "T": + case "TB": + case "TIB": + gib = checked(num * 1024L); + break; + default: + throw new ArgumentException( + $"{parameterName} '{value}' uses unsupported unit '{unit}'. Use G, GB, T, or TB. Sub-GB units (M, MB, K, KB) are not supported by PVE disk allocation.", + parameterName); + } + + return gib.ToString(CultureInfo.InvariantCulture); + } + } +} diff --git a/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerCmdlet.cs index c586338..b0213e1 100644 --- a/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerCmdlet.cs @@ -6,6 +6,7 @@ using Newtonsoft.Json.Linq; using PSProxmoxVE.Core.Client; using PSProxmoxVE.Core.Models.Vms; using PSProxmoxVE.Core.Services; +using PSProxmoxVE.Core.Utilities; namespace PSProxmoxVE.Cmdlets.Containers { @@ -59,9 +60,13 @@ namespace PSProxmoxVE.Cmdlets.Containers public int? Cores { get; set; } /// - /// Size of the root filesystem (e.g., "8G"). + /// + /// Size of the root filesystem. Accepts a bare integer in GiB ("8") or a value + /// suffixed with G/GB/T/TB (case-insensitive); the value is normalized to a + /// bare GiB count before being sent to the API. + /// /// - [Parameter(Mandatory = false, HelpMessage = "Size of the root filesystem (e.g. 8G).")] + [Parameter(Mandatory = false, HelpMessage = "Size of the root filesystem in GiB (e.g. 8 or 8G).")] public string? RootFsSize { get; set; } /// @@ -161,7 +166,10 @@ namespace PSProxmoxVE.Cmdlets.Containers { var rootFsValue = RootFsStorage!; if (!string.IsNullOrEmpty(RootFsSize)) - rootFsValue += $":{RootFsSize}"; + { + var sizeGib = SizeParser.NormalizeToGibibytes(RootFsSize!, nameof(RootFsSize)); + rootFsValue += $":{sizeGib}"; + } config["rootfs"] = rootFsValue; } diff --git a/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs index fb363f2..d0e189f 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs @@ -4,6 +4,7 @@ using Newtonsoft.Json.Linq; using PSProxmoxVE.Core.Client; using PSProxmoxVE.Core.Models.Vms; using PSProxmoxVE.Core.Services; +using PSProxmoxVE.Core.Utilities; namespace PSProxmoxVE.Cmdlets.Vms { @@ -74,9 +75,13 @@ namespace PSProxmoxVE.Cmdlets.Vms public string? Machine { get; set; } /// - /// Size of the primary disk (e.g., "32G"). + /// + /// Size of the primary disk. Accepts a bare integer in GiB ("32") or a value + /// suffixed with G/GB/T/TB (case-insensitive); the value is normalized to a + /// bare GiB count before being sent to the API. + /// /// - [Parameter(Mandatory = false, HelpMessage = "Size of the primary disk (e.g. 32G).")] + [Parameter(Mandatory = false, HelpMessage = "Size of the primary disk in GiB (e.g. 32 or 32G).")] public string? DiskSize { get; set; } /// @@ -163,7 +168,8 @@ namespace PSProxmoxVE.Cmdlets.Vms if (!string.IsNullOrEmpty(DiskStorage) && !string.IsNullOrEmpty(DiskSize)) { - var diskValue = $"{DiskStorage}:{DiskSize}"; + var sizeGib = SizeParser.NormalizeToGibibytes(DiskSize!, nameof(DiskSize)); + var diskValue = $"{DiskStorage}:{sizeGib}"; if (!string.IsNullOrEmpty(DiskFormat)) diskValue += $",format={DiskFormat}"; config["virtio0"] = diskValue; diff --git a/tests/PSProxmoxVE.Core.Tests/Utilities/SizeParserTests.cs b/tests/PSProxmoxVE.Core.Tests/Utilities/SizeParserTests.cs new file mode 100644 index 0000000..ba734b6 --- /dev/null +++ b/tests/PSProxmoxVE.Core.Tests/Utilities/SizeParserTests.cs @@ -0,0 +1,80 @@ +using System; +using PSProxmoxVE.Core.Utilities; +using Xunit; + +namespace PSProxmoxVE.Core.Tests.Utilities +{ + public class SizeParserTests + { + [Theory] + [InlineData("32", "32")] + [InlineData("32G", "32")] + [InlineData("32g", "32")] + [InlineData("32GB", "32")] + [InlineData("32gb", "32")] + [InlineData("32GiB", "32")] + [InlineData("1T", "1024")] + [InlineData("1t", "1024")] + [InlineData("1TB", "1024")] + [InlineData("1TiB", "1024")] + [InlineData("2T", "2048")] + [InlineData(" 60G ", "60")] + [InlineData("60 G", "60")] + public void NormalizeToGibibytes_AcceptedInputs_ReturnsBareGibibyteString(string input, string expected) + { + var result = SizeParser.NormalizeToGibibytes(input); + Assert.Equal(expected, result); + } + + [Theory] + [InlineData("512M")] + [InlineData("512MB")] + [InlineData("1024K")] + [InlineData("1024KB")] + [InlineData("100B")] + [InlineData("1P")] + [InlineData("1PB")] + public void NormalizeToGibibytes_UnsupportedUnit_Throws(string input) + { + var ex = Assert.Throws(() => SizeParser.NormalizeToGibibytes(input)); + Assert.Contains("unsupported unit", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + [InlineData(null)] + public void NormalizeToGibibytes_EmptyOrWhitespace_Throws(string? input) + { + Assert.Throws(() => SizeParser.NormalizeToGibibytes(input!)); + } + + [Theory] + [InlineData("abc")] + [InlineData("G")] + [InlineData("-32")] + [InlineData("32.5G")] + [InlineData("32 G B")] + public void NormalizeToGibibytes_Malformed_Throws(string input) + { + Assert.Throws(() => SizeParser.NormalizeToGibibytes(input)); + } + + [Theory] + [InlineData("0")] + [InlineData("0G")] + public void NormalizeToGibibytes_Zero_Throws(string input) + { + var ex = Assert.Throws(() => SizeParser.NormalizeToGibibytes(input)); + Assert.Contains("positive", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void NormalizeToGibibytes_UsesProvidedParameterNameInError() + { + var ex = Assert.Throws(() => SizeParser.NormalizeToGibibytes("512M", "DiskSize")); + Assert.Equal("DiskSize", ex.ParamName); + Assert.Contains("DiskSize", ex.Message); + } + } +}