Decompose the core deploy step on iscsi and ansible deploy

Following the decomposition of the core step on the 'direct' deploy
interface, this change decomposed the iscsi and ansible deploy.

Co-Authored-By: Dmitry Tantsur <dtantsur@protonmail.com>
Change-Id: I537c6f6cf66c80b67b9045ea0618b02b7b93d36c
Story: #2006963
Task: #40152

doc/source/admin/node-deployment.rst

@@ -47,8 +47,10 @@ All deploy interfaces based on ironic-python-agent (i.e. ``direct``, ``iscsi``
 and ``ansible`` and any derivatives) expose the following deploy steps:
 
 ``deploy.deploy`` (priority 100)
-  In this step the node is booted using a provisioning image, and the user
+  In this step the node is booted using a provisioning image.
-  image is written to the node's disk.
+``deploy.write_image`` (priority 80)
+  An out-of-band (``iscsi``, ``ansible``) or in-band (``direct``) step that
+  downloads and writes the image to the node.
 ``deploy.tear_down_agent`` (priority 40)
   In this step the provisioning image is shut down.
 ``deploy.switch_to_tenant_network`` (priority 30)
@@ -57,10 +59,24 @@ and ``ansible`` and any derivatives) expose the following deploy steps:
 ``deploy.boot_instance`` (priority 20)
   In this step the node is booted into the user image.
 
+Additionally, the ``iscsi`` and ``direct`` deploy interfaces have:
+
+``deploy.prepare_instance_boot`` (priority 60)
+  In this step the boot device is configured and the bootloader is installed.
+
+  .. note::
+    For the ``ansible`` deploy interface these steps are done in
+    ``deploy.write_image``.
+
 Accordingly, the following priority ranges can be used for custom deploy steps:
 
 > 100
   Out-of-band steps to run before deployment.
+81 to 99
+  In-band deploy steps to run before the image is written.
+61 to 79
+  In-band deploy steps to run after the image is written but before the
+  bootloader is installed.
 41 to 59
   In-band steps to run after the image is written the bootloader is installed.
 21 to 39
@@ -68,35 +84,6 @@ Accordingly, the following priority ranges can be used for custom deploy steps:
 1 to 19
   Any steps that are run when the user instance is already running.
 
-Direct deploy
-^^^^^^^^^^^^^
-
-The :ref:`direct-deploy` interface splits the ``deploy.deploy`` step into:
-
-
-``deploy.deploy`` (priority 100)
-  In this step the node is booted using a provisioning image.
-``deploy.write_image`` (priority 80)
-  A combination of an out-of-band and in-band step that downloads and writes
-  the image to the node. The step is executed asynchronously by the ramdisk.
-``deploy.prepare_instance_boot`` (priority 60)
-  In this step the boot device is configured and the bootloader is installed.
-
-Additional priority ranges can be used for custom deploy steps:
-
-81 to 99
-  In-band deploy steps to run before the image is written.
-61 to 79
-  In-band deploy steps to run after the image is written but before the
-  bootloader is installed.
-
-Other deploy interfaces
-^^^^^^^^^^^^^^^^^^^^^^^
-
-Priorities 60 to 99 are currently reserved and should not be used.
-
-.. TODO(dtantsur): update once iscsi and ansible are converted
-
 Writing a Deploy Step
 ---------------------
 

ironic/drivers/modules/agent_base.py

@@ -767,7 +767,53 @@ class AgentBaseMixin(object):
             task, manage_boot=self.should_manage_boot(task))
 
 
-class AgentDeployMixin(HeartbeatMixin):
+class AgentOobStepsMixin(object):
+    """Mixin with out-of-band deploy steps."""
+
+    @METRICS.timer('AgentDeployMixin.switch_to_tenant_network')
+    @base.deploy_step(priority=30)
+    @task_manager.require_exclusive_lock
+    def switch_to_tenant_network(self, task):
+        """Deploy step to switch the node to the tenant network.
+
+        :param task: a TaskManager object containing the node
+        """
+        try:
+            with manager_utils.power_state_for_network_configuration(task):
+                task.driver.network.remove_provisioning_network(task)
+                task.driver.network.configure_tenant_networks(task)
+        except Exception as e:
+            msg = (_('Error changing node %(node)s to tenant networks after '
+                     'deploy. %(cls)s: %(error)s') %
+                   {'node': task.node.uuid, 'cls': e.__class__.__name__,
+                    'error': e})
+            # NOTE(mgoddard): Don't collect logs since the node has been
+            # powered off.
+            log_and_raise_deployment_error(task, msg, collect_logs=False,
+                                           exc=e)
+
+    @METRICS.timer('AgentDeployMixin.boot_instance')
+    @base.deploy_step(priority=20)
+    @task_manager.require_exclusive_lock
+    def boot_instance(self, task):
+        """Deploy step to boot the final instance.
+
+        :param task: a TaskManager object containing the node
+        """
+        try:
+            manager_utils.node_power_action(task, states.POWER_ON)
+        except Exception as e:
+            msg = (_('Error booting node %(node)s after deploy. '
+                     '%(cls)s: %(error)s') %
+                   {'node': task.node.uuid, 'cls': e.__class__.__name__,
+                    'error': e})
+            # NOTE(mgoddard): Don't collect logs since the node has been
+            # powered off.
+            log_and_raise_deployment_error(task, msg, collect_logs=False,
+                                           exc=e)
+
+
+class AgentDeployMixin(HeartbeatMixin, AgentOobStepsMixin):
     """Mixin with deploy methods."""
 
     @METRICS.timer('AgentDeployMixin.get_clean_steps')
@@ -1169,48 +1215,6 @@ class AgentDeployMixin(HeartbeatMixin):
                     'error': e})
             log_and_raise_deployment_error(task, msg, exc=e)
 
