proxmox_disk: fix async method of resize_disk (#9256)

* proxmox_disk: fix async method of resize_disk

Rewritten resizing of disk into separated function and used async method to retrieve task result. Additionally, rewritten function to detect failed tasks early, without waiting for timeout.

* proxmox_disk: add changelog fragment

* proxmox_disk: fix sanity errors

* Apply suggestions from code review

Co-authored-by: Alexei Znamensky <[email protected]>

* proxmox_disk: workaround for legacy Proxmox

* Apply suggestions from code review

Co-authored-by: Felix Fontein <[email protected]>

* Apply suggestions from the review

---------

Co-authored-by: Alexei Znamensky <[email protected]>
Co-authored-by: Felix Fontein <[email protected]>

changelogs/fragments/9256-proxmox_disk-fix-async-method-of-resize_disk.yml

@@ -0,0 +1,4 @@
+bugfixes:
+  - proxmox_disk - fix async method and make ``resize_disk`` method handle errors correctly (https://github.com/ansible-collections/community.general/pull/9256).
+minor_changes:
+  - proxmox module utils - add method ``api_task_complete`` that can wait for task completion and return error message (https://github.com/ansible-collections/community.general/pull/9256).

plugins/module_utils/proxmox.py

@@ -8,6 +8,7 @@
 __metaclass__ = type
 
 import traceback
+from time import sleep
 
 PROXMOXER_IMP_ERR = None
 try:
@@ -70,6 +71,8 @@ def ansible_to_proxmox_bool(value):
 
 class ProxmoxAnsible(object):
     """Base class for Proxmox modules"""
+    TASK_TIMED_OUT = 'timeout expired'
+
     def __init__(self, module):
         if not HAS_PROXMOXER:
             module.fail_json(msg=missing_required_lib('proxmoxer'), exception=PROXMOXER_IMP_ERR)
@@ -167,6 +170,32 @@ def api_task_ok(self, node, taskid):
         except Exception as e:
             self.module.fail_json(msg='Unable to retrieve API task ID from node %s: %s' % (node, e))
 
+    def api_task_complete(self, node_name, task_id, timeout):
+        """Wait until the task stops or times out.
+
+        :param node_name: Proxmox node name where the task is running.
+        :param task_id: ID of the running task.
+        :param timeout: Timeout in seconds to wait for the task to complete.
+        :return: Task completion status (True/False) and ``exitstatus`` message when status=False.
+        """
+        status = {}
+        while timeout:
+            try:
+                status = self.proxmox_api.nodes(node_name).tasks(task_id).status.get()
+            except Exception as e:
+                self.module.fail_json(msg='Unable to retrieve API task ID from node %s: %s' % (node_name, e))
+
+            if status['status'] == 'stopped':
+                if status['exitstatus'] == 'OK':
+                    return True, None
+                else:
+                    return False, status['exitstatus']
+            else:
+                timeout -= 1
+                if timeout <= 0:
+                    return False, ProxmoxAnsible.TASK_TIMED_OUT
+                sleep(1)
+
     def get_pool(self, poolid):
         """Retrieve pool information
 

plugins/modules/proxmox_disk.py

@@ -439,10 +439,11 @@
 """
 
 from ansible.module_utils.basic import AnsibleModule
+
+from ansible_collections.community.general.plugins.module_utils.version import LooseVersion
 from ansible_collections.community.general.plugins.module_utils.proxmox import (proxmox_auth_argument_spec,
                                                                                 ProxmoxAnsible)
 from re import compile, match, sub
-from time import sleep
 
 
 def disk_conf_str_to_dict(config_string):
@@ -517,17 +518,18 @@ def get_create_attributes(self):
         }
         return params
 
-    def wait_till_complete_or_timeout(self, node_name, task_id):
-        timeout = self.module.params['timeout']
-        while timeout:
-            if self.api_task_ok(node_name, task_id):
-                return True
-            timeout -= 1
-            if timeout <= 0:
-                return False
-            sleep(1)
-
     def create_disk(self, disk, vmid, vm, vm_config):
+        """Create a disk in the specified virtual machine. Check if creation is required,
+        and if so, compile the disk configuration and create it by updating the virtual
+        machine configuration. After calling the API function, wait for the result.
+
+        :param disk: ID of the disk in format "<bus><number>".
+        :param vmid: ID of the virtual machine where the disk will be created.
+        :param vm: Name of the virtual machine where the disk will be created.
+        :param vm_config: Configuration of the virtual machine.
+        :return: (bool, string) Whether the task was successful or not
+            and the message to return to Ansible.
+        """
         create = self.module.params['create']
         if create == 'disabled' and disk not in vm_config:
             # NOOP
@@ -596,15 +598,31 @@ def create_disk(self, disk, vmid, vm, vm_config):
             disk_config_to_apply = {self.module.params["disk"]: config_str}
 
         current_task_id = self.proxmox_api.nodes(vm['node']).qemu(vmid).config.post(**disk_config_to_apply)
-        task_success = self.wait_till_complete_or_timeout(vm['node'], current_task_id)
+        task_success, fail_reason = self.api_task_complete(vm['node'], current_task_id, self.module.params['timeout'])
+
         if task_success:
             return True, ok_str % (disk, vmid)
         else:
-            self.module.fail_json(
-                msg=timeout_str % self.proxmox_api.nodes(vm['node']).tasks(current_task_id).log.get()[:1]
-            )
+            if fail_reason == ProxmoxAnsible.TASK_TIMED_OUT:
+                self.module.fail_json(
+                    msg=timeout_str % self.proxmox_api.nodes(vm['node']).tasks(current_task_id).log.get()[:1]
+                )
+            else:
+                self.module.fail_json(msg="Error occurred on task execution: %s" % fail_reason)
 
     def move_disk(self, disk, vmid, vm, vm_config):
+        """Call the `move_disk` API function that moves the disk to another storage and wait for the result.
+
+        :param disk: ID of disk in format "<bus><number>".
+        :param vmid: ID of virtual machine which disk will be moved.
+        :param vm: Name of virtual machine which disk will be moved.
+        :param vm_config: Virtual machine configuration.
+        :return: (bool, string) Whether the task was successful or not
+            and the message to return to Ansible.
+        """
+        disk_config = disk_conf_str_to_dict(vm_config[disk])
+        disk_storage = disk_config["storage_name"]
+
         params = dict()
         params['disk'] = disk
         params['vmid'] = vmid
@@ -618,19 +636,62 @@ def move_disk(self, disk, vmid, vm, vm_config):
         params = {k: v for k, v in params.items() if v is not None}
 
         if params.get('storage', False):
+            # Check if the disk is already in the target storage.
             disk_config = disk_conf_str_to_dict(vm_config[disk])
             if params['storage'] == disk_config['storage_name']:
-                return False
+                return False, "Disk %s already at %s storage" % (disk, disk_storage)
+
+        current_task_id = self.proxmox_api.nodes(vm['node']).qemu(vmid).move_disk.post(**params)
+        task_success, fail_reason = self.api_task_complete(vm['node'], current_task_id, self.module.params['timeout'])
 
-        task_id = self.proxmox_api.nodes(vm['node']).qemu(vmid).move_disk.post(**params)
-        task_success = self.wait_till_complete_or_timeout(vm['node'], task_id)
         if task_success:
-            return True
+            return True, "Disk %s moved from VM %s storage %s" % (disk, vmid, disk_storage)
         else:
-            self.module.fail_json(
-                msg='Reached timeout while waiting for moving VM disk. Last line in task before timeout: %s' %
-                    self.proxmox_api.nodes(vm['node']).tasks(task_id).log.get()[:1]
-            )
+            if fail_reason == ProxmoxAnsible.TASK_TIMED_OUT:
+                self.module.fail_json(
+                    msg='Reached timeout while waiting for moving VM disk. Last line in task before timeout: %s' %
+                        self.proxmox_api.nodes(vm['node']).tasks(current_task_id).log.get()[:1]
+                )
+            else:
+                self.module.fail_json(msg="Error occurred on task execution: %s" % fail_reason)
+
+    def resize_disk(self, disk, vmid, vm, vm_config):
+        """Call the `resize` API function to change the disk size and wait for the result.
+
+        :param disk: ID of disk in format "<bus><number>".
+        :param vmid: ID of virtual machine which disk will be resized.
+        :param vm: Name of virtual machine which disk will be resized.
+        :param vm_config: Virtual machine configuration.
+        :return: (Bool, string) Whether the task was successful or not
+            and the message to return to Ansible.
+        """
+        size = self.module.params['size']
+        if not match(r'^\+?\d+(\.\d+)?[KMGT]?$', size):
+            self.module.fail_json(msg="Unrecognized size pattern for disk %s: %s" % (disk, size))
+        disk_config = disk_conf_str_to_dict(vm_config[disk])
+        actual_size = disk_config['size']
+        if size == actual_size:
+            return False, "Disk %s is already %s size" % (disk, size)
+
+        # Resize disk API endpoint has changed at v8.0: PUT method become async.
+        version = self.version()
+        pve_major_version = 3 if version < LooseVersion('4.0') else version.version[0]
+        if pve_major_version >= 8:
+            current_task_id = self.proxmox_api.nodes(vm['node']).qemu(vmid).resize.set(disk=disk, size=size)
+            task_success, fail_reason = self.api_task_complete(vm['node'], current_task_id, self.module.params['timeout'])
+            if task_success:
+                return True, "Disk %s resized in VM %s" % (disk, vmid)
+            else:
+                if fail_reason == ProxmoxAnsible.TASK_TIMED_OUT:
+                    self.module.fail_json(
+                        msg="Reached timeout while resizing disk. Last line in task before timeout: %s" %
+                            self.proxmox_api.nodes(vm['node']).tasks(current_task_id).log.get()[:1]
+                    )
+                else:
+                    self.module.fail_json(msg="Error occurred on task execution: %s" % fail_reason)
+        else:
+            self.proxmox_api.nodes(vm['node']).qemu(vmid).resize.set(disk=disk, size=size)
+            return True, "Disk %s resized in VM %s" % (disk, vmid)
 
 
 def main():
@@ -769,11 +830,8 @@ def main():
 
     if state == 'present':
         try:
-            success, message = proxmox.create_disk(disk, vmid, vm, vm_config)
-            if success:
-                module.exit_json(changed=True, vmid=vmid, msg=message)
-            else:
-                module.exit_json(changed=False, vmid=vmid, msg=message)
+            changed, message = proxmox.create_disk(disk, vmid, vm, vm_config)
+            module.exit_json(changed=changed, vmid=vmid, msg=message)
         except Exception as e:
             module.fail_json(vmid=vmid, msg='Unable to create/update disk %s in VM %s: %s' % (disk, vmid, str(e)))
 
@@ -790,27 +848,15 @@ def main():
 
     elif state == 'moved':
         try:
-            disk_config = disk_conf_str_to_dict(vm_config[disk])
-            disk_storage = disk_config["storage_name"]
-            if proxmox.move_disk(disk, vmid, vm, vm_config):
-                module.exit_json(changed=True, vmid=vmid,
-                                 msg="Disk %s moved from VM %s storage %s" % (disk, vmid, disk_storage))
-            else:
-                module.exit_json(changed=False, vmid=vmid, msg="Disk %s already at %s storage" % (disk, disk_storage))
+            changed, message = proxmox.move_disk(disk, vmid, vm, vm_config)
+            module.exit_json(changed=changed, vmid=vmid, msg=message)
         except Exception as e:
             module.fail_json(msg="Failed to move disk %s in VM %s with exception: %s" % (disk, vmid, str(e)))
 
     elif state == 'resized':
         try:
-            size = module.params['size']
-            if not match(r'^\+?\d+(\.\d+)?[KMGT]?$', size):
-                module.fail_json(msg="Unrecognized size pattern for disk %s: %s" % (disk, size))
-            disk_config = disk_conf_str_to_dict(vm_config[disk])
-            actual_size = disk_config['size']
-            if size == actual_size:
-                module.exit_json(changed=False, vmid=vmid, msg="Disk %s is already %s size" % (disk, size))
-            proxmox.proxmox_api.nodes(vm['node']).qemu(vmid).resize.set(disk=disk, size=size)
-            module.exit_json(changed=True, vmid=vmid, msg="Disk %s resized in VM %s" % (disk, vmid))
+            changed, message = proxmox.resize_disk(disk, vmid, vm, vm_config)
+            module.exit_json(changed=changed, vmid=vmid, msg=message)
         except Exception as e:
             module.fail_json(msg="Failed to resize disk %s in VM %s with exception: %s" % (disk, vmid, str(e)))