Skip to content

[VM/VMSS] Improved disk caching support #2522

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.

Sign up for GitHub

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

Merged
tjprescott merged 2 commits into from
Mar 17, 2017
Merged

[VM/VMSS] Improved disk caching support #2522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
31f793e
Enable separate os/disk caching mechanisms.
tjprescott Mar 15, 2017
79cd649
Code review feedback.
tjprescott Mar 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions src/command_modules/azure-cli-vm/azure/cli/command_modules/vm/_params.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,9 @@ def get_vm_size_completion_list(prefix, action, parsed_args, **kwargs): # pylin
register_cli_argument('vm unmanaged-disk detach', 'disk_name', options_list=('--name', '-n'), help='The data disk name.')
register_cli_argument('vm unmanaged-disk', 'disk_size', help='Size of disk (GiB)', default=1023, type=int)
register_cli_argument('vm unmanaged-disk', 'new', action="store_true", help='create a new disk')
register_cli_argument('vm unmanaged-disk', 'lun', type=int, help='0-based logical unit number (LUN). Max value depends on the Virutal Machine size.')
register_cli_argument('vm unmanaged-disk', 'lun', type=int, help='0-based logical unit number (LUN). Max value depends on the Virtual Machine size.')
register_cli_argument('vm unmanaged-disk', 'vhd_uri', help="virtual hard disk's uri. For example:https://mystorage.blob.core.windows.net/vhds/d1.vhd")
register_cli_argument('vm unmanaged-disk', 'caching', help='Host caching policy', default=CachingTypes.none.value, **enum_choice_list(CachingTypes))
register_cli_argument('vm', 'caching', help='Disk caching policy', **enum_choice_list(CachingTypes))

for item in ['attach', 'detach']:
register_cli_argument('vm unmanaged-disk {}'.format(item), 'vm_name', arg_type=existing_vm_name, options_list=('--vm-name',), id_part=None)
Expand All @@ -83,7 +83,7 @@ def get_vm_size_completion_list(prefix, action, parsed_args, **kwargs): # pylin
register_cli_argument('vm disk', 'new', action="store_true", help='create a new disk')
register_cli_argument('vm disk', 'sku', arg_type=disk_sku)
register_cli_argument('vm disk', 'size_gb', options_list=('--size-gb', '-z'), help='size in GB.')
register_cli_argument('vm disk', 'lun', type=int, help='0-based logical unit number (LUN). Max value depends on the Virutal Machine size.')
register_cli_argument('vm disk', 'lun', type=int, help='0-based logical unit number (LUN). Max value depends on the Virtual Machine size.')

register_cli_argument('vm availability-set', 'availability_set_name', name_arg_type, id_part='name',
completer=get_resource_name_completion_list('Microsoft.Compute/availabilitySets'), help='Name of the availability set')
Expand Down Expand Up @@ -122,8 +122,9 @@ def get_vm_size_completion_list(prefix, action, parsed_args, **kwargs): # pylin
register_cli_argument('vmss', 'instance_id', id_part='child_name')
register_cli_argument('vmss', 'instance_ids', multi_ids_type, help='Space separated list of IDs (ex: 1 2 3 ...) or * for all instances. If not provided, the action will be applied on the scaleset itself')
register_cli_argument('vmss', 'tags', tags_type)
register_cli_argument('vmss', 'caching', help='Disk caching policy', **enum_choice_list(CachingTypes))

register_cli_argument('vmss disk', 'lun', type=int, help='0-based logical unit number (LUN). Max value depends on the Virutal Machine instance size.')
register_cli_argument('vmss disk', 'lun', type=int, help='0-based logical unit number (LUN). Max value depends on the Virtual Machine instance size.')
register_cli_argument('vmss disk', 'size_gb', options_list=('--size-gb', '-z'), help='size in GB.')
register_cli_argument('vmss disk', 'vmss_name', vmss_name_type, completer=get_resource_name_completion_list('Microsoft.Compute/virtualMachineScaleSets'))

Expand Down Expand Up @@ -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-disk-caching'], arg_group='Storage', help='Storage caching type for the VM OS disk.', **enum_choice_list([CachingTypes.read_only.value, CachingTypes.read_write.value]))
register_cli_argument(scope, 'data_caching', options_list=['--data-disk-caching'], arg_group='Storage', help='Storage caching type for the VM data disk(s).', **enum_choice_list(CachingTypes))

