From f92258e76750ba2977120c8ff7a6e0774118dc9e Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Fri, 27 Sep 2024 15:49:31 +0200 Subject: [PATCH] Reject explicit requests to power off nodes with disable_power_off Checks are added to three places: 1) Power state change API 2) Power sync loop in the conductor 3) The common node_power_action call Partial-Bug: #2077432 Change-Id: Ifcc539b32022870bf8e96aa17fdeb2d111d2a393 --- ironic/api/controllers/v1/node.py | 4 +++ ironic/conductor/manager.py | 3 +- ironic/conductor/utils.py | 7 +++- .../unit/api/controllers/v1/test_node.py | 12 +++++++ ironic/tests/unit/conductor/test_manager.py | 16 +++++++++ ironic/tests/unit/conductor/test_utils.py | 36 +++++++++++++++++-- 6 files changed, 74 insertions(+), 4 deletions(-) diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index 605d409c81..e5d10ca2d8 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -884,6 +884,10 @@ class NodeStatesController(rest.RestController): action=target, node=node_ident, state=rpc_node.provision_state) + elif (target in (ir_states.POWER_OFF, ir_states.SOFT_POWER_OFF) + and rpc_node.disable_power_off): + raise exception.PowerStateFailure(pstate=target) + api.request.rpcapi.change_node_power_state(api.request.context, rpc_node.uuid, target, timeout=timeout, diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 58654fc2d1..9a07ddc3cd 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -4111,7 +4111,8 @@ def do_sync_power_state(task, count): return count if (CONF.conductor.force_power_state_during_sync - and task.driver.power.supports_power_sync(task)): + and task.driver.power.supports_power_sync(task) + and not node.disable_power_off): LOG.warning("During sync_power_state, node %(node)s state " "'%(actual)s' does not match expected state. " "Changing hardware state to '%(state)s'.", diff --git a/ironic/conductor/utils.py b/ironic/conductor/utils.py index af44db2988..65d3a424c7 100644 --- a/ironic/conductor/utils.py +++ b/ironic/conductor/utils.py @@ -297,6 +297,12 @@ def node_power_action(task, new_state, timeout=None): new_state) node = task.node + target_state = _calculate_target_state(new_state) + if target_state == states.POWER_OFF and node.disable_power_off: + LOG.error("Refusing to power off node %s with disable_power_off " + "flag set", node.uuid) + raise exception.PowerStateFailure(pstate=new_state) + 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 @@ -306,7 +312,6 @@ def node_power_action(task, new_state, timeout=None): wipe_internal_info_on_power_off(node) node.save() return - target_state = _calculate_target_state(new_state) # Set the target_power_state and clear any last_error, if we're # starting a new operation. This will expose to other processes diff --git a/ironic/tests/unit/api/controllers/v1/test_node.py b/ironic/tests/unit/api/controllers/v1/test_node.py index f0804f7f52..eb524a778f 100644 --- a/ironic/tests/unit/api/controllers/v1/test_node.py +++ b/ironic/tests/unit/api/controllers/v1/test_node.py @@ -5548,6 +5548,18 @@ class TestPut(test_api_base.BaseApiTest): self._test_power_state_failure( states.SOFT_POWER_OFF, http_client.NOT_ACCEPTABLE, 0, "1.26") + def test_power_state_power_off_with_disable_power_off(self): + self.node.disable_power_off = True + self.node.save() + self._test_power_state_failure( + states.POWER_OFF, http_client.CONFLICT, None, None) + + def test_power_state_soft_power_off_with_disable_power_off(self): + self.node.disable_power_off = True + self.node.save() + self._test_power_state_failure( + states.SOFT_POWER_OFF, http_client.CONFLICT, None, "1.27") + def test_power_state_by_name_unsupported(self): response = self.put_json('/nodes/%s/states/power' % self.node.name, {'target': states.POWER_ON}, diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index 1067805d98..0fdd0188e4 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -5544,6 +5544,22 @@ class ManagerDoSyncPowerStateTestCase(db_base.DbTestCase): mock_power_update.assert_called_once_with( self.task.context, self.node.instance_uuid, states.POWER_OFF) + @mock.patch.object(nova, 'power_update', autospec=True) + def test_state_changed_no_sync_with_disable_power_off(self, + mock_power_update, + node_power_action): + self.config(force_power_state_during_sync=True, group='conductor') + self.node.disable_power_off = True + self._do_sync_power_state(states.POWER_ON, states.POWER_OFF) + + self.power.validate.assert_not_called() + self.power.get_power_state.assert_called_once_with(self.task) + node_power_action.assert_not_called() + self.assertEqual(states.POWER_OFF, self.node.power_state) + self.task.upgrade_lock.assert_called_once_with() + mock_power_update.assert_called_once_with( + self.task.context, self.node.instance_uuid, states.POWER_OFF) + @mock.patch('ironic.objects.node.NodeCorrectedPowerStateNotification', autospec=True) @mock.patch.object(nova, 'power_update', autospec=True) diff --git a/ironic/tests/unit/conductor/test_utils.py b/ironic/tests/unit/conductor/test_utils.py index 8d2839afd8..798a29bc66 100644 --- a/ironic/tests/unit/conductor/test_utils.py +++ b/ironic/tests/unit/conductor/test_utils.py @@ -355,7 +355,8 @@ class NodePowerActionTestCase(db_base.DbTestCase): @mock.patch.object(fake.FakePower, 'reboot', autospec=True) @mock.patch.object(fake.FakePower, 'get_power_state', autospec=True) - def test_node_power_action_power_reboot(self, get_power_mock, reboot_mock): + def _test_node_power_action_power_reboot(self, get_power_mock, reboot_mock, + disable_power_off=False): """Test for reboot a node.""" dii = {'agent_secret_token': 'token', 'agent_cached_deploy_steps': ['steps']} @@ -363,7 +364,8 @@ class NodePowerActionTestCase(db_base.DbTestCase): uuid=uuidutils.generate_uuid(), driver='fake-hardware', power_state=states.POWER_ON, - driver_internal_info=dii) + driver_internal_info=dii, + disable_power_off=disable_power_off) task = task_manager.TaskManager(self.context, node.uuid) conductor_utils.node_power_action(task, states.REBOOT) @@ -376,6 +378,12 @@ class NodePowerActionTestCase(db_base.DbTestCase): self.assertIsNone(node['last_error']) self.assertNotIn('agent_secret_token', node['driver_internal_info']) + def test_node_power_action_power_reboot(self): + self._test_node_power_action_power_reboot() + + def test_node_power_action_power_reboot_with_disable_power_off(self): + self._test_node_power_action_power_reboot(disable_power_off=True) + @mock.patch.object(fake.FakePower, 'get_power_state', autospec=True) def test_node_power_action_invalid_state(self, get_power_mock): """Test for exception when changing to an invalid power state.""" @@ -405,6 +413,30 @@ class NodePowerActionTestCase(db_base.DbTestCase): self.assertIsNone(node['target_power_state']) self.assertIsNone(node['last_error']) + @mock.patch.object(fake.FakePower, 'get_power_state', autospec=True) + def test_node_power_action_disable_power_off(self, get_power_mock): + """Test for exception when power off is disabled.""" + node = obj_utils.create_test_node(self.context, + uuid=uuidutils.generate_uuid(), + driver='fake-hardware', + power_state=states.POWER_ON, + disable_power_off=True) + task = task_manager.TaskManager(self.context, node.uuid) + + get_power_mock.return_value = states.POWER_ON + + self.assertRaises(exception.PowerStateFailure, + conductor_utils.node_power_action, + task, states.POWER_OFF) + + node.refresh() + get_power_mock.assert_not_called() + self.assertEqual(states.POWER_ON, node['power_state']) + self.assertIsNone(node['target_power_state']) + # Not updating last_error - this condition should be caught on the API + # level and reported immediately. + self.assertIsNone(node['last_error']) + @mock.patch('ironic.objects.node.NodeSetPowerStateNotification', autospec=True) @mock.patch.object(fake.FakePower, 'get_power_state', autospec=True)