diff --git a/ironic/drivers/modules/ilo/deploy.py b/ironic/drivers/modules/ilo/deploy.py index 2da4f41e67..937c989964 100644 --- a/ironic/drivers/modules/ilo/deploy.py +++ b/ironic/drivers/modules/ilo/deploy.py @@ -170,7 +170,7 @@ class IloVirtualMediaIscsiDeploy(iscsi_deploy.ISCSIDeploy): :param task: a TaskManager instance containing the node to act on. :raises: IloOperationError, if some operation on iLO failed. """ - if task.node.provision_state != states.ACTIVE: + if task.node.provision_state == states.DEPLOYING: _prepare_node_for_deploy(task) super(IloVirtualMediaIscsiDeploy, self).prepare(task) @@ -218,7 +218,7 @@ class IloVirtualMediaAgentDeploy(agent.AgentDeploy): :param task: a TaskManager instance. :raises: IloOperationError, if some operation on iLO failed. """ - if task.node.provision_state != states.ACTIVE: + if task.node.provision_state == states.DEPLOYING: _prepare_node_for_deploy(task) super(IloVirtualMediaAgentDeploy, self).prepare(task) @@ -275,7 +275,7 @@ class IloPXEDeploy(iscsi_deploy.ISCSIDeploy): :raises: IloOperationError, if some operation on iLO failed. :raises: InvalidParameterValue, if some information is invalid. """ - if task.node.provision_state != states.ACTIVE: + if task.node.provision_state == states.DEPLOYING: _prepare_node_for_deploy(task) # Check if 'boot_option' is compatible with 'boot_mode' and image. diff --git a/ironic/tests/unit/drivers/modules/ilo/test_deploy.py b/ironic/tests/unit/drivers/modules/ilo/test_deploy.py index dee0851388..55b23f58da 100644 --- a/ironic/tests/unit/drivers/modules/ilo/test_deploy.py +++ b/ironic/tests/unit/drivers/modules/ilo/test_deploy.py @@ -263,6 +263,8 @@ class IloVirtualMediaIscsiDeployTestCase(db_base.DbTestCase): autospec=True) def test_prepare(self, func_prepare_node_for_deploy, iscsi_deploy_prepare_mock): + self.node.provision_state = states.DEPLOYING + self.node.save() with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: task.driver.deploy.prepare(task) @@ -275,13 +277,22 @@ class IloVirtualMediaIscsiDeployTestCase(db_base.DbTestCase): autospec=True) def test_prepare_active_node(self, func_prepare_node_for_deploy, iscsi_deploy_prepare_mock): - self.node.provision_state = states.ACTIVE - self.node.save() - with task_manager.acquire(self.context, self.node.uuid, - shared=False) as task: - task.driver.deploy.prepare(task) - self.assertFalse(func_prepare_node_for_deploy.called) - iscsi_deploy_prepare_mock.assert_called_once_with(mock.ANY, task) + """Ensure nodes in running states are not inadvertently changed""" + test_states = list(states.STABLE_STATES) + test_states.extend([states.CLEANING, + states.CLEANWAIT, + states.INSPECTING]) + for state in test_states: + self.node.provision_state = state + self.node.save() + func_prepare_node_for_deploy.reset_mock() + iscsi_deploy_prepare_mock.reset_mock() + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + task.driver.deploy.prepare(task) + self.assertFalse(func_prepare_node_for_deploy.called) + iscsi_deploy_prepare_mock.assert_called_once_with( + mock.ANY, task) @mock.patch.object(iscsi_deploy.ISCSIDeploy, 'prepare_cleaning', spec_set=True, autospec=True) @@ -360,6 +371,8 @@ class IloVirtualMediaAgentDeployTestCase(db_base.DbTestCase): def test_prepare(self, agent_prepare_mock, func_prepare_node_for_deploy): + self.node.provision_state = states.DEPLOYING + self.node.save() with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: task.driver.deploy.prepare(task) @@ -373,12 +386,21 @@ class IloVirtualMediaAgentDeployTestCase(db_base.DbTestCase): def test_prepare_active_node(self, func_prepare_node_for_deploy, agent_prepare_mock): - with task_manager.acquire(self.context, self.node.uuid, - shared=False) as task: - task.node.provision_state = states.ACTIVE - task.driver.deploy.prepare(task) - self.assertFalse(func_prepare_node_for_deploy.called) - agent_prepare_mock.assert_called_once_with(mock.ANY, task) + """Ensure nodes in running states are not inadvertently changed""" + test_states = list(states.STABLE_STATES) + test_states.extend([states.CLEANING, + states.CLEANWAIT, + states.INSPECTING]) + for state in test_states: + self.node.provision_state = state + self.node.save() + func_prepare_node_for_deploy.reset_mock() + agent_prepare_mock.reset_mock() + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + task.driver.deploy.prepare(task) + self.assertFalse(func_prepare_node_for_deploy.called) + agent_prepare_mock.assert_called_once_with(mock.ANY, task) @mock.patch.object(deploy_utils, 'agent_get_clean_steps', spec_set=True, autospec=True) @@ -467,13 +489,15 @@ class IloPXEDeployTestCase(db_base.DbTestCase): @mock.patch.object(ilo_deploy, '_prepare_node_for_deploy', spec_set=True, autospec=True) def test_prepare(self, - prepare_node_mock, + prepare_node_for_deploy_mock, pxe_prepare_mock): + self.node.provision_state = states.DEPLOYING + self.node.save() with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: task.node.properties['capabilities'] = 'boot_mode:uefi' task.driver.deploy.prepare(task) - prepare_node_mock.assert_called_once_with(task) + prepare_node_for_deploy_mock.assert_called_once_with(task) pxe_prepare_mock.assert_called_once_with(mock.ANY, task) @mock.patch.object(iscsi_deploy.ISCSIDeploy, 'prepare', spec_set=True, @@ -481,15 +505,24 @@ class IloPXEDeployTestCase(db_base.DbTestCase): @mock.patch.object(ilo_deploy, '_prepare_node_for_deploy', spec_set=True, autospec=True) def test_prepare_active_node(self, - prepare_node_mock, + prepare_node_for_deploy_mock, pxe_prepare_mock): - with task_manager.acquire(self.context, self.node.uuid, - shared=False) as task: - task.node.provision_state = states.ACTIVE - task.node.properties['capabilities'] = 'boot_mode:uefi' - task.driver.deploy.prepare(task) - self.assertFalse(prepare_node_mock.called) - pxe_prepare_mock.assert_called_once_with(mock.ANY, task) + """Ensure nodes in running states are not inadvertently changed""" + test_states = list(states.STABLE_STATES) + test_states.extend([states.CLEANING, + states.CLEANWAIT, + states.INSPECTING]) + for state in test_states: + self.node.provision_state = state + self.node.save() + prepare_node_for_deploy_mock.reset_mock() + pxe_prepare_mock.reset_mock() + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + task.node.properties['capabilities'] = 'boot_mode:uefi' + task.driver.deploy.prepare(task) + self.assertFalse(prepare_node_for_deploy_mock.called) + pxe_prepare_mock.assert_called_once_with(mock.ANY, task) @mock.patch.object(iscsi_deploy.ISCSIDeploy, 'prepare', spec_set=True, autospec=True) @@ -498,6 +531,8 @@ class IloPXEDeployTestCase(db_base.DbTestCase): def test_prepare_uefi_whole_disk_image_fail(self, prepare_node_for_deploy_mock, pxe_prepare_mock): + self.node.provision_state = states.DEPLOYING + self.node.save() with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: task.node.properties['capabilities'] = 'boot_mode:uefi' diff --git a/releasenotes/notes/ilo-do-not-power-off-non-deploying-nodes-0a3aed7c8ea3940a.yaml b/releasenotes/notes/ilo-do-not-power-off-non-deploying-nodes-0a3aed7c8ea3940a.yaml new file mode 100644 index 0000000000..ed0dcd3cdc --- /dev/null +++ b/releasenotes/notes/ilo-do-not-power-off-non-deploying-nodes-0a3aed7c8ea3940a.yaml @@ -0,0 +1,10 @@ +--- +fixes: + - A bug was identified in the behavior of the ilo driver + where nodes that are not active but taking part of a + conductor takeover could be powered off. + In preparation for new features and functionality, + that risk encountering this bug, we are limiting the + deployment preparation steps to the ``deploying`` + state to prevent nodes from being erroniuosly + powered off.