diff --git a/nova/tests/virt/vmwareapi/test_driver_api.py b/nova/tests/virt/vmwareapi/test_driver_api.py index e7576e01165e..1a8b272fe70b 100644 --- a/nova/tests/virt/vmwareapi/test_driver_api.py +++ b/nova/tests/virt/vmwareapi/test_driver_api.py @@ -864,6 +864,120 @@ class VMwareAPIVMTestCase(test.NoDBTestCase): self.assertFalse(vmwareapi_fake.get_file(cached_image)) self.assertFalse(vmwareapi_fake.get_file(tmp_file)) + def test_spawn_disk_extend_failed_copy(self): + # Spawn instance + # copy for extend fails without creating a file + # + # Expect the copy error to be raised + self.flags(use_linked_clone=True, group='vmware') + self.wait_task = self.conn._session._wait_for_task + self.call_method = self.conn._session._call_method + + CopyError = error_util.FileFaultException + + def fake_wait_for_task(task_ref): + if task_ref == 'fake-copy-task': + raise CopyError('Copy failed!') + return self.wait_task(task_ref) + + def fake_call_method(module, method, *args, **kwargs): + if method == "CopyVirtualDisk_Task": + return 'fake-copy-task' + + return self.call_method(module, method, *args, **kwargs) + + with contextlib.nested( + mock.patch.object(self.conn._session, '_call_method', + new=fake_call_method), + mock.patch.object(self.conn._session, '_wait_for_task', + new=fake_wait_for_task)): + self.assertRaises(CopyError, self._create_vm) + + def test_spawn_disk_extend_failed_partial_copy(self): + # Spawn instance + # Copy for extend fails, leaving a file behind + # + # Expect the file to be cleaned up + # Expect the copy error to be raised + self.flags(use_linked_clone=True, group='vmware') + self.wait_task = self.conn._session._wait_for_task + self.call_method = self.conn._session._call_method + self.task_ref = None + uuid = self.fake_image_uuid + cached_image = '[%s] vmware_base/%s/%s.80.vmdk' % (self.ds, + uuid, uuid) + + CopyError = error_util.FileFaultException + + def fake_wait_for_task(task_ref): + if task_ref == self.task_ref: + self.task_ref = None + self.assertTrue(vmwareapi_fake.get_file(cached_image)) + # N.B. We don't test for -flat here because real + # CopyVirtualDisk_Task doesn't actually create it + raise CopyError('Copy failed!') + return self.wait_task(task_ref) + + def fake_call_method(module, method, *args, **kwargs): + task_ref = self.call_method(module, method, *args, **kwargs) + if method == "CopyVirtualDisk_Task": + self.task_ref = task_ref + return task_ref + + with contextlib.nested( + mock.patch.object(self.conn._session, '_call_method', + new=fake_call_method), + mock.patch.object(self.conn._session, '_wait_for_task', + new=fake_wait_for_task)): + self.assertRaises(CopyError, self._create_vm) + self.assertFalse(vmwareapi_fake.get_file(cached_image)) + + def test_spawn_disk_extend_failed_partial_copy_failed_cleanup(self): + # Spawn instance + # Copy for extend fails, leaves file behind + # File cleanup fails + # + # Expect file to be left behind + # Expect file cleanup error to be raised + self.flags(use_linked_clone=True, group='vmware') + self.wait_task = self.conn._session._wait_for_task + self.call_method = self.conn._session._call_method + self.task_ref = None + uuid = self.fake_image_uuid + cached_image = '[%s] vmware_base/%s/%s.80.vmdk' % (self.ds, + uuid, uuid) + + CopyError = error_util.FileFaultException + DeleteError = error_util.CannotDeleteFileException + + def fake_wait_for_task(task_ref): + if task_ref == self.task_ref: + self.task_ref = None + self.assertTrue(vmwareapi_fake.get_file(cached_image)) + # N.B. We don't test for -flat here because real + # CopyVirtualDisk_Task doesn't actually create it + raise CopyError('Copy failed!') + elif task_ref == 'fake-delete-task': + raise DeleteError('Delete failed!') + return self.wait_task(task_ref) + + def fake_call_method(module, method, *args, **kwargs): + if method == "DeleteDatastoreFile_Task": + return 'fake-delete-task' + + task_ref = self.call_method(module, method, *args, **kwargs) + if method == "CopyVirtualDisk_Task": + self.task_ref = task_ref + return task_ref + + with contextlib.nested( + mock.patch.object(self.conn._session, '_wait_for_task', + new=fake_wait_for_task), + mock.patch.object(self.conn._session, '_call_method', + new=fake_call_method)): + self.assertRaises(DeleteError, self._create_vm) + self.assertTrue(vmwareapi_fake.get_file(cached_image)) + def test_spawn_disk_invalid_disk_size(self): self.mox.StubOutWithMock(vmware_images, 'get_vmdk_size_and_properties') result = [82 * units.Gi, diff --git a/nova/virt/vmwareapi/vmops.py b/nova/virt/vmwareapi/vmops.py index 331f0c76f78f..b7000eb76991 100644 --- a/nova/virt/vmwareapi/vmops.py +++ b/nova/virt/vmwareapi/vmops.py @@ -34,7 +34,7 @@ from nova import context as nova_context from nova import exception from nova.network import model as network_model from nova.openstack.common import excutils -from nova.openstack.common.gettextutils import _ +from nova.openstack.common.gettextutils import _, _LE from nova.openstack.common import lockutils from nova.openstack.common import log as logging from nova.openstack.common import strutils @@ -453,27 +453,66 @@ class VMwareVMOps(object): root_vmdk_name = "%s.%s.vmdk" % (upload_name, root_gb) root_vmdk_path = str(datastore.build_path( upload_folder, root_vmdk_name)) - if not self._check_if_folder_file_exists( - ds_browser, - datastore.ref, datastore.name, - upload_folder, - upload_name + ".%s.vmdk" % root_gb): - LOG.debug("Copying root disk of size %sGb", root_gb) - try: + + # Ensure only a single thread extends the image at once. + # We do this by taking a lock on the name of the extended + # image. This allows multiple threads to create resized + # copies simultaneously, as long as they are different + # sizes. Threads attempting to create the same resized copy + # will be serialized, with only the first actually creating + # the copy. + # + # Note that the object is in a per-nova cache directory, + # so inter-nova locking is not a concern. Consequently we + # can safely use simple thread locks. + + with lockutils.lock(root_vmdk_path, + lock_file_prefix='nova-vmware-image'): + if not self._check_if_folder_file_exists( + ds_browser, + datastore.ref, datastore.name, + upload_folder, + root_vmdk_name): + LOG.debug("Copying root disk of size %sGb", + root_gb) + copy_spec = self.get_copy_virtual_disk_spec( client_factory, adapter_type, disk_type) - vm_util.copy_virtual_disk(self._session, - dc_info.ref, - uploaded_file_path, - root_vmdk_path, - copy_spec) - except Exception as e: - LOG.warning(_("Root disk file creation " - "failed - %s"), e) - if root_gb_in_kb > vmdk_file_size_in_kb: - self._extend_virtual_disk(instance, root_gb_in_kb, - root_vmdk_path, - dc_info.ref) + + # Create a copy of the base image, ensuring we + # clean up on failure + try: + vm_util.copy_virtual_disk(self._session, + dc_info.ref, + uploaded_file_path, + root_vmdk_path, + copy_spec) + except Exception as e: + with excutils.save_and_reraise_exception(): + LOG.error(_LE('Failed to copy cached ' + 'image %(source)s to ' + '%(dest)s for resize: ' + '%(error)s'), + {'source': uploaded_file_path, + 'dest': root_vmdk_path, + 'error': e.message}) + try: + ds_util.file_delete(self._session, + root_vmdk_path, + dc_info.ref) + except error_util.FileNotFoundException: + # File was never created: cleanup not + # required + pass + + # Resize the copy to the appropriate size. No need + # for cleanup up here, as _extend_virtual_disk + # already does it + if root_gb_in_kb > vmdk_file_size_in_kb: + self._extend_virtual_disk(instance, + root_gb_in_kb, + root_vmdk_path, + dc_info.ref) # Attach the root disk to the VM. if root_vmdk_path: