openstack
/
compute-hyperv
Code Issues Proposed changes

Perform proper cleanup after failed instance spawns 

This change ensures that vif ports as well as volume connections
are properly removed after an instance fails to spawn.

In order to avoid having similar issues in the future, the
'block_device_info' and 'network_info' arguments become mandatory
for the VMOps.destroy method.

Side note: for convenience reasons, one redundant unit test has
been squashed.

Closes-Bug: #1714285
Change-Id: Ifa701459b15b5a2046528fa45eca7ab382f1f7e8
This commit is contained in:
Lucian Petrut 2017-08-31 18:13:49 +03:00
parent fedb492a14
commit 6e715ed580
2 changed files with 31 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
compute_hyperv/nova/vmops.py
View File

@ -328,7 +328,7 @@ class VMOps(object):
should_plug_vifs=False)
except Exception:
with excutils.save_and_reraise_exception():
self.destroy(instance)
self.destroy(instance, network_info, block_device_info)
@contextlib.contextmanager
def wait_vif_plug_events(self, instance, network_info):
@ -805,7 +805,7 @@ class VMOps(object):
self._pathutils.check_remove_dir(instance_path)
def destroy(self, instance, network_info=None, block_device_info=None,
def destroy(self, instance, network_info, block_device_info,
destroy_disks=True):
instance_name = instance.name
LOG.info("Got request to destroy instance", instance=instance)

51
compute_hyperv/tests/unit/test_vmops.py
View File

@ -462,17 +462,19 @@ class VMOpsTestCase(test_base.HyperVBaseTestCase):
self.assertRaises(exception.InstanceExists, self._vmops.spawn,
self.context, mock_instance, mock_image_meta,
[mock.sentinel.FILE], mock.sentinel.PASSWORD,
mock.sentinel.INFO, block_device_info)
mock.sentinel.network_info, block_device_info)
elif fail is os_win_exc.HyperVException:
self.assertRaises(os_win_exc.HyperVException, self._vmops.spawn,
self.context, mock_instance, mock_image_meta,
[mock.sentinel.FILE], mock.sentinel.PASSWORD,
mock.sentinel.INFO, block_device_info)
mock_destroy.assert_called_once_with(mock_instance)
mock.sentinel.network_info, block_device_info)
mock_destroy.assert_called_once_with(mock_instance,
mock.sentinel.network_info,
block_device_info)
else:
self._vmops.spawn(self.context, mock_instance, mock_image_meta,
[mock.sentinel.FILE], mock.sentinel.PASSWORD,
mock.sentinel.INFO, block_device_info)
mock.sentinel.network_info, block_device_info)
self._vmops._vmutils.vm_exists.assert_called_once_with(
mock_instance.name)
mock_delete_disk_files.assert_called_once_with(
@ -487,14 +489,15 @@ class VMOpsTestCase(test_base.HyperVBaseTestCase):
fake_vm_gen)
mock_create_ephemerals.assert_called_once_with(
mock_instance, block_device_info['ephemerals'])
mock_get_neutron_events.assert_called_once_with(mock.sentinel.INFO)
mock_get_neutron_events.assert_called_once_with(
mock.sentinel.network_info)
mock_get_image_vm_gen.assert_called_once_with(mock_instance.uuid,
mock_image_meta)
mock_create_instance.assert_called_once_with(
self.context, mock_instance, mock.sentinel.INFO,
self.context, mock_instance, mock.sentinel.network_info,
block_device_info, fake_vm_gen, mock_image_meta)
mock_plug_vifs.assert_called_once_with(mock_instance,
mock.sentinel.INFO)
mock.sentinel.network_info)
mock_save_device_metadata.assert_called_once_with(
self.context, mock_instance, block_device_info)
mock_configdrive_required.assert_called_once_with(mock_instance)
@ -502,14 +505,14 @@ class VMOpsTestCase(test_base.HyperVBaseTestCase):
mock_create_config_drive.assert_called_once_with(
self.context, mock_instance, [mock.sentinel.FILE],
mock.sentinel.PASSWORD,
mock.sentinel.INFO)
mock.sentinel.network_info)
mock_attach_config_drive.assert_called_once_with(
mock_instance, fake_config_drive_path, fake_vm_gen)
mock_set_boot_order.assert_called_once_with(
mock_instance.name, fake_vm_gen, block_device_info)
mock_power_on.assert_called_once_with(
mock_instance,
network_info=mock.sentinel.INFO,
network_info=mock.sentinel.network_info,
should_plug_vifs=False)
def test_spawn(self):
@ -1220,15 +1223,22 @@ class VMOpsTestCase(test_base.HyperVBaseTestCase):
self._pathutils.check_remove_dir.assert_called_once_with(
exp_inst_path)
@ddt.data(True, False)
@ddt.data({},
{'vm_exists': False, 'planned_vm_exists': False},
{'vm_exists': False, 'planned_vm_exists': True})
@ddt.unpack
@mock.patch('compute_hyperv.nova.volumeops.VolumeOps.disconnect_volumes')
@mock.patch('compute_hyperv.nova.vmops.VMOps._delete_disk_files')
@mock.patch('compute_hyperv.nova.vmops.VMOps.power_off')
@mock.patch('compute_hyperv.nova.vmops.VMOps.unplug_vifs')
def test_destroy(self, vm_exists, mock_unplug_vifs, mock_power_off,
mock_delete_disk_files, mock_disconnect_volumes):
def test_destroy(self, mock_unplug_vifs, mock_power_off,
mock_delete_disk_files, mock_disconnect_volumes,
vm_exists=True,
planned_vm_exists=False):
mock_instance = fake_instance.fake_instance_obj(self.context)
self._vmops._vmutils.vm_exists.return_value = vm_exists
self._vmops._migrutils.planned_vm_exists.return_value = (
planned_vm_exists)
self._vmops.destroy(instance=mock_instance,
block_device_info=mock.sentinel.FAKE_BD_INFO,
@ -1243,11 +1253,14 @@ class VMOpsTestCase(test_base.HyperVBaseTestCase):
mock_power_off.assert_called_once_with(mock_instance)
self._vmops._vmutils.destroy_vm.assert_called_once_with(
mock_instance.name)
else:
elif planned_vm_exists:
self._vmops._migrutils.planned_vm_exists.assert_called_once_with(
mock_instance.name)
self._vmops._migrutils.destroy_planned_vm.assert_called_once_with(
mock_instance.name)
self.assertFalse(self._vmops._vmutils.destroy_vm.called)
else:
self.assertFalse(self._vmops._migrutils.destroy_planned_vm.called)
mock_unplug_vifs.assert_called_once_with(
mock_instance, mock.sentinel.fake_network_info)
@ -1257,14 +1270,6 @@ class VMOpsTestCase(test_base.HyperVBaseTestCase):
mock_instance.name,
self._pathutils.get_instance_dir.return_value)
def test_destroy_inexistent_instance(self):
mock_instance = fake_instance.fake_instance_obj(self.context)
self._vmops._vmutils.vm_exists.return_value = False
self._vmops._vmutils.planned_vm_exists.return_value = False
self._vmops.destroy(instance=mock_instance)
self.assertFalse(self._vmops._vmutils.destroy_vm.called)
@mock.patch('compute_hyperv.nova.vmops.VMOps.power_off')
def test_destroy_exception(self, mock_power_off):
mock_instance = fake_instance.fake_instance_obj(self.context)
@ -1273,7 +1278,9 @@ class VMOpsTestCase(test_base.HyperVBaseTestCase):
self._vmops._vmutils.vm_exists.return_value = True
self.assertRaises(os_win_exc.HyperVException,
self._vmops.destroy, mock_instance)
self._vmops.destroy, mock_instance,
mock.sentinel.network_info,
mock.sentinel.block_device_info)
def test_reboot_hard(self):
self._test_reboot(vmops.REBOOT_TYPE_HARD,