Fix: Convert volume to another directory instead of copying it while taking volume snapshots on KVM (#8041)

Author: GutoVeronezi

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java

@@ -1715,11 +1715,11 @@ public Answer createSnapshot(final CreateObjectCommand cmd) {
                     snapshotPath = getSnapshotPathInPrimaryStorage(primaryPool.getLocalPath(), snapshotName);
 
                     String diskLabel = takeVolumeSnapshot(resource.getDisks(conn, vmName), snapshotName, diskPath, vm);
-                    String copyResult = copySnapshotToPrimaryStorageDir(primaryPool, diskPath, snapshotPath, volume);
+                    String convertResult = convertBaseFileToSnapshotFileInPrimaryStorageDir(primaryPool, diskPath, snapshotPath, volume, cmd.getWait());
 
                     mergeSnapshotIntoBaseFile(vm, diskLabel, diskPath, snapshotName, volume, conn);
 
-                    validateCopyResult(copyResult, snapshotPath);
+                    validateConvertResult(convertResult, snapshotPath);
                 } catch (LibvirtException e) {
                     if (!e.getMessage().contains(LIBVIRT_OPERATION_NOT_SUPPORTED_MESSAGE)) {
                         throw e;
@@ -1784,8 +1784,8 @@ public Answer createSnapshot(final CreateObjectCommand cmd) {
                     }
                 } else {
                     snapshotPath = getSnapshotPathInPrimaryStorage(primaryPool.getLocalPath(), snapshotName);
-                    String copyResult = copySnapshotToPrimaryStorageDir(primaryPool, diskPath, snapshotPath, volume);
-                    validateCopyResult(copyResult, snapshotPath);
+                    String convertResult = convertBaseFileToSnapshotFileInPrimaryStorageDir(primaryPool, diskPath, snapshotPath, volume, cmd.getWait());
+                    validateConvertResult(convertResult, snapshotPath);
                 }
             }
 
@@ -1838,13 +1838,13 @@ protected void takeFullVmSnapshotForBinariesThatDoesNotSupportLiveDiskSnapshot(D
         s_logger.debug(String.format("Full VM Snapshot [%s] of VM [%s] took [%s] seconds to finish.", snapshotName, vmName, (System.currentTimeMillis() - start)/1000));
     }
 
-    protected void validateCopyResult(String copyResult, String snapshotPath) throws CloudRuntimeException, IOException {
-        if (copyResult == null) {
+    protected void validateConvertResult(String convertResult, String snapshotPath) throws CloudRuntimeException, IOException {
+        if (convertResult == null) {
             return;
         }
 
         Files.deleteIfExists(Paths.get(snapshotPath));
-        throw new CloudRuntimeException(copyResult);
+        throw new CloudRuntimeException(convertResult);
     }
 
     /**
@@ -1901,20 +1901,31 @@ protected void manuallyDeleteUnusedSnapshotFile(boolean isLibvirtSupportingFlagD
     }
 
     /**
-     * Creates the snapshot directory in the primary storage, if it does not exist; then copies the base file (VM's old writing file) to the snapshot dir..
+     * Creates the snapshot directory in the primary storage, if it does not exist; then, converts the base file (VM's old writing file) to the snapshot directory.
      * @param primaryPool Storage to create folder, if not exists;
-     * @param baseFile Base file of VM, which will be copied;
-     * @param snapshotPath Path to copy the base file;
-     * @return null if copies successfully or a error message.
+     * @param baseFile Base file of VM, which will be converted;
+     * @param snapshotPath Path to convert the base file;
+     * @return null if the conversion occurs successfully or an error message that must be handled.
      */
-    protected String copySnapshotToPrimaryStorageDir(KVMStoragePool primaryPool, String baseFile, String snapshotPath, VolumeObjectTO volume) {
+    protected String convertBaseFileToSnapshotFileInPrimaryStorageDir(KVMStoragePool primaryPool, String baseFile, String snapshotPath, VolumeObjectTO volume, int wait) {
         try {
+            s_logger.debug(String.format("Trying to convert volume [%s] (%s) to snapshot [%s].", volume, baseFile, snapshotPath));
+
             primaryPool.createFolder(TemplateConstants.DEFAULT_SNAPSHOT_ROOT_DIR);
-            Files.copy(Paths.get(baseFile), Paths.get(snapshotPath));
-            s_logger.debug(String.format("Copied %s snapshot from [%s] to [%s].", volume, baseFile, snapshotPath));
+
+            QemuImgFile srcFile = new QemuImgFile(baseFile);
+            srcFile.setFormat(PhysicalDiskFormat.QCOW2);
+
+            QemuImgFile destFile = new QemuImgFile(snapshotPath);
+            destFile.setFormat(PhysicalDiskFormat.QCOW2);
+
+            QemuImg q = new QemuImg(wait);
+            q.convert(srcFile, destFile);
+
+            s_logger.debug(String.format("Converted volume [%s] (from path \"%s\") to snapshot [%s].", volume, baseFile, snapshotPath));
             return null;
-        } catch (IOException ex) {
-            return String.format("Unable to copy %s snapshot [%s] to [%s] due to [%s].", volume, baseFile, snapshotPath, ex.getMessage());
+        } catch (QemuImgException | LibvirtException ex) {
+            return String.format("Failed to convert %s snapshot of volume [%s] to [%s] due to [%s].", volume, baseFile, snapshotPath, ex.getMessage());
         }
     }
 

plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessorTest.java

@@ -39,6 +39,9 @@
 import java.util.Set;
 import org.apache.cloudstack.storage.to.SnapshotObjectTO;
 import org.apache.cloudstack.storage.to.VolumeObjectTO;
+import org.apache.cloudstack.utils.qemu.QemuImg;
+import org.apache.cloudstack.utils.qemu.QemuImgException;
+import org.apache.cloudstack.utils.qemu.QemuImgFile;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
@@ -91,6 +94,9 @@ public class KVMStorageProcessorTest {
     @Mock
     Connect connectMock;
 
+    @Mock
+    QemuImg qemuImgMock;
+
     @Mock
     LibvirtDomainXMLParser libvirtDomainXMLParserMock;
     @Mock
@@ -251,32 +257,53 @@ public void validateTakeVolumeSnapshotSuccessReturnDiskLabel() throws LibvirtExc
 
     @Test
     @PrepareForTest(KVMStorageProcessor.class)
-    public void validateCopySnapshotToPrimaryStorageDirFailToCopyReturnErrorMessage() throws Exception {
+    public void convertBaseFileToSnapshotFileInPrimaryStorageDirTestFailToConvertWithQemuImgExceptionReturnErrorMessage() throws Exception {
         String baseFile = "baseFile";
         String snapshotPath = "snapshotPath";
         String errorMessage = "error";
-        String expectedResult = String.format("Unable to copy %s snapshot [%s] to [%s] due to [%s].", volumeObjectToMock, baseFile, snapshotPath, errorMessage);
+        String expectedResult = String.format("Failed to convert %s snapshot of volume [%s] to [%s] due to [%s].", volumeObjectToMock, baseFile, snapshotPath, errorMessage);
 
         Mockito.doReturn(true).when(kvmStoragePoolMock).createFolder(Mockito.anyString());
-        PowerMockito.mockStatic(Files.class);
-        PowerMockito.when(Files.copy(Mockito.any(Path.class), Mockito.any(Path.class), Mockito.any())).thenThrow(new IOException(errorMessage));
 
-        String result = storageProcessorSpy.copySnapshotToPrimaryStorageDir(kvmStoragePoolMock, baseFile, snapshotPath, volumeObjectToMock);
+        PowerMockito.whenNew(QemuImg.class).withArguments(Mockito.anyInt()).thenReturn(qemuImgMock);
+        Mockito.doThrow(new QemuImgException(errorMessage)).when(qemuImgMock).convert(Mockito.any(QemuImgFile.class), Mockito.any(QemuImgFile.class));
+
+        String result = storageProcessorSpy.convertBaseFileToSnapshotFileInPrimaryStorageDir(kvmStoragePoolMock, baseFile, snapshotPath, volumeObjectToMock, 1);
 
         Assert.assertEquals(expectedResult, result);
     }
 
     @Test
     @PrepareForTest(KVMStorageProcessor.class)
-    public void validateCopySnapshotToPrimaryStorageDirCopySuccessReturnNull() throws Exception {
+    public void convertBaseFileToSnapshotFileInPrimaryStorageDirTestFailToConvertWithLibvirtExceptionReturnErrorMessage() throws Exception {
         String baseFile = "baseFile";
         String snapshotPath = "snapshotPath";
+        String errorMessage = "null";
+        String expectedResult = String.format("Failed to convert %s snapshot of volume [%s] to [%s] due to [%s].", volumeObjectToMock, baseFile, snapshotPath, errorMessage);
 
         Mockito.doReturn(true).when(kvmStoragePoolMock).createFolder(Mockito.anyString());
-        PowerMockito.mockStatic(Files.class);
-        PowerMockito.when(Files.copy(Mockito.any(Path.class), Mockito.any(Path.class), Mockito.any())).thenReturn(null);
 
-        String result = storageProcessorSpy.copySnapshotToPrimaryStorageDir(kvmStoragePoolMock, baseFile, snapshotPath, volumeObjectToMock);
+        PowerMockito.whenNew(QemuImg.class).withArguments(Mockito.anyInt()).thenReturn(qemuImgMock);
+        Mockito.doThrow(LibvirtException.class).when(qemuImgMock).convert(Mockito.any(QemuImgFile.class), Mockito.any(QemuImgFile.class));
+
+        String result = storageProcessorSpy.convertBaseFileToSnapshotFileInPrimaryStorageDir(kvmStoragePoolMock, baseFile, snapshotPath, volumeObjectToMock, 1);
+
+        Assert.assertEquals(expectedResult, result);
+    }
+
+
+    @Test
+    @PrepareForTest(KVMStorageProcessor.class)
+    public void convertBaseFileToSnapshotFileInPrimaryStorageDirTestConvertSuccessReturnNull() throws Exception {
+        String baseFile = "baseFile";
+        String snapshotPath = "snapshotPath";
+
+        Mockito.doReturn(true).when(kvmStoragePoolMock).createFolder(Mockito.anyString());
+
+        PowerMockito.whenNew(QemuImg.class).withArguments(Mockito.anyInt()).thenReturn(qemuImgMock);
+        Mockito.doNothing().when(qemuImgMock).convert(Mockito.any(QemuImgFile.class), Mockito.any(QemuImgFile.class));
+
+        String result = storageProcessorSpy.convertBaseFileToSnapshotFileInPrimaryStorageDir(kvmStoragePoolMock, baseFile, snapshotPath, volumeObjectToMock, 1);
 
         Assert.assertNull(result);
     }
@@ -321,22 +348,22 @@ public void validateIsAvailablePoolSizeDividedByDiskSizeLesserThanMinRate(){
 
     @Test
     public void validateValidateCopyResultResultIsNullReturn() throws CloudRuntimeException, IOException{
-        storageProcessorSpy.validateCopyResult(null, "");
+        storageProcessorSpy.validateConvertResult(null, "");
     }
 
     @Test (expected = IOException.class)
     public void validateValidateCopyResultFailToDeleteThrowIOException() throws CloudRuntimeException, IOException{
         PowerMockito.mockStatic(Files.class);
         PowerMockito.when(Files.deleteIfExists(Mockito.any())).thenThrow(new IOException(""));
-        storageProcessorSpy.validateCopyResult("", "");
+        storageProcessorSpy.validateConvertResult("", "");
     }
 
     @Test (expected = CloudRuntimeException.class)
     @PrepareForTest(KVMStorageProcessor.class)
     public void validateValidateCopyResulResultNotNullThrowCloudRuntimeException() throws CloudRuntimeException, IOException{
         PowerMockito.mockStatic(Files.class);
         PowerMockito.when(Files.deleteIfExists(Mockito.any())).thenReturn(true);
-        storageProcessorSpy.validateCopyResult("", "");
+        storageProcessorSpy.validateConvertResult("", "");
     }
 
     @Test (expected = CloudRuntimeException.class)