From fc7c36c9ca94de6bd2481c59d7710566dd4a1ddb Mon Sep 17 00:00:00 2001 From: Kartik Bommepally Date: Mon, 30 Sep 2013 02:45:37 -0700 Subject: [PATCH] VMware ESX: Fixes vol clone & clone from snapshot The driver was earlier performing create from a source volume and create from a snapshot point by copying the source volume container folder. This is not valid in case of vSAN datastore where the volume data is not a 'file' in the container folder unlike VMFS/NFS datastores. So copying folder will only copy descriptor file and hence the source and cloned volume will end up writing to the same data location, eventually corrupting it. The fix is to copy only the VMDK file (descriptor + data) and not the entire container folder. The implementation does the following: 1. Create a volume backing 2. Delete the current VMDK 3. Make a copy of source volume's VMDK file to the destination using virtual disk manager. In case of create from snapshot, we copy the VMDK file corresponding to the snapshot point. Fixes bug: 1233034 Change-Id: I51c9870c11a6d66f9f47c1ad9aaa2abc3dd0533a --- cinder/tests/test_vmware_vmdk.py | 122 +++++----------------- cinder/volume/drivers/vmware/vmdk.py | 68 +++++------- cinder/volume/drivers/vmware/volumeops.py | 77 +------------- 3 files changed, 52 insertions(+), 215 deletions(-) diff --git a/cinder/tests/test_vmware_vmdk.py b/cinder/tests/test_vmware_vmdk.py index 973ed75e3..6a28e0866 100644 --- a/cinder/tests/test_vmware_vmdk.py +++ b/cinder/tests/test_vmware_vmdk.py @@ -942,85 +942,31 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase): m.UnsetStubs() m.VerifyAll() - def test_copy_backing(self): - """Test copy_backing.""" - m = self.mox - m.StubOutWithMock(api.VMwareAPISession, 'vim') - self._session.vim = self._vim - m.StubOutWithMock(self._session, 'invoke_api') - src_path = 'src_path' - dest_path = 'dest_path' - task = FakeMor('Task', 'my_task') - self._session.invoke_api(self._vim, 'CopyDatastoreFile_Task', - mox.IgnoreArg(), sourceName=src_path, - destinationName=dest_path).AndReturn(task) - m.StubOutWithMock(self._session, 'wait_for_task') - self._session.wait_for_task(task) - - m.ReplayAll() - self._volumeops.copy_backing(src_path, dest_path) - m.UnsetStubs() - m.VerifyAll() - - def test_register_backing(self): - """Test register_backing.""" - m = self.mox - m.StubOutWithMock(api.VMwareAPISession, 'vim') - self._session.vim = self._vim - m.StubOutWithMock(self._session, 'invoke_api') - path = 'src_path' - name = 'name' - folder = FakeMor('Folder', 'my_fol') - resource_pool = FakeMor('ResourcePool', 'my_rp') - task = FakeMor('Task', 'my_task') - self._session.invoke_api(self._vim, 'RegisterVM_Task', folder, - path=path, name=name, asTemplate=False, - pool=resource_pool).AndReturn(task) - m.StubOutWithMock(self._session, 'wait_for_task') - task_info = FakeTaskInfo('success', mox.IgnoreArg()) - self._session.wait_for_task(task).AndReturn(task_info) - - m.ReplayAll() - self._volumeops.register_backing(path, name, folder, resource_pool) - m.UnsetStubs() - m.VerifyAll() - def test_clone_backing_by_copying(self): """Test _clone_backing_by_copying.""" m = self.mox m.StubOutWithMock(self._driver.__class__, 'volumeops') self._driver.volumeops = self._volumeops volume = FakeObject() - volume['name'] = 'volume_name' - volume['size'] = 1 - m.StubOutWithMock(self._volumeops, 'get_path_name') - src_path = '[datastore1] vm/' - vmx_name = 'vm.vmx' + src_vmdk_path = "[datastore] src_vm/src_vm.vmdk" + new_vmdk_path = "[datastore] dest_vm/dest_vm.vmdk" backing = FakeMor('VirtualMachine', 'my_back') - self._volumeops.get_path_name(backing).AndReturn(src_path + vmx_name) - m.StubOutWithMock(self._volumeops, 'get_host') - host = FakeMor('HostSystem', 'my_host') - self._volumeops.get_host(backing).AndReturn(host) - m.StubOutWithMock(self._volumeops, 'get_dss_rp') - datastores = [FakeMor('Datastore', 'my_ds')] - resource_pool = FakeMor('ResourcePool', 'my_rp') - self._volumeops.get_dss_rp(host).AndReturn((datastores, resource_pool)) - m.StubOutWithMock(self._driver, '_get_folder_ds_summary') - folder = FakeMor('Folder', 'my_fol') - summary = FakeDatastoreSummary(1, 1, datastore=datastores[0], - name='datastore_name') - self._driver._get_folder_ds_summary(volume['size'], resource_pool, - datastores).AndReturn((folder, - summary)) - m.StubOutWithMock(self._volumeops, 'copy_backing') - dest_path = '[%s] %s/' % (summary.name, volume['name']) - self._volumeops.copy_backing(src_path, dest_path) - m.StubOutWithMock(self._volumeops, 'register_backing') - self._volumeops.register_backing(dest_path + vmx_name, - volume['name'], folder, resource_pool) + m.StubOutWithMock(self._driver, '_create_backing_in_inventory') + mux = self._driver._create_backing_in_inventory(volume) + mux.AndReturn(backing) + m.StubOutWithMock(self._volumeops, 'get_vmdk_path') + self._volumeops.get_vmdk_path(backing).AndReturn(new_vmdk_path) + m.StubOutWithMock(self._volumeops, 'get_dc') + datacenter = FakeMor('Datacenter', 'my_dc') + self._volumeops.get_dc(backing).AndReturn(datacenter) + m.StubOutWithMock(self._volumeops, 'delete_vmdk_file') + self._volumeops.delete_vmdk_file(new_vmdk_path, datacenter) + m.StubOutWithMock(self._volumeops, 'copy_vmdk_file') + self._volumeops.copy_vmdk_file(datacenter, src_vmdk_path, + new_vmdk_path) m.ReplayAll() - self._driver._clone_backing_by_copying(volume, backing) + self._driver._clone_backing_by_copying(volume, src_vmdk_path) m.UnsetStubs() m.VerifyAll() @@ -1035,8 +981,11 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase): src_vref['name'] = 'src_snapshot_name' backing = FakeMor('VirtualMachine', 'my_vm') self._volumeops.get_backing(src_vref['name']).AndReturn(backing) + m.StubOutWithMock(self._volumeops, 'get_vmdk_path') + src_vmdk_path = "[datastore] src_vm/src_vm.vmdk" + self._volumeops.get_vmdk_path(backing).AndReturn(src_vmdk_path) m.StubOutWithMock(self._driver, '_clone_backing_by_copying') - self._driver._clone_backing_by_copying(volume, backing) + self._driver._clone_backing_by_copying(volume, src_vmdk_path) m.ReplayAll() self._driver.create_cloned_volume(volume, src_vref) @@ -1081,25 +1030,6 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase): m.UnsetStubs() m.VerifyAll() - def test_revert_to_snapshot(self): - """Test revert_to_snapshot.""" - m = self.mox - m.StubOutWithMock(api.VMwareAPISession, 'vim') - self._session.vim = self._vim - m.StubOutWithMock(self._session, 'invoke_api') - task = FakeMor('Task', 'my_task') - snapshot = FakeMor('VirtualMachineSnapshot', 'my_snap') - self._session.invoke_api(self._vim, 'RevertToSnapshot_Task', - snapshot).AndReturn(task) - - m.StubOutWithMock(self._session, 'wait_for_task') - self._session.wait_for_task(task) - - m.ReplayAll() - self._volumeops.revert_to_snapshot(snapshot) - m.UnsetStubs() - m.VerifyAll() - def test_create_volume_from_snapshot(self): """Test create_volume_from_snapshot.""" m = self.mox @@ -1116,15 +1046,11 @@ class VMwareEsxVmdkDriverTestCase(test.TestCase): snapshot_mor = FakeMor('VirtualMachineSnapshot', 'my_snap') self._volumeops.get_snapshot(backing, snapshot['name']).AndReturn(snapshot_mor) + m.StubOutWithMock(self._volumeops, 'get_vmdk_path') + src_vmdk_path = "[datastore] src_vm/src_vm-001.vmdk" + self._volumeops.get_vmdk_path(snapshot_mor).AndReturn(src_vmdk_path) m.StubOutWithMock(self._driver, '_clone_backing_by_copying') - clone = FakeMor('VirtualMachine', 'my_clone') - self._driver._clone_backing_by_copying(volume, - backing).AndReturn(clone) - clone_snap = FakeMor('VirtualMachineSnapshot', 'my_clone_snap') - self._volumeops.get_snapshot(clone, - snapshot['name']).AndReturn(clone_snap) - m.StubOutWithMock(self._volumeops, 'revert_to_snapshot') - self._volumeops.revert_to_snapshot(clone_snap) + self._driver._clone_backing_by_copying(volume, src_vmdk_path) m.ReplayAll() self._driver.create_volume_from_snapshot(volume, snapshot) diff --git a/cinder/volume/drivers/vmware/vmdk.py b/cinder/volume/drivers/vmware/vmdk.py index a63d63bce..f48435325 100644 --- a/cinder/volume/drivers/vmware/vmdk.py +++ b/cinder/volume/drivers/vmware/vmdk.py @@ -469,51 +469,39 @@ class VMwareEsxVmdkDriver(driver.VolumeDriver): """ self._delete_snapshot(snapshot) - def _clone_backing_by_copying(self, volume, backing): - """Creates volume clone. + def _clone_backing_by_copying(self, volume, src_vmdk_path): + """Clones volume backing. - Here we copy the backing on a datastore under the host and then - register the copied backing to the inventory. - It is assumed here that all the source backing files are in the - same folder on the datastore. + Creates a backing for the input volume and replaces its VMDK file + with the input VMDK file copy. :param volume: New Volume object - :param backing: Reference to backing entity that must be cloned - :return: Reference to the cloned backing + :param src_vmdk_path: VMDK file path of the source volume backing """ - src_path_name = self.volumeops.get_path_name(backing) - (datastore_name, - folder_path, filename) = volumeops.split_datastore_path(src_path_name) - # Pick a datastore where to create the full clone under same host - host = self.volumeops.get_host(backing) - (datastores, resource_pool) = self.volumeops.get_dss_rp(host) - (folder, summary) = self._get_folder_ds_summary(volume['size'], - resource_pool, - datastores) - src_path = '[%s] %s' % (datastore_name, folder_path) - dest_path = '[%s] %s/' % (summary.name, volume['name']) - # Copy source backing files to a destination location - self.volumeops.copy_backing(src_path, dest_path) - # Register the backing to the inventory - dest_path_name = '%s%s' % (dest_path, filename) - clone = self.volumeops.register_backing(dest_path_name, - volume['name'], folder, - resource_pool) - LOG.info(_("Successfully cloned new backing: %s.") % clone) - return clone + + # Create a backing + backing = self._create_backing_in_inventory(volume) + new_vmdk_path = self.volumeops.get_vmdk_path(backing) + datacenter = self.volumeops.get_dc(backing) + # Deleting the current VMDK file + self.volumeops.delete_vmdk_file(new_vmdk_path, datacenter) + # Copying the source VMDK file + self.volumeops.copy_vmdk_file(datacenter, src_vmdk_path, new_vmdk_path) + LOG.info(_("Successfully cloned new backing: %(back)s from " + "source VMDK file: %(vmdk)s.") % + {'back': backing, 'vmdk': src_vmdk_path}) def _create_cloned_volume(self, volume, src_vref): """Creates volume clone. If source volume's backing does not exist, then pass. - Here we copy the backing on a datastore under the host and then - register the copied backing to the inventory. - It is assumed here that all the src_vref backing files are in the - same folder on the datastore. + Creates a backing and replaces its VMDK file with a copy of the + source backing's VMDK file. :param volume: New Volume object :param src_vref: Volume object that must be cloned """ + backing = self.volumeops.get_backing(src_vref['name']) if not backing: LOG.info(_("There is no backing for the source volume: " @@ -522,7 +510,8 @@ class VMwareEsxVmdkDriver(driver.VolumeDriver): {'svol': src_vref['name'], 'vol': volume['name']}) return - self._clone_backing_by_copying(volume, backing) + src_vmdk_path = self.volumeops.get_vmdk_path(backing) + self._clone_backing_by_copying(volume, src_vmdk_path) def create_cloned_volume(self, volume, src_vref): """Creates volume clone. @@ -537,12 +526,12 @@ class VMwareEsxVmdkDriver(driver.VolumeDriver): If the snapshot does not exist or source volume's backing does not exist, then pass. - Else we perform _create_cloned_volume and then revert the backing to - the appropriate snapshot point. + Else creates clone of source volume backing by copying its VMDK file. :param volume: Volume object :param snapshot: Snapshot object """ + backing = self.volumeops.get_backing(snapshot['volume_name']) if not backing: LOG.info(_("There is no backing for the source snapshot: " @@ -559,13 +548,8 @@ class VMwareEsxVmdkDriver(driver.VolumeDriver): "volume: %(vol)s.") % {'snap': snapshot['name'], 'vol': volume['name']}) return - clone = self._clone_backing_by_copying(volume, backing) - # Reverting the clone to the snapshot point. - snapshot_moref = self.volumeops.get_snapshot(clone, snapshot['name']) - self.volumeops.revert_to_snapshot(snapshot_moref) - LOG.info(_("Successfully reverted clone: %(clone)s to snapshot: " - "%(snapshot)s.") % - {'clone': clone, 'snapshot': snapshot_moref}) + src_vmdk_path = self.volumeops.get_vmdk_path(snapshot_moref) + self._clone_backing_by_copying(volume, src_vmdk_path) def create_volume_from_snapshot(self, volume, snapshot): """Creates a volume from a snapshot. diff --git a/cinder/volume/drivers/vmware/volumeops.py b/cinder/volume/drivers/vmware/volumeops.py index a407e00f0..8f9972d35 100644 --- a/cinder/volume/drivers/vmware/volumeops.py +++ b/cinder/volume/drivers/vmware/volumeops.py @@ -583,36 +583,6 @@ class VMwareVolumeOps(object): self._session.wait_for_task(task) LOG.info(_("Successfully deleted file: %s.") % file_path) - def copy_backing(self, src_folder_path, dest_folder_path): - """Copy the backing folder recursively onto the destination folder. - - This method overwrites all the files at the destination if present - by deleting them first. - - :param src_folder_path: Datastore path of the source folder - :param dest_folder_path: Datastore path of the destination - """ - LOG.debug(_("Copying backing files from %(src)s to %(dest)s.") % - {'src': src_folder_path, 'dest': dest_folder_path}) - fileManager = self._session.vim.service_content.fileManager - try: - task = self._session.invoke_api(self._session.vim, - 'CopyDatastoreFile_Task', - fileManager, - sourceName=src_folder_path, - destinationName=dest_folder_path) - LOG.debug(_("Initiated copying of backing via task: %s.") % task) - self._session.wait_for_task(task) - LOG.info(_("Successfully copied backing to %s.") % - dest_folder_path) - except error_util.VimFaultException as excep: - if FILE_ALREADY_EXISTS not in excep.fault_list: - raise excep - # There might be files on datastore due to previous failed attempt - # We clean the folder up and retry the copy - self.delete_file(dest_folder_path) - self.copy_backing(src_folder_path, dest_folder_path) - def get_path_name(self, backing): """Get path name of the backing. @@ -623,49 +593,6 @@ class VMwareVolumeOps(object): self._session.vim, backing, 'config.files').vmPathName - def register_backing(self, path, name, folder, resource_pool): - """Register backing to the inventory. - - :param path: Datastore path to the backing - :param name: Name with which we register the backing - :param folder: Reference to the folder entity - :param resource_pool: Reference to the resource pool entity - :return: Reference to the backing that is registered - """ - try: - LOG.debug(_("Registering backing at path: %s to inventory.") % - path) - task = self._session.invoke_api(self._session.vim, - 'RegisterVM_Task', folder, - path=path, name=name, - asTemplate=False, - pool=resource_pool) - LOG.debug(_("Initiated registring backing, task: %s.") % task) - task_info = self._session.wait_for_task(task) - backing = task_info.result - LOG.info(_("Successfully registered backing: %s.") % backing) - return backing - except error_util.VimFaultException as excep: - if ALREADY_EXISTS not in excep.fault_list: - raise excep - # If the vmx is already registered to the inventory that may - # happen due to previous failed attempts, then we simply retrieve - # the backing moref based on name and return. - return self.get_backing(name) - - def revert_to_snapshot(self, snapshot): - """Revert backing to a snapshot point. - - :param snapshot: Reference to the snapshot entity - """ - LOG.debug(_("Reverting backing to snapshot: %s.") % snapshot) - task = self._session.invoke_api(self._session.vim, - 'RevertToSnapshot_Task', - snapshot) - LOG.debug(_("Initiated reverting snapshot via task: %s.") % task) - self._session.wait_for_task(task) - LOG.info(_("Successfully reverted to snapshot: %s.") % snapshot) - def get_entity_name(self, entity): """Get name of the managed entity. @@ -719,8 +646,8 @@ class VMwareVolumeOps(object): force=True) LOG.debug(_("Initiated copying disk data via task: %s.") % task) self._session.wait_for_task(task) - LOG.info(_("Successfully copied disk data to: %s.") % - dest_vmdk_file_path) + LOG.info(_("Successfully copied disk at: %(src)s to: %(dest)s.") % + {'src': src_vmdk_file_path, 'dest': dest_vmdk_file_path}) def delete_vmdk_file(self, vmdk_file_path, dc_ref): """Delete given vmdk files. -- 2.45.2