[SQL] 'az sql mi update`: Add` --tier `and` --family` parameters to change hw generation #11852

v-nestan · 2020-01-14T17:47:14Z

This change adds support for changing existing managed instance hardware generation.

TFS item link

History Notes:
[SQL] Update SQL Managed Instance cmdlet az sql mi update with two new parameters: tier and family.

This checklist is used to make sure that common guidelines for a pull request are followed.

The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).
I adhere to the Command Guidelines.

jsntcy · 2020-01-15T02:25:03Z

@Juliehzl, please help review it, thanks.

jaredmoo · 2020-01-15T02:39:51Z

src/azure-cli/azure/cli/command_modules/sql/custom.py

@@ -2134,6 +2134,12 @@ def _find_managed_instance_sku_from_capabilities(cli_ctx, location, sku):
    # Find family level capability, based on requested sku properties
    family_capability = _find_family_capability(sku, edition_capability.supported_families)

+    # Check whether hardware family is being changed to a deprecated hardware family
+    if not is_instance_create:
+        if current_sku_family == 'Gen5' and 'Gen4' in family_capability.sku:


Why is this being done client-side instead of server-side (MI provision operation authorizer)? These sorts of policy decisions should not be coded into CLI

The idea was to fast fail if a customer tries to change hardware family to a deprecated hardware family. Synced with PM, and we agreed to remove this check as you suggested. Thanks

jaredmoo · 2020-01-15T02:41:25Z

src/azure-cli/azure/cli/command_modules/sql/_params.py

+                'family',
+                'name',
+                'tier',
+            ])


Can you show the output of az sql mi create -h and az sql mi update -h, and make sure that sku-related params have same UX as az sql db create -h?

I've aligned them

az sql mi create -h

az sql mi update -h

az sql db create -h

Juliehzl · 2020-01-19T05:53:18Z

src/azure-cli/HISTORY.rst

@@ -74,6 +74,10 @@ Release History
 * Add new parameters `--enable-delete-retention` and `--delete-retention-days` to support managing delete retention policy for storage account blob-service-properties.


+**SQL**


Please rearrange the release note in the right position and use active voice for your commands

Juliehzl · 2020-01-19T05:54:11Z

src/azure-cli/azure/cli/command_modules/sql/_params.py

@@ -1156,7 +1156,8 @@ def _configure_security_policy_storage_params(arg_ctx):

        c.argument('tier',
                   arg_type=tier_param_type,
-                   help='The edition component of the sku. Allowed values: GeneralPurpose, BusinessCritical.')
+                   help='The edition component of the sku. Allowed values include:'


please use get_enum_types() for allowed values.

Done. I've done the same thing for family too

Juliehzl

@v-nestan please fix CI errors and if @jaredmoo agree, I am fine to merge it.

Juliehzl · 2020-01-19T05:57:57Z

src/azure-cli/azure/cli/command_modules/sql/_help.py

@@ -559,6 +559,8 @@
 examples:
  - name: Updates a mi with specified parameters and with identity
    text: az sql mi update -g mygroup -n myinstance -i -p mypassword --license-type mylicensetype --capacity vcorecapacity --storage storagesize
+  - name: Updates mi edition and hardware family


Updates -> Update

Juliehzl · 2020-01-19T05:59:06Z

src/azure-cli/azure/cli/command_modules/sql/_help.py

@@ -559,6 +559,8 @@
 examples:
  - name: Updates a mi with specified parameters and with identity
    text: az sql mi update -g mygroup -n myinstance -i -p mypassword --license-type mylicensetype --capacity vcorecapacity --storage storagesize
+  - name: Updates mi edition and hardware family
+    text: az sql mi update -g mygroup -n myinstance -tier GeneralPurpose -family Gen5


It should be --tier and --family.

Corrected. Thanks

Juliehzl · 2020-01-19T06:01:23Z

Please follow above template for your PR title in future.

Fixing _help.py Using get_enum_type() for allowed values

…/v-nestan/azure-cli into nemanjas/mi_change_hw_generation Merging with latest.

jaredmoo · 2020-01-27T23:12:57Z

src/azure-cli/azure/cli/command_modules/sql/_params.py

@@ -56,7 +56,9 @@
    ComputeModelType,
    DatabaseCapabilitiesAdditionalDetails,
    ElasticPoolCapabilitiesAdditionalDetails,
-    FailoverPolicyType
+    FailoverPolicyType,
+    ManagedInstanceTiers,


