From b5e34e16731b47d1930bcdc99d184b1319493f1a Mon Sep 17 00:00:00 2001 From: Mark Goddard Date: Wed, 13 Mar 2019 17:20:36 +0000 Subject: [PATCH] Check for deploy.deploy deploy step in heartbeat Change-Id: Id160e37348c940055c312fbfdb05a27d13012f52 --- ironic/drivers/modules/agent_base_vendor.py | 45 ++++-- .../drivers/modules/test_agent_base_vendor.py | 138 +++++++++++++++++- 2 files changed, 169 insertions(+), 14 deletions(-) diff --git a/ironic/drivers/modules/agent_base_vendor.py b/ironic/drivers/modules/agent_base_vendor.py index 5ba8ff910d..fff5216733 100644 --- a/ironic/drivers/modules/agent_base_vendor.py +++ b/ironic/drivers/modules/agent_base_vendor.py @@ -250,6 +250,24 @@ class HeartbeatMixin(object): :returns: True if the deployment is completed. False otherwise """ + def in_core_deploy_step(self, task): + """Check if we are in the deploy.deploy deploy step. + + Assumes that we are in the DEPLOYWAIT state. + + :param task: a TaskManager instance + :returns: True if the current deploy step is deploy.deploy. + """ + # TODO(mgoddard): Remove this 'if' in the Train release, after the + # deprecation period for supporting drivers with no deploy steps. + if not task.node.driver_internal_info.get('deploy_steps'): + return True + + step = task.node.deploy_step + return (step + and step['interface'] == 'deploy' + and step['step'] == 'deploy') + def reboot_to_instance(self, task): """Method invoked after the deployment is completed. @@ -333,17 +351,22 @@ class HeartbeatMixin(object): LOG.debug('Heartbeat from node %(node)s in maintenance mode; ' 'not taking any action.', {'node': node.uuid}) return - elif (node.provision_state == states.DEPLOYWAIT - and not self.deploy_has_started(task)): - msg = _('Node failed to deploy.') - self.continue_deploy(task) - elif (node.provision_state == states.DEPLOYWAIT - and self.deploy_is_done(task)): - msg = _('Node failed to move to active state.') - self.reboot_to_instance(task) - elif (node.provision_state == states.DEPLOYWAIT - and self.deploy_has_started(task)): - node.touch_provisioning() + # NOTE(mgoddard): Only handle heartbeats during DEPLOYWAIT if we + # are currently in the core deploy.deploy step. Other deploy steps + # may cause the agent to boot, but we should not trigger deployment + # at that point. + elif node.provision_state == states.DEPLOYWAIT: + if self.in_core_deploy_step(task): + if not self.deploy_has_started(task): + msg = _('Node failed to deploy.') + self.continue_deploy(task) + elif self.deploy_is_done(task): + msg = _('Node failed to move to active state.') + self.reboot_to_instance(task) + else: + node.touch_provisioning() + else: + node.touch_provisioning() elif node.provision_state == states.CLEANWAIT: node.touch_provisioning() if not node.clean_step: diff --git a/ironic/tests/unit/drivers/modules/test_agent_base_vendor.py b/ironic/tests/unit/drivers/modules/test_agent_base_vendor.py index c175032c81..5a21ffe289 100644 --- a/ironic/tests/unit/drivers/modules/test_agent_base_vendor.py +++ b/ironic/tests/unit/drivers/modules/test_agent_base_vendor.py @@ -80,6 +80,97 @@ class HeartbeatMixinTest(AgentDeployMixinBaseTest): super(HeartbeatMixinTest, self).setUp() self.deploy = agent_base_vendor.HeartbeatMixin() + @mock.patch.object(agent_base_vendor.HeartbeatMixin, + 'in_core_deploy_step', autospec=True) + @mock.patch.object(agent_base_vendor.HeartbeatMixin, + 'deploy_has_started', autospec=True) + @mock.patch.object(agent_base_vendor.HeartbeatMixin, 'continue_deploy', + autospec=True) + @mock.patch.object(agent_base_vendor.HeartbeatMixin, + 'reboot_to_instance', autospec=True) + def test_heartbeat_continue_deploy(self, rti_mock, cd_mock, + deploy_started_mock, + in_deploy_mock): + in_deploy_mock.return_value = True + deploy_started_mock.return_value = False + self.node.provision_state = states.DEPLOYWAIT + self.node.save() + with task_manager.acquire(self.context, self.node.uuid, + shared=True) as task: + self.deploy.heartbeat(task, 'url', '3.2.0') + self.assertFalse(task.shared) + self.assertEqual( + 'url', task.node.driver_internal_info['agent_url']) + self.assertEqual( + '3.2.0', + task.node.driver_internal_info['agent_version']) + cd_mock.assert_called_once_with(self.deploy, task) + self.assertFalse(rti_mock.called) + + @mock.patch.object(agent_base_vendor.HeartbeatMixin, + 'in_core_deploy_step', autospec=True) + @mock.patch.object(agent_base_vendor.HeartbeatMixin, + 'deploy_has_started', autospec=True) + @mock.patch.object(agent_base_vendor.HeartbeatMixin, + 'deploy_is_done', autospec=True) + @mock.patch.object(agent_base_vendor.HeartbeatMixin, 'continue_deploy', + autospec=True) + @mock.patch.object(agent_base_vendor.HeartbeatMixin, + 'reboot_to_instance', autospec=True) + def test_heartbeat_reboot_to_instance(self, rti_mock, cd_mock, + deploy_is_done_mock, + deploy_started_mock, + in_deploy_mock): + in_deploy_mock.return_value = True + deploy_started_mock.return_value = True + deploy_is_done_mock.return_value = True + self.node.provision_state = states.DEPLOYWAIT + self.node.save() + with task_manager.acquire(self.context, self.node.uuid, + shared=True) as task: + self.deploy.heartbeat(task, 'url', '3.2.0') + self.assertFalse(task.shared) + self.assertEqual( + 'url', task.node.driver_internal_info['agent_url']) + self.assertEqual( + '3.2.0', + task.node.driver_internal_info['agent_version']) + self.assertFalse(cd_mock.called) + rti_mock.assert_called_once_with(self.deploy, task) + + @mock.patch.object(agent_base_vendor.HeartbeatMixin, + 'in_core_deploy_step', autospec=True) + @mock.patch.object(agent_base_vendor.HeartbeatMixin, + 'deploy_has_started', autospec=True) + @mock.patch.object(agent_base_vendor.HeartbeatMixin, + 'deploy_is_done', autospec=True) + @mock.patch.object(agent_base_vendor.HeartbeatMixin, 'continue_deploy', + autospec=True) + @mock.patch.object(agent_base_vendor.HeartbeatMixin, + 'reboot_to_instance', autospec=True) + def test_heartbeat_not_in_core_deploy_step(self, rti_mock, cd_mock, + deploy_is_done_mock, + deploy_started_mock, + in_deploy_mock): + # Check that heartbeats do not trigger deployment actions when not in + # the deploy.deploy step. + in_deploy_mock.return_value = False + self.node.provision_state = states.DEPLOYWAIT + self.node.save() + with task_manager.acquire(self.context, self.node.uuid, + shared=True) as task: + self.deploy.heartbeat(task, 'url', '3.2.0') + self.assertFalse(task.shared) + self.assertEqual( + 'url', task.node.driver_internal_info['agent_url']) + self.assertEqual( + '3.2.0', + task.node.driver_internal_info['agent_version']) + self.assertFalse(deploy_started_mock.called) + self.assertFalse(deploy_is_done_mock.called) + self.assertFalse(cd_mock.called) + self.assertFalse(rti_mock.called) + @mock.patch.object(agent_base_vendor.HeartbeatMixin, 'continue_deploy', autospec=True) @mock.patch.object(agent_base_vendor.HeartbeatMixin, @@ -157,6 +248,8 @@ class HeartbeatMixinTest(AgentDeployMixinBaseTest): self.assertEqual(0, rti_mock.call_count) self.assertEqual(0, cd_mock.call_count) + @mock.patch.object(agent_base_vendor.HeartbeatMixin, + 'in_core_deploy_step', autospec=True) @mock.patch.object(agent_base_vendor.HeartbeatMixin, 'deploy_has_started', autospec=True) @mock.patch.object(deploy_utils, 'set_failed_state', autospec=True) @@ -164,7 +257,9 @@ class HeartbeatMixinTest(AgentDeployMixinBaseTest): autospec=True) @mock.patch.object(agent_base_vendor.LOG, 'exception', autospec=True) def test_heartbeat_deploy_done_fails(self, log_mock, done_mock, - failed_mock, deploy_started_mock): + failed_mock, deploy_started_mock, + in_deploy_mock): + in_deploy_mock.return_value = True deploy_started_mock.return_value = True done_mock.side_effect = Exception('LlamaException') with task_manager.acquire( @@ -179,6 +274,8 @@ class HeartbeatMixinTest(AgentDeployMixinBaseTest): 'Exception: LlamaException for node %(node)s', {'node': task.node.uuid}) + @mock.patch.object(agent_base_vendor.HeartbeatMixin, + 'in_core_deploy_step', autospec=True) @mock.patch.object(agent_base_vendor.HeartbeatMixin, 'deploy_has_started', autospec=True) @mock.patch.object(deploy_utils, 'set_failed_state', autospec=True) @@ -187,7 +284,9 @@ class HeartbeatMixinTest(AgentDeployMixinBaseTest): @mock.patch.object(agent_base_vendor.LOG, 'exception', autospec=True) def test_heartbeat_deploy_done_raises_with_event(self, log_mock, done_mock, failed_mock, - deploy_started_mock): + deploy_started_mock, + in_deploy_mock): + in_deploy_mock.return_value = True deploy_started_mock.return_value = True with task_manager.acquire( self.context, self.node['uuid'], shared=False) as task: @@ -341,12 +440,16 @@ class HeartbeatMixinTest(AgentDeployMixinBaseTest): task, 'Asynchronous exception: Node failed to perform ' 'rescue operation. Exception: some failure for node') + @mock.patch.object(agent_base_vendor.HeartbeatMixin, + 'in_core_deploy_step', autospec=True) @mock.patch.object(objects.node.Node, 'touch_provisioning', autospec=True) @mock.patch.object(agent_base_vendor.HeartbeatMixin, 'deploy_has_started', autospec=True) def test_heartbeat_touch_provisioning_and_url_save(self, mock_deploy_started, - mock_touch): + mock_touch, + mock_in_deploy): + mock_in_deploy.return_value = True mock_deploy_started.return_value = True self.node.provision_state = states.DEPLOYWAIT @@ -381,6 +484,35 @@ class HeartbeatMixinTest(AgentDeployMixinBaseTest): task.node.driver_internal_info['agent_last_heartbeat']) self.assertEqual(provision_state, task.node.provision_state) + def test_in_core_deploy_step(self): + self.node.deploy_step = { + 'interface': 'deploy', 'step': 'deploy', 'priority': 100} + info = self.node.driver_internal_info + info['deploy_steps'] = [self.node.deploy_step] + self.node.driver_internal_info = info + self.node.save() + with task_manager.acquire( + self.context, self.node.uuid, shared=False) as task: + self.assertTrue(self.deploy.in_core_deploy_step(task)) + + def test_in_core_deploy_step_no_steps_list(self): + # Need to handle drivers without deploy step support, remove in the + # Train release. + with task_manager.acquire( + self.context, self.node.uuid, shared=False) as task: + self.assertTrue(self.deploy.in_core_deploy_step(task)) + + def test_in_core_deploy_step_in_other_step(self): + self.node.deploy_step = { + 'interface': 'deploy', 'step': 'other-step', 'priority': 100} + info = self.node.driver_internal_info + info['deploy_steps'] = [self.node.deploy_step] + self.node.driver_internal_info = info + self.node.save() + with task_manager.acquire( + self.context, self.node.uuid, shared=False) as task: + self.assertFalse(self.deploy.in_core_deploy_step(task)) + class AgentRescueTests(AgentDeployMixinBaseTest):