From 624e927fcdfabdb86d22fb1b268961ddfeaedbea Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Wed, 30 Mar 2016 14:21:20 -0400 Subject: [PATCH] Don't power off non-deploying iLO nodes in takeover The iLO driver, when the conductor takeover logic was triggered would power off the node if it was not in the ACTIVE state, which could potentially be harmful to the hardware. Presently, takeover is only called for active nodes, however in the future, features like ADOPTION will leverage take_over and evolution of redundancy support may drive additional states that the node is in when takeover is called. This changes the behavior and unit tests accordingly such that only nodes in deploying state perform deployment related method calls and thus prevent accdiental actions occuring. Change-Id: I244c3f31d0ad26194887cfb9b79f96b5111296c6 Closes-Bug: #1563949 --- ironic/drivers/modules/ilo/deploy.py | 6 +- .../unit/drivers/modules/ilo/test_deploy.py | 81 +++++++++++++------ ...-non-deploying-nodes-0a3aed7c8ea3940a.yaml | 10 +++ 3 files changed, 71 insertions(+), 26 deletions(-) create mode 100644 releasenotes/notes/ilo-do-not-power-off-non-deploying-nodes-0a3aed7c8ea3940a.yaml 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.