From 93bd867bef984e6b65f834f0c42068140b3f7b0c Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Tue, 17 Sep 2019 12:48:44 +0200 Subject: [PATCH] Do not log an error on heartbeat in deploying/cleaning/rescuing It is completely normal for IPA to heartbeat in these states while ironic is busy doing something with the node. Just ignore it. Also fixes the incorrect unit test on invalid states that was only testing available in reality. Partially reverts the effect of 2b8581af8c4faec1ca4b2c9714c6c2b491c5578a. Change-Id: I2d7218285de97f4769a0e42797c272af95413fa2 --- ironic/drivers/modules/agent_base_vendor.py | 9 +++-- .../drivers/modules/test_agent_base_vendor.py | 34 ++++++++++++++++--- 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/ironic/drivers/modules/agent_base_vendor.py b/ironic/drivers/modules/agent_base_vendor.py index 0fa2d45487..8cd88c2930 100644 --- a/ironic/drivers/modules/agent_base_vendor.py +++ b/ironic/drivers/modules/agent_base_vendor.py @@ -67,11 +67,14 @@ VENDOR_PROPERTIES = { 'older deploy ramdisks. Defaults to False. Optional.') } -__HEARTBEAT_RECORD_ONLY = (states.ENROLL, states.MANAGEABLE, - states.AVAILABLE) +__HEARTBEAT_RECORD_ONLY = (states.ENROLL, states.MANAGEABLE, states.AVAILABLE, + states.CLEANING, states.DEPLOYING, states.RESCUING) _HEARTBEAT_RECORD_ONLY = frozenset(__HEARTBEAT_RECORD_ONLY) -_HEARTBEAT_ALLOWED = (states.DEPLOYWAIT, states.CLEANWAIT, states.RESCUEWAIT) +_HEARTBEAT_ALLOWED = (states.DEPLOYWAIT, states.CLEANWAIT, states.RESCUEWAIT, + # These are allowed but don't cause any actions since + # they're also in HEARTBEAT_RECORD_ONLY. + states.DEPLOYING, states.CLEANING, states.RESCUING) HEARTBEAT_ALLOWED = frozenset(_HEARTBEAT_ALLOWED) _FASTTRACK_HEARTBEAT_ALLOWED = (states.DEPLOYWAIT, states.CLEANWAIT, 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 185e063600..6d842d6181 100644 --- a/ironic/tests/unit/drivers/modules/test_agent_base_vendor.py +++ b/ironic/tests/unit/drivers/modules/test_agent_base_vendor.py @@ -231,6 +231,7 @@ class HeartbeatMixinTest(AgentDeployMixinBaseTest): self.assertEqual(0, rti_mock.call_count) self.assertEqual(0, cd_mock.call_count) + @mock.patch.object(agent_base_vendor.LOG, 'error', autospec=True) @mock.patch.object(agent_base_vendor.HeartbeatMixin, 'continue_deploy', autospec=True) @mock.patch.object(agent_base_vendor.HeartbeatMixin, @@ -238,19 +239,25 @@ class HeartbeatMixinTest(AgentDeployMixinBaseTest): @mock.patch.object(manager_utils, 'notify_conductor_resume_clean', autospec=True) def test_heartbeat_noops_in_wrong_state(self, ncrc_mock, rti_mock, - cd_mock): - allowed = {states.DEPLOYWAIT, states.CLEANWAIT} + cd_mock, log_mock): + allowed = {states.DEPLOYWAIT, states.CLEANWAIT, states.RESCUEWAIT, + states.DEPLOYING, states.CLEANING, states.RESCUING} for state in set(states.machine.states) - allowed: - for m in (ncrc_mock, rti_mock, cd_mock): + for m in (ncrc_mock, rti_mock, cd_mock, log_mock): m.reset_mock() with task_manager.acquire(self.context, self.node.uuid, shared=True) as task: - self.node.provision_state = state + task.node.provision_state = state self.deploy.heartbeat(task, 'url', '1.0.0') self.assertTrue(task.shared) + self.assertNotIn('agent_last_heartbeat', + task.node.driver_internal_info) self.assertEqual(0, ncrc_mock.call_count) self.assertEqual(0, rti_mock.call_count) self.assertEqual(0, cd_mock.call_count) + log_mock.assert_called_once_with(mock.ANY, + {'node': self.node.uuid, + 'state': state}) @mock.patch.object(agent_base_vendor.HeartbeatMixin, 'in_core_deploy_step', autospec=True) @@ -469,6 +476,25 @@ class HeartbeatMixinTest(AgentDeployMixinBaseTest): task.node.driver_internal_info['agent_last_heartbeat']) mock_touch.assert_called_once_with(mock.ANY) + @mock.patch.object(agent_base_vendor.LOG, 'error', autospec=True) + def test_heartbeat_records_cleaning_deploying(self, log_mock): + for provision_state in (states.CLEANING, states.DEPLOYING): + self.node.driver_internal_info = {} + self.node.provision_state = provision_state + self.node.save() + with task_manager.acquire( + self.context, self.node.uuid, shared=False) as task: + self.deploy.heartbeat(task, 'http://127.0.0.1:8080', '3.2.0') + self.assertEqual('http://127.0.0.1:8080', + task.node.driver_internal_info['agent_url']) + self.assertEqual('3.2.0', + task.node.driver_internal_info[ + 'agent_version']) + self.assertIsNotNone( + task.node.driver_internal_info['agent_last_heartbeat']) + self.assertEqual(provision_state, task.node.provision_state) + self.assertFalse(log_mock.called) + def test_heartbeat_records_fast_track(self): self.config(fast_track=True, group='deploy') for provision_state in [states.ENROLL, states.MANAGEABLE,