Skip to content

Conversation

@abageria
Copy link

Description

Mandatory Checklist

  • SHOULD update ChangeLog.md file(s) appropriately
    • Update src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.
      • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header in the past tense.
    • Should not change ChangeLog.md if no new release is required, such as fixing test case only.
  • SHOULD regenerate markdown help files if there is cmdlet API change. Instruction
  • SHOULD have proper test coverage for changes in pull request.
  • SHOULD NOT adjust version of module manually in pull request

@azure-client-tools-bot-prd
Copy link

Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status.

@microsoft-github-policy-service
Copy link
Contributor

Thank you for your contribution @abageria! We will review the pull request and get back to you soon.

@abageria
Copy link
Author

@microsoft-github-policy-service agree company="Microsoft"

@abageria
Copy link
Author

/azp run

@azure-pipelines
Copy link
Contributor

Commenter does not have sufficient privileges for PR 28681 in repo Azure/azure-powershell

@isra-fel
Copy link
Member

Hi @abageria if you are an employer of Microsoft please join the "Azure" organization here: https://aka.ms/AzureGitHub

[Parameter(ParameterSetName = ASRParameterSets.AzureToAzure, HelpMessage = "Specify the platform fault domain to be used by the failover Vm in target recovery region.")]
[Parameter(ParameterSetName = ASRParameterSets.AzureToAzureWithoutDiskDetails, HelpMessage = "Specify the platform fault domain to be used by the failover Vm in target recovery region.")]
[ValidateNotNullOrEmpty]
public int? PlatformFaultDomain { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Its data type is nullable int, but we are also validating that it can't be null. These two are contradicting each other.

#create virtual Machine scale set
$stnd = "Standard"
$vmssConfig = New-AzVmssConfig -Location $recoveryLocation -PlatformFaultDomainCount 1 -SinglePlacementGroup 0 -SecurityType $stnd
$vmssConfig = New-AzVmssConfig -Location $recoveryLocation -PlatformFaultDomainCount 2 -SinglePlacementGroup 0 -SecurityType $stnd
Copy link
Contributor

Choose a reason for hiding this comment

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

get the value of platformFaultDomainCount from helper class like you are doing for platformFaultDomain.

#Validate vmss Set in replicated vm properties
$pe = Get-AzRecoveryServicesAsrReplicationProtectedItem -ProtectionContainer $pc -Name $vmName
Assert-NotNull($pe.providerSpecificDetails.RecoveryVirtualMachineScaleSetId)
Assert-NotNull($pe.providerSpecificDetails.PlatformFaultDomain)
Copy link
Contributor

Choose a reason for hiding this comment

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

Also validate Equal

WaitForJobCompletion -JobId $updateDRjob.Name
$pe = Get-AzRecoveryServicesAsrReplicationProtectedItem -ProtectionContainer $pc -Name $vmName
Assert-NotNull($pe.providerSpecificDetails.RecoveryVirtualMachineScaleSetId)
Assert-NotNull($pe.providerSpecificDetails.PlatformFaultDomain)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

$rgId = "/subscriptions/7c943c1b-5122-4097-90c8-861411bdd574/resourceGroups/vijami-alertrg"

$ReprotectJob = Update-AzRecoveryServicesAsrProtectionDirection -AzureToAzure -ReplicationProtectedItem $ReplicationProtectedItem -ProtectionContainerMapping $RecoveryMapping -LogStorageAccountId $CacheStorageAccount -RecoveryResourceGroupID $rgId
$platformFaultDomain = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

don't have constants here, get all these values from helper class.

$ReprotectJob = Update-AzRecoveryServicesAsrProtectionDirection -AzureToAzure -ReplicationProtectedItem $ReplicationProtectedItem -ProtectionContainerMapping $RecoveryMapping -LogStorageAccountId $CacheStorageAccount -RecoveryResourceGroupID $rgId
$platformFaultDomain = 1

$ReprotectJob = Update-AzRecoveryServicesAsrProtectionDirection -AzureToAzure -ReplicationProtectedItem $ReplicationProtectedItem -ProtectionContainerMapping $RecoveryMapping -LogStorageAccountId $CacheStorageAccount -RecoveryResourceGroupID $rgId -PlatformFaultDomain $platformFaultDomain
Copy link
Contributor

Choose a reason for hiding this comment

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

Get protected item and perform validations.

/// Gets or sets the platform fault domain for replication protected item after failover.
/// </summary>
[Parameter]
[ValidateNotNullOrEmpty]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Member

@isra-fel isra-fel left a comment

Choose a reason for hiding this comment

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

Please also add a new entry to ChangeLog.md

@github-actions
Copy link

This PR was labeled "needs-revision" because it has unresolved review comments or CI failures.
Please resolve all open review comments and make sure all CI checks are green. Refer to our guide to troubleshoot common CI failures.

@isra-fel
Copy link
Member

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

Copilot AI review requested due to automatic review settings October 30, 2025 18:43
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 adds support for Platform Fault Domain and Availability Zone parameters in Azure Site Recovery protection workflows. The changes include:

  • Adding PlatformFaultDomain and RecoveryAvailabilityZone parameters to cmdlets for Enable, Update, and Switch Protection operations
  • Updating the SDK to version 2025-08-01 API
  • Adding test coverage for the new parameters
  • Extending VMware CBT and InMageRcm scenarios with disk configuration properties (IOPS, throughput, disk size)

Reviewed Changes

Copilot reviewed 10 out of 50 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
Update-AzRecoveryServicesAsrProtectionDirection.md Added documentation for RecoveryAvailabilityZone and PlatformFaultDomain parameters
Set-AzRecoveryServicesAsrReplicationProtectedItem.md Added documentation for PlatformFaultDomain parameter
New-AzRecoveryServicesAsrReplicationProtectedItem.md Added documentation for PlatformFaultDomain parameter
UpdateAzureRmRecoveryServicesAsrProtectionDirection.cs Added RecoveryAvailabilityZone and PlatformFaultDomain properties to cmdlet
SetAzureRmRecoveryServicesAsrReplicationProtectedItem.cs Added PlatformFaultDomain property and validation logic
NewAzureRmRecoveryServicesAsrReplicationProtectedItem.cs Added PlatformFaultDomain property with incorrect XML doc
AsrA2ATests.ps1 Added test coverage for new parameters with typo in variable name
A2ATestsHelper.ps1 Added helper functions with naming inconsistencies
README.md Updated SDK commit hash and API version
SDK Generated files Generated code from updated OpenAPI spec including new models and operations
ChangeLog.md Added release notes entry

$recoveryNetMapping = getRecoveryNetworkMapping
$platformFaultDomain = getPlatformDomain
$platformFaultDomain1 = getPlatformDomain1
$platoformFaultDomainCount2 = getPlatformDomainCount2
Copy link

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

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

Corrected spelling of 'platoformFaultDomainCount2' to 'platformFaultDomainCount2'.

Copilot uses AI. Check for mistakes.
$recoveryNetMapping = getRecoveryNetworkMapping
$platformFaultDomain = getPlatformDomain
$platformFaultDomain1 = getPlatformDomain1
$platoformFaultDomainCount2 = getPlatformDomainCount2
Copy link

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

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

The function 'getPlatformDomainCount2' is being called but is not defined in A2ATestsHelper.ps1. Based on the helper file, the correct function name should be 'getPlatformUpdateDomain2' (though that function itself has a naming issue noted separately).

Copilot uses AI. Check for mistakes.
Comment on lines +1934 to +1935
$platformFaultDomain = getPlatformDomain
$platformFaultDomain1 = getPlatformDomain1
Copy link

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

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

Functions 'getPlatformDomain' and 'getPlatformDomain1' are being called but are not defined in A2ATestsHelper.ps1. The correct function names based on the helper file are 'getPlatformFaultDomain' and 'getPlatformUpdateDomain1'.

Copilot uses AI. Check for mistakes.
public ASRReplicationProtectionCluster ReplicationProtectionCluster { get; set; }

/// <summary>
/// Gets or sets the resource ID of virtual machine scale set to failover this virtual machine to.
Copy link

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

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

The XML documentation comment is incorrect. This property is for PlatformFaultDomain, not for virtual machine scale set resource ID. The comment should describe the platform fault domain property instead.

Copilot uses AI. Check for mistakes.
Comment on lines +771 to +772
this.KeyEncryptionVaultId),
PlatformFaultDomain = this.PlatformFaultDomain
Copy link

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

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

The PlatformFaultDomain is assigned from 'this.PlatformFaultDomain' instead of the local variable 'platformFaultDomain' that was set earlier (lines 429, 705-707). This bypasses the logic that preserves the existing value when the parameter is not bound, making the fallback behavior ineffective.

Copilot uses AI. Check for mistakes.
@isra-fel
Copy link
Member

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants