From bcf6c12269168c5b4f0d9d4d3212e813f1827494 Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Tue, 28 Feb 2023 10:18:16 -0800 Subject: [PATCH] Clean out agent token even if power is already off While investigating a very curious report, I discovered that if somehow the power was *already* turned off to a node, say through an incorrect BMC *or* human action, and Ironic were to pick it up (as it does by default, because it checks before applying the power state, then it would not wipe the token information, preventing the agent from connecting on the next action/attempt/operation. We now remove the token on all calls to conductor utilities node_power_action method when appropriate, even if no other work is required. Change-Id: Ie89e8be9ad2887467f277772445d4bef79fa5ea1 --- ironic/conductor/utils.py | 7 ++++++ ironic/tests/unit/conductor/test_utils.py | 25 +++++++++++++++++++ ...power-off-token-wipe-e7d605997f00d39d.yaml | 6 +++++ 3 files changed, 38 insertions(+) create mode 100644 releasenotes/notes/fix-power-off-token-wipe-e7d605997f00d39d.yaml diff --git a/ironic/conductor/utils.py b/ironic/conductor/utils.py index c107f076f3..29f5f1d250 100644 --- a/ironic/conductor/utils.py +++ b/ironic/conductor/utils.py @@ -297,6 +297,13 @@ def node_power_action(task, new_state, timeout=None): node = task.node if _can_skip_state_change(task, new_state): + # NOTE(TheJulia): Even if we are not changing the power state, + # we need to wipe the token out, just in case for some reason + # the power was turned off outside of our interaction/management. + if new_state in (states.POWER_OFF, states.SOFT_POWER_OFF, + states.REBOOT, states.SOFT_REBOOT): + wipe_internal_info_on_power_off(node) + node.save() return target_state = _calculate_target_state(new_state) diff --git a/ironic/tests/unit/conductor/test_utils.py b/ironic/tests/unit/conductor/test_utils.py index a424e51329..e8e12cc9ce 100644 --- a/ironic/tests/unit/conductor/test_utils.py +++ b/ironic/tests/unit/conductor/test_utils.py @@ -281,6 +281,31 @@ class NodePowerActionTestCase(db_base.DbTestCase): self.assertNotIn('agent_cached_deploy_steps', node['driver_internal_info']) + @mock.patch.object(fake.FakePower, 'get_power_state', autospec=True) + def test_node_power_action_power_off_already(self, get_power_mock): + """Test node_power_action to turn node power off, but already off.""" + dii = {'agent_secret_token': 'token', + 'agent_cached_deploy_steps': ['steps']} + node = obj_utils.create_test_node(self.context, + uuid=uuidutils.generate_uuid(), + driver='fake-hardware', + power_state=states.POWER_ON, + driver_internal_info=dii) + task = task_manager.TaskManager(self.context, node.uuid) + + get_power_mock.return_value = states.POWER_OFF + + conductor_utils.node_power_action(task, states.POWER_OFF) + + node.refresh() + get_power_mock.assert_called_once_with(mock.ANY, mock.ANY) + self.assertEqual(states.POWER_OFF, node['power_state']) + self.assertIsNone(node['target_power_state']) + self.assertIsNone(node['last_error']) + self.assertNotIn('agent_secret_token', node['driver_internal_info']) + self.assertNotIn('agent_cached_deploy_steps', + node['driver_internal_info']) + @mock.patch.object(fake.FakePower, 'get_power_state', autospec=True) def test_node_power_action_power_off_pregenerated_token(self, get_power_mock): diff --git a/releasenotes/notes/fix-power-off-token-wipe-e7d605997f00d39d.yaml b/releasenotes/notes/fix-power-off-token-wipe-e7d605997f00d39d.yaml new file mode 100644 index 0000000000..14a489b465 --- /dev/null +++ b/releasenotes/notes/fix-power-off-token-wipe-e7d605997f00d39d.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Fixes an issue where an agent token could be inadvertently orphaned + if a node is already in the target power state when we attempt to turn + the node off.