style nitpick: It should be singular, i.g. ManagedInstanceTier & ManagedInstanceFamily

I've removed those because they are not needed any more since I've removed get_enum_type()

jaredmoo · 2020-01-27T23:16:23Z

src/azure-cli/azure/cli/command_modules/sql/_params.py

-                   help='The compute generation component of the sku. '
-                   'Allowed values include: Gen4, Gen5.')
+                   arg_type=mi_family_param_type,
+                   help='The compute generation component of the sku.')


Does get_enum_type() throw error if you don't specify one of the allowed values? This will prevent forward compatibility (i.e. when we release Gen1234, customers will not be able to manually type it in until they download newer CLI version) - this has previously been a frustration for SQL DB customers. We should allow customer to type in anything, then if we validate it must be against a dynamic list (e.g. capabilities API) and NOT a hardcoded list.

I've checked now and it does throws an error. For example

az sql mi update --subscription "DS-CloudLifter_jovanc_R&D_60843" --resource-group 201tmpl --name nestan-testing109 --family Gen52
az sql mi update: 'Gen52' is not a valid value for '--family'. See 'az sql mi update --help'.

The most similar choice to 'Gen52' is:
Gen5

I'll remove the get_enum_type :)

Thanks for pointing this up

jaredmoo · 2020-01-27T23:28:12Z

src/azure-cli/azure/cli/command_modules/sql/custom.py

-        return sku
-
-    if not _any_sku_values_specified(sku):
+    if is_instance_create and not _any_sku_values_specified(sku):


Why is is_instance_create logic needed?

You are right it is redundant.
The idea was to protect from getting a default sku name when sku components are not entered during update SLO

However, since SKU for existing instance is populated before this method is called, default sku will not be taken for update SLO.

I'll remove it

jaredmoo · 2020-01-27T23:30:48Z

src/azure-cli/azure/cli/command_modules/sql/custom.py

        instance,
        administrator_login_password=None,
        license_type=None,
        vcores=None,
        storage_size_in_gb=None,
        assign_identity=False,
        proxy_override=None,
-        public_data_endpoint_enabled=None):
+        public_data_endpoint_enabled=None,
+        sku=None):


In the interest of alignment & reducing overall complexity, would it make sense to use same syntax as db_update function (i.e. split it up into components):

tier=None, family=None, capacity=None,

?

Just remembered that MI has vcores property, not capacity. Blergh

I'll replace sku with tier and family. I'll omit the third component of sku because it is not needed

jaredmoo · 2020-01-27T23:33:07Z

src/azure-cli/azure/cli/command_modules/sql/custom.py

