-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TargetVmSize, AvSet, Sql RP and ResourceTagging changes for V2A and H2A #21
TargetVmSize, AvSet, Sql RP and ResourceTagging changes for V2A and H2A #21
Conversation
bringing Asr one sdk azure- master in sync with master branch for azure/azure-powershell
Multiple IP per nic - cmdlet changes
…neSdk/azure-powershell into newApiVersion/asr2021-02-10
…s for V2A and H2A
@@ -15,8 +15,8 @@ Enables replication for an ASR protectable item by creating a replication protec | |||
### EnterpriseToEnterprise (Default) | |||
``` | |||
New-AzRecoveryServicesAsrReplicationProtectedItem [-VmmToVmm] -ProtectableItem <ASRProtectableItem> | |||
-Name <String> -ProtectionContainerMapping <ASRProtectionContainerMapping> [-WaitForCompletion] | |||
[-DefaultProfile <IAzureContextContainer>] [-WhatIf] [-Confirm] [<CommonParameters>] | |||
-Name <String> -ProtectionContainerMapping <ASRProtectionContainerMapping> [-UseManagedDisk <String>] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UseManagedDisk [](start = 78, length = 14)
This is E2E right??? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove from here. It's by overlook. #Resolved
[-RecoveryVmTag <System.Collections.Generic.IDictionary`2[System.String,System.String]>] | ||
[-DiskTag <System.Collections.Generic.IDictionary`2[System.String,System.String]>] | ||
[-RecoveryNicTag <System.Collections.Generic.IDictionary`2[System.String,System.String]>] | ||
[-UseManagedDisk <String>] [-WaitForCompletion] -DiskType <String> [-DiskEncryptionSetId <String>] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[-UseManagedDisk ] [](start = 1, length = 26)
Do we want to expose in V2A??? The SRS decides on its own right based on Geo
Also in this we have DiskType so this shd be MD only #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In powershell also, we don't expose this for V2A. Will remove it. It has come from auto generation of help file. #Resolved
[-RecoveryVmTag <System.Collections.Generic.IDictionary`2[System.String,System.String]>] | ||
[-DiskTag <System.Collections.Generic.IDictionary`2[System.String,System.String]>] | ||
[-RecoveryNicTag <System.Collections.Generic.IDictionary`2[System.String,System.String]>] | ||
[-UseManagedDisk <String>] [-WaitForCompletion] [-DiskEncryptionSetId <String>] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UseManagedDisk [](start = 3, length = 14)
Please check @anmolbhatia289 #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[-RecoveryVmTag <System.Collections.Generic.IDictionary`2[System.String,System.String]>] | ||
[-DiskTag <System.Collections.Generic.IDictionary`2[System.String,System.String]>] | ||
[-RecoveryNicTag <System.Collections.Generic.IDictionary`2[System.String,System.String]>] | ||
[-WaitForCompletion] [-DiskEncryptionSetId <String>] | ||
[-DefaultProfile <IAzureContextContainer>] [-WhatIf] [-Confirm] [<CommonParameters>] | ||
``` | ||
|
||
### EnterpriseToAzure | ||
``` | ||
New-AzRecoveryServicesAsrReplicationProtectedItem [-HyperVToAzure] -ProtectableItem <ASRProtectableItem> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HyperVToAzure [](start = 52, length = 13)
Actually E2A also we have all right? then why this split #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type: System.String | ||
Parameter Sets: VMwareToAzureWithDiskType, VMwareToAzure, HyperVSiteToAzure | ||
Aliases: | ||
Accepted values: NoLicenseType, PAYG, AHUB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NoLicenseType [](start = 17, length = 13)
I remember 4 values in SRS #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't considered the NotSpecified case because this parameter itself won't be given if the user doesn't want to specify.
Similar to LicenseType which is already present. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to double check with Prachetos on how is NoLicenseType different from NotSpecified.
In reply to: 601548721 [](ancestors = 601548721)
Type: System.String | ||
Parameter Sets: (All) | ||
Aliases: | ||
Accepted values: NoLicenseType, PAYG, AHUB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NoLicenseType [](start = 17, length = 13)
Same as above #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will cross check on this and will add in next PR if any change required.
In reply to: 601541382 [](ancestors = 601541382)
@@ -864,6 +868,25 @@ public ASRHyperVReplicaAzureSpecificRPIDetails(HyperVReplicaAzureReplicationDeta | |||
/// </summary> | |||
public string RecoveryProximityPlacementGroupId { get; set; } | |||
|
|||
/// <summary> | |||
/// Gets or sets the SQL Server license type to the machine to in the event of a failover. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
machine to [](start = 60, length = 10)
Please correct the description, something like
Gets or sets the SQL Server license type of the machine in the event of a failover. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -1182,6 +1209,26 @@ public ASRInMageAzureV2SpecificRPIDetails(InMageAzureV2ReplicationDetails detail | |||
/// Gets or sets the proximity placement group Id for replication protected item after failover. | |||
/// </summary> | |||
public string RecoveryProximityPlacementGroupId { get; set; } | |||
|
|||
/// <summary> | |||
/// Gets or sets the SQL Server license type to the machine to in the event of a failover. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ts the SQL Server license type to the [](start = 22, length = 37)
Same as above #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. #Resolved
[Parameter(ParameterSetName = ASRParameterSets.HyperVSiteToAzure, HelpMessage = "Specify the SQL Server license type of the VM.")] | ||
[ValidateNotNullOrEmpty] | ||
[ValidateSet( | ||
Constants.NoLicenseType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NoLicenseType [](start = 22, length = 13)
Same as above #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SqlServerLicenseType = this.SqlServerLicenseType, | ||
TargetVmTags = this.RecoveryVmTag, | ||
TargetNicTags = this.RecoveryNicTag, | ||
SeedManagedDiskTags = this.DiskTag, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SeedManagedDiskTags = this.DiskTag [](start = 16, length = 34)
If RecoveryAzureStorageAccountId is not null or string.empty, then also we shd set this. Otherwise service will throw exception.
@anmolbhatia289 #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
providerSettings.TargetVmSize = this.Size; | ||
providerSettings.SqlServerLicenseType = this.SqlServerLicenseType; | ||
providerSettings.TargetVmTags = this.RecoveryVmTag; | ||
providerSettings.TargetManagedDiskTags = this.DiskTag; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
providerSettings.TargetManagedDiskTags = this.DiskTag; [](start = 12, length = 54)
Only if useManagedDisks is true else service will throw error.
Should we throw error in powershell or we want to surface the srs exception?? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Parameter] | ||
[ValidateNotNullOrEmpty] | ||
[ValidateSet( | ||
Constants.NoLicenseType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NoLicenseType [](start = 22, length = 13)
Same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to check on this once if we need to consider NotSpecified.
In reply to: 601549109 [](ancestors = 601549109)
var sqlServerLicenseType = this.SqlServerLicenseType; | ||
var recoveryVmTag = this.RecoveryVmTag; | ||
var recoveryNicTag = this.RecoveryNicTag; | ||
var diskTag = this.DiskTag; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DiskTag [](start = 35, length = 7)
What if for H2A useManagedDisk is false? SRS will throw exception so are we fine with that? or we want to throw from powershell #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TargetProximityPlacementGroupId = proximityPlacementGroupId, | ||
SqlServerLicenseType = sqlServerLicenseType, | ||
TargetVmTags = recoveryVmTag, | ||
TargetManagedDiskTags = diskTag, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
argetManagedDiskTags = diskTa [](start = 33, length = 29)
question as above #Resolved
$diskTag.Add("DiskTag1","powershellDisk") | ||
$nicTag = New-Object "System.Collections.Generic.Dictionary``2[System.String,System.String]" | ||
$nicTag.Add("NicTag1","powershellNic") | ||
$EnableDRjob = New-AsrReplicationProtectedItem -ProtectableItem $VM -Name $VM.Name -ProtectionContainerMapping $pcm -RecoveryAzureStorageAccountId $StorageAccountID -OSDiskName $VMName -OS Windows -RecoveryResourceGroupId $RecoveryResourceGroupId -RecoveryProximityPlacementGroupId $ppg -UseManagedDisk true -RecoveryAvailabilitySetId $avset -Size $size -SqlServerLicenseType $sqlLicenseType -RecoveryVmTag $vmTag -RecoveryNicTag $nicTag -DiskTag $diskTag -RecoveryAzureNetworkId $AzureVmNetworkId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AsrReplicationProtectedItem [](start = 23, length = 27)
In future we should wait for enable and IR to finish and check all these are coming fine, for now u can check in armclient #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now checked from portal. Will do it next release.
In reply to: 601630510 [](ancestors = 601630510)
$rpi = Get-AsrReplicationProtectedItem -ProtectionContainer $pc -FriendlyName $VMName | ||
|
||
$ppgSet="/subscriptions/b364ed8d-4279-4bf8-8fd1-56f8fa0ae05c/resourceGroups/Arpita-air/providers/Microsoft.Compute/proximityPlacementGroups/h2atestppgupdate" | ||
$currentJob = Set-AsrReplicationProtectedItem -InputObject $rpi -RecoveryProximityPlacementGroupId $ppgSet -UpdateNic $rpi.NicDetailsList[0].NicId -RecoveryNetworkId $AzureNetworkID -RecoveryNicSubnetName $subnet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$currentJob [](start = 4, length = 11)
Please set network else in future error will come
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have set the network during enable protection. So no need to do from here.
In reply to: 601631470 [](ancestors = 601631470)
$rpi = Get-AsrReplicationProtectedItem -ProtectionContainer $pc -FriendlyName $VMName | ||
Assert-NotNull($rpi.ProviderSpecificDetails.RecoveryProximityPlacementGroupId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did we remove, we should add check for value as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add this check. Will take care of it when I do add tests for V2A.
In reply to: 601631955 [](ancestors = 601631955)
$rpi = Get-AsrReplicationProtectedItem -ProtectionContainer $pc -FriendlyName $VMName | ||
Assert-NotNull($rpi.ProviderSpecificDetails.RecoveryProximityPlacementGroupId) | ||
Assert-NotNull($rpi.ProviderSpecificDetails.RecoveryVmTag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RecoveryVmTag [](start = 48, length = 13)
All assert, we should add check for value as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[-RecoveryAvailabilitySet <String>] [-RecoveryAvailabilityZone <String>] | ||
[-RecoveryProximityPlacementGroupId <String>] [-EnableAcceleratedNetworkingOnRecovery] | ||
[-RecoveryBootDiagStorageAccountId <String>] | ||
[-RecoveryAvailabilitySet <String>] [-SqlServerLicenseType <String>] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should update the description based on provider #Resolved
@@ -181,6 +184,21 @@ Accept pipeline input: False | |||
Accept wildcard characters: False | |||
``` | |||
|
|||
### -DiskTag | |||
Specify the tags for the disks of the VM. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please update the description so that user knows for what scenarios, this is applicable. Same for all tags #Resolved
@@ -501,6 +549,22 @@ Accept pipeline input: False | |||
Accept wildcard characters: False | |||
``` | |||
|
|||
### -SqlServerLicenseType | |||
Specify the SQL Server license type of the VM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VM [](start = 43, length = 2)
this is applicable to which providers #Resolved
@@ -864,6 +868,25 @@ public ASRHyperVReplicaAzureSpecificRPIDetails(HyperVReplicaAzureReplicationDeta | |||
/// </summary> | |||
public string RecoveryProximityPlacementGroupId { get; set; } | |||
|
|||
/// <summary> | |||
/// Gets or sets the SQL Server license type of the machine to in the event of a failover. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to [](start = 68, length = 2)
"to" we need to remove #Resolved
@@ -1182,6 +1209,26 @@ public ASRInMageAzureV2SpecificRPIDetails(InMageAzureV2ReplicationDetails detail | |||
/// Gets or sets the proximity placement group Id for replication protected item after failover. | |||
/// </summary> | |||
public string RecoveryProximityPlacementGroupId { get; set; } | |||
|
|||
/// <summary> | |||
/// Gets or sets the SQL Server license type of the machine to in the event of a failover. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to [](start = 68, length = 2)
"to" we need to remove #Resolved
} | ||
else | ||
{ | ||
providerSettings.TargetManagedDiskTags = this.DiskTag; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
providerSettings.TargetManagedDiskTags = this.DiskTag [](start = 20, length = 53)
we can assign irrespective of the check like
providerSettings.TargetManagedDiskTags = this.DiskTag;
if(this.UseManagedDisk == Constants.False && this.DiskTag != null && this.DiskTag.Count > 0)
{
throw new PSArgumentException(
string.Format(
Resources.DiskTagCannotBeSet,
this.DiskTag,
this.UseManagedDisk));
}
or u can move the error above #Resolved
{ | ||
if(diskTag != null || | ||
diskTag.Count > 0) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are checking effectively (this.DiskTag == null ||
this.DiskTag.Count == 0)
twice
something like
if (this.DiskTag == null ||
this.DiskTag.Count == 0)
{
diskTag = providerSpecificDetails.TargetManagedDiskTags;
}
else if(useManagedDisk == Constants.False)
{throw new PSArgumentException(
string.Format(
Resources.DiskTagCannotBeSet,
diskTag,
useManagedDisk));
} #Resolved
providerSettings.TargetVmTags = this.RecoveryVmTag; | ||
providerSettings.TargetNicTags = this.RecoveryNicTag; | ||
|
||
if(this.DiskTag != null || this.DiskTag.Count > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|| [](start = 36, length = 2)
&& #Resolved
Shall we have another constant for Sql License type ? In reply to: 807333660 [](ancestors = 807333660) Refers to: src/RecoveryServices/RecoveryServices.SiteRecovery/Models/PSConstants.cs:288 in 838a284. [](commit_id = 838a284, deletion_comment = False) |
this.DiskTag.Count == 0) | ||
{ | ||
diskTag = providerSpecificDetails.TargetManagedDiskTags; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} [](start = 20, length = 1)
V2A edit also we had to throw error right #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -513,4 +513,7 @@ Recommended Action: To cleanup the resources created by Test failover run the Te | |||
<data name="IPConfigNotFoundInVMNic" xml:space="preserve"> | |||
<value>IP Config "{0}" not found in VM NIC "{1}".</value> | |||
</data> | |||
<data name="DiskTagCannotBeSet" xml:space="preserve"> | |||
<value>Disk Tag "{0}" cannot be set if UseManagedDisk is "{1}".</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tag [](start = 16, length = 3)
Tag(s) #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Upgrade to new storage dataplane SDK * [Storage] Support dfs sas encryptionscope * Update DependencyAnalyzer.cs (#21) Co-authored-by: Dingmeng Xue <dixue@microsoft.com>
This PR includes changes for following for both V2A and H2A
TargetVmSize in enable protection,
AvSet in enable protection
Sql License type in enable, update and get APIs
ResourceTagging [VmTags, NicTags and DiskTags] in enable, update and get APIs
Design PRs:
Azure/azure-powershell-cmdlet-review-pr#879
Azure/azure-powershell-cmdlet-review-pr#888
Testing:
H2A is done.
V2A is done.
Added test cases and recordings for V2A and H2A