Skip to content

Conversation

@gungazoo
Copy link
Contributor

This PR closes #
Bug 36304283: Run Command to create new storage policies needs "compression" option for ESA vSAN

The changes in this PR are as follows:

  • If creating in SDDC with ESA adds compression by default. It can be disabled
  • If SDDC has ESA and OSA it will create two different policies and put either ESA or OSA in the name

I have read the contributor guidelines and have completed the following:

  • Formatted the code using VSCode default formatter for PowerShell.
  • Tested the code end-to-end against an SDDC.
  • Documented the functions using standard PowerShell markup and applied AVSAttribute to newly exported functions.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR modernizes the New-AVSStoragePolicy function to support both ESA (Express Storage Architecture) and OSA (Original Storage Architecture) vSAN clusters, with ESA-specific compression support. The implementation has been completely rewritten to use PowerCLI cmdlets instead of low-level SPBM Views API.

Key Changes:

  • Automatic detection of ESA/OSA architectures and creation of separate policies with appropriate suffixes
  • ESA-specific compression support (enabled by default, can be disabled with -NoCompression)
  • Simplified parameter model using PowerCLI cmdlets (New-SpbmRule, New-SpbmRuleSet, New-SpbmStoragePolicy)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1016 to 1031
if ($FailureToleranceMethod -eq "None" -and $FailuresToTolerate -ne 0) {
Write-Error "Failure tolerance method 'None' requires FailuresToTolerate = 0"
throw
}

