From dc6f92c973272e62d07c69703e2094a3142201a8 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Wed, 12 Feb 2020 15:15:30 +0100 Subject: [PATCH] Refactoring: finish splitting do_node_deploy This change splits ConductorManager.do_node_deploy into two new functions: validate_node and start_deploy. The only small behavior change is validating that an FSM action is possible before making any changes (previously we did it in the very end, potentiall after modifying a node). Change-Id: I635a30264f25d758850fd57c0f07e7a68b332fe3 --- ironic/conductor/deployments.py | 87 +++++++++++++++++++++ ironic/conductor/manager.py | 67 +--------------- ironic/tests/unit/conductor/test_manager.py | 4 +- 3 files changed, 92 insertions(+), 66 deletions(-) diff --git a/ironic/conductor/deployments.py b/ironic/conductor/deployments.py index 470bed67b6..f94afd8ea2 100644 --- a/ironic/conductor/deployments.py +++ b/ironic/conductor/deployments.py @@ -21,7 +21,9 @@ from oslo_utils import excutils from oslo_utils import versionutils from ironic.common import exception +from ironic.common.glance_service import service_utils as glance_utils from ironic.common.i18n import _ +from ironic.common import images from ironic.common import release_mappings as versions from ironic.common import states from ironic.common import swift @@ -41,6 +43,91 @@ METRICS = metrics_utils.get_metrics_logger(__name__) _SEEN_NO_DEPLOY_STEP_DEPRECATIONS = set() +def validate_node(task, event='deploy'): + """Validate that a node is suitable for deployment/rebuilding. + + :param task: a TaskManager instance. + :param event: event to process: deploy or rebuild. + :raises: NodeInMaintenance, NodeProtected, InvalidStateRequested + """ + if task.node.maintenance: + raise exception.NodeInMaintenance(op=_('provisioning'), + node=task.node.uuid) + + if event == 'rebuild' and task.node.protected: + raise exception.NodeProtected(node=task.node.uuid) + + if not task.fsm.is_actionable_event(event): + raise exception.InvalidStateRequested( + action=event, node=task.node.uuid, state=task.node.provision_state) + + +@METRICS.timer('start_deploy') +@task_manager.require_exclusive_lock +def start_deploy(task, manager, configdrive=None, event='deploy'): + """Start deployment or rebuilding on a node. + + This function does not check the node suitability for deployment, it's left + up to the caller. + + :param task: a TaskManager instance. + :param manager: a ConductorManager to run tasks on. + :param configdrive: a configdrive, if requested. + :param event: event to process: deploy or rebuild. + """ + node = task.node + # Record of any pre-existing agent_url should be removed + # except when we are in fast track conditions. + if not utils.is_fast_track(task): + utils.remove_agent_url(node) + + if event == 'rebuild': + # Note(gilliard) Clear these to force the driver to + # check whether they have been changed in glance + # NOTE(vdrok): If image_source is not from Glance we should + # not clear kernel and ramdisk as they're input manually + if glance_utils.is_glance_image( + node.instance_info.get('image_source')): + instance_info = node.instance_info + instance_info.pop('kernel', None) + instance_info.pop('ramdisk', None) + node.instance_info = instance_info + + driver_internal_info = node.driver_internal_info + # Infer the image type to make sure the deploy driver + # validates only the necessary variables for different + # image types. + # NOTE(sirushtim): The iwdi variable can be None. It's up to + # the deploy driver to validate this. + iwdi = images.is_whole_disk_image(task.context, node.instance_info) + driver_internal_info['is_whole_disk_image'] = iwdi + node.driver_internal_info = driver_internal_info + node.save() + + try: + task.driver.power.validate(task) + task.driver.deploy.validate(task) + utils.validate_instance_info_traits(task.node) + conductor_steps.validate_deploy_templates(task) + except exception.InvalidParameterValue as e: + raise exception.InstanceDeployFailure( + _("Failed to validate deploy or power info for node " + "%(node_uuid)s. Error: %(msg)s") % + {'node_uuid': node.uuid, 'msg': e}, code=e.code) + + try: + task.process_event( + event, + callback=manager._spawn_worker, + call_args=(do_node_deploy, task, + manager.conductor.id, configdrive), + err_handler=utils.provisioning_error_handler) + except exception.InvalidState: + raise exception.InvalidStateRequested( + action=event, node=task.node.uuid, + state=task.node.provision_state) + + @METRICS.timer('do_node_deploy') @task_manager.require_exclusive_lock def do_node_deploy(task, conductor_id=None, configdrive=None): diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index e01ffaf4ad..557309b8ce 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -56,7 +56,6 @@ from oslo_utils import uuidutils from ironic.common import driver_factory from ironic.common import exception from ironic.common import faults -from ironic.common.glance_service import service_utils as glance_utils from ironic.common.i18n import _ from ironic.common import images from ironic.common import network @@ -822,6 +821,7 @@ class ConductorManager(base_manager.BaseConductorManager): :raises: NodeProtected if the node is protected. """ LOG.debug("RPC do_node_deploy called for node %s.", node_id) + event = 'rebuild' if rebuild else 'deploy' # NOTE(comstud): If the _sync_power_states() periodic task happens # to have locked this node, we'll fail to acquire the lock. The @@ -829,69 +829,8 @@ class ConductorManager(base_manager.BaseConductorManager): # want to add retries or extra synchronization here. with task_manager.acquire(context, node_id, shared=False, purpose='node deployment') as task: - node = task.node - # Record of any pre-existing agent_url should be removed - # except when we are in fast track conditions. - if not utils.is_fast_track(task): - utils.remove_agent_url(node) - if node.maintenance: - raise exception.NodeInMaintenance(op=_('provisioning'), - node=node.uuid) - - if rebuild: - if node.protected: - raise exception.NodeProtected(node=node.uuid) - - event = 'rebuild' - - # Note(gilliard) Clear these to force the driver to - # check whether they have been changed in glance - # NOTE(vdrok): If image_source is not from Glance we should - # not clear kernel and ramdisk as they're input manually - if glance_utils.is_glance_image( - node.instance_info.get('image_source')): - instance_info = node.instance_info - instance_info.pop('kernel', None) - instance_info.pop('ramdisk', None) - node.instance_info = instance_info - else: - event = 'deploy' - - driver_internal_info = node.driver_internal_info - # Infer the image type to make sure the deploy driver - # validates only the necessary variables for different - # image types. - # NOTE(sirushtim): The iwdi variable can be None. It's up to - # the deploy driver to validate this. - iwdi = images.is_whole_disk_image(context, node.instance_info) - driver_internal_info['is_whole_disk_image'] = iwdi - node.driver_internal_info = driver_internal_info - node.save() - - try: - task.driver.power.validate(task) - task.driver.deploy.validate(task) - utils.validate_instance_info_traits(task.node) - conductor_steps.validate_deploy_templates(task) - except exception.InvalidParameterValue as e: - raise exception.InstanceDeployFailure( - _("Failed to validate deploy or power info for node " - "%(node_uuid)s. Error: %(msg)s") % - {'node_uuid': node.uuid, 'msg': e}, code=e.code) - - LOG.debug("do_node_deploy Calling event: %(event)s for node: " - "%(node)s", {'event': event, 'node': node.uuid}) - try: - task.process_event( - event, - callback=self._spawn_worker, - call_args=(deployments.do_node_deploy, task, - self.conductor.id, configdrive), - err_handler=utils.provisioning_error_handler) - except exception.InvalidState: - raise exception.InvalidStateRequested( - action=event, node=task.node.uuid, - state=task.node.provision_state) + deployments.validate_node(task, event=event) + deployments.start_deploy(task, self, configdrive, event=event) @METRICS.timer('ConductorManager.continue_node_deploy') def continue_node_deploy(self, context, node_id): diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index 4f334d68a0..f0bb23a157 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -1341,7 +1341,7 @@ class ServiceDoNodeDeployTestCase(mgr_utils.ServiceSetUpMixin, self.assertIsNone(node.last_error) # Verify reservation has been cleared. self.assertIsNone(node.reservation) - mock_iwdi.assert_called_once_with(self.context, node.instance_info) + self.assertFalse(mock_iwdi.called) self.assertNotIn('is_whole_disk_image', node.driver_internal_info) def test_do_node_deploy_maintenance(self, mock_iwdi): @@ -1761,7 +1761,7 @@ class ServiceDoNodeDeployTestCase(mgr_utils.ServiceSetUpMixin, self.assertIsNone(node.last_error) # Verify reservation has been cleared. self.assertIsNone(node.reservation) - mock_iwdi.assert_called_once_with(self.context, node.instance_info) + self.assertFalse(mock_iwdi.called) self.assertNotIn('is_whole_disk_image', node.driver_internal_info) def test_do_node_deploy_rebuild_protected(self, mock_iwdi):