From 631fe9d8cc81dcd9a3d59948990cb858d01171d2 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Fri, 25 Sep 2020 14:31:22 +0200 Subject: [PATCH] Handle conductor_affinity earlier in the deployment process Not sure why we do it only after the first deploy step, seems more logical to it before running steps. Will simplify further refactoring. Change-Id: Ia102c8697a9d23253c8b86fef71fc13ab9d68753 --- ironic/conductor/deployments.py | 17 +++++------ ironic/conductor/manager.py | 2 +- .../tests/unit/conductor/test_deployments.py | 29 +++++++++---------- ironic/tests/unit/conductor/test_manager.py | 10 +++---- 4 files changed, 28 insertions(+), 30 deletions(-) diff --git a/ironic/conductor/deployments.py b/ironic/conductor/deployments.py index 780b302c59..3d02dd1435 100644 --- a/ironic/conductor/deployments.py +++ b/ironic/conductor/deployments.py @@ -201,11 +201,17 @@ def do_node_deploy(task, conductor_id=None, configdrive=None): _("No deploy steps returned by the driver")) raise exception.InstanceDeployFailure(msg) - do_next_deploy_step(task, 0, conductor_id) + if conductor_id is not None: + # Update conductor_affinity to reference this conductor's ID + # since there may be local persistent state + node.conductor_affinity = conductor_id + node.save() + + do_next_deploy_step(task, 0) @task_manager.require_exclusive_lock -def do_next_deploy_step(task, step_index, conductor_id): +def do_next_deploy_step(task, step_index): """Do deployment, starting from the specified deploy step. :param task: a TaskManager instance with an exclusive lock @@ -275,13 +281,6 @@ def do_next_deploy_step(task, step_index, conductor_id): 'the remaining deploy steps', task.node) return - if ind == 0: - # We've done the very first deploy step. - # Update conductor_affinity to reference this conductor's ID - # since there may be local persistent state - node.conductor_affinity = conductor_id - node.save() - # Check if the step is done or not. The step should return # states.DEPLOYWAIT if the step is still being executed, or # None if the step is done. diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 7860a125ce..d48ee4f4eb 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -928,7 +928,7 @@ class ConductorManager(base_manager.BaseConductorManager): task.spawn_after( self._spawn_worker, deployments.do_next_deploy_step, - task, next_step_index, self.conductor.id) + task, next_step_index) @METRICS.timer('ConductorManager.do_node_tear_down') @messaging.expected_exceptions(exception.NoFreeConductorWorker, diff --git a/ironic/tests/unit/conductor/test_deployments.py b/ironic/tests/unit/conductor/test_deployments.py index 395c643a1e..bafe3363f1 100644 --- a/ironic/tests/unit/conductor/test_deployments.py +++ b/ironic/tests/unit/conductor/test_deployments.py @@ -161,6 +161,8 @@ class DoNodeDeployTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): self.assertEqual( fast_track, bool(task.node.driver_internal_info.get('agent_secret_token'))) + self.assertEqual(self.service.conductor.id, + task.node.conductor_affinity) def test__do_node_deploy_ok(self): self._test__do_node_deploy_ok() @@ -434,7 +436,7 @@ class DoNextDeployStepTestCase(mgr_utils.ServiceSetUpMixin, task = task_manager.TaskManager(self.context, node.uuid) task.process_event('deploy') - deployments.do_next_deploy_step(task, None, self.service.conductor.id) + deployments.do_next_deploy_step(task, None) node.refresh() self.assertEqual(states.ACTIVE, node.provision_state) @@ -455,14 +457,13 @@ class DoNextDeployStepTestCase(mgr_utils.ServiceSetUpMixin, task = task_manager.TaskManager(self.context, node.uuid) task.process_event('deploy') - deployments.do_next_deploy_step(task, 0, self.service.conductor.id) + deployments.do_next_deploy_step(task, 0) node.refresh() self.assertEqual(states.DEPLOYWAIT, node.provision_state) self.assertEqual(states.ACTIVE, node.target_provision_state) self.assertEqual(expected_first_step, node.deploy_step) self.assertEqual(0, node.driver_internal_info['deploy_step_index']) - self.assertEqual(self.service.conductor.id, node.conductor_affinity) mock_execute.assert_called_once_with(mock.ANY, task, self.deploy_steps[0]) @@ -487,7 +488,7 @@ class DoNextDeployStepTestCase(mgr_utils.ServiceSetUpMixin, task = task_manager.TaskManager(self.context, node.uuid) task.process_event('deploy') - deployments.do_next_deploy_step(task, 0, self.service.conductor.id) + deployments.do_next_deploy_step(task, 0) node.refresh() self.assertIsNone(node.last_error) @@ -495,7 +496,6 @@ class DoNextDeployStepTestCase(mgr_utils.ServiceSetUpMixin, self.assertEqual(states.ACTIVE, node.target_provision_state) self.assertEqual(expected_first_step, node.deploy_step) self.assertEqual(0, node.driver_internal_info['deploy_step_index']) - self.assertEqual(self.service.conductor.id, node.conductor_affinity) mock_execute.assert_called_once_with(mock.ANY, task, self.deploy_steps[0]) @@ -517,7 +517,7 @@ class DoNextDeployStepTestCase(mgr_utils.ServiceSetUpMixin, task = task_manager.TaskManager(self.context, node.uuid) task.process_event('resume') - deployments.do_next_deploy_step(task, 1, self.service.conductor.id) + deployments.do_next_deploy_step(task, 1) node.refresh() self.assertEqual(states.DEPLOYWAIT, node.provision_state) @@ -553,7 +553,7 @@ class DoNextDeployStepTestCase(mgr_utils.ServiceSetUpMixin, task = task_manager.TaskManager(self.context, node.uuid) task.process_event('resume') - deployments.do_next_deploy_step(task, None, self.service.conductor.id) + deployments.do_next_deploy_step(task, None) node.refresh() # Deploying should be complete without calling additional steps self.assertEqual(states.ACTIVE, node.provision_state) @@ -595,7 +595,7 @@ class DoNextDeployStepTestCase(mgr_utils.ServiceSetUpMixin, task = task_manager.TaskManager(self.context, node.uuid) task.process_event('deploy') - deployments.do_next_deploy_step(task, 0, self.service.conductor.id) + deployments.do_next_deploy_step(task, 0) # Deploying should be complete node.refresh() @@ -629,7 +629,7 @@ class DoNextDeployStepTestCase(mgr_utils.ServiceSetUpMixin, task = task_manager.TaskManager(self.context, node.uuid) task.process_event('deploy') - deployments.do_next_deploy_step(task, 0, self.service.conductor.id) + deployments.do_next_deploy_step(task, 0) # Deploying should be complete node.refresh() @@ -663,7 +663,7 @@ class DoNextDeployStepTestCase(mgr_utils.ServiceSetUpMixin, task = task_manager.TaskManager(self.context, node.uuid) task.process_event('deploy') - deployments.do_next_deploy_step(task, 0, self.service.conductor.id) + deployments.do_next_deploy_step(task, 0) # Make sure we go to DEPLOYFAIL, clear deploy_steps node.refresh() @@ -702,8 +702,7 @@ class DoNextDeployStepTestCase(mgr_utils.ServiceSetUpMixin, task = task_manager.TaskManager(self.context, node.uuid) task.process_event('deploy') - deployments.do_next_deploy_step(task, None, - self.service.conductor.id) + deployments.do_next_deploy_step(task, None) # Deploying should be complete without calling additional steps node.refresh() @@ -729,7 +728,7 @@ class DoNextDeployStepTestCase(mgr_utils.ServiceSetUpMixin, task = task_manager.TaskManager(self.context, node.uuid) task.process_event('deploy') - deployments.do_next_deploy_step(task, 0, self.service.conductor.id) + deployments.do_next_deploy_step(task, 0) # Make sure we go to DEPLOYFAIL, clear deploy_steps node.refresh() @@ -763,7 +762,7 @@ class DoNextDeployStepTestCase(mgr_utils.ServiceSetUpMixin, with task_manager.acquire( self.context, node.uuid, shared=False) as task: - deployments.do_next_deploy_step(task, 0, mock.ANY) + deployments.do_next_deploy_step(task, 0) self._stop_service() node.refresh() @@ -797,7 +796,7 @@ class DoNextDeployStepTestCase(mgr_utils.ServiceSetUpMixin, with task_manager.acquire( self.context, node.uuid, shared=False) as task: - deployments.do_next_deploy_step(task, 0, mock.ANY) + deployments.do_next_deploy_step(task, 0) self._stop_service() node.refresh() diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index a45fb82197..568cb1a75a 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -1788,7 +1788,7 @@ class ContinueNodeDeployTestCase(mgr_utils.ServiceSetUpMixin, self.assertEqual(tgt_prv_state, node.target_provision_state) mock_spawn.assert_called_with(mock.ANY, deployments.do_next_deploy_step, - mock.ANY, 1, mock.ANY) + mock.ANY, 1) @mock.patch('ironic.drivers.modules.fake.FakeDeploy.get_deploy_steps', autospec=True) @@ -1818,7 +1818,7 @@ class ContinueNodeDeployTestCase(mgr_utils.ServiceSetUpMixin, self.assertEqual(new_steps, node.driver_internal_info['deploy_steps']) mock_spawn.assert_called_with(mock.ANY, deployments.do_next_deploy_step, - mock.ANY, 1, mock.ANY) + mock.ANY, 1) @mock.patch.object(task_manager.TaskManager, 'process_event', autospec=True) @@ -1848,7 +1848,7 @@ class ContinueNodeDeployTestCase(mgr_utils.ServiceSetUpMixin, self.assertEqual(tgt_prv_state, node.target_provision_state) mock_spawn.assert_called_with(mock.ANY, deployments.do_next_deploy_step, - mock.ANY, 1, mock.ANY) + mock.ANY, 1) self.assertFalse(mock_event.called) @mock.patch('ironic.conductor.manager.ConductorManager._spawn_worker', @@ -1877,7 +1877,7 @@ class ContinueNodeDeployTestCase(mgr_utils.ServiceSetUpMixin, expected_step_index = 0 mock_spawn.assert_called_with(mock.ANY, deployments.do_next_deploy_step, - mock.ANY, expected_step_index, mock.ANY) + mock.ANY, expected_step_index) def test_continue_node_deploy_skip_step(self): self._continue_node_deploy_skip_step() @@ -1905,7 +1905,7 @@ class ContinueNodeDeployTestCase(mgr_utils.ServiceSetUpMixin, self.assertNotIn('deployment_polling', node.driver_internal_info) mock_spawn.assert_called_with(mock.ANY, deployments.do_next_deploy_step, - mock.ANY, 1, mock.ANY) + mock.ANY, 1) @mock.patch.object(conductor_steps, 'validate_deploy_templates', autospec=True)