@@ -2211,6 +2221,16 @@ def managed_instance_update(
    instance.proxy_override = (
        proxy_override or instance.proxy_override)



Should you set sku.name = None here?

I think it makes sense to null out the sku name here, because there is no requested sku name, and you don't yet know what the sku name will be (it will be found based on tier & family).

Yes, it makes sense to set it to None here

Removing redundant parameter `is_instance_create` Replacing `sku` with `tier` and `family` in `managed_instance_update` for consistency with db_update

yonzhan · 2020-01-28T08:30:06Z

add to S165.

petrajkogit · 2020-01-28T11:46:31Z

src/azure-cli/HISTORY.rst

@@ -51,6 +51,10 @@ Release History
 * `az storage remove`: Change `--inlcude` and `--exclude` parameters to `--include-path`, `--include-pattern`, `--exclude-path` and`--exclude-pattern` parameters
 * `az storage sync`: Add `--include-pattern`, `--exclude-path` and`--exclude-pattern` parameters

+**SQL**
+
+* Update az sql mi update with two new properties tier and family.


nit: please format this string little better. It sound weird: "Update az sql mi update". Also "properties: tier and family"

Done. Thanks

petrajkogit · 2020-01-28T12:01:23Z

src/azure-cli/azure/cli/command_modules/sql/custom.py

@@ -2098,21 +2098,18 @@ def encryption_protector_update(
 #                sql managed instance         #
 ###############################################

-


removing this line is causing validation to fail, please return it

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

src/azure-cli/azure/cli/command_modules/sql/custom.py

@@ -2211,6 +2211,16 @@ def managed_instance_update(
    instance.proxy_override = (
        proxy_override or instance.proxy_override)

+    instance.sku.name = None


tiny nit: consider commenting the reason why you do this, since it would not be obvious when first reading

…w_generation Syncing with the latest q

Juliehzl

LGTM in general. Please make CI pass.

v-nestan requested review from jaredmoo and Juliehzl as code owners January 14, 2020 17:47

jaredmoo reviewed Jan 15, 2020

View reviewed changes

jsntcy assigned v-nestan Jan 17, 2020

Juliehzl reviewed Jan 19, 2020

View reviewed changes

Juliehzl requested changes Jan 19, 2020

View reviewed changes

Juliehzl reviewed Jan 19, 2020

View reviewed changes

Juliehzl changed the title ~~Nemanjas/mi change hw generation~~ [SQL] 'az sql mi update: Add --tier and --family` parameters to change hw generation Jan 19, 2020

Nemanja added 3 commits January 27, 2020 12:35

Managed instance hardware generation change

525432a

Addressing comments

7ee769e

Fixing History.rst

c2f1037

Fixing _help.py Using get_enum_type() for allowed values

v-nestan force-pushed the nemanjas/mi_change_hw_generation branch from c055879 to c2f1037 Compare January 27, 2020 14:59

v-nestan and others added 8 commits January 27, 2020 16:36

Merge branch 'dev' into nemanjas/mi_change_hw_generation

1707762

Managed instance hardware generation change

5fd1147

Addressing comments

9868403

Fixing History.rst

3431753

Fixing _help.py Using get_enum_type() for allowed values

Merging with dev branch

098fbff

Merge branch 'nemanjas/mi_change_hw_generation' of https://github.com…

3e3b2b6

…/v-nestan/azure-cli into nemanjas/mi_change_hw_generation Merging with latest.

Fixing History.rst merge error

6a8664c

Removing duplicate line from History

dd22363

jaredmoo reviewed Jan 27, 2020

View reviewed changes

Removing get_enum_type()

e43b537

Removing redundant parameter `is_instance_create` Replacing `sku` with `tier` and `family` in `managed_instance_update` for consistency with db_update

yonzhan assigned Juliehzl Jan 28, 2020

yonzhan added this to the S164 milestone Jan 28, 2020

yonzhan modified the milestones: S164, S165 Jan 28, 2020

yonzhan requested a review from qianwens January 28, 2020 08:29

Fixing error with sku not defined

d2659bf

petrajkogit reviewed Jan 28, 2020

View reviewed changes

petrajkogit approved these changes Jan 28, 2020

View reviewed changes

petrajkogit reviewed Jan 28, 2020

View reviewed changes

jaredmoo reviewed Jan 28, 2020

View reviewed changes

jaredmoo approved these changes Jan 28, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/dev' into nemanjas/mi_change_h…

c3127c6

…w_generation Syncing with the latest q

Juliehzl approved these changes Jan 30, 2020

View reviewed changes

Nemanja and others added 4 commits January 30, 2020 07:58

Fixing style issue. Updating History file to be more clear.

52149a8

Fixing indenting issue in test_sql_commands.py

01b86ea

Updating History

1ecf059

Merge branch 'dev' into nemanjas/mi_change_hw_generation

ead4319

Juliehzl merged commit fd39d4b into Azure:dev Feb 9, 2020

		@@ -74,6 +74,10 @@ Release History
		* Add new parameters `--enable-delete-retention` and `--delete-retention-days` to support managing delete retention policy for storage account blob-service-properties.


		SQL

		@@ -2211,6 +2221,16 @@ def managed_instance_update(
		instance.proxy_override = (
		proxy_override or instance.proxy_override)

		@@ -2098,21 +2098,18 @@ def encryption_protector_update(
		# sql managed instance #
		###############################################

[SQL] 'az sql mi update: Add --tier and --family` parameters to change hw generation #11852

[SQL] 'az sql mi update: Add --tier and --family` parameters to change hw generation #11852

Conversation

v-nestan commented Jan 14, 2020 • edited by fengzhou-msft Loading

jsntcy commented Jan 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredmoo Jan 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Juliehzl Jan 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Juliehzl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Juliehzl commented Jan 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yonzhan commented Jan 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Juliehzl left a comment

Choose a reason for hiding this comment

[SQL] 'az sql mi update`: Add` --tier `and` --family` parameters to change hw generation #11852

[SQL] 'az sql mi update`: Add` --tier `and` --family` parameters to change hw generation #11852

v-nestan commented Jan 14, 2020 •

edited by fengzhou-msft

Loading

jaredmoo Jan 15, 2020 •

edited

Loading

Juliehzl Jan 19, 2020 •

edited

Loading