Skip to content
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

[VM/VMSS] Improved disk caching support #2522

Merged
merged 2 commits into from Mar 17, 2017

Conversation

@tjprescott
Copy link
Member

commented Mar 15, 2017

Closes #2497.

  • VMSS create no longer adds template variables regarding storage account unless storage accounts will be created. While these didn't do anything before if storage accounts weren't created, it was confusing when you would view the template.
  • Adds an alias to vm/vmss create for OS disk caching. The existing --storage-caching exists alongside --os-caching. The intent is that --storage-caching will be deprecated and removed in the future. (Issue #2523)
  • Adds --data-caching to vm/vmss create which is the caching type used for data disks.
  • Adds --caching to vm disk/vmss disk/vm unmanaged-disk attach command.
  • Reflects on the SDK's CachingType object to expose all caching types (including None). Due to a bug in the Swagger (Azure/azure-rest-api-specs#1044), I manually omit "None" from the list of OS Disk caching types.
  • Removes the hard coded default values for caching and allows the service to apply defaults if not specified. For VMSS, which defaults to a supposedly invalid caching value of "None", I set the default to "ReadWrite" to match VM create.

@tjprescott tjprescott added the Compute label Mar 15, 2017

@tjprescott tjprescott added this to the Sprint 14 milestone Mar 15, 2017

@tjprescott tjprescott requested a review from yugangw-msft Mar 15, 2017

@tjprescott tjprescott force-pushed the tjprescott:SupportBigOlVMSS branch from aa2e3b3 to 2e8e7b8 Mar 15, 2017

@codecov-io

This comment has been minimized.

Copy link

commented Mar 15, 2017

Codecov Report

Merging #2522 into master will increase coverage by <.01%.
The diff coverage is 89.47%.

@@            Coverage Diff            @@
##           master   #2522      +/-   ##
=========================================
+ Coverage    72.1%   72.1%   +<.01%     
=========================================
  Files         362     362              
  Lines       19790   19791       +1     
  Branches     2920    2920              
=========================================
+ Hits        14269   14270       +1     
  Misses       4600    4600              
  Partials      921     921
Impacted Files Coverage Δ
.../azure/cli/command_modules/vm/_template_builder.py 80.95% <0%> (ø)
...ure-cli-vm/azure/cli/command_modules/vm/_params.py 96.05% <100%> (+0.01%)
...zure-cli-vm/azure/cli/command_modules/vm/custom.py 73.82% <100%> (ø)
...cli-vm/azure/cli/command_modules/vm/_validators.py 72.17% <75%> (ø)

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 9a31807...79cd649. Read the comment docs.

@yugangw-msft
Copy link
Contributor

left a comment

LGTM to overall. Left a few suggestions

@@ -253,7 +253,7 @@ def build_vm_resource( # pylint: disable=too-many-locals
name, location, tags, size, storage_profile, nics, admin_username,
availability_set_id=None, admin_password=None, ssh_key_value=None, ssh_key_path=None,
image_reference=None, os_disk_name=None, custom_image_os_type=None,
storage_caching=None, storage_sku=None,
os_caching=None, data_caching=None, storage_sku=None,

This comment has been minimized.

Copy link
@yugangw-msft

yugangw-msft Mar 16, 2017

Contributor

I suggest exposing --os-disk-caching and --data-disk-caching. Were this PR out last month, these short names look pretty good; but now with customdata and secret support, it is necessary to be a bit complete to avoid potential confusions.

This comment has been minimized.

Copy link
@tjprescott

tjprescott Mar 16, 2017

Author Member

I prefer to keep this short. It is consistent with --os-type (which refers to a disk) and is in the storage argument group, so I think it's pretty clear.

This comment has been minimized.

Copy link
@yugangw-msft

yugangw-msft Mar 17, 2017

Contributor

With help it will be clear, but there is also a scenario that when you read existing commands, the meaning should be clear w/o referring to help.
There are lots and lots of caching mechanisms in the whole stack from CPU cache to the web page cache at the top, so when I talk about cache, I have to be very specific on the term and context, otherwise people would not understand. This is why I am not quite into the argument naming of --os-caching, and --data-cache, because they could mean lots of different things.
However, I would not let my own experience/knowledge to determine the naming for wide audience. So /cc: @gbowerman @gatneil, if they are fine with this short naming, I will be fine too.

This comment has been minimized.

Copy link
@gbowerman

gbowerman Mar 17, 2017

Member

Although longer, we probably need 'disk' in the names for clarity, e.g. --os-disk-caching, --data-disk-caching, or --os-disk-cache, --data-disk-cache.

This comment has been minimized.

Copy link
@tjprescott

tjprescott Mar 17, 2017

Author Member

done

@@ -403,7 +403,7 @@ def _build_data_disks(profile, data_disk_sizes_gb, image_data_disks,
'lun': lun,
'createOption': "empty",
'diskSizeGB': int(size),
'caching': storage_caching,
'caching': data_caching,

This comment has been minimized.

Copy link
@yugangw-msft

yugangw-msft Mar 16, 2017

Contributor

Please set the caching on data disks coming with the managed image itself. The code is about 10 lines above

@@ -542,7 +543,7 @@ def build_vmss_resource(name, naming_prefix, location, tags, overprovision, upgr
elif storage_profile in [StorageProfile.ManagedPirImage, StorageProfile.ManagedPirImage]:

This comment has been minimized.

Copy link
@yugangw-msft

yugangw-msft Mar 16, 2017

Contributor

since we are here, please fix it to be [StorageProfile.ManagedPirImage, StorageProfile.ManagedCustomImage]

@@ -289,7 +289,7 @@ def _validate_vm_create_storage_profile(namespace, for_scale_set=False):

elif namespace.storage_profile == StorageProfile.SASpecializedOSDisk:
required = ['os_type', 'attach_os_disk', 'use_unmanaged_disk']
forbidden = ['os_disk_name', 'storage_caching', 'image', 'storage_account',
forbidden = ['os_disk_name', 'os_caching', 'image', 'storage_account',

This comment has been minimized.

Copy link
@yugangw-msft

yugangw-msft Mar 16, 2017

Contributor

i think for any unmanaged storage profiles, the data_caching would not apply, hence we should error

@@ -222,11 +223,12 @@ def get_vm_size_completion_list(prefix, action, parsed_args, **kwargs): # pylin
register_cli_argument(scope, 'public_ip_address_allocation', help=None, arg_group='Network', **enum_choice_list(['dynamic', 'static']))
register_cli_argument(scope, 'public_ip_address_dns_name', help='Globally unique DNS name for a newly created Public IP.', arg_group='Network')
register_cli_argument(scope, 'secrets', multi_ids_type, help='One or many Key Vault secrets as JSON strings or files via \'@<file path>\' containing \'[{ "sourceVault": { "id": "value" }, "vaultCertificates": [{ "certificateUrl": "value", "certificateStore": "cert store name (only on windows)"}] }]\'', type=file_type, completer=FilesCompleter())
register_cli_argument(scope, 'os_caching', options_list=['--storage-caching', '--os-caching'], arg_group='Storage', help='Storage caching type for the VM OS disk.', **enum_choice_list(CachingTypes))

This comment has been minimized.

Copy link
@yugangw-msft

yugangw-msft Mar 16, 2017

Contributor

My understanding is we should set Read/Write to OS Disk for VMSS, as the service default as None is not favorable

This comment has been minimized.

Copy link
@tjprescott

tjprescott Mar 16, 2017

Author Member

Yes, the latest email says it should be whatever VM is, which is ReadWrite.

This comment has been minimized.

Copy link
@tjprescott

tjprescott Mar 16, 2017

Author Member

Also, I suppressed "None" from the enum for OS caching.

@tjprescott tjprescott force-pushed the tjprescott:SupportBigOlVMSS branch from 51fc58e to 2df18bc Mar 16, 2017

@tjprescott tjprescott force-pushed the tjprescott:SupportBigOlVMSS branch from 2df18bc to 79cd649 Mar 17, 2017

@tjprescott

This comment has been minimized.

Copy link
Member Author

commented Mar 17, 2017

@yugangw-msft changes made

@tjprescott tjprescott merged commit b9c09d2 into Azure:master Mar 17, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tjprescott tjprescott deleted the tjprescott:SupportBigOlVMSS branch Mar 17, 2017

thegalah pushed a commit to thegalah/azure-cli that referenced this pull request Mar 21, 2017
Matthew Duong
Merge remote-tracking branch 'Azure/master'
* Azure/master: (478 commits)
  vm live test: allow more valid power states on vmss test verifications (Azure#2564)
  rbac:catch more graph error (Azure#2567)
  appservice: support to create plan when create a webapp (Azure#2550)
  Update storage tests (Azure#2556)
  Change PEP8 check filter from whitelist to blacklist (Azure#2557)
  Add scenario tests documentation (Azure#2555)
  [ACS] Adding support for configuring a default ACS cluster (Azure#2554)
  [ACS] Provide a short name alias for the orchestrator type flag (Azure#2553)
  Sql Import/Export CLI commands and test (Azure#2538)
  Fix format bug. (Azure#2549)
  [VM/VMSS] Improved disk caching support (Azure#2522)
  VM/VMSS: incorporate credentials validation logic used by portal (Azure#2537)
  Script that creates packaged releases package archive (Azure#2508)
  Adding alias for defaults flag (Azure#2540)
  Add wait commands and --no-wait support (Azure#2524)
  choice list outside of named arguments (Azure#2521)
  Fixed test failure in test_sql_db_mgmt. (Azure#2530)
  core: support login using service principal with a cert (Azure#2457)
  Add note about being in preview (Azure#2512)
  vm:fix distro check mechanism used by disk encryption (Azure#2511)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.