Nemanjas/mi change hw generation #10892

v-nestan · 2020-01-14T13:28:56Z

Description

This change adds support for changing existing managed instance hardware generation.
It can be changed using SkuName and ComputeGeneration parameters.

TFS item link

Checklist

I have read the Submitting Changes section of CONTRIBUTING.md
The title of the PR is clear and informative
The appropriate ChangeLog.md file(s) has been updated:
- For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
- A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
The PR does not introduce breaking changes
If applicable, the changes made in the PR have proper test coverage
For public API changes to cmdlets:
- a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
- the markdown help files have been regenerated using the commands listed here

NMijat1024 · 2020-01-14T17:08:23Z

src/Sql/Sql/ChangeLog.md

@@ -27,6 +27,7 @@
 ## Version 2.1.1
 * Update references in .psd1 to use relative path
 * Upgraded storage creation in Vulnerability Assessment auto enablement to StorageV2
+* Adding support for changing existing managed instance hardware generation


Sql Managed Instance

NMijat1024 · 2020-01-14T17:17:05Z

src/Sql/Sql/ManagedInstance/Cmdlet/SetAzureSqlManagedInstance.cs

+        /// </summary>
+        [Parameter(Mandatory = false,
+            HelpMessage = "The compute generation for the instance.")]
+        [ValidateNotNullOrEmpty]


Is it possible to add custom validators as attributes above fields?

I've checked SQL DB, instance pools and managed instance new and set cmdlets and couldn't find the example of that so I decided to go this way. If it is possible, it is not common.

AFAIK there is a completer for resource group name, if that exists then it must just be an ARM call so you could also call capabilities to find edition, sku name, compute gen, etc. So probably possible. But let's not block release on it :)

stefanb995 · 2020-01-14T17:46:34Z

src/Sql/Sql/ManagedInstance/Cmdlet/SetAzureSqlManagedInstance.cs

