openstack/nova
Code Issues Proposed changes

Ironic: Call unprovison for nodes in DEPLOYING state

Browse Source 
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
This commit is contained in:
Lucas Alvares Gomes 2015-08-05 11:51:01 +01:00
parent 8cf2228ea1
commit bfe52542f5
3 changed files with 28 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

58
nova/tests/unit/virt/ironic/test_driver.py
View File

@ -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')

9
nova/virt/ironic/driver.py
View File

@ -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)

11
nova/virt/ironic/ironic_states.py
View File

@ -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. """