Azure / azure-sdk-for-python

This repository is for active development of the Azure SDK for Python. For consumers of the SDK we recommend visiting our public developer docs at https://learn.microsoft.com/python/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-python.
MIT License
4.56k stars 2.78k forks source link

add_data_disk does nothing #267

Closed flavianh closed 8 years ago

flavianh commented 9 years ago

My azure version is 0.9.0

Here is the test code:

volume = {'logical_disk_size_in_gb': 10, 'disk_label': '<the_label>', 'lun': 15, 'host_caching': 'ReadOnly', 'media_link': '<the_media_link>'}
a = sms.add_data_disk(<my_service>, <my_deployment>, <my_role>, **volume)

I have set async to True in servicemanagement;servicemanagementclient._ServiceManagementClient._perform_post and added print statements in servicemanagement;servicemanagementclient._ServiceManagementClient.perform_post to get (sanitized):

header:
[('Content-Length', '373'), ('x-ms-version', '2014-06-01'), ('Content-Type', 'application/atom+xml;type=entry;charset=utf-8')]

path:
/my_subscription_id/services/hostedservices/<my_service>/deployments/<my_deployment>/roles/<my_role>/DataDisks

body:
<DataVirtualHardDisk xmlns:i="http://www.w3.org/2001/XMLSchema-instance" xmlns="http://schemas.microsoft.com/windowsazure">
    <HostCaching>ReadOnly</HostCaching>
    <DiskLabel><the_label></DiskLabel>
    <Lun>0</Lun>
    <LogicalDiskSizeInGB>10</LogicalDiskSizeInGB>
    <MediaLink><the_media_link></MediaLink>
</DataVirtualHardDisk>
flavianh commented 9 years ago

So apparently the media link was already taken. With the same parameters, add_disk raised azure.WindowsAzureConflictError: Conflict (Conflict). I think add_data_disk should do the same

huguesv commented 9 years ago

I can't tell from your original post, what did add_data_disk do? Conflict error is raised when we get a 409 back from Azure service.

flavianh commented 9 years ago

That's my point, it just returned an AsyncSomething instance with the request id in it, and that's all. When I used add_disk it raised the conflict error, and I was able to identify that my media_link was already in use

huguesv commented 9 years ago

In case anyone else runs into issues with add_data_disk:

The way to get the errors for async operations, such as add_data_disk, is to call get_operation_status:

result = service.add_data_disk(service_name, deployment_name, role_name, lun, None, None, label, None, None, url)

# later, check periodically to see how the async operation is doing
op_result = service.get_operation_status(result.request_id)
print(op_result.status)
print(vars(op_result.error))

.status will be InProgress, Succeeded, or something else in case of an error.

The fact that add_data_disk is async and add_disk isn't is somewhat unfortunate, and out of our control in the python sdk (the REST api is designed that way).

In the unit tests, we have a helper function that blocks (_wait_for_async), which you can adapt for your needs, if blocking is what you want: https://github.com/Azure/azure-sdk-for-python/blob/master/tests/test_servicemanagementservice.py#L195

In the future, it would be nice to take advantage of Python 3.4's asyncio.

One thing we can do for now is clearly mark which operations are async and which aren't in the documentation comments for each method.

huguesv commented 9 years ago

I've extracted the common code that waits for async from the unit tests into a utility function on ServiceManagementService: wait_for_operation_status. I tried to make it as generic as possible, with some reasonable defaults for displaying progress.

lmazuel commented 8 years ago

I think the initial problem is now fixed by @huguesv code. Please feel free to re-open if you still have a problem even with @huguesv suggestion.

I also recommend you to use the latest SDK version which handles more clearly the async methods. Please also note that when we will add the async support, it will be on the new SDK and not on the legacy one.