-    @METRICS.timer('AgentDeployMixin.switch_networking')
-    @base.deploy_step(priority=30)
-    @task_manager.require_exclusive_lock
-    def switch_to_tenant_network(self, task):
-        """Deploy step to switch the node to the tenant network.
-
-        :param task: a TaskManager object containing the node
-        """
-        try:
-            with manager_utils.power_state_for_network_configuration(task):
-                task.driver.network.remove_provisioning_network(task)
-                task.driver.network.configure_tenant_networks(task)
-        except Exception as e:
-            msg = (_('Error changing node %(node)s to tenant networks after '
-                     'deploy. %(cls)s: %(error)s') %
-                   {'node': task.node.uuid, 'cls': e.__class__.__name__,
-                    'error': e})
-            # NOTE(mgoddard): Don't collect logs since the node has been
-            # powered off.
-            log_and_raise_deployment_error(task, msg, collect_logs=False,
-                                           exc=e)
-
-    @METRICS.timer('AgentDeployMixin.boot_instance')
-    @base.deploy_step(priority=20)
-    @task_manager.require_exclusive_lock
-    def boot_instance(self, task):
-        """Deploy step to boot the final instance.
-
-        :param task: a TaskManager object containing the node
-        """
-        try:
-            manager_utils.node_power_action(task, states.POWER_ON)
-        except Exception as e:
-            msg = (_('Error booting node %(node)s after deploy. '
-                     '%(cls)s: %(error)s') %
-                   {'node': task.node.uuid, 'cls': e.__class__.__name__,
-                    'error': e})
-            # NOTE(mgoddard): Don't collect logs since the node has been
-            # powered off.
-            log_and_raise_deployment_error(task, msg, collect_logs=False,
-                                           exc=e)
-
     # TODO(dtantsur): remove in W
     @METRICS.timer('AgentDeployMixin.reboot_and_finish_deploy')
     def reboot_and_finish_deploy(self, task):

ironic/drivers/modules/ansible/deploy.py

@@ -375,9 +375,13 @@ def _get_clean_steps(node, interface=None, override_priorities=None):
     return steps
 
 
-class AnsibleDeploy(agent_base.HeartbeatMixin, base.DeployInterface):
+class AnsibleDeploy(agent_base.HeartbeatMixin,
+                    agent_base.AgentOobStepsMixin,
+                    base.DeployInterface):
     """Interface for deploy-related actions."""
 
+    has_decomposed_deploy_steps = True
+
     def __init__(self):
         super(AnsibleDeploy, self).__init__()
         # NOTE(pas-ha) overriding agent creation as we won't be
@@ -442,12 +446,22 @@ class AnsibleDeploy(agent_base.HeartbeatMixin, base.DeployInterface):
         manager_utils.node_power_action(task, states.REBOOT)
         return states.DEPLOYWAIT
 
+    def process_next_step(self, task, step_type):
+        """Start the next clean/deploy step if the previous one is complete.
+
+        :param task: a TaskManager instance
+        :param step_type: "clean" or "deploy"
+        """
+        # Run the next step as soon as agent heartbeats in deploy.deploy
+        if step_type == 'deploy' and self.in_core_deploy_step(task):
+            manager_utils.notify_conductor_resume_deploy(task)
+
     @staticmethod
     def _required_image_info(task):
         """Gather and save needed image info while the context is good.
 
         Gather image info that will be needed later, during the
-        continue_deploy execution, where the context won't be the same
+        write_image execution, where the context won't be the same
         anymore, since coming from the server's heartbeat.
         """
         node = task.node
@@ -586,35 +600,30 @@ class AnsibleDeploy(agent_base.HeartbeatMixin, base.DeployInterface):
         manager_utils.restore_power_state_if_needed(
             task, power_state_to_restore)
 
-    @METRICS.timer('AnsibleDeploy.continue_deploy')
+    @METRICS.timer('AnsibleDeploy.write_image')
-    def continue_deploy(self, task):
+    @base.deploy_step(priority=80)
+    def write_image(self, task):
         # NOTE(pas-ha) the lock should be already upgraded in heartbeat,
         # just setting its purpose for better logging
         task.upgrade_lock(purpose='deploy')
-        task.process_event('resume')
         # NOTE(pas-ha) this method is called from heartbeat processing only,
         # so we are sure we need this particular method, not the general one
         node_address = _get_node_ip(task)
         self._ansible_deploy(task, node_address)
-        self.reboot_to_instance(task)
+        LOG.info('Ansible complete deploy on node %s', task.node.uuid)
-
-    @METRICS.timer('AnsibleDeploy.reboot_to_instance')
-    def reboot_to_instance(self, task):
-        node = task.node
-        LOG.info('Ansible complete deploy on node %s', node.uuid)
-
-        LOG.debug('Rebooting node %s to instance', node.uuid)
         manager_utils.node_set_boot_device(task, 'disk', persistent=True)
-        self.reboot_and_finish_deploy(task)
-        task.driver.boot.clean_up_ramdisk(task)
 
-        # TODO(dtantsur): remove these two calls when this function becomes a
+    @METRICS.timer('AnsibleDeploy.tear_down_agent')
-        # real deploy step.
+    @base.deploy_step(priority=40)
-        task.process_event('wait')
+    @task_manager.require_exclusive_lock
-        manager_utils.notify_conductor_resume_deploy(task)
+    def tear_down_agent(self, task):
+        """A deploy step to tear down the agent.
 
-    @METRICS.timer('AnsibleDeploy.reboot_and_finish_deploy')
+        Shuts down the machine and removes it from the provisioning
-    def reboot_and_finish_deploy(self, task):
+        network.
+
+        :param task: a TaskManager object containing the node
+        """
         wait = CONF.ansible.post_deploy_get_power_state_retry_interval * 1000
         attempts = CONF.ansible.post_deploy_get_power_state_retries + 1
 
@@ -652,13 +661,6 @@ class AnsibleDeploy(agent_base.HeartbeatMixin, base.DeployInterface):
                     manager_utils.node_power_action(task, states.POWER_OFF)
             else:
                 manager_utils.node_power_action(task, states.POWER_OFF)
