vm_params need to start/shutdown/reboot etc VM if requested

Signed-off-by: Justin Cinkelj <justin.cinkelj@xlab.si>

Author: justinc1

plugins/module_utils/vm.py

@@ -295,6 +295,8 @@ def __init__(
         # self._did_start_work = False
         # Now, VM was rebooted when it was running at start, then stopped and powered on:
         # .power_state in [] and any(_did_nice_shutdown_work, _did_force_shutdown_work) and _was_start_tried
+        self._was_reboot_tried = False
+        self._was_reset_tried = False
 
     @property
     def nic_list(self):
@@ -853,40 +855,87 @@ def get_vm_and_boot_devices(cls, ansible_dict, rest_client):
         ]
         return vm, boot_devices_uuid, boot_devices_ansible
 
-    def update_vm_power_state(self, module, rest_client, desired_power_action):
+    def update_vm_power_state(
+        self, module, rest_client, desired_power_action, ignore_repeated_request: bool
+    ):
         """Sets the power state to what is stored in self.power_state"""
+
         # desired_power_action must be present in FROM_ANSIBLE_TO_HYPERCORE_POWER_ACTION's keys
+        def assert_or_ignore_repeated_request(msg):
+            if ignore_repeated_request:
+                # Do not start/stop/shutdown VM twice.
+                # Normally we want to assert if a second start/stop/shutdown is tried.
+                # But as very last module change it makes sense to push VM into requested power_state,
+                # without knowing what did module already do with VM power_state.
+                # So in this special case, allow a second call, and just silently ignore
+                # it if VM power_state was already set to desired state.
+                return
+            else:
+                raise AssertionError(msg)
+
         if not self._power_state:
             raise errors.ScaleComputingError("No information about VM's power state.")
 
         # keep a record what was done
         if desired_power_action == "start":
             if self._was_start_tried:
-                raise AssertionError("VM _was_start_tried already set")
+                return assert_or_ignore_repeated_request(
+                    "VM _was_start_tried already set"
+                )
             self._was_start_tried = True
         if desired_power_action == "shutdown":
             if self._was_nice_shutdown_tried:
-                raise AssertionError("VM _was_nice_shutdown_tried already set")
+                return assert_or_ignore_repeated_request(
+                    "VM _was_nice_shutdown_tried already set"
+                )
             self._was_nice_shutdown_tried = True
         if desired_power_action == "stop":
             if self._was_force_shutdown_tried:
-                raise AssertionError("VM _was_force_shutdown_tried already set")
+                return assert_or_ignore_repeated_request(
+                    "VM _was_force_shutdown_tried already set"
+                )
             self._was_force_shutdown_tried = True
-        # reboot, reset are not included
-
-        task_tag = rest_client.create_record(
-            "/rest/v1/VirDomain/action",
-            [
-                dict(
-                    virDomainUUID=self.uuid,
-                    actionType=FROM_ANSIBLE_TO_HYPERCORE_POWER_ACTION[
-                        desired_power_action
-                    ],
-                    cause="INTERNAL",
+        if desired_power_action == "reboot":
+            if self._was_reboot_tried:
+                return assert_or_ignore_repeated_request(
+                    "VM _was_reboot_tried already set"
                 )
-            ],
-            module.check_mode,
-        )
+            self._was_reboot_tried = True
+        if desired_power_action == "reset":
+            if self._was_reset_tried:
+                return assert_or_ignore_repeated_request(
+                    "VM _was_reset_tried already set"
+                )
+            self._was_reset_tried = True
+
+        try:
+            task_tag = rest_client.create_record(
+                "/rest/v1/VirDomain/action",
+                [
+                    dict(
+                        virDomainUUID=self.uuid,
+                        actionType=FROM_ANSIBLE_TO_HYPERCORE_POWER_ACTION[
+                            desired_power_action
+                        ],
+                        cause="INTERNAL",
+                    )
+                ],
+                module.check_mode,
+            )
+        except errors.UnexpectedAPIResponse as ex:
+            if desired_power_action != "reset":
+                raise
+            # We are allowed to send reset only if VM is in
+            # RUNNING or SHUTDOWN (as in middle of shutting down, but not yet fully shutdown).
+            # If VM is already shutoff, the request fails.
+            # Ignore this special case.
+            # The whole RESET is not even exposed on HyperCore UI,
+            # maybe we should remove it from ansible.
+            if ex.response_status != 500:
+                # the '500 b'{"error":"An internal error occurred"}'' is the one to ignore
+                raise
+            module.warn("Ignoring failed VM RESET")
+            return
         TaskTag.wait_task(rest_client, task_tag)
 
     @classmethod
@@ -936,7 +985,7 @@ def vm_shutdown_forced(self, module, rest_client, reboot=False):
         if vm_fresh_data["state"] in ["SHUTOFF", "SHUTDOWN"]:
             return True
         if module.params["force_reboot"] and self._was_nice_shutdown_tried:
-            self.update_vm_power_state(module, rest_client, "stop")
+            self.update_vm_power_state(module, rest_client, "stop", False)
             # force shutdown should always work. If not, we need to pool for state change.
             # Maybe we need to pool for state change anyway -
             # TaskTag might be finished before VM is really off.
@@ -959,7 +1008,7 @@ def wait_shutdown(self, module, rest_client):
             # and module.params["shutdown_timeout"]  # default is 300 anyway
             and not self._was_nice_shutdown_tried
         ):
