From e34adabdf527768dc8dc526b513433332047a273 Mon Sep 17 00:00:00 2001 From: Vipin Balachandran Date: Tue, 10 Nov 2015 13:59:57 +0530 Subject: [PATCH] VMware: Raise DiskNotFound for missing disk device If volume detach fails after reconfiguring the VM to remove volume vmdk then it will not be possible to detach the volume again. This is because the VMware driver fails the detach operation by raising StorageError when it finds that the volume disk device is missing. Due to this, the volume state will remain 'in-use'. The compute manager expects the driver to raise DiskNotFound if the disk is missing during detach operation. It ignores the exception and allows the detach to complete. This patch fixes the problem with the VMware driver by throwing DiskNotFound instead of StorageError if the volume disk device is missing. Change-Id: I4074d240a3468534c0a269f6cd160f399e8fa18c Closes-Bug: #1514910 --- .../unit/virt/vmwareapi/test_driver_api.py | 35 ++++- .../unit/virt/vmwareapi/test_volumeops.py | 144 ++++++++++++++++++ nova/virt/vmwareapi/driver.py | 2 +- nova/virt/vmwareapi/volumeops.py | 4 +- 4 files changed, 180 insertions(+), 5 deletions(-) diff --git a/nova/tests/unit/virt/vmwareapi/test_driver_api.py b/nova/tests/unit/virt/vmwareapi/test_driver_api.py index a11609b8b2cc..33cc02b484a4 100644 --- a/nova/tests/unit/virt/vmwareapi/test_driver_api.py +++ b/nova/tests/unit/virt/vmwareapi/test_driver_api.py @@ -703,9 +703,9 @@ class VMwareAPIVMTestCase(test.NoDBTestCase): self.assertFalse(mock_destroy.called) @mock.patch.object(driver.VMwareVCDriver, 'detach_volume', - side_effect=exception.StorageError(reason='oh man')) + side_effect=exception.DiskNotFound(message='oh man')) @mock.patch.object(vmops.VMwareVMOps, 'destroy') - def test_destroy_with_attached_volumes_with_storage_error( + def test_destroy_with_attached_volumes_with_disk_not_found( self, mock_destroy, mock_detach_volume): self._create_vm() connection_info = {'data': 'fake-data', 'serial': 'volume-fake-id'} @@ -1446,6 +1446,37 @@ class VMwareAPIVMTestCase(test.NoDBTestCase): self.instance) self.assertFalse(mock_reboot.called) + @mock.patch('nova.virt.driver.block_device_info_get_mapping') + @mock.patch('nova.virt.vmwareapi.driver.VMwareVCDriver.detach_volume') + def test_detach_instance_volumes( + self, detach_volume, block_device_info_get_mapping): + self._create_vm() + + def _mock_bdm(connection_info, device_name): + return {'connection_info': connection_info, + 'device_name': device_name} + + disk_1 = _mock_bdm(mock.sentinel.connection_info_1, 'dev1') + disk_2 = _mock_bdm(mock.sentinel.connection_info_2, 'dev2') + block_device_info_get_mapping.return_value = [disk_1, disk_2] + + detach_volume.side_effect = [None, exception.DiskNotFound("Error")] + + with mock.patch.object(self.conn, '_vmops') as vmops: + block_device_info = mock.sentinel.block_device_info + self.conn._detach_instance_volumes(self.instance, + block_device_info) + + block_device_info_get_mapping.assert_called_once_with( + block_device_info) + vmops.power_off.assert_called_once_with(self.instance) + self.assertEqual(vm_states.STOPPED, self.instance.vm_state) + exp_detach_calls = [mock.call(mock.sentinel.connection_info_1, + self.instance, 'dev1'), + mock.call(mock.sentinel.connection_info_2, + self.instance, 'dev2')] + self.assertEqual(exp_detach_calls, detach_volume.call_args_list) + def test_destroy(self): self._create_vm() info = self._get_info() diff --git a/nova/tests/unit/virt/vmwareapi/test_volumeops.py b/nova/tests/unit/virt/vmwareapi/test_volumeops.py index 81a2e6f884bd..592636669a5b 100644 --- a/nova/tests/unit/virt/vmwareapi/test_volumeops.py +++ b/nova/tests/unit/virt/vmwareapi/test_volumeops.py @@ -158,6 +158,66 @@ class VMwareVolumeOpsTestCase(test.NoDBTestCase): mock.sentinel.vm_ref, mock.sentinel.extra_config) + def _fake_connection_info(self): + return {'driver_volume_type': 'vmdk', + 'serial': 'volume-fake-id', + 'data': {'volume': 'vm-10', + 'volume_id': 'volume-fake-id'}} + + @mock.patch.object(volumeops.VMwareVolumeOps, '_get_volume_uuid') + @mock.patch.object(vm_util, 'get_vmdk_backed_disk_device') + def test_get_vmdk_backed_disk_device(self, get_vmdk_backed_disk_device, + get_volume_uuid): + session = mock.Mock() + self._volumeops._session = session + hardware_devices = mock.sentinel.hardware_devices + session._call_method.return_value = hardware_devices + + disk_uuid = mock.sentinel.disk_uuid + get_volume_uuid.return_value = disk_uuid + + device = mock.sentinel.device + get_vmdk_backed_disk_device.return_value = device + + vm_ref = mock.sentinel.vm_ref + connection_info = self._fake_connection_info() + ret = self._volumeops._get_vmdk_backed_disk_device( + vm_ref, connection_info['data']) + + self.assertEqual(device, ret) + session._call_method.assert_called_once_with( + vutil, "get_object_property", vm_ref, "config.hardware.device") + get_volume_uuid.assert_called_once_with( + vm_ref, connection_info['data']['volume_id']) + get_vmdk_backed_disk_device.assert_called_once_with(hardware_devices, + disk_uuid) + + @mock.patch.object(volumeops.VMwareVolumeOps, '_get_volume_uuid') + @mock.patch.object(vm_util, 'get_vmdk_backed_disk_device') + def test_get_vmdk_backed_disk_device_with_missing_disk_device( + self, get_vmdk_backed_disk_device, get_volume_uuid): + session = mock.Mock() + self._volumeops._session = session + hardware_devices = mock.sentinel.hardware_devices + session._call_method.return_value = hardware_devices + + disk_uuid = mock.sentinel.disk_uuid + get_volume_uuid.return_value = disk_uuid + + get_vmdk_backed_disk_device.return_value = None + + vm_ref = mock.sentinel.vm_ref + connection_info = self._fake_connection_info() + self.assertRaises(exception.DiskNotFound, + self._volumeops._get_vmdk_backed_disk_device, + vm_ref, connection_info['data']) + session._call_method.assert_called_once_with( + vutil, "get_object_property", vm_ref, "config.hardware.device") + get_volume_uuid.assert_called_once_with( + vm_ref, connection_info['data']['volume_id']) + get_vmdk_backed_disk_device.assert_called_once_with(hardware_devices, + disk_uuid) + def test_detach_volume_vmdk(self): vmdk_info = vm_util.VmdkInfo('fake-path', 'lsiLogic', 'thin', @@ -238,6 +298,90 @@ class VMwareVolumeOpsTestCase(test.NoDBTestCase): mock.sentinel.vm_ref, connection_info['data']) self.assertTrue(get_vmdk_info.called) + @mock.patch.object(vm_util, 'get_vm_ref') + @mock.patch.object(vm_util, 'get_rdm_disk') + @mock.patch.object(volumeops.VMwareVolumeOps, '_iscsi_get_target') + @mock.patch.object(volumeops.VMwareVolumeOps, 'detach_disk_from_vm') + def test_detach_volume_iscsi(self, detach_disk_from_vm, iscsi_get_target, + get_rdm_disk, get_vm_ref): + vm_ref = mock.sentinel.vm_ref + get_vm_ref.return_value = vm_ref + + device_name = mock.sentinel.device_name + disk_uuid = mock.sentinel.disk_uuid + iscsi_get_target.return_value = (device_name, disk_uuid) + + session = mock.Mock() + self._volumeops._session = session + hardware_devices = mock.sentinel.hardware_devices + session._call_method.return_value = hardware_devices + + device = mock.sentinel.device + get_rdm_disk.return_value = device + + connection_info = self._fake_connection_info() + instance = mock.sentinel.instance + self._volumeops._detach_volume_iscsi(connection_info, instance) + + get_vm_ref.assert_called_once_with(session, instance) + iscsi_get_target.assert_called_once_with(connection_info['data']) + session._call_method.assert_called_once_with( + vutil, "get_object_property", vm_ref, "config.hardware.device") + get_rdm_disk.assert_called_once_with(hardware_devices, disk_uuid) + detach_disk_from_vm.assert_called_once_with( + vm_ref, instance, device, destroy_disk=True) + + @mock.patch.object(vm_util, 'get_vm_ref') + @mock.patch.object(volumeops.VMwareVolumeOps, '_iscsi_get_target') + def test_detach_volume_iscsi_with_missing_iscsi_target( + self, iscsi_get_target, get_vm_ref): + vm_ref = mock.sentinel.vm_ref + get_vm_ref.return_value = vm_ref + + iscsi_get_target.return_value = (None, None) + + connection_info = self._fake_connection_info() + instance = mock.sentinel.instance + self.assertRaises( + exception.StorageError, self._volumeops._detach_volume_iscsi, + connection_info, instance) + + get_vm_ref.assert_called_once_with(self._volumeops._session, instance) + iscsi_get_target.assert_called_once_with(connection_info['data']) + + @mock.patch.object(vm_util, 'get_vm_ref') + @mock.patch.object(vm_util, 'get_rdm_disk') + @mock.patch.object(volumeops.VMwareVolumeOps, '_iscsi_get_target') + @mock.patch.object(volumeops.VMwareVolumeOps, 'detach_disk_from_vm') + def test_detach_volume_iscsi_with_missing_disk_device( + self, detach_disk_from_vm, iscsi_get_target, + get_rdm_disk, get_vm_ref): + vm_ref = mock.sentinel.vm_ref + get_vm_ref.return_value = vm_ref + + device_name = mock.sentinel.device_name + disk_uuid = mock.sentinel.disk_uuid + iscsi_get_target.return_value = (device_name, disk_uuid) + + session = mock.Mock() + self._volumeops._session = session + hardware_devices = mock.sentinel.hardware_devices + session._call_method.return_value = hardware_devices + + get_rdm_disk.return_value = None + + connection_info = self._fake_connection_info() + instance = mock.sentinel.instance + self.assertRaises( + exception.DiskNotFound, self._volumeops._detach_volume_iscsi, + connection_info, instance) + get_vm_ref.assert_called_once_with(session, instance) + iscsi_get_target.assert_called_once_with(connection_info['data']) + session._call_method.assert_called_once_with( + vutil, "get_object_property", vm_ref, "config.hardware.device") + get_rdm_disk.assert_called_once_with(hardware_devices, disk_uuid) + self.assertFalse(detach_disk_from_vm.called) + def _test_attach_volume_vmdk(self, adapter_type=None): connection_info = {'driver_volume_type': constants.DISK_FORMAT_VMDK, 'serial': 'volume-fake-id', diff --git a/nova/virt/vmwareapi/driver.py b/nova/virt/vmwareapi/driver.py index 9368161f3ab8..7ab6c7e22f09 100644 --- a/nova/virt/vmwareapi/driver.py +++ b/nova/virt/vmwareapi/driver.py @@ -433,7 +433,7 @@ class VMwareVCDriver(driver.ComputeDriver): try: self.detach_volume(connection_info, instance, disk.get('device_name')) - except exception.StorageError: + except exception.DiskNotFound: LOG.warning(_LW('The volume %s does not exist!'), disk.get('device_name'), instance=instance) diff --git a/nova/virt/vmwareapi/volumeops.py b/nova/virt/vmwareapi/volumeops.py index 2614b1223175..2d79b39fd154 100644 --- a/nova/virt/vmwareapi/volumeops.py +++ b/nova/virt/vmwareapi/volumeops.py @@ -511,7 +511,7 @@ class VMwareVolumeOps(object): device = vm_util.get_vmdk_backed_disk_device(hardware_devices, disk_uuid) if not device: - raise exception.StorageError(reason=_("Unable to find volume")) + raise exception.DiskNotFound(message=_("Unable to find volume")) return device def _detach_volume_vmdk(self, connection_info, instance): @@ -568,7 +568,7 @@ class VMwareVolumeOps(object): "config.hardware.device") device = vm_util.get_rdm_disk(hardware_devices, uuid) if device is None: - raise exception.StorageError(reason=_("Unable to find volume")) + raise exception.DiskNotFound(message=_("Unable to find volume")) self.detach_disk_from_vm(vm_ref, instance, device, destroy_disk=True) LOG.debug("Detached ISCSI: %s", connection_info, instance=instance)