-            power_state_to_restore = (
-                manager_utils.power_on_node_if_needed(task))
-            task.driver.network.remove_provisioning_network(task)
-            task.driver.network.configure_tenant_networks(task)
-            manager_utils.restore_power_state_if_needed(
-                task, power_state_to_restore)
-            manager_utils.node_power_action(task, states.POWER_ON)
         except Exception as e:
             msg = (_('Error rebooting node %(node)s after deploy. '
                      'Error: %(error)s') %

ironic/drivers/modules/iscsi_deploy.py

@@ -602,6 +602,8 @@ class ISCSIDeploy(agent_base.AgentDeployMixin, agent_base.AgentBaseMixin,
                   base.DeployInterface):
     """iSCSI Deploy Interface for deploy-related actions."""
 
+    has_decomposed_deploy_steps = True
+
     def get_properties(self):
         return agent_base.VENDOR_PROPERTIES
 
@@ -647,14 +649,12 @@ class ISCSIDeploy(agent_base.AgentDeployMixin, agent_base.AgentBaseMixin,
         """
         node = task.node
         if manager_utils.is_fast_track(task):
+            # NOTE(mgoddard): For fast track we can mostly skip this step and
+            # proceed to the next step (i.e. write_image).
             LOG.debug('Performing a fast track deployment for %(node)s.',
                       {'node': task.node.uuid})
             deploy_utils.cache_instance_image(task.context, node)
             check_image_size(task)
-            # Update the database for the API and the task tracking resumes
-            # the state machine state going from DEPLOYWAIT -> DEPLOYING
-            task.process_event('wait')
-            self.continue_deploy(task)
         elif task.driver.storage.should_write_image(task):
             # Standard deploy process
             deploy_utils.cache_instance_image(task.context, node)
@@ -666,29 +666,16 @@ class ISCSIDeploy(agent_base.AgentDeployMixin, agent_base.AgentBaseMixin,
                 manager_utils.node_power_action(task, states.REBOOT)
             info = task.node.driver_internal_info
             info.pop('deployment_reboot', None)
+            info.pop('deployment_uuids', None)
             task.node.driver_internal_info = info
             task.node.save()
 
             return states.DEPLOYWAIT
-        else:
-            # Boot to an Storage Volume
 
-            # TODO(TheJulia): At some point, we should de-dupe this code
+    @METRICS.timer('ISCSIDeploy.write_image')
-            # as it is nearly identical to the agent deploy interface.
+    @base.deploy_step(priority=90)
-            # This is not being done now as it is expected to be
-            # refactored in the near future.
-            manager_utils.node_power_action(task, states.POWER_OFF)
-            with manager_utils.power_state_for_network_configuration(task):
-                task.driver.network.remove_provisioning_network(task)
-                task.driver.network.configure_tenant_networks(task)
-            task.driver.boot.prepare_instance(task)
-            manager_utils.node_power_action(task, states.POWER_ON)
-
-            return None
-
-    @METRICS.timer('AgentDeployMixin.continue_deploy')
     @task_manager.require_exclusive_lock
-    def continue_deploy(self, task):
+    def write_image(self, task):
         """Method invoked when deployed using iSCSI.
 
         This method is invoked during a heartbeat from an agent when
@@ -701,18 +688,39 @@ class ISCSIDeploy(agent_base.AgentDeployMixin, agent_base.AgentBaseMixin,
         :raises: InstanceDeployFailure, if it encounters some error during
             the deploy.
         """
-        task.process_event('resume')
+        if not task.driver.storage.should_write_image(task):
+            LOG.debug('Skipping write_image for node %s', task.node.uuid)
+            return
+
         node = task.node
         LOG.debug('Continuing the deployment on node %s', node.uuid)
 
         uuid_dict_returned = do_agent_iscsi_deploy(task, self._client)
+        utils.set_node_nested_field(node, 'driver_internal_info',
+                                    'deployment_uuids', uuid_dict_returned)
+        node.save()
+
+    @METRICS.timer('ISCSIDeploy.prepare_instance_boot')
+    @base.deploy_step(priority=80)
+    def prepare_instance_boot(self, task):
+        if not task.driver.storage.should_write_image(task):
+            task.driver.boot.prepare_instance(task)
+            return
+
+        node = task.node
+        try:
+            uuid_dict_returned = node.driver_internal_info['deployment_uuids']
+        except KeyError:
+            raise exception.InstanceDeployFailure(
+                _('Invalid internal state: the write_image deploy step has '
+                  'not been called before prepare_instance_boot'))
         root_uuid = uuid_dict_returned.get('root uuid')
         efi_sys_uuid = uuid_dict_returned.get('efi system partition uuid')
         prep_boot_part_uuid = uuid_dict_returned.get(
             'PrEP Boot partition uuid')
+
         self.prepare_instance_to_boot(task, root_uuid, efi_sys_uuid,
                                       prep_boot_part_uuid=prep_boot_part_uuid)
-        self.reboot_and_finish_deploy(task)
 
     @METRICS.timer('ISCSIDeploy.prepare')
     @task_manager.require_exclusive_lock
@@ -788,3 +796,6 @@ class ISCSIDeploy(agent_base.AgentDeployMixin, agent_base.AgentBaseMixin,
         """
         deploy_utils.destroy_images(task.node.uuid)
         super(ISCSIDeploy, self).clean_up(task)
+        if utils.pop_node_nested_field(task.node, 'driver_internal_info',
+                                       'deployment_uuids'):
+            task.node.save()

ironic/tests/unit/drivers/modules/ansible/test_deploy.py

@@ -890,13 +890,11 @@ class TestAnsibleDeploy(AnsibleDeployTestCaseBase):
             run_playbook_mock.assert_called_once_with(
                 task.node, 'test_pl', ironic_nodes, 'test_k')
 
-    @mock.patch.object(utils, 'power_on_node_if_needed', autospec=True)
     @mock.patch.object(fake.FakePower, 'get_power_state',
                        return_value=states.POWER_OFF, autospec=True)
     @mock.patch.object(utils, 'node_power_action', autospec=True)
-    def test_reboot_and_finish_deploy_force_reboot(
+    def test_tear_down_agent_force_reboot(
-            self, power_action_mock, get_pow_state_mock,
+            self, power_action_mock, get_pow_state_mock):
-            power_on_node_if_needed_mock):
         d_info = self.node.driver_info
         d_info['deploy_forces_oob_reboot'] = True
         self.node.driver_info = d_info
@@ -906,27 +904,15 @@ class TestAnsibleDeploy(AnsibleDeployTestCaseBase):
         self.node.provision_state = states.DEPLOYING
         self.node.save()
 
-        power_on_node_if_needed_mock.return_value = None
         with task_manager.acquire(self.context, self.node.uuid) as task:
-            with mock.patch.object(task.driver, 'network',
+            self.driver.tear_down_agent(task)
-                                   autospec=True) as net_mock:
+            power_action_mock.assert_called_once_with(task, states.POWER_OFF)
-                self.driver.reboot_and_finish_deploy(task)
-                net_mock.remove_provisioning_network.assert_called_once_with(
-                    task)
-                net_mock.configure_tenant_networks.assert_called_once_with(
-                    task)
-            expected_power_calls = [((task, states.POWER_OFF),),
-                                    ((task, states.POWER_ON),)]
-            self.assertEqual(expected_power_calls,
-                             power_action_mock.call_args_list)
         get_pow_state_mock.assert_not_called()
 
-    @mock.patch.object(utils, 'power_on_node_if_needed', autospec=True)
     @mock.patch.object(ansible_deploy, '_run_playbook', autospec=True)
     @mock.patch.object(utils, 'node_power_action', autospec=True)
-    def test_reboot_and_finish_deploy_soft_poweroff_retry(
+    def test_tear_down_agent_soft_poweroff_retry(
-            self, power_action_mock, run_playbook_mock,
+            self, power_action_mock, run_playbook_mock):
-            power_on_node_if_needed_mock):
         self.config(group='ansible',
                     post_deploy_get_power_state_retry_interval=0)
         self.config(group='ansible',
@@ -937,84 +923,38 @@ class TestAnsibleDeploy(AnsibleDeployTestCaseBase):
         self.node.driver_internal_info = di_info
         self.node.save()
 
-        power_on_node_if_needed_mock.return_value = None
         with task_manager.acquire(self.context, self.node.uuid) as task:
-            with mock.patch.object(task.driver, 'network',
+            with mock.patch.object(task.driver.power,
-                                   autospec=True) as net_mock:
+                                   'get_power_state',
-                with mock.patch.object(task.driver.power,
+                                   return_value=states.POWER_ON,
-                                       'get_power_state',
+                                   autospec=True) as p_mock:
-                                       return_value=states.POWER_ON,
+                self.driver.tear_down_agent(task)
-                                       autospec=True) as p_mock:
+                p_mock.assert_called_with(task)
-                    self.driver.reboot_and_finish_deploy(task)
+                self.assertEqual(2, len(p_mock.mock_calls))
-                    p_mock.assert_called_with(task)
+            power_action_mock.assert_called_once_with(task, states.POWER_OFF)
-                    self.assertEqual(2, len(p_mock.mock_calls))
-                net_mock.remove_provisioning_network.assert_called_once_with(
-                    task)
-                net_mock.configure_tenant_networks.assert_called_once_with(
-                    task)
-            power_action_mock.assert_has_calls(
-                [mock.call(task, states.POWER_OFF),
-                 mock.call(task, states.POWER_ON)])
-            expected_power_calls = [((task, states.POWER_OFF),),
-                                    ((task, states.POWER_ON),)]
-            self.assertEqual(expected_power_calls,
-                             power_action_mock.call_args_list)
             run_playbook_mock.assert_called_once_with(
                 task.node, 'shutdown.yaml', mock.ANY, mock.ANY)
 
+    @mock.patch.object(utils, 'node_set_boot_device', autospec=True)
     @mock.patch.object(ansible_deploy, '_get_node_ip', autospec=True,
                        return_value='1.2.3.4')
-    def test_continue_deploy(self, getip_mock):
+    def test_write_image(self, getip_mock, bootdev_mock):
-        self.node.provision_state = states.DEPLOYWAIT
+        self.node.provision_state = states.DEPLOYING
         self.node.target_provision_state = states.ACTIVE
         self.node.save()
         with task_manager.acquire(self.context, self.node.uuid) as task:
             with mock.patch.multiple(self.driver, autospec=True,
                                      _ansible_deploy=mock.DEFAULT,
                                      reboot_to_instance=mock.DEFAULT):
-                self.driver.continue_deploy(task)
+                result = self.driver.write_image(task)
+                self.assertIsNone(result)
                 getip_mock.assert_called_once_with(task)
                 self.driver._ansible_deploy.assert_called_once_with(
                     task, '1.2.3.4')
-                self.driver.reboot_to_instance.assert_called_once_with(task)
-            self.assertEqual(states.ACTIVE, task.node.target_provision_state)
-            self.assertEqual(states.DEPLOYING, task.node.provision_state)
-
-    @mock.patch.object(utils, 'notify_conductor_resume_deploy', autospec=True)
-    @mock.patch.object(utils, 'node_set_boot_device', autospec=True)
-    def test_reboot_to_instance(self, bootdev_mock, resume_mock):
-        self.node.provision_state = states.DEPLOYING
-        self.node.deploy_step = {
-            'step': 'deploy', 'priority': 100, 'interface': 'deploy'}
-        self.node.save()
-        with task_manager.acquire(self.context, self.node.uuid) as task:
-            with mock.patch.object(self.driver, 'reboot_and_finish_deploy',
-                                   autospec=True):
-                task.driver.boot = mock.Mock()
-                self.driver.reboot_to_instance(task)
                 bootdev_mock.assert_called_once_with(task, 'disk',
                                                      persistent=True)
-                resume_mock.assert_called_once_with(task)
+            self.assertEqual(states.ACTIVE, task.node.target_provision_state)
-                self.driver.reboot_and_finish_deploy.assert_called_once_with(
+            self.assertEqual(states.DEPLOYING, task.node.provision_state)
-                    task)
-                task.driver.boot.clean_up_ramdisk.assert_called_once_with(
-                    task)
-
-    @mock.patch.object(utils, 'restore_power_state_if_needed', autospec=True)
-    @mock.patch.object(utils, 'power_on_node_if_needed', autospec=True)
-    @mock.patch.object(utils, 'node_power_action', autospec=True)
-    def test_tear_down_with_smartnic_port(
-            self, power_mock, power_on_node_if_needed_mock,
-            restore_power_state_mock):
-        with task_manager.acquire(
-                self.context, self.node['uuid'], shared=False) as task:
-            power_on_node_if_needed_mock.return_value = states.POWER_OFF
-            driver_return = self.driver.tear_down(task)
-            power_mock.assert_called_once_with(task, states.POWER_OFF)
-            self.assertEqual(driver_return, states.DELETED)
-            power_on_node_if_needed_mock.assert_called_once_with(task)
-            restore_power_state_mock.assert_called_once_with(
-                task, states.POWER_OFF)
 
     @mock.patch.object(flat_network.FlatNetwork, 'add_provisioning_network',
                        autospec=True)
@@ -1105,36 +1045,3 @@ class TestAnsibleDeploy(AnsibleDeployTestCaseBase):
             power_on_node_if_needed_mock.assert_called_once_with(task)
             restore_power_state_mock.assert_called_once_with(
                 task, states.POWER_OFF)
-
-    @mock.patch.object(flat_network.FlatNetwork, 'remove_provisioning_network',
-                       autospec=True)
-    @mock.patch.object(flat_network.FlatNetwork, 'configure_tenant_networks',
-                       autospec=True)
-    @mock.patch.object(utils, 'restore_power_state_if_needed', autospec=True)
-    @mock.patch.object(utils, 'power_on_node_if_needed', autospec=True)
-    @mock.patch.object(fake.FakePower, 'get_power_state',
-                       return_value=states.POWER_OFF, autospec=True)
-    @mock.patch.object(utils, 'node_power_action', autospec=True)
-    def test_reboot_and_finish_deploy_with_smartnic_port(
-            self, power_action_mock, get_pow_state_mock,
-            power_on_node_if_needed_mock, restore_power_state_mock,
-            configure_tenant_networks_mock, remove_provisioning_network_mock):
-        d_info = self.node.driver_info
-        d_info['deploy_forces_oob_reboot'] = True
-        self.node.driver_info = d_info
-        self.node.save()
-        self.config(group='ansible',
-                    post_deploy_get_power_state_retry_interval=0)
-        self.node.provision_state = states.DEPLOYING
-        self.node.save()
-        power_on_node_if_needed_mock.return_value = states.POWER_OFF
-        with task_manager.acquire(self.context, self.node.uuid) as task:
-            self.driver.reboot_and_finish_deploy(task)
-            expected_power_calls = [((task, states.POWER_OFF),),
-                                    ((task, states.POWER_ON),)]
-            self.assertEqual(
-                expected_power_calls, power_action_mock.call_args_list)
-            power_on_node_if_needed_mock.assert_called_once_with(task)
-            restore_power_state_mock.assert_called_once_with(
-                task, states.POWER_OFF)
-        get_pow_state_mock.assert_not_called()

ironic/tests/unit/drivers/modules/test_iscsi_deploy.py

@@ -348,8 +348,8 @@ class IscsiDeployMethodsTestCase(db_base.DbTestCase):
             'node': self.node.uuid,
             'params': log_params,
         }
-        uuid_dict_returned = {'root uuid': '12345678-87654321'}
+        deployment_uuids = {'root uuid': '12345678-87654321'}
-        deploy_mock.return_value = uuid_dict_returned
+        deploy_mock.return_value = deployment_uuids
 
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=False) as task:
@@ -362,7 +362,7 @@ class IscsiDeployMethodsTestCase(db_base.DbTestCase):
             self.assertIsNone(task.node.last_error)
             mock_image_cache.assert_called_once_with()
             mock_image_cache.return_value.clean_up.assert_called_once_with()
-            self.assertEqual(uuid_dict_returned, retval)
+            self.assertEqual(deployment_uuids, retval)
             mock_disk_layout.assert_called_once_with(task.node, mock.ANY)
 
     @mock.patch.object(iscsi_deploy, 'LOG', autospec=True)
@@ -392,8 +392,8 @@ class IscsiDeployMethodsTestCase(db_base.DbTestCase):
             'node': self.node.uuid,
             'params': log_params,
         }
-        uuid_dict_returned = {'disk identifier': '87654321'}
+        deployment_uuids = {'disk identifier': '87654321'}
-        deploy_mock.return_value = uuid_dict_returned
+        deploy_mock.return_value = deployment_uuids
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=False) as task:
             task.node.driver_internal_info['is_whole_disk_image'] = True
