[ironic] Don't remove instance info twice in destroy
During teardown, at the end of the ironic virt driver's destroy method, we call _cleanup_deploy, which since https://review.opendev.org/#/c/563722/ removes instance_info from the ironic node. Given that we're destroying the node, the instance_info will have been cleared from the node anyway, so we don't need to do this. Further, it can cause tear down to fail if automated cleaning is enabled in ironic. This happens because ironic prevents updates to nodes that are undergoing a state transition, as is the case during cleaning. The virt driver will retry this request by default every 2 seconds with 60 attempts. Typically this is not long enough for cleaning to complete, so tear down fails with the following error: Conflict: Node a00696d5-32ba-475e-9528-59bf11cffea6 can not be updated while a state transition is in progress. (HTTP 409) This change skips the instance info update in _cleanup_deploy in the case of tear down. Change-Id: Iea337f73c231db2cb9d9f639b92475daaede6793 Closes-Bug: #1815799 (cherry picked from commit060e42b7fd
) (cherry picked from commit95ace7cf8d
)
This commit is contained in:
parent
c19d602f9b
commit
1ed3f8f9e4
@ -1809,7 +1809,8 @@ class IronicDriverTestCase(test.NoDBTestCase):
|
||||
|
||||
mock_node.get_by_instance_uuid.assert_called_with(
|
||||
instance.uuid, fields=ironic_driver._NODE_FIELDS)
|
||||
mock_cleanup_deploy.assert_called_with(node, instance, network_info)
|
||||
mock_cleanup_deploy.assert_called_with(node, instance, network_info,
|
||||
remove_instance_info=False)
|
||||
|
||||
# For states that makes sense check if set_provision_state has
|
||||
# been called
|
||||
@ -1842,7 +1843,8 @@ class IronicDriverTestCase(test.NoDBTestCase):
|
||||
mock_sps.side_effect = exception.NovaException()
|
||||
self.assertRaises(exception.NovaException, self.driver.destroy,
|
||||
self.ctx, instance, None, None)
|
||||
mock_clean.assert_called_once_with(node, instance, None)
|
||||
mock_clean.assert_called_once_with(node, instance, None,
|
||||
remove_instance_info=False)
|
||||
|
||||
@mock.patch.object(FAKE_CLIENT.node, 'update')
|
||||
@mock.patch.object(ironic_driver.IronicDriver,
|
||||
@ -1861,7 +1863,8 @@ class IronicDriverTestCase(test.NoDBTestCase):
|
||||
mock_update.side_effect = SystemError('unexpected error')
|
||||
self.assertRaises(SystemError, self.driver.destroy,
|
||||
self.ctx, instance, None, None)
|
||||
mock_clean.assert_called_once_with(node, instance, None)
|
||||
mock_clean.assert_called_once_with(node, instance, None,
|
||||
remove_instance_info=False)
|
||||
|
||||
@mock.patch.object(FAKE_CLIENT.node, 'set_provision_state')
|
||||
@mock.patch.object(ironic_driver.IronicDriver,
|
||||
@ -2692,6 +2695,24 @@ class IronicDriverTestCase(test.NoDBTestCase):
|
||||
mock_call.has_calls(
|
||||
[mock.call('node.update', node.uuid, expected_patch)])
|
||||
|
||||
@mock.patch.object(ironic_driver.IronicDriver, '_stop_firewall')
|
||||
@mock.patch.object(ironic_driver.IronicDriver, '_unplug_vifs')
|
||||
@mock.patch.object(ironic_driver.IronicDriver,
|
||||
'_cleanup_volume_target_info')
|
||||
@mock.patch.object(cw.IronicClientWrapper, 'call')
|
||||
def test__cleanup_deploy_no_remove_ii(self, mock_call, mock_vol,
|
||||
mock_unvif, mock_stop_fw):
|
||||
# TODO(TheJulia): This REALLY should be updated to cover all of the
|
||||
# calls that take place.
|
||||
node = ironic_utils.get_test_node(driver='fake')
|
||||
instance = fake_instance.fake_instance_obj(self.ctx,
|
||||
node=node.uuid)
|
||||
self.driver._cleanup_deploy(node, instance, remove_instance_info=False)
|
||||
mock_vol.assert_called_once_with(instance)
|
||||
mock_unvif.assert_called_once_with(node, instance, None)
|
||||
mock_stop_fw.assert_called_once_with(instance, None)
|
||||
self.assertFalse(mock_call.called)
|
||||
|
||||
|
||||
class IronicDriverSyncTestCase(IronicDriverTestCase):
|
||||
|
||||
|
@ -471,11 +471,13 @@ class IronicDriver(virt_driver.ComputeDriver):
|
||||
'reason': e},
|
||||
instance=instance)
|
||||
|
||||
def _cleanup_deploy(self, node, instance, network_info=None):
|
||||
def _cleanup_deploy(self, node, instance, network_info=None,
|
||||
remove_instance_info=True):
|
||||
self._cleanup_volume_target_info(instance)
|
||||
self._unplug_vifs(node, instance, network_info)
|
||||
self._stop_firewall(instance, network_info)
|
||||
self._remove_instance_info_from_node(node, instance)
|
||||
if remove_instance_info:
|
||||
self._remove_instance_info_from_node(node, instance)
|
||||
|
||||
def _wait_for_active(self, instance):
|
||||
"""Wait for the node to be marked as ACTIVE in Ironic."""
|
||||
@ -1264,7 +1266,12 @@ class IronicDriver(virt_driver.ComputeDriver):
|
||||
# removed from ironic node.
|
||||
self._remove_instance_info_from_node(node, instance)
|
||||
finally:
|
||||
self._cleanup_deploy(node, instance, network_info)
|
||||
# NOTE(mgoddard): We don't need to remove instance info at this
|
||||
# point since we will have already done it. The destroy will only
|
||||
# succeed if this method returns without error, so we will end up
|
||||
# removing the instance info eventually.
|
||||
self._cleanup_deploy(node, instance, network_info,
|
||||
remove_instance_info=False)
|
||||
|
||||
LOG.info('Successfully unprovisioned Ironic node %s',
|
||||
node.uuid, instance=instance)
|
||||
|
Loading…
Reference in New Issue
Block a user