-            self.update_vm_power_state(module, rest_client, "shutdown")
+            self.update_vm_power_state(module, rest_client, "shutdown", False)
             shutdown_timeout = module.params["shutdown_timeout"]
             start = time()
             while 1:
@@ -983,14 +1032,14 @@ def vm_power_up(self, module, rest_client):
         #   - VM was initially stopped and
         #   - module param power_state is omitted or contains "stop".
         if self.was_vm_shutdown() and self._initially_running:
-            self.update_vm_power_state(module, rest_client, "start")
+            self.update_vm_power_state(module, rest_client, "start", False)
             return
         # Also start VM if module power_state requires a power on.
         # Field _power_action is set only if VM instance was created with from_ansible();
         # it is None if VM instance was created with from_hypercore().
         requested_power_action = module.params.get("power_state")
         if requested_power_action == "start":
-            self.update_vm_power_state(module, rest_client, "start")
+            self.update_vm_power_state(module, rest_client, "start", False)
 
     def was_vm_shutdown(self) -> bool:
         """
@@ -1236,21 +1285,26 @@ def set_vm_params(cls, module, rest_client, vm, param_subset: List[str]):
             endpoint = "{0}/{1}".format("/rest/v1/VirDomain", vm.uuid)
             task_tag = rest_client.update_record(endpoint, payload, module.check_mode)
             TaskTag.wait_task(rest_client, task_tag)
+
+            # shutdown VM if it needs to be rebooted to apply NIC/disk changes
             if ManageVMParams._needs_reboot(
                 module, changed_parameters
             ) and vm._power_action not in ["stop", "stopped", "shutdown"]:
                 vm.do_shutdown_steps(module, rest_client)
+
             return (
                 True,
                 dict(
                     before=ManageVMParams._build_before_diff(vm, module),
                     after=ManageVMParams._build_after_diff(module, rest_client),
                 ),
+                changed_parameters,
             )
         else:
             return (
                 False,
                 dict(before=None, after=None),
+                changed_parameters,
             )
 
     @classmethod

plugins/modules/vm.py

@@ -444,10 +444,10 @@ def _set_nics(module, rest_client, vm_before: VM):
 
 
 def _set_vm_params(module, rest_client, vm, param_subset: List[str]):
-    changed_params, diff = ManageVMParams.set_vm_params(
+    changed_params, diff, changed_parameters = ManageVMParams.set_vm_params(
         module, rest_client, vm, param_subset
     )
-    return changed_params
+    return changed_params, changed_parameters
 
 
 def ensure_present(module, rest_client):
@@ -457,7 +457,7 @@ def ensure_present(module, rest_client):
         existing_boot_order = vm_before.get_boot_device_order()
         # machineType needs to be set to uefi/vtpm first,
         # next nvram/vtpm disk can be added.
-        changed_params_1 = _set_vm_params(
+        changed_params_1, changed_parameters_1 = _set_vm_params(
             module, rest_client, vm_before, param_subset=["machine_type"]
         )
         changed_disks = _set_disks(module, rest_client, vm_before)
@@ -470,7 +470,7 @@ def ensure_present(module, rest_client):
         # since boot order cannot be set when the vm is running.
         # set_vm_params updates VM's name, description, tags, memory, number of CPU,
         # changed the power state and/or assigns the snapshot schedule to the VM
-        changed_params_2 = _set_vm_params(
+        changed_params_2, changed_parameters_2 = _set_vm_params(
             module, rest_client, vm_before, param_subset=[]
         )
         changed = any(
@@ -505,7 +505,7 @@ def ensure_present(module, rest_client):
         # Set power state
         if module.params["power_state"] != "shutdown":
             vm_created.update_vm_power_state(
-                module, rest_client, module.params["power_state"]
+                module, rest_client, module.params["power_state"], False
             )
         changed = True
         name_field = "vm_name"
@@ -527,7 +527,7 @@ def ensure_absent(module, rest_client):
     if vm:
         if vm._power_state != "shutdown":  # First, shut it off and then delete
             # TODO ==shutdown or ==stopped ??
-            vm.update_vm_power_state(module, rest_client, "stop")
+            vm.update_vm_power_state(module, rest_client, "stop", False)
         task_tag = rest_client.delete_record(
             "{0}/{1}".format("/rest/v1/VirDomain", vm.uuid), module.check_mode
         )

plugins/modules/vm_params.py

@@ -132,13 +132,24 @@ def run(module, rest_client):
     # Update VM's name, description, tags, memory, number of CPUs, power_state and/or assign snapshot schedule.
     # In case if reboot is needed, set_vm_params will shutdown the vm
     # In case if reboot is not needed, set_vm_params will set power_state as specified in the module.params["power_state"]
-    changed, diff = ManageVMParams.set_vm_params(
+    changed, diff, changed_parameters = ManageVMParams.set_vm_params(
         module, rest_client, vm, param_subset=[]
     )
     if module.params["power_state"] not in ["shutdown", "stop"]:
         # VM will be powered on in case if reboot is needed and module.params["power_state"] in ["start", "reboot", "reset"]
         # if reboot is not needed, vm_power_up doesn't do anything
         vm.vm_power_up(module, rest_client)
+
+    # shutdown or start VM if ansible module power_state param require power state change
+    if changed_parameters.get("power_state"):
+        requested_power_action = module.params["power_state"]
+        # vm.vm_power_up can already start VM, and a second start here would assert.
+        # ManageVMParams.set_vm_params can already shutdown/stop VM.
+        ignore_repeated_request = True
+        vm.update_vm_power_state(
+            module, rest_client, requested_power_action, ignore_repeated_request
+        )
+
     return changed, vm.was_vm_rebooted(), diff
 
 

tests/integration/targets/i_vm_params/tasks/02_machine_type.yml

@@ -210,8 +210,7 @@
         - not output.records[0].snapshot_schedule
 
   # ===================================================================
-  # Set nonexisting snapshot schedule
-  - name: Set VMs power_state - applied
+  - name: Set VMs power_state - reset
     scale_computing.hypercore.vm_params:
       vm_name: "{{ vm_name }}"
       power_state: reset
@@ -225,14 +224,18 @@
         - output is changed
         - output.vm_rebooted is false  # true  # todo - is this reported? should we pretend reboot==reset - it is not same.
 
+    # HC3 returned error for RESET, but it did reset VM (HC3 v9.4.0)
+    # After RESET, VM was 10-15 sec in stopped state.
+  - name: Wait on VM stop/start to finish
+    ansible.builtin.pause:
+      seconds: 20
   - name: Check VMs power_state isn't changed
     scale_computing.hypercore.vm_info:
       vm_name: "{{ vm_name }}"
     register: output
   - ansible.builtin.assert:
       that:
         - output is succeeded
-        # HC3 9.2.22 - RESET is accepted, but VM remains stopped as it was stopped before.
         - output.records[0].power_state == "started"
 
   # ===================================================================

tests/unit/plugins/module_utils/test_vm.py

@@ -2060,7 +2060,9 @@ def test_set_vm_params(self, create_module, rest_client, mocker):
         mocker.patch(
             "ansible_collections.scale_computing.hypercore.plugins.module_utils.vm.VM.wait_shutdown"
         ).return_value = True
-        changed, diff = ManageVMParams.set_vm_params(module, rest_client, vm_before, [])
+        changed, diff, changed_parameters = ManageVMParams.set_vm_params(
+            module, rest_client, vm_before, []
+        )
 
         assert changed is True
         assert diff == {
@@ -2083,6 +2085,15 @@ def test_set_vm_params(self, create_module, rest_client, mocker):
                 "snapshot_schedule": "",
             },
         }
+        assert changed_parameters == {
+            "vm_name": True,
+            "description": True,
+            "tags": False,
+            "memory": False,
+            "vcpu": True,
+            "power_state": False,
+            "snapshot_schedule": False,
+        }
 
     def test_run_no_change(self, create_module, rest_client, mocker):
         module = create_module(
@@ -2118,13 +2129,23 @@ def test_run_no_change(self, create_module, rest_client, mocker):
             snapshot_schedule="",
         )
 
-        changed, diff = ManageVMParams.set_vm_params(module, rest_client, vm_before, [])
+        changed, diff, changed_parameters = ManageVMParams.set_vm_params(
+            module, rest_client, vm_before, []
+        )
 
         assert changed is False
         assert diff == {
             "before": None,
             "after": None,
         }
+        assert changed_parameters == {
+            "description": False,
+            "tags": False,
+            "memory": False,
+            "vcpu": False,
+            "power_state": False,
+            "snapshot_schedule": False,
+        }
         assert vm_before.was_vm_rebooted() is False
 
 

tests/unit/plugins/modules/test_vm.py

@@ -812,7 +812,7 @@ def test_ensure_present_update_record_no_changes(
         ).return_value = None
         mocker.patch(
             "ansible_collections.scale_computing.hypercore.plugins.modules.vm._set_vm_params"
-        ).return_value = False
+        ).return_value = (False, {})
         mocker.patch(
             "ansible_collections.scale_computing.hypercore.plugins.modules.vm._set_disks"
         ).return_value = False