register_cli_argument('vm create', 'vm_name', name_arg_type, id_part=None, help='Name of the virtual machine.', validator=process_vm_create_namespace, completer=None)
register_cli_argument('vm create', 'attach_os_disk', help='Attach an existing OS disk to the VM. Can use the name or ID of a managed disk or the URI to an unmanaged disk VHD.')
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', 'storage_caching', help='Storage caching type for the VM OS disk. Default: ReadWrite.', arg_group='Storage', **enum_choice_list(['ReadWrite', 'ReadOnly']))

register_cli_argument('vmss create', 'vmss_name', name_arg_type, id_part=None, help='Name of the virtual machine scale set.', validator=process_vmss_create_namespace)
register_cli_argument('vmss create', 'load_balancer', help='Name to use when creating a new load balancer (default) or referencing an existing one. Can also reference an existing load balancer by ID or specify "" for none.', arg_group='Load Balancer')
Expand All @@ -237,7 +239,6 @@ def get_vm_size_completion_list(prefix, action, parsed_args, **kwargs): # pylin
register_cli_argument('vmss create', 'disable_overprovision', help='Overprovision option (see https://azure.microsoft.com/en-us/documentation/articles/virtual-machine-scale-sets-overview/ for details).', action='store_true')
register_cli_argument('vmss create', 'upgrade_policy_mode', help=None, **enum_choice_list(UpgradeMode))
register_cli_argument('vmss create', 'vm_sku', help='Size of VMs in the scale set. See https://azure.microsoft.com/en-us/pricing/details/virtual-machines/ for size info.')
register_cli_argument('vmss create', 'storage_caching', help='Storage caching type for the VM OS disk. Default: ReadOnly.', arg_group='Storage', **enum_choice_list(['ReadWrite', 'ReadOnly']))

register_cli_argument('vm encryption', 'volume_type', help='Type of volume that the encryption operation is performed on', **enum_choice_list(['DATA', 'OS', 'ALL']))
register_cli_argument('vm encryption', 'force', action='store_true', help='continue with encryption operations regardless of the warnings')
Expand Down
28 changes: 15 additions & 13 deletions src/command_modules/azure-cli-vm/azure/cli/command_modules/vm/_template_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

@yugangw-msft yugangw-msft Mar 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@tjprescott tjprescott Mar 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@yugangw-msft yugangw-msft Mar 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@gbowerman gbowerman Mar 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@tjprescott tjprescott Mar 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