#VSANObjectReservation
Write-Information "vSANObjectReservation set to: $vSANObjectSpaceReservation"
If ($vSANObjectSpaceReservation -gt 0) {
Write-Information "Creating vSANObjectReservation Subprofile"
$Subprofile = new-object VMware.Spbm.Views.PbmCapabilityInstance
$Subprofile.Id = New-Object VMware.Spbm.Views.PbmCapabilityMetadataUniqueId
$Subprofile.Id.Namespace = "VSAN"
$Subprofile.Id.Id = "proportionalCapacity"
$Subprofile.Constraint = New-Object VMware.Spbm.Views.PbmCapabilityConstraintInstance
$Subprofile.Constraint[0].PropertyInstance = New-Object VMware.Spbm.Views.PbmCapabilityPropertyInstance
$Subprofile.Constraint[0].PropertyInstance[0].id = $Subprofile.Id.Id
$Subprofile.Constraint[0].PropertyInstance[0].value = $vSANObjectSpaceReservation
If (($profilespec.Constraints.SubProfiles | Where-Object { $_.Name -eq "VSAN" }).count -eq 0) {
$profilespec.Constraints.SubProfiles += new-object VMware.Spbm.Views.PbmCapabilitySubProfile -Property @{"Name" = "VSAN" }
Write-Information "Added VSAN Subprofile to ProfileSpec"
($profilespec.Constraints.SubProfiles | Where-Object { $_.Name -eq "VSAN" }).Capability += $subprofile
}
Else { ($profilespec.Constraints.SubProfiles | Where-Object { $_.Name -eq "VSAN" }).Capability += $subprofile }
if ($FailuresToTolerate -eq 0 -and $FailureToleranceMethod -ne "None") {
Write-Error "FailuresToTolerate = 0 requires FailureToleranceMethod = 'None'"
throw
}
if ($FailureToleranceMethod -eq "RAID5" -and $FailuresToTolerate -ne 1) {
Write-Error "RAID-5 erasure coding requires FailuresToTolerate = 1"
throw
}
if ($FailureToleranceMethod -eq "RAID6" -and $FailuresToTolerate -ne 2) {
Write-Error "RAID-6 erasure coding requires FailuresToTolerate = 2"
throw
}
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The validation logic throws errors for invalid FTT/FailureToleranceMethod combinations but uses throw after Write-Error. This is redundant - either use Write-Error with return or just use throw with a message. The current approach will display the error twice and the exception message will just be the line number rather than the descriptive error.

Copilot uses AI. Check for mistakes.

[Parameter(Mandatory = $false,
HelpMessage = "Site disaster tolerance method")]
[ValidateSet("None", "Dual", "Preferred")]
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DisasterTolerance parameter only supports "None", "Dual", and "Preferred" values, but the old implementation also supported "Secondary" and "NoneStretch" for stretch clusters. Removing these options without documentation is a breaking change that may affect users with stretch cluster configurations.

Suggested change
[ValidateSet("None", "Dual", "Preferred")]
[ValidateSet("None", "Dual", "Preferred", "Secondary", "NoneStretch")]

Copilot uses AI. Check for mistakes.
Comment on lines 921 to +980
param(
#Add parameterSetNames to allow for vSAN, Tags, VMEncryption, StorageIOControl, vSANDirect to be optional.
[Parameter(Mandatory = $true)]
[string]
$Name,
[Parameter(Mandatory = $false)]
[string]
$Description,
[Parameter(Mandatory = $false)]
[ValidateSet("None", "Dual", "Preferred", "Secondary", "NoneStretch")]
[string]
$vSANSiteDisasterTolerance,
[Parameter(Mandatory = $false)]
[ValidateSet("None", "R1FTT1", "R5FTT1", "R1FTT2", "R6FTT2", "R1FTT3")]
[string]
$vSANFailuresToTolerate,
[Parameter(Mandatory = $false)]
[ValidateSet("None", "PreIO", "PostIO")]
[string]
$VMEncryption = "None",
[Parameter(Mandatory = $false)]
[Parameter(Mandatory = $true,
HelpMessage = "Name for the new storage policy")]
[ValidateNotNullOrEmpty()]
[string]$PolicyName,

[Parameter(Mandatory = $false,
HelpMessage = "Number of host failures to tolerate (0-3)")]
[ValidateRange(0, 3)]
[int]$FailuresToTolerate = 1,

[Parameter(Mandatory = $false,
HelpMessage = "Failure tolerance method: None, RAID1, RAID5, RAID6")]
[ValidateSet("None", "RAID1", "RAID5", "RAID6")]
[string]$FailureToleranceMethod = "RAID1",

[Parameter(Mandatory = $false,
HelpMessage = "Site disaster tolerance method")]
[ValidateSet("None", "Dual", "Preferred")]
[string]$DisasterTolerance = "None",

[Parameter(Mandatory = $false,
HelpMessage = "Enable VM encryption")]
[bool]$VmEncryption = $false,

[Parameter(Mandatory = $false,
HelpMessage = "Object space reservation percentage (0-100)")]
[ValidateRange(0, 100)]
[int]
$vSANObjectSpaceReservation,
[Parameter(Mandatory = $false)]
[int]$ObjectSpaceReservation = 0,

[Parameter(Mandatory = $false,
HelpMessage = "Number of disk stripes per object (1-12)")]
[ValidateRange(1, 12)]
[int]
$vSANDiskStripesPerObject,
[Parameter(Mandatory = $false)]
[ValidateRange(0, 2147483647)]
[int]
$vSANIOLimit,
[Parameter(Mandatory = $false)]
[ValidateRange(0, 100)]
[int]
$vSANCacheReservation,
[Parameter(Mandatory = $false)]
[boolean]
$vSANChecksumDisabled,
[Parameter(Mandatory = $false)]
[boolean]
$vSANForceProvisioning,
[Parameter(Mandatory = $false)]
[string]
$Tags,
[Parameter(Mandatory = $false)]
[string]
$NotTags,
[Parameter(Mandatory = $false)]
[Boolean]
$Overwrite
[int]$StripesPerObject = 1,

)
[Parameter(Mandatory = $false,
HelpMessage = "IOPS limit (0 = unlimited)")]
[ValidateRange(0, [int]::MaxValue)]
[int]$IopsLimit = 0,

[Parameter(Mandatory = $false,
HelpMessage = "Flash cache reservation percentage (0-100)")]
[ValidateRange(0, 100)]
[int]$CacheReservation = 0,

[Parameter(Mandatory = $false,
HelpMessage = "Disable object checksum")]
[bool]$DisableChecksum = $false,

Begin {
# Internal helper to add or append a VSAN capability instance to the profile spec ensuring the VSAN subprofile exists
function Add-VsanCapabilityInstanceLocal {
param(
[Parameter(Mandatory)] [string] $Id,
[Parameter(Mandatory)] $Value,
[Parameter(Mandatory)] $ProfileSpecRef
)
$subProf = ($ProfileSpecRef.Constraints.SubProfiles | Where-Object { $_.Name -eq 'VSAN' })
if (-not $subProf) {
$ProfileSpecRef.Constraints.SubProfiles += New-Object VMware.Spbm.Views.PbmCapabilitySubProfile -Property @{ Name = 'VSAN' }
Write-Information 'Added VSAN Subprofile to ProfileSpec'
$subProf = ($ProfileSpecRef.Constraints.SubProfiles | Where-Object { $_.Name -eq 'VSAN' })
}
$cap = New-Object VMware.Spbm.Views.PbmCapabilityInstance
$cap.Id = New-Object VMware.Spbm.Views.PbmCapabilityMetadataUniqueId
$cap.Id.Namespace = 'VSAN'
$cap.Id.Id = $Id
$cap.Constraint = New-Object VMware.Spbm.Views.PbmCapabilityConstraintInstance
$cap.Constraint[0].PropertyInstance = New-Object VMware.Spbm.Views.PbmCapabilityPropertyInstance
$cap.Constraint[0].PropertyInstance[0].id = $cap.Id.Id
$cap.Constraint[0].PropertyInstance[0].value = $Value
$subProf.Capability += $cap
return $cap
}
#Cleanup Wildcard and Code Injection Characters
Write-Information "Cleaning up Wildcard and Code Injection Characters from Name value: $Name"
$Name = Limit-WildcardsandCodeInjectionCharacters -String $Name
Write-Information "Name value after cleanup: $Name"
Write-Information "Cleaning up Wildcard and Code Injection Characters from Description value: $Description"
If (![string]::IsNullOrEmpty($Description)) { $Description = Limit-WildcardsandCodeInjectionCharacters -String $Description }
Write-Information "Description value after cleanup: $Description"
[Parameter(Mandatory = $false,
HelpMessage = "Force provisioning even if resources are insufficient")]
[bool]$ForceProvisioning = $false,

#Protected Policy Object Name Validation Check
If (Test-AVSProtectedObjectName -Name $Name) {
Write-Error "$Name is a protected policy name. Please choose a different policy name."
break
}
[Parameter(Mandatory = $false,
HelpMessage = "Disable compression (ESA only)")]
[switch]$NoCompression,

#Check for existing policy
$ExistingPolicy = Get-AVSStoragePolicy -Name $Name
Write-Information ("Existing Policy: " + $ExistingPolicy.name)
if ($ExistingPolicy -and !$Overwrite) {
Write-Error "Storage Policy $Name already exists. Set -Overwrite to $true to overwrite existing policy."
break
}
if (!$ExistingPolicy -and $Overwrite) {
Write-Error "Storage Policy $Name does not exist. Set -Overwrite to $false to create new policy."
break
}
Write-Information "Overwrite value set to: $Overwrite"
Switch ($Overwrite) {
$true {
$pbmprofileresourcetype = new-object vmware.spbm.views.PbmProfileResourceType
$pbmprofileresourcetype.ResourceType = "STORAGE" # No other known valid value.
$profilespec = new-object VMware.Spbm.Views.PbmCapabilityProfileUpdateSpec
$profilespec.Name = $Name
$profilespec.Constraints = new-object vmware.spbm.views.PbmCapabilitySubProfileConstraints
If (![string]::IsNullOrEmpty($Description)) { $profilespec.Description = $Description }
}
$false {
$pbmprofileresourcetype = new-object vmware.spbm.views.PbmProfileResourceType
$pbmprofileresourcetype.ResourceType = "STORAGE" # No other known valid value.
$profilespec = new-object VMware.Spbm.Views.PbmCapabilityProfileCreateSpec
$profilespec.ResourceType = $pbmprofileresourcetype
$profilespec.Name = $Name
$profilespec.Constraints = new-object vmware.spbm.views.PbmCapabilitySubProfileConstraints
If (![string]::IsNullOrEmpty($Description)) { $profilespec.Description = $Description }
$profilespec.Category = "REQUIREMENT" #Valid options are REQUIREMENT = vSAN Storage Policies or RESOURCE = ?? or DATA_SERVICE_POLICY = Common Storage Policies such encryption and storage IO.
Write-Information "Profile Name set to: $($profilespec.Name)"
Write-Information "Profile Category set to: $($profilespec.Category)"
}
}
Write-Information "Getting SPBM Capabilities"
$SPBMCapabilities = Get-AVSSPBMCapabilities
Foreach ($Capability in $SPBMCapabilities) {
Write-Information "SPBM Capability: NameSpace: $($Capability.NameSpace), SubCategory: $($Capability.SubCategory), CapabilityMetaData Count: $($Capability.CapabilityMetadata.Count)"
}
[Parameter(Mandatory = $false,
HelpMessage = "Description for the storage policy")]
[string]$Description
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple breaking changes exist in the parameter names compared to the old implementation. Parameters renamed include: Name to PolicyName, vSANFailuresToTolerate to FailuresToTolerate, vSANSiteDisasterTolerance to DisasterTolerance, VMEncryption to VmEncryption, and many others. This completely breaks backward compatibility. All removed parameters like Tags, NotTags, Overwrite, vSANDiskStripesPerObject, vSANCacheReservation, etc. should either be reimplemented or documented as removed features.

Copilot uses AI. Check for mistakes.
Comment on lines +922 to +925
[Parameter(Mandatory = $true,
HelpMessage = "Name for the new storage policy")]
[ValidateNotNullOrEmpty()]
[string]$PolicyName,
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old implementation had input sanitization using Limit-WildcardsandCodeInjectionCharacters for the Name and Description parameters to prevent code injection. This security measure has been removed in the new implementation, potentially exposing the function to injection attacks if user input is passed directly to the VMware APIs.

Copilot uses AI. Check for mistakes.

# VM Encryption
if ($VmEncryption) {
$rules += New-SpbmRule -Capability (Get-SpbmCapability -Name "VSAN.dataService.dataAtRestEncryption" -Server $VIServer) -Value $true
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The VmEncryption parameter uses the type [bool] but based on the old implementation, VM encryption should reference a separate encryption policy (Pre-IO or Post-IO). The new implementation appears to set encryption directly, but doesn't allow users to specify Pre-IO vs Post-IO encryption which was supported in the old version. This is a feature regression.

Suggested change
$rules += New-SpbmRule -Capability (Get-SpbmCapability -Name "VSAN.dataService.dataAtRestEncryption" -Server $VIServer) -Value $true
# Support both legacy policy-based encryption (e.g., Pre-IO/Post-IO) and
# the newer boolean toggle for data-at-rest encryption.
if ($VmEncryption -is [string] -and -not [string]::IsNullOrWhiteSpace($VmEncryption)) {
# Treat VmEncryption as the name of an existing VSAN storage policy
# (for example, a Pre-IO or Post-IO encryption policy) and merge its rules.
$encryptionPolicy, $null = Get-StoragePolicyInternal -StoragePolicyName $VmEncryption
if ($null -ne $encryptionPolicy -and $null -ne $encryptionPolicy.AnyOfRuleSets) {
foreach ($ruleSet in $encryptionPolicy.AnyOfRuleSets) {
if ($null -ne $ruleSet.Rules) {
$rules += $ruleSet.Rules
}
}
}
}
elseif ($VmEncryption -is [bool] -and $VmEncryption) {
# Backward-compatible behavior: simply enable data-at-rest encryption.
$rules += New-SpbmRule -Capability (Get-SpbmCapability -Name "VSAN.dataService.dataAtRestEncryption" -Server $VIServer) -Value $true
}

Copilot uses AI. Check for mistakes.
Write-Information "Added VSAN Subprofile to ProfileSpec"
($profilespec.Constraints.SubProfiles | Where-Object { $_.Name -eq "VSAN" }).Capability += $subprofile
begin {
Write-Verbose "Using vCenter Server: $($VIServer.Name)"
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable $VIServer is used without being defined or passed as a parameter. This will cause the function to fail unless there's a global/script-scoped variable set elsewhere. PowerCLI cmdlets default to using the default server connection, so the -Server $VIServer parameters should either be removed or $VIServer should be properly defined/obtained.

Suggested change
Write-Verbose "Using vCenter Server: $($VIServer.Name)"
$connectedVIServer = Get-VIServer -ErrorAction SilentlyContinue | Select-Object -First 1
if ($connectedVIServer) {
Write-Verbose "Using vCenter Server: $($connectedVIServer.Name)"
} else {
Write-Verbose "Using default vCenter Server connection."
}

Copilot uses AI. Check for mistakes.
Comment on lines +1005 to +1010
throw
}

#vSANDiskStripesPerObject
Write-Information "vSANDiskStripesPerObject value is: $vSANDiskStripesPerObject"
If ($vSANDiskStripesPerObject -gt 0) {
Write-Information "Creating vSAN Disk Stripes Subprofile"
$Subprofile = new-object VMware.Spbm.Views.PbmCapabilityInstance
$Subprofile.Id = New-Object VMware.Spbm.Views.PbmCapabilityMetadataUniqueId
$Subprofile.Id.Namespace = "VSAN"
$Subprofile.Id.Id = "stripeWidth"
$Subprofile.Constraint = New-Object VMware.Spbm.Views.PbmCapabilityConstraintInstance
$Subprofile.Constraint[0].PropertyInstance = New-Object VMware.Spbm.Views.PbmCapabilityPropertyInstance
$Subprofile.Constraint[0].PropertyInstance[0].id = $Subprofile.Id.Id
$Subprofile.Constraint[0].PropertyInstance[0].value = $vSANDiskStripesPerObject
If (($profilespec.Constraints.SubProfiles | Where-Object { $_.Name -eq "VSAN" }).count -eq 0) {
$profilespec.Constraints.SubProfiles += new-object VMware.Spbm.Views.PbmCapabilitySubProfile -Property @{"Name" = "VSAN" }
Write-Information "Added VSAN Subprofile to ProfileSpec"
($profilespec.Constraints.SubProfiles | Where-Object { $_.Name -eq "VSAN" }).Capability += $subprofile
}
Else { ($profilespec.Constraints.SubProfiles | Where-Object { $_.Name -eq "VSAN" }).Capability += $subprofile }
if ($typesToCreate.Count -eq 0) {
Write-Error "No vSAN clusters found."
throw
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using throw without an error message is a poor practice. Each throw statement should include a meaningful error message that helps users understand what failed. For example, instead of just throw, use throw "Failed to enumerate clusters: $($_.Exception.Message)".

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants