From bfe52542f5449391b9dc152a90fd79afebcb3ff2 Mon Sep 17 00:00:00 2001 From: Lucas Alvares Gomes Date: Wed, 5 Aug 2015 11:51:01 +0100 Subject: [PATCH] Ironic: Call unprovison for nodes in DEPLOYING state This patch is making the Nova ironic driver to try to unprovision the node even if it's in DEPLOYING state. Current Ironic will not accept aborting the deployment when it's in DEPLOYING state but with the retry mechanism it may work once the state is moved to ACTIVE or DEPLOYWAIT. Prior to this patch the logic was to not even try to unprovision the node if it's in DEPLOYING and just go ahead and clean the instance but that behavior is dangerous and could leave orphan active instances in Ironic. With this patch at least if the unprovision fails in Ironic we can make sure that the instance won't be deleted from Nova. The tests for the destroy() method were refactored to extend testing destroy() being called with all provision state methods in Ironic instead of picking certain ones; A helper function was created to avoid code duplication on the tests. Partial-Bug: #1477490 Change-Id: I227eac73a9043dc242b7a0908bc27b628b830c3c --- nova/tests/unit/virt/ironic/test_driver.py | 58 +++++----------------- nova/virt/ironic/driver.py | 9 ++-- nova/virt/ironic/ironic_states.py | 11 +++- 3 files changed, 28 insertions(+), 50 deletions(-) diff --git a/nova/tests/unit/virt/ironic/test_driver.py b/nova/tests/unit/virt/ironic/test_driver.py index e4b0c98bd5c4..fc6464ac48d5 100644 --- a/nova/tests/unit/virt/ironic/test_driver.py +++ b/nova/tests/unit/virt/ironic/test_driver.py @@ -1131,12 +1131,12 @@ class IronicDriverTestCase(test.NoDBTestCase): @mock.patch.object(FAKE_CLIENT, 'node') @mock.patch.object(ironic_driver.IronicDriver, '_cleanup_deploy') - def test_destroy(self, mock_cleanup_deploy, mock_node): + def _test_destroy(self, state, mock_cleanup_deploy, mock_node): node_uuid = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' network_info = 'foo' node = ironic_utils.get_test_node(driver='fake', uuid=node_uuid, - provision_state=ironic_states.ACTIVE) + provision_state=state) instance = fake_instance.fake_instance_obj(self.ctx, node=node_uuid) def fake_set_provision_state(*_): @@ -1145,54 +1145,22 @@ class IronicDriverTestCase(test.NoDBTestCase): mock_node.get_by_instance_uuid.return_value = node mock_node.set_provision_state.side_effect = fake_set_provision_state self.driver.destroy(self.ctx, instance, network_info, None) - mock_node.set_provision_state.assert_called_once_with(node_uuid, - 'deleted') + mock_node.get_by_instance_uuid.assert_called_with(instance.uuid) mock_cleanup_deploy.assert_called_with(self.ctx, node, instance, network_info) - @mock.patch.object(FAKE_CLIENT, 'node') - @mock.patch.object(ironic_driver.IronicDriver, '_cleanup_deploy') - def test_destroy_ignore_unexpected_state(self, mock_cleanup_deploy, - mock_node): - node_uuid = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' - network_info = 'foo' + # For states that makes sense check if set_provision_state has + # been called + if state in ironic_driver._UNPROVISION_STATES: + mock_node.set_provision_state.assert_called_once_with( + node_uuid, 'deleted') + else: + self.assertFalse(mock_node.set_provision_state.called) - node = ironic_utils.get_test_node(driver='fake', uuid=node_uuid, - provision_state=ironic_states.DELETING) - instance = fake_instance.fake_instance_obj(self.ctx, node=node_uuid) - - mock_node.get_by_instance_uuid.return_value = node - self.driver.destroy(self.ctx, instance, network_info, None) - self.assertFalse(mock_node.set_provision_state.called) - mock_node.get_by_instance_uuid.assert_called_with(instance.uuid) - mock_cleanup_deploy.assert_called_with(self.ctx, node, instance, - network_info) - - @mock.patch.object(FAKE_CLIENT, 'node') - @mock.patch.object(ironic_driver.IronicDriver, '_cleanup_deploy') - def _test_destroy_cleaning(self, mock_cleanup_deploy, mock_node, - state=None): - node_uuid = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' - network_info = 'foo' - - node = ironic_utils.get_test_node( - driver='fake', uuid=node_uuid, - provision_state=state) - instance = fake_instance.fake_instance_obj(self.ctx, node=node_uuid) - - mock_node.get_by_instance_uuid.return_value = node - self.driver.destroy(self.ctx, instance, network_info, None) - self.assertFalse(mock_node.set_provision_state.called) - mock_node.get_by_instance_uuid.assert_called_with(instance.uuid) - mock_cleanup_deploy.assert_called_with(self.ctx, node, instance, - network_info) - - def test_destroy_cleaning(self): - self._test_destroy_cleaning(state=ironic_states.CLEANING) - - def test_destroy_cleanwait(self): - self._test_destroy_cleaning(state=ironic_states.CLEANWAIT) + def test_destroy(self): + for state in ironic_states.PROVISION_STATE_LIST: + self._test_destroy(state) @mock.patch.object(FAKE_CLIENT.node, 'set_provision_state') @mock.patch.object(ironic_driver, '_validate_instance_and_node') diff --git a/nova/virt/ironic/driver.py b/nova/virt/ironic/driver.py index 820ab440eed8..d7d32c15f790 100644 --- a/nova/virt/ironic/driver.py +++ b/nova/virt/ironic/driver.py @@ -112,6 +112,10 @@ _POWER_STATE_MAP = { ironic_states.POWER_OFF: power_state.SHUTDOWN, } +_UNPROVISION_STATES = (ironic_states.ACTIVE, ironic_states.DEPLOYFAIL, + ironic_states.ERROR, ironic_states.DEPLOYWAIT, + ironic_states.DEPLOYING) + def map_power_state(state): try: @@ -899,10 +903,7 @@ class IronicDriver(virt_driver.ComputeDriver): # without raising any exceptions. return - if node.provision_state in (ironic_states.ACTIVE, - ironic_states.DEPLOYFAIL, - ironic_states.ERROR, - ironic_states.DEPLOYWAIT): + if node.provision_state in _UNPROVISION_STATES: self._unprovision(self.ironicclient, instance, node) self._cleanup_deploy(context, node, instance, network_info) diff --git a/nova/virt/ironic/ironic_states.py b/nova/virt/ironic/ironic_states.py index 5bb9ec4a9376..162dbcd74767 100644 --- a/nova/virt/ironic/ironic_states.py +++ b/nova/virt/ironic/ironic_states.py @@ -128,7 +128,6 @@ This is the provision state used when inspection is started. A successfully inspected node shall transition to MANAGEABLE status. """ - INSPECTFAIL = 'inspect failed' """ Node inspection failed. """ @@ -145,3 +144,13 @@ POWER_OFF = 'power off' REBOOT = 'rebooting' """ Node is rebooting. """ + +################## +# Helper constants +################## + +PROVISION_STATE_LIST = (NOSTATE, MANAGEABLE, AVAILABLE, ACTIVE, DEPLOYWAIT, + DEPLOYING, DEPLOYFAIL, DEPLOYDONE, DELETING, DELETED, + CLEANING, CLEANWAIT, CLEANFAIL, ERROR, REBUILD, + INSPECTING, INSPECTFAIL) +""" A list of all provision states. """