os_publisher=None, os_offer=None, os_sku=None, os_version=None, os_vhd_uri=None,
attach_os_disk=None, data_disk_sizes_gb=None, image_data_disks=None,
custom_data=None, secrets=None):
Expand Down Expand Up @@ -296,7 +296,7 @@ def _build_storage_profile():
'osDisk': {
'createOption': 'fromImage',
'name': os_disk_name,
'caching': storage_caching,
'caching': os_caching,
'osType': custom_image_os_type,
'image': {'uri': image_reference},
'vhd': {'uri': os_vhd_uri}
Expand All @@ -306,7 +306,7 @@ def _build_storage_profile():
'osDisk': {
'createOption': 'fromImage',
'name': os_disk_name,
'caching': storage_caching,
'caching': os_caching,
'vhd': {'uri': os_vhd_uri}
},
'imageReference': {
Expand All @@ -328,7 +328,7 @@ def _build_storage_profile():
'osDisk': {
'createOption': 'fromImage',
'name': os_disk_name,
'caching': storage_caching,
'caching': os_caching,
'managedDisk': {'storageAccountType': storage_sku}
},
'imageReference': {
Expand All @@ -342,7 +342,7 @@ def _build_storage_profile():
'osDisk': {
'createOption': 'fromImage',
'name': os_disk_name,
'caching': storage_caching,
'caching': os_caching,
'managedDisk': {'storageAccountType': storage_sku}
},
"imageReference": {
Expand All @@ -361,7 +361,7 @@ def _build_storage_profile():
}
profile = storage_profiles[storage_profile.name]
return _build_data_disks(profile, data_disk_sizes_gb, image_data_disks,
storage_caching, storage_sku)
data_caching, storage_sku)

vm_properties = {
'hardwareProfile': {'vmSize': size},
Expand Down Expand Up @@ -389,21 +389,22 @@ def _build_storage_profile():


def _build_data_disks(profile, data_disk_sizes_gb, image_data_disks,
storage_caching, storage_sku):
data_caching, storage_sku):
if data_disk_sizes_gb is not None:
profile['dataDisks'] = []
for image_data_disk in image_data_disks or []:
profile['dataDisks'].append({
'lun': image_data_disk.lun,
'createOption': "fromImage",
'caching': data_caching
})
lun = max([d.lun for d in image_data_disks]) + 1 if image_data_disks else 0
for size in data_disk_sizes_gb:
profile['dataDisks'].append({
'lun': lun,
'createOption': "empty",
'diskSizeGB': int(size),
'caching': storage_caching,
'caching': data_caching,
Copy link
Contributor

@yugangw-msft yugangw-msft Mar 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'managedDisk': {'storageAccountType': storage_sku}
})
lun = lun + 1
Expand Down Expand Up @@ -498,7 +499,8 @@ def build_vmss_resource(name, naming_prefix, location, tags, overprovision, upgr
vm_sku, instance_count, ip_config_name, nic_name, subnet_id,
admin_username, authentication_type,
storage_profile, os_disk_name,
storage_caching, storage_sku, data_disk_sizes_gb, image_data_disks, os_type,
os_caching, data_caching, storage_sku, data_disk_sizes_gb,
image_data_disks, os_type,
image=None, admin_password=None, ssh_key_value=None, ssh_key_path=None,
os_publisher=None, os_offer=None, os_sku=None, os_version=None,
backend_address_pool_id=None, inbound_nat_pool_id=None,
Expand Down Expand Up @@ -526,7 +528,7 @@ def build_vmss_resource(name, naming_prefix, location, tags, overprovision, upgr
if storage_profile in [StorageProfile.SACustomImage, StorageProfile.SAPirImage]:
storage_properties['osDisk'] = {
'name': os_disk_name,
'caching': storage_caching,
'caching': os_caching,
'createOption': 'FromImage',
}

Expand All @@ -539,10 +541,10 @@ def build_vmss_resource(name, naming_prefix, location, tags, overprovision, upgr
})
else:
storage_properties['osDisk']['vhdContainers'] = "[variables('vhdContainers')]"
elif storage_profile in [StorageProfile.ManagedPirImage, StorageProfile.ManagedPirImage]:
elif storage_profile in [StorageProfile.ManagedPirImage, StorageProfile.ManagedCustomImage]:
storage_properties['osDisk'] = {
'createOption': 'FromImage',
'caching': storage_caching,
'caching': os_caching,
'managedDisk': {'storageAccountType': storage_sku}
}

Expand All @@ -559,7 +561,7 @@ def build_vmss_resource(name, naming_prefix, location, tags, overprovision, upgr
}

storage_profile = _build_data_disks(storage_properties, data_disk_sizes_gb,
image_data_disks, storage_caching,
image_data_disks, data_caching,
storage_sku)

# Build OS Profile
Expand Down
10 changes: 5 additions & 5 deletions src/command_modules/azure-cli-vm/azure/cli/command_modules/vm/_validators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ def _get_storage_profile_description(profile):

storage_profile_param_options = {
'os_disk_name': '--os-disk-name',
'storage_caching': '--storage-caching',
'os_caching': '--os-caching',
'os_type': '--os-type',
'attach_os_disk': '--attach-os-disk',
'image': '--image',
Expand Down Expand Up @@ -275,21 +275,21 @@ def _validate_vm_create_storage_profile(namespace, for_scale_set=False):

elif namespace.storage_profile == StorageProfile.ManagedSpecializedOSDisk:
required = ['os_type', 'attach_os_disk']
forbidden = ['os_disk_name', 'storage_caching', 'storage_account',
forbidden = ['os_disk_name', 'os_caching', 'storage_account',
'storage_container_name', 'use_unmanaged_disk', 'storage_sku']
_validate_managed_disk_sku(namespace.storage_sku)

elif namespace.storage_profile == StorageProfile.SAPirImage:
required = ['image', 'use_unmanaged_disk']
forbidden = ['os_type', 'attach_os_disk', 'data_disk_sizes_gb']
forbidden = ['os_type', 'data_caching', 'attach_os_disk', 'data_disk_sizes_gb']

elif namespace.storage_profile == StorageProfile.SACustomImage:
required = ['image', 'os_type', 'use_unmanaged_disk']
forbidden = ['attach_os_disk', 'data_disk_sizes_gb']
forbidden = ['attach_os_disk', 'data_caching', 'data_disk_sizes_gb']

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', 'data_caching', 'image', 'storage_account',
'storage_container_name', 'data_disk_sizes_gb', 'storage_sku']

else:
Expand Down
Loading