From 064cc372cf4fae801925e73f82a1bc72bf40fd6c Mon Sep 17 00:00:00 2001 From: Clint Branham Date: Tue, 24 Mar 2026 14:43:33 -0500 Subject: [PATCH 1/5] feat(storage): add -Target and -Portal parameters to New-PveStorage for iSCSI New-PveStorage now supports configuring iSCSI storage backends natively: -Target: iSCSI target IQN (e.g. iqn.2024-01.com.example:storage) -Portal: iSCSI portal address (host:port, defaults to server:3260) Also refactored ProcessRecord to use AddIfNotEmpty helper, reducing cognitive complexity. Added unit tests for all iSCSI/NFS parameter metadata and a new SharedStorage.Tests.ps1 integration test file that tests NFS and iSCSI storage create/verify/status/delete lifecycle against the Docker-based storage containers. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../Cmdlets/Storage/NewPveStorageCmdlet.cs | 35 +++- .../Integration/SharedStorage.Tests.ps1 | 179 ++++++++++++++++++ .../Storage/Get-PveStorage.Tests.ps1 | 42 ++++ 3 files changed, 246 insertions(+), 10 deletions(-) create mode 100644 tests/PSProxmoxVE.Tests/Integration/SharedStorage.Tests.ps1 diff --git a/src/PSProxmoxVE/Cmdlets/Storage/NewPveStorageCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Storage/NewPveStorageCmdlet.cs index d4797a9..95a67a2 100644 --- a/src/PSProxmoxVE/Cmdlets/Storage/NewPveStorageCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Storage/NewPveStorageCmdlet.cs @@ -63,6 +63,14 @@ namespace PSProxmoxVE.Cmdlets.Storage [Parameter(Mandatory = false, HelpMessage = "Ceph monitor host list.")] public string? MonHost { get; set; } + /// iSCSI target IQN (for "iscsi" type). + [Parameter(Mandatory = false, HelpMessage = "iSCSI target IQN (e.g. iqn.2024-01.com.example:storage).")] + public string? Target { get; set; } + + /// iSCSI portal address (for "iscsi" type). Defaults to server:3260 if not specified. + [Parameter(Mandatory = false, HelpMessage = "iSCSI portal address (host:port).")] + public string? Portal { get; set; } + /// Whether this storage is shared across cluster nodes. [Parameter(Mandatory = false, HelpMessage = "Storage is shared across cluster nodes.")] public SwitchParameter Shared { get; set; } @@ -75,6 +83,12 @@ namespace PSProxmoxVE.Cmdlets.Storage [Parameter(Mandatory = false, HelpMessage = "Limit access to these nodes (comma-separated).")] public string? Nodes { get; set; } + private static void AddIfNotEmpty(Dictionary data, string key, string? value) + { + if (!string.IsNullOrEmpty(value)) + data[key] = value!; + } + protected override void ProcessRecord() { if (!ShouldProcess(Storage, "Create PVE Storage")) @@ -98,16 +112,17 @@ namespace PSProxmoxVE.Cmdlets.Storage ["type"] = Type }; - if (!string.IsNullOrEmpty(Content)) data["content"] = Content!; - if (!string.IsNullOrEmpty(Path)) data["path"] = Path!; - if (!string.IsNullOrEmpty(Server)) data["server"] = Server!; - if (!string.IsNullOrEmpty(Export)) data["export"] = Export!; - if (!string.IsNullOrEmpty(VgName)) data["vgname"] = VgName!; - if (!string.IsNullOrEmpty(ThinPool)) data["thinpool"] = ThinPool!; - if (!string.IsNullOrEmpty(Pool)) data["pool"] = Pool!; - if (!string.IsNullOrEmpty(CephPool)) data["pool"] = CephPool!; - if (!string.IsNullOrEmpty(MonHost)) data["monhost"] = MonHost!; - if (!string.IsNullOrEmpty(Nodes)) data["nodes"] = Nodes!; + AddIfNotEmpty(data, "content", Content); + AddIfNotEmpty(data, "path", Path); + AddIfNotEmpty(data, "server", Server); + AddIfNotEmpty(data, "export", Export); + AddIfNotEmpty(data, "vgname", VgName); + AddIfNotEmpty(data, "thinpool", ThinPool); + AddIfNotEmpty(data, "pool", Pool ?? CephPool); + AddIfNotEmpty(data, "monhost", MonHost); + AddIfNotEmpty(data, "target", Target); + AddIfNotEmpty(data, "portal", Portal); + AddIfNotEmpty(data, "nodes", Nodes); if (Shared.IsPresent) data["shared"] = "1"; if (Disable.IsPresent) data["disable"] = "1"; diff --git a/tests/PSProxmoxVE.Tests/Integration/SharedStorage.Tests.ps1 b/tests/PSProxmoxVE.Tests/Integration/SharedStorage.Tests.ps1 new file mode 100644 index 0000000..3409a3a --- /dev/null +++ b/tests/PSProxmoxVE.Tests/Integration/SharedStorage.Tests.ps1 @@ -0,0 +1,179 @@ +#Requires -Module Pester +<# +.SYNOPSIS + Pester 5 integration tests for shared storage backends (NFS, iSCSI). + + These tests require the multi-node integration test infrastructure with + Docker-based storage services (iSCSI target + NFS server). They are + SKIPPED when the storage env vars are not set. + + Required environment variables (in addition to base integration vars): + PVETEST_STORAGE_VM_IP - IP of the storage host running iSCSI/NFS + PVETEST_ISCSI_IQN - iSCSI target IQN + PVETEST_NFS_EXPORT - NFS export path (e.g. 10.0.0.1:/srv/nfs/shared) + + WARNING: These tests CREATE and DESTROY storage definitions on the target + PVE node. Never run against a production cluster. +#> + +BeforeAll { + . $PSScriptRoot/../_TestHelper.ps1 + + # --- Base integration vars --- + $baseVars = @('PVETEST_HOST', 'PVETEST_PORT', 'PVETEST_APITOKEN', 'PVETEST_NODE') + $script:SkipReason = $null + + foreach ($var in $baseVars) { + if (-not [System.Environment]::GetEnvironmentVariable($var)) { + $script:SkipReason = "No live Proxmox VE target configured. Set: $($baseVars -join ', ')" + break + } + } + + # --- Shared storage vars --- + $script:StorageVmIp = [System.Environment]::GetEnvironmentVariable('PVETEST_STORAGE_VM_IP') + $script:IscsiIqn = [System.Environment]::GetEnvironmentVariable('PVETEST_ISCSI_IQN') + $script:NfsExport = [System.Environment]::GetEnvironmentVariable('PVETEST_NFS_EXPORT') + + $script:Host_ = [System.Environment]::GetEnvironmentVariable('PVETEST_HOST') + $portEnv = [System.Environment]::GetEnvironmentVariable('PVETEST_PORT') + $script:Port = [int]$(if ($portEnv) { $portEnv } else { '8006' }) + $script:ApiToken = [System.Environment]::GetEnvironmentVariable('PVETEST_APITOKEN') + $script:Node = [System.Environment]::GetEnvironmentVariable('PVETEST_NODE') + + # Track created storages for cleanup + $script:CreatedStorages = [System.Collections.Generic.List[string]]::new() + + function script:Skip-IfNoTarget { + if ($script:SkipReason) { + Set-ItResult -Skipped -Because $script:SkipReason + return $true + } + return $false + } + + function script:Skip-IfNoSharedStorage { + if (Skip-IfNoTarget) { return $true } + if (-not $script:StorageVmIp -or -not $script:IscsiIqn -or -not $script:NfsExport) { + Set-ItResult -Skipped -Because 'Shared storage env vars not set (PVETEST_STORAGE_VM_IP, PVETEST_ISCSI_IQN, PVETEST_NFS_EXPORT)' + return $true + } + return $false + } +} + +AfterAll { + # Best-effort cleanup of any storages we created + foreach ($name in $script:CreatedStorages) { + try { Remove-PveStorage -Storage $name -Confirm:$false -ErrorAction Stop } + catch { Write-Warning "Cleanup: failed to remove storage '$name': $_" } + } +} + +Describe 'Shared Storage — Integration' -Tag 'Integration' { + + # ------------------------------------------------------------------- + Context 'Connection' { + It 'Should connect to PVE node' { + if (Skip-IfNoTarget) { return } + + $session = Connect-PveServer ` + -Server $script:Host_ ` + -Port $script:Port ` + -ApiToken $script:ApiToken ` + -SkipCertificateCheck ` + -PassThru + + $session | Should -Not -BeNullOrEmpty + } + } + + # ------------------------------------------------------------------- + Context 'NFS Storage' { + It 'Should create NFS storage' { + if (Skip-IfNoSharedStorage) { return } + + # Parse server and export from the full NFS export path (e.g. "10.0.0.1:/srv/nfs/shared") + $nfsParts = $script:NfsExport -split ':', 2 + $nfsServer = $nfsParts[0] + $nfsExportPath = $nfsParts[1] + + $result = New-PveStorage -Storage 'pester-nfs' -Type 'nfs' ` + -Server $nfsServer ` + -Export $nfsExportPath ` + -Content 'images,iso,backup' ` + -Shared ` + -ErrorAction Stop + + $result | Should -Not -BeNullOrEmpty + $result.Storage | Should -Be 'pester-nfs' + $script:CreatedStorages.Add('pester-nfs') + } + + It 'Should list and find the NFS storage' { + if (Skip-IfNoSharedStorage) { return } + + $storages = Get-PveStorage -Node $script:Node + $nfs = $storages | Where-Object { $_.Storage -eq 'pester-nfs' } + $nfs | Should -Not -BeNullOrEmpty + $nfs.Type | Should -Be 'nfs' + } + + It 'Should get NFS storage status' { + if (Skip-IfNoSharedStorage) { return } + + $status = Get-PveStorageStatus -Node $script:Node -Storage 'pester-nfs' -ErrorAction Stop + $status | Should -Not -BeNullOrEmpty + $status.Active | Should -Be 1 + } + + It 'Should remove NFS storage' { + if (Skip-IfNoSharedStorage) { return } + + { Remove-PveStorage -Storage 'pester-nfs' -Confirm:$false -ErrorAction Stop } | + Should -Not -Throw + $script:CreatedStorages.Remove('pester-nfs') + } + } + + # ------------------------------------------------------------------- + Context 'iSCSI Storage' { + It 'Should create iSCSI storage with -Target parameter' { + if (Skip-IfNoSharedStorage) { return } + + $result = New-PveStorage -Storage 'pester-iscsi' -Type 'iscsi' ` + -Server $script:StorageVmIp ` + -Target $script:IscsiIqn ` + -Shared ` + -ErrorAction Stop + + $result | Should -Not -BeNullOrEmpty + $result.Storage | Should -Be 'pester-iscsi' + $script:CreatedStorages.Add('pester-iscsi') + } + + It 'Should list and find the iSCSI storage' { + if (Skip-IfNoSharedStorage) { return } + + $storages = Get-PveStorage -Node $script:Node + $iscsi = $storages | Where-Object { $_.Storage -eq 'pester-iscsi' } + $iscsi | Should -Not -BeNullOrEmpty + $iscsi.Type | Should -Be 'iscsi' + } + + It 'Should get iSCSI storage status' { + if (Skip-IfNoSharedStorage) { return } + + $status = Get-PveStorageStatus -Node $script:Node -Storage 'pester-iscsi' -ErrorAction Stop + $status | Should -Not -BeNullOrEmpty + } + + It 'Should remove iSCSI storage' { + if (Skip-IfNoSharedStorage) { return } + + { Remove-PveStorage -Storage 'pester-iscsi' -Confirm:$false -ErrorAction Stop } | + Should -Not -Throw + $script:CreatedStorages.Remove('pester-iscsi') + } + } +} diff --git a/tests/PSProxmoxVE.Tests/Storage/Get-PveStorage.Tests.ps1 b/tests/PSProxmoxVE.Tests/Storage/Get-PveStorage.Tests.ps1 index 8e40591..e66f721 100644 --- a/tests/PSProxmoxVE.Tests/Storage/Get-PveStorage.Tests.ps1 +++ b/tests/PSProxmoxVE.Tests/Storage/Get-PveStorage.Tests.ps1 @@ -171,6 +171,48 @@ Describe 'New-PveStorage' { $isMandatory | Should -Not -BeNullOrEmpty } } + + Context 'iSCSI/NFS parameters' { + It 'Should have Target parameter for iSCSI IQN' { + Skip-IfMissing 'New-PveStorage' + $script:Cmd.Parameters.ContainsKey('Target') | Should -BeTrue + } + + It 'Target should not be Mandatory' { + Skip-IfMissing 'New-PveStorage' + $isMandatory = $script:Cmd.Parameters['Target'].ParameterSets.Values | + Where-Object { $_.IsMandatory } + $isMandatory | Should -BeNullOrEmpty + } + + It 'Should have Portal parameter for iSCSI portal' { + Skip-IfMissing 'New-PveStorage' + $script:Cmd.Parameters.ContainsKey('Portal') | Should -BeTrue + } + + It 'Portal should not be Mandatory' { + Skip-IfMissing 'New-PveStorage' + $isMandatory = $script:Cmd.Parameters['Portal'].ParameterSets.Values | + Where-Object { $_.IsMandatory } + $isMandatory | Should -BeNullOrEmpty + } + + It 'Should have Server parameter for NFS/iSCSI server' { + Skip-IfMissing 'New-PveStorage' + $script:Cmd.Parameters.ContainsKey('Server') | Should -BeTrue + } + + It 'Should have Export parameter for NFS export path' { + Skip-IfMissing 'New-PveStorage' + $script:Cmd.Parameters.ContainsKey('Export') | Should -BeTrue + } + + It 'Should have Shared switch parameter' { + Skip-IfMissing 'New-PveStorage' + $script:Cmd.Parameters.ContainsKey('Shared') | Should -BeTrue + $script:Cmd.Parameters['Shared'].ParameterType | Should -Be ([System.Management.Automation.SwitchParameter]) + } + } } # --------------------------------------------------------------------------- From c385f4a620b42da2847c327f223f2905b8e4822f Mon Sep 17 00:00:00 2001 From: Clint Branham Date: Tue, 24 Mar 2026 14:57:45 -0500 Subject: [PATCH 2/5] fix(tests): correct NFS and iSCSI storage test parameters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NFS and iSCSI are implicitly shared in PVE — the API rejects an explicit 'shared' parameter. iSCSI uses 'portal' (not 'server') for the target address. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../PSProxmoxVE.Tests/Integration/SharedStorage.Tests.ps1 | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/PSProxmoxVE.Tests/Integration/SharedStorage.Tests.ps1 b/tests/PSProxmoxVE.Tests/Integration/SharedStorage.Tests.ps1 index 3409a3a..1e02b57 100644 --- a/tests/PSProxmoxVE.Tests/Integration/SharedStorage.Tests.ps1 +++ b/tests/PSProxmoxVE.Tests/Integration/SharedStorage.Tests.ps1 @@ -98,11 +98,11 @@ Describe 'Shared Storage — Integration' -Tag 'Integration' { $nfsServer = $nfsParts[0] $nfsExportPath = $nfsParts[1] + # Note: NFS is implicitly shared in PVE — do not pass -Shared $result = New-PveStorage -Storage 'pester-nfs' -Type 'nfs' ` -Server $nfsServer ` -Export $nfsExportPath ` -Content 'images,iso,backup' ` - -Shared ` -ErrorAction Stop $result | Should -Not -BeNullOrEmpty @@ -141,10 +141,11 @@ Describe 'Shared Storage — Integration' -Tag 'Integration' { It 'Should create iSCSI storage with -Target parameter' { if (Skip-IfNoSharedStorage) { return } + # iSCSI uses -Portal (not -Server) for the target address. + # iSCSI is implicitly shared in PVE — do not pass -Shared. $result = New-PveStorage -Storage 'pester-iscsi' -Type 'iscsi' ` - -Server $script:StorageVmIp ` + -Portal $script:StorageVmIp ` -Target $script:IscsiIqn ` - -Shared ` -ErrorAction Stop $result | Should -Not -BeNullOrEmpty From 2bf7f45ebfcee81913d58bb563b7248d580f2b13 Mon Sep 17 00:00:00 2001 From: Clint Branham Date: Tue, 24 Mar 2026 15:24:06 -0500 Subject: [PATCH 3/5] fix(ci): mount /lib/modules into NFS container for automatic kernel module loading The NFS server image already has modprobe logic built in but needs access to host kernel modules. Avoids requiring manual modprobe on the runner host. Co-Authored-By: Claude Opus 4.6 (1M context) --- tests/infrastructure/docker-compose.storage.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/infrastructure/docker-compose.storage.yml b/tests/infrastructure/docker-compose.storage.yml index 3993428..6d04dee 100644 --- a/tests/infrastructure/docker-compose.storage.yml +++ b/tests/infrastructure/docker-compose.storage.yml @@ -53,6 +53,7 @@ services: privileged: true volumes: - nfs-data:/srv/nfs/shared + - /lib/modules:/lib/modules:ro environment: NFS_EXPORT_0: /srv/nfs/shared *(rw,sync,no_subtree_check,no_root_squash) restart: unless-stopped From 79c227f131704f5edb795af70b561b32870c6edf Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 24 Mar 2026 20:30:56 +0000 Subject: [PATCH 4/5] Initial plan From f76ff513e96ba23e2095db01514b5dfb80a34ce6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 24 Mar 2026 20:36:46 +0000 Subject: [PATCH 5/5] fix(storage): implement Portal defaulting for iSCSI, fix Pool/CephPool selection, add NFS format validation Co-authored-by: GoodOlClint <151449+GoodOlClint@users.noreply.github.com> Agent-Logs-Url: https://github.com/GoodOlClint/PSProxmoxVE/sessions/d84722e4-409e-4ced-bfbc-f323715b8bdc --- .../Cmdlets/Storage/NewPveStorageCmdlet.cs | 25 ++++++++++++++++--- .../Integration/SharedStorage.Tests.ps1 | 7 ++++++ .../infrastructure/docker-compose.storage.yml | 3 +++ 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/src/PSProxmoxVE/Cmdlets/Storage/NewPveStorageCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Storage/NewPveStorageCmdlet.cs index 95a67a2..f8d643a 100644 --- a/src/PSProxmoxVE/Cmdlets/Storage/NewPveStorageCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Storage/NewPveStorageCmdlet.cs @@ -1,3 +1,4 @@ +using System; using System.Collections.Generic; using System.Management.Automation; using Newtonsoft.Json.Linq; @@ -114,14 +115,32 @@ namespace PSProxmoxVE.Cmdlets.Storage AddIfNotEmpty(data, "content", Content); AddIfNotEmpty(data, "path", Path); - AddIfNotEmpty(data, "server", Server); AddIfNotEmpty(data, "export", Export); AddIfNotEmpty(data, "vgname", VgName); AddIfNotEmpty(data, "thinpool", ThinPool); - AddIfNotEmpty(data, "pool", Pool ?? CephPool); + AddIfNotEmpty(data, "pool", !string.IsNullOrEmpty(Pool) ? Pool : CephPool); AddIfNotEmpty(data, "monhost", MonHost); AddIfNotEmpty(data, "target", Target); - AddIfNotEmpty(data, "portal", Portal); + + // For iSCSI types, 'server' is not a valid API field; derive portal from Server if Portal omitted. + bool isIscsiType = string.Equals(Type, "iscsi", StringComparison.OrdinalIgnoreCase) + || string.Equals(Type, "iscsidirect", StringComparison.OrdinalIgnoreCase); + if (isIscsiType) + { + string? portalValue; + if (!string.IsNullOrEmpty(Portal)) + portalValue = Portal; + else if (!string.IsNullOrEmpty(Server)) + portalValue = $"{Server}:3260"; + else + portalValue = null; + AddIfNotEmpty(data, "portal", portalValue); + } + else + { + AddIfNotEmpty(data, "server", Server); + AddIfNotEmpty(data, "portal", Portal); + } AddIfNotEmpty(data, "nodes", Nodes); if (Shared.IsPresent) data["shared"] = "1"; if (Disable.IsPresent) data["disable"] = "1"; diff --git a/tests/PSProxmoxVE.Tests/Integration/SharedStorage.Tests.ps1 b/tests/PSProxmoxVE.Tests/Integration/SharedStorage.Tests.ps1 index 1e02b57..62e07fc 100644 --- a/tests/PSProxmoxVE.Tests/Integration/SharedStorage.Tests.ps1 +++ b/tests/PSProxmoxVE.Tests/Integration/SharedStorage.Tests.ps1 @@ -95,6 +95,13 @@ Describe 'Shared Storage — Integration' -Tag 'Integration' { # Parse server and export from the full NFS export path (e.g. "10.0.0.1:/srv/nfs/shared") $nfsParts = $script:NfsExport -split ':', 2 + if (-not $nfsParts -or + $nfsParts.Count -ne 2 -or + [string]::IsNullOrWhiteSpace($nfsParts[0]) -or + [string]::IsNullOrWhiteSpace($nfsParts[1])) { + Set-ItResult -Skipped -Because "PVETEST_NFS_EXPORT must be in 'server:/export/path' format (current value: '$($script:NfsExport)')" + return + } $nfsServer = $nfsParts[0] $nfsExportPath = $nfsParts[1] diff --git a/tests/infrastructure/docker-compose.storage.yml b/tests/infrastructure/docker-compose.storage.yml index 6d04dee..40eb511 100644 --- a/tests/infrastructure/docker-compose.storage.yml +++ b/tests/infrastructure/docker-compose.storage.yml @@ -53,6 +53,9 @@ services: privileged: true volumes: - nfs-data:/srv/nfs/shared + # NOTE: /lib/modules is required by the NFS kernel module loader. + # This mount is Linux-host-specific and will fail on macOS/Windows Docker Desktop. + # Integration tests using this compose file must run on a Linux host (e.g. GitHub-hosted runners or the dev container). - /lib/modules:/lib/modules:ro environment: NFS_EXPORT_0: /srv/nfs/shared *(rw,sync,no_subtree_check,no_root_squash)