@@ -406,7 +406,7 @@ class IscsiDeployMethodsTestCase(db_base.DbTestCase):
             self.assertIsNone(task.node.last_error)
             mock_image_cache.assert_called_once_with()
             mock_image_cache.return_value.clean_up.assert_called_once_with()
-            self.assertEqual(uuid_dict_returned, retval)
+            self.assertEqual(deployment_uuids, retval)
 
     def _test_get_deploy_info(self, extra_instance_info=None):
         if extra_instance_info is None:
@@ -489,8 +489,8 @@ class IscsiDeployMethodsTestCase(db_base.DbTestCase):
         driver_internal_info = {'agent_url': 'http://1.2.3.4:1234'}
         self.node.driver_internal_info = driver_internal_info
         self.node.save()
-        uuid_dict_returned = {'root uuid': 'some-root-uuid'}
+        deployment_uuids = {'root uuid': 'some-root-uuid'}
-        continue_deploy_mock.return_value = uuid_dict_returned
+        continue_deploy_mock.return_value = deployment_uuids
         expected_iqn = 'iqn.2008-10.org.openstack:%s' % self.node.uuid
 
         with task_manager.acquire(self.context, self.node.uuid,
@@ -504,7 +504,7 @@ class IscsiDeployMethodsTestCase(db_base.DbTestCase):
             self.assertEqual(
                 'some-root-uuid',
                 task.node.driver_internal_info['root_uuid_or_disk_id'])
-            self.assertEqual(ret_val, uuid_dict_returned)
+            self.assertEqual(ret_val, deployment_uuids)
 
     @mock.patch.object(iscsi_deploy, 'continue_deploy', autospec=True)
     def test_do_agent_iscsi_deploy_preserve_ephemeral(self,
@@ -517,8 +517,8 @@ class IscsiDeployMethodsTestCase(db_base.DbTestCase):
             'agent_url': 'http://1.2.3.4:1234'}
         self.node.driver_internal_info = driver_internal_info
         self.node.save()
-        uuid_dict_returned = {'root uuid': 'some-root-uuid'}
+        deployment_uuids = {'root uuid': 'some-root-uuid'}
-        continue_deploy_mock.return_value = uuid_dict_returned
+        continue_deploy_mock.return_value = deployment_uuids
         expected_iqn = 'iqn.2008-10.org.openstack:%s' % self.node.uuid
 
         with task_manager.acquire(self.context, self.node.uuid,
@@ -831,54 +831,31 @@ class ISCSIDeployTestCase(db_base.DbTestCase):
             self.assertNotIn(
                 'deployment_reboot', task.node.driver_internal_info)
 
+    @mock.patch.object(manager_utils, 'node_power_action', autospec=True)
     @mock.patch.object(noop_storage.NoopStorage, 'should_write_image',
                        autospec=True)
-    @mock.patch.object(flat_network.FlatNetwork,
+    def test_deploy_storage_should_write_image_false(
-                       'configure_tenant_networks',
+            self, mock_write, mock_node_power_action):
-                       spec_set=True, autospec=True)
-    @mock.patch.object(flat_network.FlatNetwork,
-                       'remove_provisioning_network',
-                       spec_set=True, autospec=True)
-    @mock.patch.object(pxe.PXEBoot,
-                       'prepare_instance',
-                       spec_set=True, autospec=True)
-    @mock.patch.object(manager_utils, 'node_power_action', autospec=True)
-    @mock.patch.object(iscsi_deploy, 'check_image_size', autospec=True)
-    @mock.patch.object(deploy_utils, 'cache_instance_image', autospec=True)
-    def test_deploy_storage_check_write_image_false(self,
-                                                    mock_cache_instance_image,
-                                                    mock_check_image_size,
-                                                    mock_node_power_action,
-                                                    mock_prepare_instance,
-                                                    mock_remove_network,
-                                                    mock_tenant_network,
-                                                    mock_write):
         mock_write.return_value = False
         self.node.provision_state = states.DEPLOYING
         self.node.deploy_step = {
             'step': 'deploy', 'priority': 50, 'interface': 'deploy'}
         self.node.save()
-        with task_manager.acquire(self.context,
+        with task_manager.acquire(
-                                  self.node.uuid, shared=False) as task:
+                self.context, self.node.uuid, shared=False) as task:
             ret = task.driver.deploy.deploy(task)
             self.assertIsNone(ret)
-            self.assertFalse(mock_cache_instance_image.called)
+            self.assertFalse(mock_node_power_action.called)
-            self.assertFalse(mock_check_image_size.called)
-            mock_remove_network.assert_called_once_with(mock.ANY, task)
-            mock_tenant_network.assert_called_once_with(mock.ANY, task)
-            mock_prepare_instance.assert_called_once_with(mock.ANY, task)
-            self.assertEqual(2, mock_node_power_action.call_count)
-            self.assertEqual(states.DEPLOYING, task.node.provision_state)
 
     @mock.patch.object(iscsi_deploy, 'check_image_size', autospec=True)
     @mock.patch.object(deploy_utils, 'cache_instance_image', autospec=True)
-    @mock.patch.object(iscsi_deploy.ISCSIDeploy, 'continue_deploy',
+    @mock.patch.object(iscsi_deploy.ISCSIDeploy, 'write_image',
                        autospec=True)
     @mock.patch('ironic.conductor.utils.is_fast_track', autospec=True)
     @mock.patch.object(pxe.PXEBoot, 'prepare_instance', autospec=True)
     @mock.patch('ironic.conductor.utils.node_power_action', autospec=True)
     def test_deploy_fast_track(self, power_mock, mock_pxe_instance,
-                               mock_is_fast_track, continue_deploy_mock,
+                               mock_is_fast_track, write_image_mock,
                                cache_image_mock, check_image_size_mock):
         mock_is_fast_track.return_value = True
         self.node.target_provision_state = states.ACTIVE
@@ -889,16 +866,17 @@ class ISCSIDeployTestCase(db_base.DbTestCase):
         self.node.save()
         with task_manager.acquire(
                 self.context, self.node['uuid'], shared=False) as task:
-            task.driver.deploy.deploy(task)
+            result = task.driver.deploy.deploy(task)
+            self.assertIsNone(result)
             self.assertFalse(power_mock.called)
             self.assertFalse(mock_pxe_instance.called)
             task.node.refresh()
-            self.assertEqual(states.DEPLOYWAIT, task.node.provision_state)
+            self.assertEqual(states.DEPLOYING, task.node.provision_state)
             self.assertEqual(states.ACTIVE,
                              task.node.target_provision_state)
             cache_image_mock.assert_called_with(mock.ANY, task.node)
             check_image_size_mock.assert_called_with(task)
-            continue_deploy_mock.assert_called_with(mock.ANY, task)
+            self.assertFalse(write_image_mock.called)
 
     @mock.patch.object(noop_storage.NoopStorage, 'detach_volumes',
                        autospec=True)
@@ -995,90 +973,95 @@ class ISCSIDeployTestCase(db_base.DbTestCase):
             agent_execute_clean_step_mock.assert_called_once_with(
                 task, {'some-step': 'step-info'}, 'clean')
 
-    @mock.patch.object(agent_base.AgentDeployMixin,
-                       'reboot_and_finish_deploy', autospec=True)
     @mock.patch.object(iscsi_deploy, 'do_agent_iscsi_deploy', autospec=True)
-    def test_continue_deploy_netboot(self, do_agent_iscsi_deploy_mock,
+    def test_write_image(self, do_agent_iscsi_deploy_mock):
-                                     reboot_and_finish_deploy_mock):
 
         self.node.instance_info = {
             'capabilities': {'boot_option': 'netboot'}}
         self.node.provision_state = states.DEPLOYWAIT
         self.node.target_provision_state = states.ACTIVE
         self.node.save()
-        uuid_dict_returned = {'root uuid': 'some-root-uuid'}
+        deployment_uuids = {'root uuid': 'some-root-uuid'}
-        do_agent_iscsi_deploy_mock.return_value = uuid_dict_returned
+        do_agent_iscsi_deploy_mock.return_value = deployment_uuids
         self.node.save()
+        with task_manager.acquire(self.context, self.node.uuid) as task:
+            task.driver.deploy.write_image(task)
+            do_agent_iscsi_deploy_mock.assert_called_once_with(
+                task, task.driver.deploy._client)
+            self.assertEqual(
+                task.node.driver_internal_info['deployment_uuids'],
+                deployment_uuids)
+
+    @mock.patch.object(noop_storage.NoopStorage, 'should_write_image',
+                       autospec=True)
+    @mock.patch.object(iscsi_deploy, 'do_agent_iscsi_deploy', autospec=True)
+    def test_write_image_bfv(self, do_agent_iscsi_deploy_mock,
+                             should_write_image_mock):
+        should_write_image_mock.return_value = False
+        with task_manager.acquire(self.context, self.node.uuid) as task:
+            task.driver.deploy.write_image(task)
+            self.assertFalse(do_agent_iscsi_deploy_mock.called)
+
+    def test_prepare_instance_boot_netboot(self):
+        deployment_uuids = {'root uuid': 'some-root-uuid'}
+        self.node.instance_info = {
+            'capabilities': {'boot_option': 'netboot'}}
+        self.node.provision_state = states.DEPLOYWAIT
+        self.node.target_provision_state = states.ACTIVE
+        info = self.node.driver_internal_info
+        info['deployment_uuids'] = deployment_uuids
+        self.node.driver_internal_info = info
+        self.node.save()
+
         with task_manager.acquire(self.context, self.node.uuid) as task:
             with mock.patch.object(
                     task.driver.boot, 'prepare_instance') as m_prep_instance:
-                task.driver.deploy.continue_deploy(task)
+                task.driver.deploy.prepare_instance_boot(task)
-                do_agent_iscsi_deploy_mock.assert_called_once_with(
-                    task, task.driver.deploy._client)
-                reboot_and_finish_deploy_mock.assert_called_once_with(
-                    mock.ANY, task)
                 m_prep_instance.assert_called_once_with(task)
 
     @mock.patch.object(fake.FakeManagement, 'set_boot_device', autospec=True)
-    @mock.patch.object(agent_base.AgentDeployMixin,
-                       'reboot_and_finish_deploy', autospec=True)
     @mock.patch.object(agent_base.AgentDeployMixin,
                        'configure_local_boot', autospec=True)
-    @mock.patch.object(iscsi_deploy, 'do_agent_iscsi_deploy', autospec=True)
+    def test_prepare_instance_boot_localboot(self, configure_local_boot_mock,
-    def test_continue_deploy_localboot(self, do_agent_iscsi_deploy_mock,
+                                             set_boot_device_mock):
-                                       configure_local_boot_mock,
-                                       reboot_and_finish_deploy_mock,
-                                       set_boot_device_mock):
 
-        self.node.instance_info = {
+        deployment_uuids = {'root uuid': 'some-root-uuid'}
-            'capabilities': {'boot_option': 'local'}}
         self.node.provision_state = states.DEPLOYWAIT
         self.node.target_provision_state = states.ACTIVE
+        info = self.node.driver_internal_info
+        info['deployment_uuids'] = deployment_uuids
+        self.node.driver_internal_info = info
         self.node.save()
-        uuid_dict_returned = {'root uuid': 'some-root-uuid'}
-        do_agent_iscsi_deploy_mock.return_value = uuid_dict_returned
 
         with task_manager.acquire(self.context, self.node.uuid) as task:
-            task.driver.deploy.continue_deploy(task)
+            task.driver.deploy.prepare_instance_boot(task)
-            do_agent_iscsi_deploy_mock.assert_called_once_with(
-                task, task.driver.deploy._client)
             configure_local_boot_mock.assert_called_once_with(
                 task.driver.deploy, task, root_uuid='some-root-uuid',
                 efi_system_part_uuid=None, prep_boot_part_uuid=None)
-            reboot_and_finish_deploy_mock.assert_called_once_with(
-                task.driver.deploy, task)
             set_boot_device_mock.assert_called_once_with(
                 mock.ANY, task, device=boot_devices.DISK, persistent=True)
 
     @mock.patch.object(fake.FakeManagement, 'set_boot_device', autospec=True)
-    @mock.patch.object(agent_base.AgentDeployMixin,
-                       'reboot_and_finish_deploy', autospec=True)
     @mock.patch.object(agent_base.AgentDeployMixin,
                        'configure_local_boot', autospec=True)
-    @mock.patch.object(iscsi_deploy, 'do_agent_iscsi_deploy', autospec=True)
+    def test_prepare_instance_boot_localboot_uefi(
-    def test_continue_deploy_localboot_uefi(self, do_agent_iscsi_deploy_mock,
+            self, configure_local_boot_mock, set_boot_device_mock):
-                                            configure_local_boot_mock,
+        deployment_uuids = {'root uuid': 'some-root-uuid',
-                                            reboot_and_finish_deploy_mock,
+                            'efi system partition uuid': 'efi-part-uuid'}
-                                            set_boot_device_mock):
-
         self.node.instance_info = {
             'capabilities': {'boot_option': 'local'}}
         self.node.provision_state = states.DEPLOYWAIT
         self.node.target_provision_state = states.ACTIVE
+        info = self.node.driver_internal_info
+        info['deployment_uuids'] = deployment_uuids
+        self.node.driver_internal_info = info
         self.node.save()
-        uuid_dict_returned = {'root uuid': 'some-root-uuid',
-                              'efi system partition uuid': 'efi-part-uuid'}
-        do_agent_iscsi_deploy_mock.return_value = uuid_dict_returned
 
         with task_manager.acquire(self.context, self.node.uuid) as task:
-            task.driver.deploy.continue_deploy(task)
+            task.driver.deploy.prepare_instance_boot(task)
-            do_agent_iscsi_deploy_mock.assert_called_once_with(
-                task, task.driver.deploy._client)
             configure_local_boot_mock.assert_called_once_with(
                 task.driver.deploy, task, root_uuid='some-root-uuid',
                 efi_system_part_uuid='efi-part-uuid', prep_boot_part_uuid=None)
-            reboot_and_finish_deploy_mock.assert_called_once_with(
-                task.driver.deploy, task)
             set_boot_device_mock.assert_called_once_with(
                 mock.ANY, task, device=boot_devices.DISK, persistent=True)
 
@@ -1157,49 +1140,6 @@ class ISCSIDeployTestCase(db_base.DbTestCase):
                                   self.node.uuid, shared=False) as task:
             self.assertEqual(0, len(task.volume_targets))
 
-    @mock.patch.object(manager_utils, 'restore_power_state_if_needed',
-                       autospec=True)
-    @mock.patch.object(manager_utils, 'power_on_node_if_needed', autospec=True)
-    @mock.patch.object(noop_storage.NoopStorage, 'should_write_image',
-                       autospec=True)
-    @mock.patch.object(flat_network.FlatNetwork,
-                       'configure_tenant_networks',
-                       spec_set=True, autospec=True)
-    @mock.patch.object(flat_network.FlatNetwork,
-                       'remove_provisioning_network',
-                       spec_set=True, autospec=True)
-    @mock.patch.object(pxe.PXEBoot,
-                       'prepare_instance',
-                       spec_set=True, autospec=True)
-    @mock.patch.object(manager_utils, 'node_power_action', autospec=True)
-    @mock.patch.object(iscsi_deploy, 'check_image_size', autospec=True)
-    @mock.patch.object(deploy_utils, 'cache_instance_image', autospec=True)
-    def test_deploy_storage_check_write_image_false_with_smartnic_port(
-            self, mock_cache_instance_image, mock_check_image_size,
-            mock_node_power_action, mock_prepare_instance,
-            mock_remove_network, mock_tenant_network, mock_write,
-            power_on_node_if_needed_mock, restore_power_state_mock):
-        mock_write.return_value = False
-        self.node.provision_state = states.DEPLOYING
-        self.node.deploy_step = {
-            'step': 'deploy', 'priority': 50, 'interface': 'deploy'}
-        self.node.save()
-        with task_manager.acquire(
-                self.context, self.node.uuid, shared=False) as task:
-            power_on_node_if_needed_mock.return_value = states.POWER_OFF
-            ret = task.driver.deploy.deploy(task)
-            self.assertIsNone(ret)
-            self.assertFalse(mock_cache_instance_image.called)
-            self.assertFalse(mock_check_image_size.called)
-            mock_remove_network.assert_called_once_with(mock.ANY, task)
-            mock_tenant_network.assert_called_once_with(mock.ANY, task)
-            mock_prepare_instance.assert_called_once_with(mock.ANY, task)
-            self.assertEqual(2, mock_node_power_action.call_count)
-            self.assertEqual(states.DEPLOYING, task.node.provision_state)
-            power_on_node_if_needed_mock.assert_called_once_with(task)
-            restore_power_state_mock.assert_called_once_with(
-                task, states.POWER_OFF)
-
 
 # Cleanup of iscsi_deploy with pxe boot interface
 class CleanUpFullFlowTestCase(db_base.DbTestCase):

releasenotes/notes/iscsi-ansible-steps-817b52269d2455b0.yaml

@@ -0,0 +1,28 @@
+---
+features:
+  - |
+    The ``deploy`` deploy step of the ``iscsi`` deploy interface has been
+    split into three deploy steps:
+
+    * ``deploy`` itself (priority 100) boots the deploy ramdisk
+
+    * ``write_image`` (priority 80) writes the image to the disk exposed
+      via iSCSI.
+
+    * ``prepare_instance_boot`` (priority 60) prepares the boot device and
+      writes the bootloader (if needed).
+
+    Priorities 81 to 99 to be used for in-band deploy steps that run before
+    the image is written. Priorities 61 to 79 can be used for in-band deploy
+    steps that modify the written image before the bootloader is installed.
+  - |
+    The ``deploy`` deploy step of the ``ansible`` deploy interface has been
+    split into two deploy steps:
+
+    * ``deploy`` itself (priority 100) boots the deploy ramdisk
+
+    * ``write_image`` (priority 80) writes the image to the disk and configures
+      the bootloader.
+
+    Priorities 81 to 99 to be used for in-band deploy steps that run before
+    the image is written.