[VM] Allow VM creation with specialized VHD #2256

tjprescott · 2017-02-23T23:47:16Z

Fixes #1961. Adds a storage profile for unmanaged specialized VHD. Refactors existing logic to (hopefully) be easier to maintain.

tjprescott · 2017-02-24T00:22:50Z

@yugangw-msft the tests all pass on my machine, so that's mysterious. The logic is in place though if you could take a look. I'm not looking to merge this until sometime next week.

yugangw-msft

Overall this change looks very good to me. I suggest add a few unit tests on throwing exceptions when we detect invalid/unnecessary/conflicting arguments

yugangw-msft · 2017-02-24T22:57:55Z

src/command_modules/azure-cli-vm/azure/cli/command_modules/vm/_template_builder.py

@@ -254,7 +255,7 @@ def build_vm_resource(  # pylint: disable=too-many-locals
        image_reference=None, os_disk_name=None, custom_image_os_type=None,
        storage_caching=None, storage_sku=None,
        os_publisher=None, os_offer=None, os_sku=None, os_version=None, os_vhd_uri=None,
-        managed_os_disk=None, data_disk_sizes_gb=None, image_data_disks=None,
+        attach_os_disk=None, data_disk_sizes_gb=None, image_data_disks=None,


The current name looks like a bit like a boolean flag. Maybe os_disk_to_attach? Up to you though.

I just used the same name as the command's parameter.

yugangw-msft · 2017-02-25T18:50:51Z

src/command_modules/azure-cli-vm/azure/cli/command_modules/vm/_validators.py

+
+def _get_storage_profile_description(profile):
+    if profile == StorageProfile.SACustomImage:
+        return 'unmanaged disk from custom image'


suggest to tweak the word a bit. This option means 'use unmanaged disk when create the VM from unmanaged custom image'

yugangw-msft · 2017-02-25T18:58:41Z

src/command_modules/azure-cli-vm/azure/cli/command_modules/vm/_validators.py

+    if profile == StorageProfile.SACustomImage:
+        return 'unmanaged disk from custom image'
+    elif profile == StorageProfile.SAPirImage:
+        return 'unmanaged disk from PIR image'


PIR probably is an internal term, I am thinking we should just call Azure Marketplace images. I will get it clarified the first time on Monday

yugangw-msft · 2017-02-25T18:59:23Z

src/command_modules/azure-cli-vm/azure/cli/command_modules/vm/_validators.py

+    elif profile == StorageProfile.SAPirImage:
+        return 'unmanaged disk from PIR image'
+    elif profile == StorageProfile.SASpecializedOSDisk:
+        return 'unmanaged VHD OS disk'


suggest attach to existing unmanaged os disk

yugangw-msft · 2017-02-25T19:00:21Z

src/command_modules/azure-cli-vm/azure/cli/command_modules/vm/_validators.py

+    elif profile == StorageProfile.SASpecializedOSDisk:
+        return 'unmanaged VHD OS disk'
+    elif profile == StorageProfile.ManagedCustomImage:
+        return 'managed disk from custom image'


suggest create VM from managed custom image

yugangw-msft · 2017-02-25T19:15:38Z

src/command_modules/azure-cli-vm/azure/cli/command_modules/vm/_validators.py

+    if namespace.storage_profile == StorageProfile.ManagedPirImage:
+        required = ['image']
+        forbidden = ['os_type', 'attach_os_disk', 'storage_account',
+                     'storage_container_name', 'use_unmanaged_disk']


adding use_unmanaged_disk to the list will be a no-op since it is a valid argument we used to figure out the profile type. I assume you put here still to get the code simpler. If yes, then it is fine

Right. It's here more for documentation purposes. If in the future the logic to quickly guess the profile is removed, this would still remain accurate.

yugangw-msft · 2017-02-25T19:16:17Z

src/command_modules/azure-cli-vm/azure/cli/command_modules/vm/_validators.py

+    elif namespace.storage_profile == StorageProfile.ManagedSpecializedOSDisk:
+        required = ['os_type', 'attach_os_disk']
+        forbidden = ['os_disk_name', 'storage_caching', 'storage_account',
+                     'storage_container_name', 'use_unmanaged_disk']


same question on use_unmanaged_disk

i think storage_sku should be forbidden as well, same for the SASpecializedOSDisk option below

yugangw-msft · 2017-02-25T19:31:58Z

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

@@ -1424,9 +1424,9 @@ def create_vm(vm_name, resource_group_name, image=None,
              public_ip_address=None, public_ip_address_allocation='dynamic',
              public_ip_address_dns_name=None,
              os_disk_name=None, os_type=None, storage_account=None,
-              storage_caching='ReadWrite', storage_container_name='vhds',
+              storage_caching=None, storage_container_name=None,


Any reason we like to change this? I thought it is recommended by putting default value in a function's signature rather in param.py.

Yes, it is. The reason is because since these are in the list of "forbidden" parameters for certain profiles, then having a default means that these always throw. I toyed with using the "MarkSpecifiedAction" on these, but that always seemed like a hacky workaround (and it has demonstrably introduced bugs) and this was a simpler solution. Previously, we silently ignored these parameters when they were defaulted or specified, but we have said that we do not want to do that.

yugangw-msft · 2017-02-25T19:32:54Z

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

@@ -1546,6 +1551,9 @@ def create_vm(vm_name, resource_group_name, image=None,
    client = get_mgmt_service_client(ResourceManagementClient).deployments
    properties = DeploymentProperties(template=template, parameters={}, mode='incremental')
    if validate:
+        from pprint import pprint
+        import sys
+        pprint(template, sys.stderr)


Guess it is temp change you will remove later?

No, I actually put this in here just for you. 😄 You mentioned how pulling the template out of the --debug output was frustrating so I figured this would be more helpful. Thoughts?

Thanks! Then definitely keep. I suggest we use logger.warning for anything go to stderr.

I'll give it a try--just wasn't sure if pprint would play nicely with a logger warning.

yugangw-msft · 2017-02-25T19:33:05Z

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

@@ -1701,6 +1711,9 @@ def create_vmss(vmss_name, resource_group_name, image,
    client = get_mgmt_service_client(ResourceManagementClient).deployments
    properties = DeploymentProperties(template=template, parameters={}, mode='incremental')
    if validate:
+        from pprint import pprint
+        import sys
+        pprint(template, sys.stderr)


Super intentional 💯

derekbekoe · 2017-02-28T01:26:08Z

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

    register_cli_argument(scope, 'storage_sku', help='The sku of storage account to persist VM. By default, only Standard_LRS and Premium_LRS are allowed. Using with --use-unmanaged-disk, all are available.', arg_group='Storage', **enum_choice_list(SkuName))
-    register_cli_argument(scope, 'storage_container_name', help="Only applicable when use with '--use-unmanaged-disk'. Name of the storage container for the VM OS disk.", arg_group='Storage')
+    register_cli_argument(scope, 'storage_container_name', help="Only applicable when use with '--use-unmanaged-disk'. Name of the storage container for the VM OS disk. Default: vhds", arg_group='Storage')


Why not default='vhds'?

Because placing it here (or even as default in the register_cli_argument) results in the validator always failing because for N/A storage profiles, this has a value. There are a few ways to handle this:

have the validators inspect the namespace as it was provided and before defaults were applied. This is what I have always preferred but it would require infrastructure changes and got shot down.

Apply the "MarkSpecifiedAction" to these parameters. This appends "SET" to the value when it is supplied by the user but not when it is defaulted, which permits you to distinguish between the a default value and the user specifying the same value as the default. However, this has proven to introduce bugs and is basically a hacky workaround, so I don't want to do that. (I tried the implementation, it was messy, I removed it).

Make these None and then apply the default in-code and in the help text. While not ideal, it's the simplest fix. This is also the official guidance if a complex type is the default (for example a default dictionary or list) so it's not totally contrary to guidance.

Keep the defaults and don't validate. This means that if the user specifies a value that is N/A, it will be silently ignored. We've gone to great lengths to throw errors when users specify invalid parameter combinations, so, especially for our most complex command, this didn't seem like something to let slide.

Given those 4 options, I really prefer 1, but I' okay with (and chose) 3.

I agree we can go with 3. This is the trade-off which might only be limited on VM/VMSS create which have such large number of arguments

derekbekoe · 2017-02-28T01:33:57Z

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

 register_cli_argument('vm create', 'availability_set', help='Name or ID of an existing availability set to add the VM to. None by default.')
-register_cli_argument('vm create', 'managed_os_disk', options_list=('--attach-os-disk',), help='create VM by attaching to an existing managed OS disk', arg_group='Storage')
+register_cli_argument('vm create', 'storage_caching', help='Storage caching type for the VM OS disk. Default: ReadWrite.', arg_group='Storage', **enum_choice_list(['ReadWrite', 'ReadOnly']))


Same here. Why is default hard-coded in the help string?

tjprescott · 2017-02-28T23:32:54Z

@yugangw-msft, @derekbekoe comments addressed. Let me know what you think.

derekbekoe

LGTM

yugangw-msft

2 more feedback, the rest LGTM

yugangw-msft · 2017-02-28T23:36:30Z

src/command_modules/azure-cli-vm/azure/cli/command_modules/vm/_validators.py

+    # set default storage SKU if not provided and using an image based OS
+    if not namespace.storage_sku and namespace.storage_profile \
+        not in [StorageProfile.ManagedSpecializedOSDisk, StorageProfile.SASpecializedOSDisk]:
+        namespace.storage_sku = 'Standard_LRS'


The default one should be Premium_LRS for VM and Standard_LRS for VMSS. Yep, it was per discussion with CRP team 2 weeks ago

yugangw-msft · 2017-02-28T23:38:04Z

src/command_modules/azure-cli-vm/azure/cli/command_modules/vm/_vm_utils.py

+    logger = azlogging.get_az_logger(__name__)
+    io = StringIO()
+    pprint(template, io)
+    logger.warning(io.getvalue())


Just a question, can we do json.dumps( indent=2) and then give it to logger? Or `pprint' can format the text better?

I'll check it out and pick the better of the 2

Yep, json.dumps looks better. Also, I changed the logging level to info so you have to use --verbose to see this. I thought that was more appropriate than displaying it by default or requiring --debug.

yugangw-msft

There is a minor style check error you need to address before merge

codecov-io · 2017-03-01T17:50:59Z

Codecov Report

Merging #2256 into master will increase coverage by 0.03%.
The diff coverage is 61.87%.

@@            Coverage Diff             @@
##           master    #2256      +/-   ##
==========================================
+ Coverage   72.95%   72.99%   +0.03%     
==========================================
  Files         458      458              
  Lines       19499    19568      +69     
  Branches     2578     2765     +187     
==========================================
+ Hits        14226    14283      +57     
+ Misses       4390     4350      -40     
- Partials      883      935      +52

Impacted Files	Coverage Δ
...ure-cli-vm/azure/cli/command_modules/vm/_params.py	`95.97% <100%> (+0.02%)`	✅
...re-cli-vm/azure/cli/command_modules/vm/commands.py	`92.16% <100%> (+0.04%)`	✅
.../azure/cli/command_modules/vm/_template_builder.py	`84.14% <100%> (+0.09%)`	✅
...e-cli-vm/azure/cli/command_modules/vm/_vm_utils.py	`74.07% <16.66%> (-7.18%)`	❌
...zure-cli-vm/azure/cli/command_modules/vm/custom.py	`73.59% <58.33%> (-0.13%)`	❌
...cli-vm/azure/cli/command_modules/vm/_validators.py	`73.69% <60.9%> (+0.22%)`	✅
...li-role/azure/cli/command_modules/role/commands.py	`82.5% <0%> (ø)`	✅
...torage/azure/cli/command_modules/storage/custom.py	`87.5% <0%> (ø)`	✅
...etwork/azure/cli/command_modules/network/custom.py	`58.81% <0%> (ø)`	✅
...-storage/azure/cli/command_modules/storage/util.py	`19.23% <0%> (ø)`	✅
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f264f8...5863d44. Read the comment docs.

tjprescott added Compute az vm/vmss/image/disk/snapshot do-not-merge labels Feb 23, 2017

tjprescott added this to the Sprint 13 milestone Feb 23, 2017

tjprescott requested a review from yugangw-msft February 23, 2017 23:47

azurecla added the cla-not-required label Feb 23, 2017

tjprescott removed the do-not-merge label Feb 24, 2017

yugangw-msft reviewed Feb 25, 2017

View reviewed changes

derekbekoe reviewed Feb 28, 2017

View reviewed changes

tjprescott added 4 commits February 28, 2017 15:28

Enable specialized VHD. Refactor logic.

a86ca27

Re-re-record ACS test...

a296cec

Troubleshoot CI.

6a3212d

Address code review feedback.

6b7e0cc

derekbekoe approved these changes Feb 28, 2017

View reviewed changes

yugangw-msft reviewed Feb 28, 2017

View reviewed changes

Code review feedback.

5863d44

yugangw-msft approved these changes Mar 1, 2017

View reviewed changes

tjprescott merged commit 8fb6b56 into Azure:master Mar 1, 2017

tjprescott deleted the AttachNativeOSDisk branch March 1, 2017 17:59

[VM] Allow VM creation with specialized VHD #2256

[VM] Allow VM creation with specialized VHD #2256

Conversation

tjprescott commented Feb 23, 2017 • edited Loading

tjprescott commented Feb 24, 2017

yugangw-msft 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

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

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

tjprescott commented Feb 28, 2017

derekbekoe left a comment

Choose a reason for hiding this comment

yugangw-msft 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

Choose a reason for hiding this comment

yugangw-msft left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 1, 2017 • edited Loading

Codecov Report

tjprescott commented Feb 23, 2017 •

edited

Loading

codecov-io commented Mar 1, 2017 •

edited

Loading