From 3d32729fb3f22dd8027adcc8e5b93464ada26b6b Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Tue, 28 Feb 2023 10:18:16 -0800 Subject: 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 (cherry picked from commit bcf6c12269168c5b4f0d9d4d3212e813f1827494) --- ironic/conductor/utils.py | 7 ++++++ ironic/tests/unit/conductor/test_utils.py | 25 ++++++++++++++++++++++ .../fix-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 cdf3a99ee..402ec2241 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 a29da21a7..563ac0b70 100644 --- a/ironic/tests/unit/conductor/test_utils.py +++ b/ironic/tests/unit/conductor/test_utils.py @@ -303,6 +303,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 000000000..14a489b46 --- /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. -- cgit v1.2.1