@@ -246,6 +420,7 @@ protected override IEnumerable<AzureSqlManagedInstanceModel> PersistChanges(IEnu
        /// </summary>
        public override void ExecuteCmdlet()
        {
+            // System.Diagnostics.Debugger.Launch();


Remove this

Leftover from debugging. Removed.

stefanb995 · 2020-01-14T17:47:54Z

src/Sql/Sql/ManagedInstance/Cmdlet/SetAzureSqlManagedInstance.cs

+        {
+            ModelAdapter = InitModelAdapter();
+            AzureSqlManagedInstanceModel existingInstance = ModelAdapter.GetManagedInstance(this.ResourceGroupName, this.Name);
+            Management.Internal.Resources.Models.Sku Sku = new Management.Internal.Resources.Models.Sku();


Great catch. Thanks. Fixed

stefanb995 · 2020-01-14T17:48:23Z

src/Sql/Sql/ManagedInstance/Cmdlet/SetAzureSqlManagedInstance.cs

+
+            ModelAdapter = InitModelAdapter();
+            AzureSqlManagedInstanceModel existingInstance = ModelAdapter.GetManagedInstance(this.ResourceGroupName, this.Name);
+            Management.Internal.Resources.Models.Sku Sku = new Management.Internal.Resources.Models.Sku();


stefanb995 · 2020-01-14T17:49:32Z

src/Sql/Sql/help/Set-AzSqlInstance.md

@@ -290,6 +325,21 @@ Accept pipeline input: True (ByPropertyName)
 Accept wildcard characters: False
 ```

+### -SkuName
+The SKU name for the instance e.g. 'BC_Gen4'.


As Gen4 is becoming deprecated, maybe it's better to have BC_Gen5 here

It is much better. Fixed

stefanb995 · 2020-01-14T17:52:01Z

src/Sql/Sql/ManagedInstance/Cmdlet/SetAzureSqlManagedInstance.cs

+        /// Gets or sets the instance SKU name
+        /// </summary>
+        [Parameter(Mandatory = false,
+            HelpMessage = "The SKU name for the instance e.g. 'GP_Gen4','BC_Gen4'.")]


I suggest adding 'GP_Gen5','BC_Gen5' in help message as Gen4 is becoming deprecated

Nice catch. Fixed

stefanb995 · 2020-01-14T18:04:55Z

src/Sql/Sql/ManagedInstance/Cmdlet/SetAzureSqlManagedInstance.cs

-                string computeGeneration = existingInstance.Sku.Name.Contains(Constants.ComputeGenerationGen4) ? Constants.ComputeGenerationGen4 : Constants.ComputeGenerationGen5;
-                string editionShort = AzureSqlManagedInstanceAdapter.GetInstanceSkuPrefix(Edition);
-                Sku.Name = editionShort + "_" + computeGeneration;
-                Sku.Tier = Edition;
+                Sku.Name = SkuName;


Why not populating Sku.Tier and Sku.Family here?

I've omitted for brevity because Sku.Name is enough for upsert

If you need to explain in PR, it's worth adding a code comment :)

Nice tip. Thanks for that. Adding comment

stefanb995 · 2020-01-14T18:05:30Z

src/Sql/Sql/ManagedInstance/Cmdlet/SetAzureSqlManagedInstance.cs

+            string currentEdition = existingInstance.Sku.Tier;
+
+            // Get current hardware family
+            string currentComputeGeneration = existingInstance.Sku.Name.Contains(Constants.ComputeGenerationGen4) ? Constants.ComputeGenerationGen4 : Constants.ComputeGenerationGen5;


Are these always populated for existing managed instances?

When getting managed instance, Sku.Name is always populated

jaredmoo · 2020-01-15T07:38:18Z

src/Sql/Sql/ManagedInstance/Cmdlet/SetAzureSqlManagedInstance.cs

+                {
+                    throw new PSArgumentException(
+                        string.Format(Microsoft.Azure.Commands.Sql.Properties.Resources.InstancePoolInstanceCannotChangeHardwareFamily, this.Name, existingInstance.InstancePoolName),
+                        "Edition");


Why all these validations on client side? Isn't server side doing the same validation? Having business logic on the client side is generally a bad idea because PowerShell/CLI is installed on user's machine, so they need to download updates when our policies change - this is really bad experience.

Removed all validation except prompting from confirmation that the changing hardware family is irreversible.

jaredmoo · 2020-01-15T07:41:21Z

src/Sql/Sql/ManagedInstance/Cmdlet/SetAzureSqlManagedInstance.cs

+
+            string edition = null;
+
+            if (skuName.Equals(Constants.GeneralPurposeGen4, StringComparison.InvariantCultureIgnoreCase)


This hardcoding of sku names / edition names on client side will add maintenance cost every time a new edition / gen is released. I think we need a different strategy here. For example, in CLI code we use capabilities API to find SKU based on requested parameters. Alternatively, perhaps you can reuse some of the existing edition/sku handling logic from db/elastic pool.

Not needed any more since SkuName parameter is removed.

…to nemanjas/mi_change_hw_generation Sync with latest

Adding unit test

…e_hw_generation Merging to the latest.

jaredmoo · 2020-01-28T22:10:40Z

src/Sql/Sql.Test/UnitTests/AzureSqlInstanceUnitTests.cs

+
+        [Fact]
+        [Trait(Category.AcceptanceType, Category.CheckIn)]
+        public void GetHardwareGenerationFromSkuName()


Nice to see!

jaredmoo · 2020-01-28T22:12:41Z

src/Sql/Sql/ManagedInstance/Cmdlet/SetAzureSqlManagedInstance.cs

+            HelpMessage = "The SKU name for the instance e.g. 'GP_Gen5','BC_Gen5'.")]
+        [ValidateNotNullOrEmpty]
+        [PSArgumentCompleter(Constants.GeneralPurposeGen5, Constants.BusinessCriticalGen5)]
+        public string SkuName { get; set; }


I think this should be a different parameter set than compute&family params.

As we discussed offline, we will remove SkuName from parameters.

jaredmoo · 2020-01-28T22:16:32Z

src/Sql/Sql/ManagedInstance/Cmdlet/SetAzureSqlManagedInstance.cs

+        /// </summary>
+        [Parameter(Mandatory = false,
+            HelpMessage = "The compute generation for the instance.")]
+        [ValidateNotNullOrEmpty]


AFAIK there is a completer for resource group name, if that exists then it must just be an ARM call so you could also call capabilities to find edition, sku name, compute gen, etc. So probably possible. But let's not block release on it :)

jaredmoo · 2020-01-28T22:17:09Z

src/Sql/Sql/ManagedInstance/Cmdlet/SetAzureSqlManagedInstance.cs

+
+            bool isHardwareFamilyChanged = !string.IsNullOrWhiteSpace(requestedHardwareFamily) && !currentHardwareFamily.Equals(requestedHardwareFamily, StringComparison.InvariantCultureIgnoreCase);
+
+            // Check whether hardware family is being changed to a deprecated hardware family


Don't you mean, check whether it's being changed away from a deprecated family?

I think this code is wrong. Instead of checking if requested == Gen5, it should be checking if current == Gen4. Moving Gen7->Gen5 shouldn't require confirmation, right?

You are absolutely correct. Brilliant catch. Fixing this

jaredmoo · 2020-01-28T22:22:08Z

src/Sql/Sql/ManagedInstance/Cmdlet/SetAzureSqlManagedInstance.cs

+            string edition = null;
+
+            if (skuName.Equals(Constants.GeneralPurposeGen4, StringComparison.InvariantCultureIgnoreCase)
+                || skuName.Equals(Constants.GeneralPurposeGen5, StringComparison.InvariantCultureIgnoreCase))


Can you just check StartsWith("GP_") or StartsWith("BC_"), instead of having every combination programmed ?

Even better would be if you implemented a function which is the opposite of GetInstanceSkuPrefix()

Actually, this method is also redundant. I'll remove it

jaredmoo · 2020-01-28T22:22:59Z

src/Sql/Sql/ManagedInstance/Cmdlet/SetAzureSqlManagedInstance.cs

+            string currentEdition = existingInstance.Sku.Tier;
+
+            // Get current hardware family
+            string currentComputeGeneration = existingInstance.Sku.Name.Contains(Constants.ComputeGenerationGen4) ? Constants.ComputeGenerationGen4 : Constants.ComputeGenerationGen5;


This looks like it would cause problems when next compute gen is released. Please distinguish between Gen4, Gen5, and "unknown"

I will use SqlSkuUtils.GetHardwareGenerationFromSkuName instead this if.

jaredmoo · 2020-01-28T22:23:37Z

src/Sql/Sql/ManagedInstance/Cmdlet/SetAzureSqlManagedInstance.cs

-                string computeGeneration = existingInstance.Sku.Name.Contains(Constants.ComputeGenerationGen4) ? Constants.ComputeGenerationGen4 : Constants.ComputeGenerationGen5;
-                string editionShort = AzureSqlManagedInstanceAdapter.GetInstanceSkuPrefix(Edition);
-                Sku.Name = editionShort + "_" + computeGeneration;
-                Sku.Tier = Edition;
+                Sku.Name = SkuName;


If you need to explain in PR, it's worth adding a code comment :)

jaredmoo · 2020-01-28T22:27:04Z

src/Sql/Sql/ManagedInstance/Services/AzureSqlManagedInstanceAdapter.cs

+        /// Get hardware family from SKU name.
+        /// </summary>
+        /// <returns>The model to send to the update</returns>
+        public static string GetHardwareGenerationFromSkuName(string skuName)


consider moving to SqlSkuUtils.cs

Note that, in order to run unit tests, SqlSkuUtils had to be changed to public

jaredmoo · 2020-01-28T22:27:35Z

src/Sql/Sql/Properties/Resources.resx

+    <value>Do you want to proceed?</value>
+  </data>
+  <data name="ChangingHardwareFamilyIsIrreversable" xml:space="preserve">
+    <value>For this update operation provided compute generation parameter is Gen5. Once the hardware generation is upgraded, it is not possible to switch back to Gen4 compute generation as this hardware is being deprecated.</value>


This message will not be accurate when next gen is released and you upgrade Gen4->Gen7

Updated to fill it in dinamically

jaredmoo · 2020-01-28T22:28:11Z

src/Sql/Sql/Properties/Resources.resx

+    <value>You cannot change hardware family.</value>
+  </data>
+  <data name="InvalidManagedInstanceSkuName" xml:space="preserve">
+    <value>Provided SkuName '{0}' is invalid '{1}'. Please enter a valid SkuName value and execute command again.</value>


It would be nice to give example sku name so that customer has an better idea of what we're asking for

This message is no longer needed. Removing it.

…e_hw_generation

isra-fel · 2020-02-03T05:57:12Z

/azp run

azure-pipelines · 2020-02-03T05:57:27Z

Azure Pipelines successfully started running 3 pipeline(s).

…e_hw_generation

isra-fel · 2020-02-04T02:35:48Z

/azp run

azure-pipelines · 2020-02-04T02:36:04Z

Azure Pipelines successfully started running 3 pipeline(s).

isra-fel · 2020-02-04T02:39:53Z

src/Sql/Sql/ChangeLog.md

@@ -31,6 +31,7 @@ Fix New-AzSqlDatabaseSecondary cmdlet to check for PartnerDatabaseName existence
 ## Version 2.1.1
 * Update references in .psd1 to use relative path
 * Upgraded storage creation in Vulnerability Assessment auto enablement to StorageV2
+* Adding support for changing existing Sql Managed Instance hardware generation


Changes for the upcoming release should go under the section titled "Upcoming Release".

isra-fel · 2020-02-04T02:42:12Z

Hi @v-nestan please fix the problem in Changelog.md . Besides, in order for CI to be triggered, please join Azure orgnization: https://aka.ms/AzureGithub thanks

…e_hw_generation

isra-fel · 2020-02-04T07:36:52Z

/azp run

azure-pipelines · 2020-02-04T07:37:11Z

Azure Pipelines successfully started running 3 pipeline(s).

…neration Nemanjas/mi change hw generation

Nemanja added 2 commits January 14, 2020 11:50

Adding support for changing managed instance hardware generation.

e3c71ca

Updating ChangeLog.md

0e29647

v-nestan requested review from jaredmoo, petrajkogit, SanjaMalesevic and NMijat1024 January 14, 2020 17:00

NMijat1024 reviewed Jan 14, 2020

View reviewed changes

v-nestan requested a review from cormacpayne January 14, 2020 17:40

stefanb995 reviewed Jan 14, 2020

View reviewed changes

jaredmoo reviewed Jan 15, 2020

View reviewed changes

isra-fel self-assigned this Jan 15, 2020

isra-fel added the needs-review label Jan 15, 2020

Nemanja added 2 commits January 15, 2020 18:58

Merge branch 'master' of https://github.com/Azure/azure-powershell in…

d234f53

…to nemanjas/mi_change_hw_generation Sync with latest

Addressing comments

24f5055

Adding unit test

isra-fel assigned v-nestan and unassigned isra-fel Jan 20, 2020

isra-fel added needs-revision and removed needs-review labels Jan 20, 2020

Nemanja added 2 commits January 28, 2020 10:21

Adding new recordings for three tests.

642f1e8

Merge remote-tracking branch 'upstream/master' into nemanjas/mi_chang…

54a7579

…e_hw_generation Merging to the latest.

jaredmoo reviewed Jan 28, 2020

View reviewed changes

Nemanja added 2 commits January 31, 2020 10:40

Merge remote-tracking branch 'upstream/master' into nemanjas/mi_chang…

e221055

…e_hw_generation

Removing SKU name

87d0440

jaredmoo previously approved these changes Jan 31, 2020

View reviewed changes

isra-fel added needs-review and removed needs-revision labels Feb 3, 2020

isra-fel assigned isra-fel and unassigned v-nestan Feb 3, 2020

Nemanja added 2 commits February 3, 2020 18:42

Merge remote-tracking branch 'upstream/master' into nemanjas/mi_chang…

7aa1970

…e_hw_generation

Adding new session records

95456cb

v-nestan dismissed jaredmoo’s stale review via 95456cb February 3, 2020 17:44

jaredmoo previously approved these changes Feb 3, 2020

View reviewed changes

isra-fel approved these changes Feb 4, 2020

View reviewed changes

isra-fel added Waiting for CI and removed needs-review labels Feb 4, 2020

isra-fel requested changes Feb 4, 2020

View reviewed changes

isra-fel added needs-revision and removed Waiting for CI labels Feb 4, 2020

isra-fel assigned v-nestan and unassigned isra-fel Feb 4, 2020

Nemanja added 2 commits February 4, 2020 08:07

Merge remote-tracking branch 'upstream/master' into nemanjas/mi_chang…

9fd3678

…e_hw_generation

Moving ChangeLog.md entry to Upcoming release.

2c77f92

v-nestan dismissed jaredmoo’s stale review via 2c77f92 February 4, 2020 07:09

isra-fel approved these changes Feb 4, 2020

View reviewed changes

isra-fel merged commit 940d05c into Azure:master Feb 4, 2020

dingmeng-xue pushed a commit to dingmeng-xue/azure-powershell that referenced this pull request Mar 29, 2020

Merge pull request Azure#10892 from v-nestan/nemanjas/mi_change_hw_ge…

d1ba239

…neration Nemanjas/mi change hw generation


		string edition = null;

		if (skuName.Equals(Constants.GeneralPurposeGen4, StringComparison.InvariantCultureIgnoreCase)


		bool isHardwareFamilyChanged = !string.IsNullOrWhiteSpace(requestedHardwareFamily) && !currentHardwareFamily.Equals(requestedHardwareFamily, StringComparison.InvariantCultureIgnoreCase);

		// Check whether hardware family is being changed to a deprecated hardware family

Nemanjas/mi change hw generation #10892

Nemanjas/mi change hw generation #10892

Uh oh!

Conversation

v-nestan commented Jan 14, 2020

Description

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

v-nestan Jan 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

v-nestan Jan 15, 2020 •

edited

Loading