From 295a166a7c926fa55d20969c0171905b9b4f8cc8 Mon Sep 17 00:00:00 2001 From: vicwicker Date: Sat, 16 Sep 2023 15:25:31 +0200 Subject: [PATCH 1/7] [azure arm] also delete the VM OS disk if it is a managed disk VMs on Azure can be launched with managed or unamanaged disks. Unmanaged disks are typically launched from specific VHDs. Managed disks are managed by Azure and they do not have a VHD associated that we can remove (e.g., ubuntu image offered by Azure). Deleting a VM on the Azure Web console allows you to choose whether you want to delete the OS disk as well, independently of this is managed or unmanaged. The original code would allow that only for unmanaged disks via the 'ex_destroy_vhd' parameter but, as said, this is incomplete as VMs can be launched from Azure managed disks. This change proposes to introduce a new parameter 'ex_destroy_os_disk' to delete the VM OS disk independently of their type. Ideally, this parameter should default to True as (i) that is the Azure's default option and (ii) it would align with 'ex_destroy_vhd'. However, this might pose backward compatibility issues if libcloud starts deleting managed OS disks by default as well. I am not sure what the libcloud maintainers' take is in this regard but this would be my proposal. --- libcloud/compute/drivers/azure_arm.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/libcloud/compute/drivers/azure_arm.py b/libcloud/compute/drivers/azure_arm.py index 75e1b70200..6bb73f9286 100644 --- a/libcloud/compute/drivers/azure_arm.py +++ b/libcloud/compute/drivers/azure_arm.py @@ -808,6 +808,7 @@ def destroy_node( ex_destroy_vhd=True, ex_poll_qty=10, ex_poll_wait=10, + ex_destroy_os_disk=False, ): """ Destroy a node. @@ -830,6 +831,9 @@ def destroy_node( :param ex_poll_wait: Delay in seconds between retries (default 10). :type node: ``int`` + :param ex_destroy_os_disk: Destroy the OS disk associated with + this node (default True). + :return: True if the destroy was successful, raises exception otherwise. :rtype: ``bool`` @@ -912,6 +916,25 @@ def destroy_node( raise time.sleep(10) + # Optionally destroy the OS disk + if ex_destroy_os_disk: + disk_id = node.extra['properties']['storageProfile']['osDisk']['managedDisk']['id'] + retries = ex_poll_qty + while retries > 0: + try: + self.connection.request( + disk_id, params={"api-version": DISK_API_VERSION}, method="DELETE" + ) + break + except BaseHTTPError as h: + retries -= 1 + if h.code in (204, 404): + break + elif retries > 0: + time.sleep(ex_poll_wait) + else: + raise + return True def create_volume( From c8f7a8106a114fa6ecf51e06fc0ee36aaecd3767 Mon Sep 17 00:00:00 2001 From: vicwicker Date: Wed, 20 Sep 2023 22:25:39 +0200 Subject: [PATCH 2/7] small fixes --- libcloud/compute/drivers/azure_arm.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/libcloud/compute/drivers/azure_arm.py b/libcloud/compute/drivers/azure_arm.py index 6bb73f9286..c901284ae5 100644 --- a/libcloud/compute/drivers/azure_arm.py +++ b/libcloud/compute/drivers/azure_arm.py @@ -860,7 +860,7 @@ def destroy_node( raise # Poll until the node actually goes away (otherwise attempt to delete - # NIC and VHD will fail with "resource in use" errors). + # NIC, VHD and Disk will fail with "resource in use" errors). retries = ex_poll_qty while do_node_polling and retries > 0: try: @@ -928,7 +928,8 @@ def destroy_node( break except BaseHTTPError as h: retries -= 1 - if h.code in (204, 404): + if h.code in (200, 204): + # destroy disk is ok or accepted (but deferred) break elif retries > 0: time.sleep(ex_poll_wait) From 0573e3935b0445e6dbebc0a9860f830097af6c7f Mon Sep 17 00:00:00 2001 From: vicwicker Date: Fri, 22 Sep 2023 21:04:45 +0200 Subject: [PATCH 3/7] improvements and unit test --- libcloud/compute/drivers/azure_arm.py | 40 +++++++-------- ...rosoft_Compute_disks_test_node_disk_1.json | 51 +++++++++++++++++++ libcloud/test/compute/test_azure_arm.py | 43 ++++++++++++++++ 3 files changed, 111 insertions(+), 23 deletions(-) create mode 100644 libcloud/test/compute/fixtures/azure_arm/_subscriptions_99999999_resourceGroups_000000_providers_Microsoft_Compute_disks_test_node_disk_1.json diff --git a/libcloud/compute/drivers/azure_arm.py b/libcloud/compute/drivers/azure_arm.py index c901284ae5..871cfeb7ae 100644 --- a/libcloud/compute/drivers/azure_arm.py +++ b/libcloud/compute/drivers/azure_arm.py @@ -832,14 +832,15 @@ def destroy_node( :type node: ``int`` :param ex_destroy_os_disk: Destroy the OS disk associated with - this node (default True). + this node either this is a managed disk or a VHD (default False). + :type nod: ``bool`` :return: True if the destroy was successful, raises exception otherwise. :rtype: ``bool`` """ - do_node_polling = ex_destroy_nic or ex_destroy_vhd + do_node_polling = ex_destroy_nic or ex_destroy_vhd or ex_destroy_os_disk # This returns a 202 (Accepted) which means that the delete happens # asynchronously. @@ -860,7 +861,7 @@ def destroy_node( raise # Poll until the node actually goes away (otherwise attempt to delete - # NIC, VHD and Disk will fail with "resource in use" errors). + # NIC, VHD and OS Disk will fail with "resource in use" errors). retries = ex_poll_qty while do_node_polling and retries > 0: try: @@ -893,7 +894,7 @@ def destroy_node( # Optionally clean up OS disk VHD. vhd = node.extra["properties"]["storageProfile"]["osDisk"].get("vhd") - if ex_destroy_vhd and vhd is not None: + if (ex_destroy_os_disk or ex_destroy_vhd) and vhd is not None: retries = ex_poll_qty resourceGroup = node.id.split("/")[4] while retries > 0: @@ -916,25 +917,18 @@ def destroy_node( raise time.sleep(10) - # Optionally destroy the OS disk - if ex_destroy_os_disk: - disk_id = node.extra['properties']['storageProfile']['osDisk']['managedDisk']['id'] - retries = ex_poll_qty - while retries > 0: - try: - self.connection.request( - disk_id, params={"api-version": DISK_API_VERSION}, method="DELETE" - ) - break - except BaseHTTPError as h: - retries -= 1 - if h.code in (200, 204): - # destroy disk is ok or accepted (but deferred) - break - elif retries > 0: - time.sleep(ex_poll_wait) - else: - raise + # Optionally destroy the OS managed disk + managed_disk = node.extra["properties"]["storageProfile"]["osDisk"].get("managedDisk") + if ex_destroy_os_disk and managed_disk is not None: + managed_disk_id = managed_disk["id"] + try: + self.connection.request( + managed_disk_id, params={"api-version": DISK_API_VERSION}, method="DELETE" + ) + except BaseHTTPError as h: + if h.code not in (202, 204): + # 202 or 204 mean destroy is accepted (but deferred) or already destroyed + raise return True diff --git a/libcloud/test/compute/fixtures/azure_arm/_subscriptions_99999999_resourceGroups_000000_providers_Microsoft_Compute_disks_test_node_disk_1.json b/libcloud/test/compute/fixtures/azure_arm/_subscriptions_99999999_resourceGroups_000000_providers_Microsoft_Compute_disks_test_node_disk_1.json new file mode 100644 index 0000000000..985c0d3aa1 --- /dev/null +++ b/libcloud/test/compute/fixtures/azure_arm/_subscriptions_99999999_resourceGroups_000000_providers_Microsoft_Compute_disks_test_node_disk_1.json @@ -0,0 +1,51 @@ +{ + "properties": { + "vmId": "99999999-9999-9999-9999-999999999999", + "hardwareProfile": { + "vmSize": "Standard_A1" + }, + "storageProfile": { + "osDisk": { + "osType": "Linux", + "name": "test-disk-1", + "createOption": "FromImage", + "caching": "ReadWrite", + "managedDisk": { + "storageAccountType": "Standard_LRS", + "id": "/subscriptions/99999999-9999-9999-9999-999999999999/resourceGroups/000000/providers/Microsoft.Compute/disks/test-disk-1" + } + }, + "dataDisks": [ + { + "lun": 0, + "name": "DD0-9999", + "createOption": "Attach", + "caching": "None", + "managedDisk": { + "storageAccountType": "Standard_LRS", + "id": "/subscriptions/99999999-9999-9999-9999-999999999999/resourceGroups/000000/providers/Microsoft.Compute/disks/DD0-9999" + }, + "diskSizeGB": 8 + } + ] + }, + "networkProfile": { + "networkInterfaces": [ + { + "id": "/subscriptions/99999999-9999-9999-9999-999999999999/resourceGroups/0000/providers/Microsoft.Network/networkInterfaces/test-nic", + "properties": { + "primary": true + } + } + ] + }, + "provisioningState": "Succeeded" + }, + "type": "Microsoft.Compute/virtualMachines", + "location": "eastus", + "tags": { + "tag_key1": "tag_val1" + }, + "id": "/subscriptions/99999999-9999-9999-9999-999999999999/resourceGroups/0000/providers/Microsoft.Compute/virtualMachines/test_vm", + "name": "test_vm" +} diff --git a/libcloud/test/compute/test_azure_arm.py b/libcloud/test/compute/test_azure_arm.py index ed9dfbf033..038850c466 100644 --- a/libcloud/test/compute/test_azure_arm.py +++ b/libcloud/test/compute/test_azure_arm.py @@ -376,6 +376,49 @@ def error(e, **kwargs): self.assertTrue(ret) self.assertEqual(4, time_sleep_mock.call_count) # Retries + @mock.patch("time.sleep", return_value=None) + def test_destroy_node__os_managed_disk(self, time_sleep_mock): + def error(e, **kwargs): + raise e(**kwargs) + + node = self.driver.list_nodes()[0] + + # ok responses + for response in (200, 202, 204): + AzureMockHttp.responses = [ + # OK to the DELETE request + lambda f: (httplib.OK, None, {}, "OK"), + # 404 - Node destroyed + lambda f: error(BaseHTTPError, code=404, message="Not found"), + # 200 - NIC destroyed + lambda f: (httplib.OK, None, {}, "OK"), + # 200 - managed OS disk destroyed + lambda f: (httplib.OK, None, {}, "OK") + if response == 200 + else error(BaseHTTPError, code=response, message="Deleted or deferred"), + ] + ret = self.driver.destroy_node(node, ex_destroy_os_disk=True) + self.assertTrue(ret) + + @mock.patch("time.sleep", return_value=None) + def test_destroy_node__os_managed_disk_error(self, time_sleep_mock): + def error(e, **kwargs): + raise e(**kwargs) + + node = self.driver.list_nodes()[0] + AzureMockHttp.responses = [ + # OK to the DELETE request + lambda f: (httplib.OK, None, {}, "OK"), + # 404 - Node destroyed + lambda f: error(BaseHTTPError, code=404, message="Not found"), + # 200 - NIC destroyed + lambda f: (httplib.OK, None, {}, "OK"), + # 500 - transient error when trying to destroy the OS disk + lambda f: error(BaseHTTPError, code=500, message="Cloud weather"), + ] + with self.assertRaises(BaseHTTPError): + self.driver.destroy_node(node, ex_destroy_os_disk=True) + @mock.patch("time.sleep", return_value=None) def test_destroy_node__destroy_nic_retries(self, time_sleep_mock): def error(e, **kwargs): From 79de6d7333eccc446e73aa55c0abe25d39a8ef91 Mon Sep 17 00:00:00 2001 From: vicwicker Date: Tue, 3 Oct 2023 12:22:24 +0200 Subject: [PATCH 4/7] start addressing review comments --- libcloud/compute/drivers/azure_arm.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libcloud/compute/drivers/azure_arm.py b/libcloud/compute/drivers/azure_arm.py index 871cfeb7ae..37ff772cf5 100644 --- a/libcloud/compute/drivers/azure_arm.py +++ b/libcloud/compute/drivers/azure_arm.py @@ -808,7 +808,7 @@ def destroy_node( ex_destroy_vhd=True, ex_poll_qty=10, ex_poll_wait=10, - ex_destroy_os_disk=False, + ex_destroy_os_disk=True, ): """ Destroy a node. @@ -832,7 +832,7 @@ def destroy_node( :type node: ``int`` :param ex_destroy_os_disk: Destroy the OS disk associated with - this node either this is a managed disk or a VHD (default False). + this node either if it is a managed disk (default True). :type nod: ``bool`` :return: True if the destroy was successful, raises exception @@ -894,7 +894,7 @@ def destroy_node( # Optionally clean up OS disk VHD. vhd = node.extra["properties"]["storageProfile"]["osDisk"].get("vhd") - if (ex_destroy_os_disk or ex_destroy_vhd) and vhd is not None: + if ex_destroy_vhd and vhd is not None: retries = ex_poll_qty resourceGroup = node.id.split("/")[4] while retries > 0: @@ -918,7 +918,7 @@ def destroy_node( time.sleep(10) # Optionally destroy the OS managed disk - managed_disk = node.extra["properties"]["storageProfile"]["osDisk"].get("managedDisk") + managed_disk = node.extra.get("properties", {}).get("storageProfile", {}).get("osDisk", {}).get("managedDisk") if ex_destroy_os_disk and managed_disk is not None: managed_disk_id = managed_disk["id"] try: From 772db1117fb0d91a3b43a6477705e79bdbfe8b35 Mon Sep 17 00:00:00 2001 From: vicwicker Date: Sat, 7 Oct 2023 12:50:12 +0200 Subject: [PATCH 5/7] run black --- libcloud/compute/drivers/azure_arm.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/libcloud/compute/drivers/azure_arm.py b/libcloud/compute/drivers/azure_arm.py index 37ff772cf5..ae6e2dd510 100644 --- a/libcloud/compute/drivers/azure_arm.py +++ b/libcloud/compute/drivers/azure_arm.py @@ -918,7 +918,12 @@ def destroy_node( time.sleep(10) # Optionally destroy the OS managed disk - managed_disk = node.extra.get("properties", {}).get("storageProfile", {}).get("osDisk", {}).get("managedDisk") + managed_disk = ( + node.extra.get("properties", {}) + .get("storageProfile", {}) + .get("osDisk", {}) + .get("managedDisk") + ) if ex_destroy_os_disk and managed_disk is not None: managed_disk_id = managed_disk["id"] try: From 412b765c82acdcd408354f012adc9979c518defc Mon Sep 17 00:00:00 2001 From: vicwicker Date: Sat, 7 Oct 2023 12:58:28 +0200 Subject: [PATCH 6/7] add release notes --- docs/upgrade_notes.rst | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/upgrade_notes.rst b/docs/upgrade_notes.rst index 27c9c3e0ec..230850a4d5 100644 --- a/docs/upgrade_notes.rst +++ b/docs/upgrade_notes.rst @@ -5,6 +5,17 @@ This page describes how to upgrade from a previous version to a new version which contains backward incompatible or semi-incompatible changes and how to preserve the old behavior when this is possible. +Libcloud 3.9.0 +-------------- + +* [AZURE ARM] Added a new argument to destroy_node() to also delete node's managed + OS disk as part of the node's deletion. Defaults to true. This can be reverted by + setting the argument to false in the call: + + .. sourcecode:: python + + destroy_node(..., ex_destroy_os_disk=False) + Libcloud 3.8.0 -------------- From f3bca2a6ea1f97f4b3eda802b989402f98f0d13a Mon Sep 17 00:00:00 2001 From: vicwicker Date: Sat, 7 Oct 2023 13:49:03 +0200 Subject: [PATCH 7/7] fix typo --- libcloud/compute/drivers/azure_arm.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libcloud/compute/drivers/azure_arm.py b/libcloud/compute/drivers/azure_arm.py index ae6e2dd510..9c540fec73 100644 --- a/libcloud/compute/drivers/azure_arm.py +++ b/libcloud/compute/drivers/azure_arm.py @@ -833,7 +833,7 @@ def destroy_node( :param ex_destroy_os_disk: Destroy the OS disk associated with this node either if it is a managed disk (default True). - :type nod: ``bool`` + :type node: ``bool`` :return: True if the destroy was successful, raises exception otherwise.