Resync VM snapshot merge process after failure

engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java

@@ -86,6 +86,7 @@ public class KvmFileBasedStorageVmSnapshotStrategy extends StorageVMSnapshotStra
 
     private static final List<Storage.StoragePoolType> supportedStoragePoolTypes = List.of(Storage.StoragePoolType.Filesystem, Storage.StoragePoolType.NetworkFilesystem, Storage.StoragePoolType.SharedMountPoint);
     private static final String KVM_FILE_BASED_STORAGE_SNAPSHOT_NVRAM = "kvmFileBasedStorageSnapshotNvram";
+    private static final int DEFAULT_MERGE_TIMEOUT = 10800;
 
     @Inject
     protected SnapshotDataStoreDao snapshotDataStoreDao;
@@ -442,9 +443,11 @@ private List<SnapshotVO> mergeSnapshots(VMSnapshotVO vmSnapshotVO, VMSnapshotVO
         }
 
         MergeDiskOnlyVmSnapshotCommand mergeDiskOnlyVMSnapshotCommand = new MergeDiskOnlyVmSnapshotCommand(snapshotMergeTreeToList, userVm.getState(), userVm.getName());
+        mergeDiskOnlyVMSnapshotCommand.setWait(DEFAULT_MERGE_TIMEOUT);
         Answer answer = agentMgr.easySend(hostId, mergeDiskOnlyVMSnapshotCommand);
         if (answer == null || !answer.getResult()) {
-            throw new CloudRuntimeException(String.format("Failed to merge VM snapshot [%s] due to %s.", vmSnapshotVO.getUuid(), answer != null ? answer.getDetails() : "Communication failure"));
+            throw new CloudRuntimeException(String.format("Failed to merge VM snapshot [%s] due to %s. Consider trying again later to make sure the VM metadata is synchronized.",
+                    vmSnapshotVO.getUuid(), answer != null ? answer.getDetails() : "Communication failure"));
         }
 
         logger.debug("Updating metadata of VM snapshot [{}] and its child [{}].", vmSnapshotVO.getUuid(), childSnapshot.getUuid());
@@ -481,6 +484,7 @@ private List<SnapshotVO> mergeCurrentDeltaOnSnapshot(VMSnapshotVO vmSnapshotVo,
         }
 
         MergeDiskOnlyVmSnapshotCommand mergeDiskOnlyVMSnapshotCommand = new MergeDiskOnlyVmSnapshotCommand(snapshotMergeTreeTOList, userVmVO.getState(), userVmVO.getName());
+        mergeDiskOnlyVMSnapshotCommand.setWait(DEFAULT_MERGE_TIMEOUT);
 
         Answer answer = agentMgr.easySend(hostId, mergeDiskOnlyVMSnapshotCommand);
         if (answer == null || !answer.getResult()) {

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

@@ -6363,14 +6363,15 @@ public static String generateSecretUUIDFromString(String seed) {
      * @param snapshotName Name of the snapshot;
      * @param volume VolumeObjectTO of the corresponding volume;
      * @param conn Libvirt connection;
+     * @param timeout the timeout in milisseconds
      * @throws LibvirtException
      */
     public void mergeSnapshotIntoBaseFile(Domain vm, String diskLabel, String baseFilePath, String topFilePath, boolean active, String snapshotName, VolumeObjectTO volume,
-            Connect conn) throws LibvirtException {
+            Connect conn, long timeout) throws LibvirtException {
         if (AgentPropertiesFileHandler.getPropertyValue(AgentProperties.LIBVIRT_EVENTS_ENABLED)) {
             mergeSnapshotIntoBaseFileWithEventsAndConfigurableTimeout(vm, diskLabel, baseFilePath, topFilePath, active, snapshotName, volume, conn);
         } else {
-            mergeSnapshotIntoBaseFileWithoutEvents(vm, diskLabel, baseFilePath, topFilePath, active, snapshotName, volume, conn);
+            mergeSnapshotIntoBaseFileWithoutEvents(vm, diskLabel, baseFilePath, topFilePath, active, snapshotName, volume, conn, timeout);
         }
     }
 
@@ -6435,27 +6436,43 @@ protected void mergeSnapshotIntoBaseFileWithEventsAndConfigurableTimeout(Domain
 
     /**
      * Merges the snapshot into base file to keep volume and VM behavior after stopping - starting.
-     * @param vm Domain of the VM;
-     * @param diskLabel Disk label to manage snapshot and base file;
-     * @param baseFilePath Path of the base file;
-     * @param snapshotName Name of the snapshot;
+     *
+     * @param vm
+     *         Domain of the VM;
+     * @param diskLabel
+     *         Disk label to manage snapshot and base file;
+     * @param baseFilePath
+     *         Path of the base file;
+     * @param snapshotName
+     *         Name of the snapshot;
+     * @param timeout
      * @throws LibvirtException
      */
-    protected void mergeSnapshotIntoBaseFileWithoutEvents(Domain vm, String diskLabel, String baseFilePath, String topFilePath, boolean active, String snapshotName, VolumeObjectTO volume, Connect conn) throws LibvirtException {
+    protected void mergeSnapshotIntoBaseFileWithoutEvents(Domain vm, String diskLabel, String baseFilePath, String topFilePath, boolean active, String snapshotName, VolumeObjectTO volume, Connect conn,
+            long timeout) throws LibvirtException {
         boolean isLibvirtSupportingFlagDeleteOnCommandVirshBlockcommit = LibvirtUtilitiesHelper.isLibvirtSupportingFlagDeleteOnCommandVirshBlockcommit(conn);
         String vmName = vm.getName();
         String mergeCommand = buildMergeCommand(vmName, diskLabel, baseFilePath, topFilePath, active, isLibvirtSupportingFlagDeleteOnCommandVirshBlockcommit);
-        String mergeResult = Script.runSimpleBashScript(mergeCommand);
+        Script mergeScript = new Script("/bin/bash", new Duration(timeout));
+        mergeScript.add("-c");
+        mergeScript.add(mergeCommand);
+        String mergeResult = mergeScript.execute(new OutputInterpreter.OneLineParser());
+        int exitCode = -1;
+        try {
+             exitCode = mergeScript.getExitValue();
+        } catch (IllegalThreadStateException ignored) {
+        }
 
-        if (mergeResult == null) {
+        if (mergeResult == null && exitCode == 0) {
             logger.debug("Successfully merged snapshot [{}] into VM [{}] {} base file.", snapshotName, vmName, volume);
             manuallyDeleteUnusedSnapshotFile(isLibvirtSupportingFlagDeleteOnCommandVirshBlockcommit, getSnapshotTemporaryPath(baseFilePath, snapshotName));
             return;
         }
 
-        String errorMsg = String.format("Failed to merge snapshot [%s] into VM [%s] %s base file. Command [%s] resulted in [%s]. If the VM is stopped and then started, it"
-                + " will start to write in the base file again. All changes made between the snapshot and the VM stop will be in the snapshot. If the VM is stopped, the snapshot must be"
-                + " merged into the base file manually.", snapshotName, vmName, volume, mergeCommand, mergeResult);
+        String errorMsg = String.format("Failed to merge snapshot [%s] into VM [%s] %s base file. Command [%s] resulted in [%s] with exit code [%s]." +
+                " If the VM is stopped and then started, it" +
+                " will start to write in the base file again. All changes made between the snapshot and the VM stop will be in the snapshot. If the VM is stopped," +
+                " the snapshot must be merged into the base file manually.", snapshotName, vmName, volume, mergeCommand, mergeResult, exitCode);
 
         logger.warn("%s VM XML: [{}].", errorMsg, vm.getXMLDesc(0));
         throw new CloudRuntimeException(errorMsg);

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java

@@ -131,6 +131,7 @@ public boolean parseDomainXML(String domXML) {
                                 fmt = DiskDef.DiskFmtType.valueOf(diskFmtType.toUpperCase());
                             }
                             def.defFileBasedDisk(diskFile, diskLabel, DiskDef.DiskBus.valueOf(bus.toUpperCase()), fmt);
+                            parseBackingFiles(disk, def);
                         } else if (device.equalsIgnoreCase("cdrom")) {
                             def.defISODisk(diskFile, i+1, diskLabel, DiskDef.DiskType.FILE);
                         }
@@ -405,6 +406,21 @@ public boolean parseDomainXML(String domXML) {
         return false;
     }
 
+    private void parseBackingFiles(Element disk, DiskDef def) {
+        NodeList backingStoreNodeList = disk.getElementsByTagName("backingStore");
+        List<String> backingStoreList = new ArrayList<>();
+        while (backingStoreNodeList.getLength() > 0) {
+            Element backingStore = (Element)backingStoreNodeList.item(0);
+            String path = getAttrValue("source", "file", backingStore);
+            if (StringUtils.isEmpty(path)) {
+                break;
+            }
+            backingStoreList.add(path.substring(path.lastIndexOf(File.separator)+1));
+            backingStoreNodeList = backingStore.getElementsByTagName("backingStore");
+        }
+        def.setBackingStoreList(backingStoreList);
+    }
+
     /**
      * Parse the memballoon tag.
      * @param devices the devices tag.

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java

@@ -806,6 +806,7 @@ public String toString() {
         private BlockIOSize logicalBlockIOSize = null;
         private BlockIOSize physicalBlockIOSize = null;
         private DiskGeometry geometry = null;
+        private List<String> backingStoreList = null; // Ordered list of backing stores, the first in the list is the immediate backing store, and the last in the list is the base
 
         public DiscardType getDiscard() {
             return _discard;
@@ -1177,6 +1178,14 @@ public String getSourcePath() {
             return _sourcePath;
         }
 
+        public List<String> getBackingStoreList() {
+            return backingStoreList;
+        }
+
+        public void setBackingStoreList(List<String> backingStoreList) {
+            this.backingStoreList = backingStoreList;
+        }
+
         @Override
         public String toString() {
             StringBuilder diskBuilder = new StringBuilder();

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMergeDiskOnlyVMSnapshotCommandWrapper.java

@@ -38,8 +38,10 @@
 import org.apache.cloudstack.utils.qemu.QemuImg;
 import org.apache.cloudstack.utils.qemu.QemuImgException;
 import org.apache.cloudstack.utils.qemu.QemuImgFile;
+import org.apache.commons.lang3.StringUtils;
 import org.libvirt.Connect;
 import org.libvirt.Domain;
+import org.libvirt.DomainBlockJobInfo;
 import org.libvirt.LibvirtException;
 
 import java.io.IOException;
@@ -51,6 +53,10 @@
 @ResourceWrapper(handles = MergeDiskOnlyVmSnapshotCommand.class)
 public class LibvirtMergeDiskOnlyVMSnapshotCommandWrapper extends CommandWrapper<MergeDiskOnlyVmSnapshotCommand, Answer, LibvirtComputingResource> {
 
+    // Extracted from https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainBlockJobType
+    private static final int VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT = 3;
+    private static final int VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT = 4;
+
     @Override
     public Answer execute(MergeDiskOnlyVmSnapshotCommand command, LibvirtComputingResource serverResource) {
         VirtualMachine.State vmState = command.getVmState();
@@ -66,24 +72,32 @@ public Answer execute(MergeDiskOnlyVmSnapshotCommand command, LibvirtComputingRe
     }
 
     protected Answer mergeDiskOnlySnapshotsForStoppedVM(MergeDiskOnlyVmSnapshotCommand cmd, LibvirtComputingResource resource) throws QemuImgException, LibvirtException {
-        QemuImg qemuImg = new QemuImg(resource.getCmdsTimeout());
         KVMStoragePoolManager storageManager = resource.getStoragePoolMgr();
-
         List<SnapshotMergeTreeTO> snapshotMergeTreeTOList = cmd.getSnapshotMergeTreeToList();
 
         logger.debug("Merging disk-only snapshots for stopped VM [{}] using the following Snapshot Merge Trees [{}].", cmd.getVmName(), snapshotMergeTreeTOList);
-
+        long timeout = cmd.getWait() * 1000L;
         for (SnapshotMergeTreeTO snapshotMergeTreeTO : snapshotMergeTreeTOList) {
-            DataTO parentTo = snapshotMergeTreeTO.getParent();
+            if (timeout <= 0) {
+                logger.warn("Disk-only VM snapshot for VM [{}] timed out. Consider increasing the timeout and try again later.", cmd.getVmName());
+                return new Answer(cmd, false, null);
+            }
+            long startTime = System.currentTimeMillis();
+            QemuImg qemuImg = new QemuImg(timeout);
+            SnapshotObjectTO parentTo = (SnapshotObjectTO) snapshotMergeTreeTO.getParent();
             PrimaryDataStoreTO primaryDataStoreTO = (PrimaryDataStoreTO) parentTo.getDataStore();
             KVMStoragePool storagePool = storageManager.getStoragePool(primaryDataStoreTO.getPoolType(), primaryDataStoreTO.getUuid());
             String childLocalPath = storagePool.getLocalPathFor(snapshotMergeTreeTO.getChild().getPath());
-
+            Path childPath = Path.of(childLocalPath);
             QemuImgFile parent = new QemuImgFile(storagePool.getLocalPathFor(parentTo.getPath()), QemuImg.PhysicalDiskFormat.QCOW2);
             QemuImgFile child = new QemuImgFile(childLocalPath, QemuImg.PhysicalDiskFormat.QCOW2);
 
-            logger.debug("Committing child delta [{}] into parent snapshot [{}].", parentTo, snapshotMergeTreeTO.getChild());
-            qemuImg.commit(child, parent, true);
+            if (!childPath.toFile().exists()) {
+                logger.info("Volume [{}] was already merged in a previous attempt. Skipping merge for it.", parentTo.getVolume().getUuid());
+            } else {
+                logger.debug("Committing child delta [{}] into parent snapshot [{}].", parentTo, snapshotMergeTreeTO.getChild());
+                qemuImg.commit(child, parent, true);
+            }
 
             List<QemuImgFile> grandChildren = snapshotMergeTreeTO.getGrandChildren().stream()
                     .map(snapshotTo -> new QemuImgFile(storagePool.getLocalPathFor(snapshotTo.getPath()), QemuImg.PhysicalDiskFormat.QCOW2))
@@ -96,10 +110,11 @@ protected Answer mergeDiskOnlySnapshotsForStoppedVM(MergeDiskOnlyVmSnapshotComma
 
             logger.debug("Deleting child at [{}] as it is useless.", childLocalPath);
             try {
-                Files.deleteIfExists(Path.of(childLocalPath));
+                Files.deleteIfExists(childPath);
             } catch (IOException e) {
                 return new Answer(cmd, e);
             }
+            timeout = timeout - (System.currentTimeMillis() - startTime);
         }
         return new Answer(cmd, true, null);
     }
@@ -113,38 +128,88 @@ protected Answer mergeDiskOnlySnapshotsForRunningVM(MergeDiskOnlyVmSnapshotComma
         Domain domain = resource.getDomain(conn, vmName);
         List<LibvirtVMDef.DiskDef> disks = resource.getDisks(conn, vmName);
         KVMStoragePoolManager storageManager = resource.getStoragePoolMgr();
-        QemuImg qemuImg = new QemuImg(resource.getCmdsTimeout());
+        long timeout = cmd.getWait() * 1000L;
 
         logger.debug("Merging disk-only snapshots for running VM [{}] using the following Snapshot Merge Trees [{}].", vmName, snapshotMergeTreeTOList);
 
         for (SnapshotMergeTreeTO mergeTreeTO : snapshotMergeTreeTOList) {
+            if (timeout <= 0) {
+                logger.warn("Disk-only VM snapshot for VM [{}] timed out. Consider increasing the timeout and try again later.", vmName);
+                return new Answer(cmd, false, null);
+            }
+            long startTime = System.currentTimeMillis();
             DataTO childTO = mergeTreeTO.getChild();
             SnapshotObjectTO parentSnapshotTO = (SnapshotObjectTO) mergeTreeTO.getParent();
             VolumeObjectTO volumeObjectTO = parentSnapshotTO.getVolume();
             KVMStoragePool storagePool = libvirtUtilitiesHelper.getPrimaryPoolFromDataTo(volumeObjectTO, storageManager);
 
             boolean active = DataObjectType.VOLUME.equals(childTO.getObjectType());
-            String label = resource.getDiskWithPathOfVolumeObjectTO(disks, volumeObjectTO).getDiskLabel();
-            String parentSnapshotLocalPath = storagePool.getLocalPathFor(parentSnapshotTO.getPath());
-            String childDeltaPath = storagePool.getLocalPathFor(childTO.getPath());
 
-            logger.debug("Found label [{}] for [{}]. Will merge delta at [{}] into delta at [{}].", label, volumeObjectTO, parentSnapshotLocalPath, childDeltaPath);
+            LibvirtVMDef.DiskDef diskDef = getDiskDef(disks, volumeObjectTO, childTO, parentSnapshotTO);
+            if (diskDef == null) {
+                logger.warn("Volume [{}] does not match any record we have. This must be manually normalized.", volumeObjectTO.getUuid());
+                return new Answer(cmd, false, null);
+            }
+            String label = diskDef.getDiskLabel();
+
+            DomainBlockJobInfo jobInfo = domain.getBlockJobInfo(label, 0);
+            if (jobInfo == null || jobInfo.type == 0 && jobInfo.end == 0 && jobInfo.cur == 0) {
+                logger.debug("No block job found for disk [{}] of VM [{}]. Continuing the merge process.", label, vmName);
+            } else if (jobInfo.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT || jobInfo.type == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT) {
+                logger.debug("Found a commit block job for disk [{}] of VM [{}]. We will abort it to continue with the VM snapshot merge process.", label, vmName);
+                domain.blockJobAbort(label, 0);
+            } else {
+                String message = String.format("There is a block job executing that is not a commit for disk [%s] of VM [%s]. We will abort the snapshot merge. Consider trying " +
+                        "again later after this is done.", label, vmName);
+                logger.debug(message);
+                return new Answer(cmd, false, message);
+            }
+
+            boolean volumeAlreadyMerged = isVolumeAlreadyMerged(diskDef, childTO);
+            String parentSnapshotLocalPath = storagePool.getLocalPathFor(parentSnapshotTO.getPath());
 
-            resource.mergeSnapshotIntoBaseFile(domain, label, parentSnapshotLocalPath, childDeltaPath, active, childTO.getPath(),
-                    volumeObjectTO, conn);
+            if (volumeAlreadyMerged) {
+                logger.info("Volume [{}] was already merged in a previous attempt. Skipping merge for it.", volumeObjectTO.getUuid());
+            } else {
+                String childDeltaPath = storagePool.getLocalPathFor(childTO.getPath());
+                logger.debug("Found label [{}] for [{}]. Will merge delta at [{}] into delta at [{}].", label, volumeObjectTO, parentSnapshotLocalPath, childDeltaPath);
+                resource.mergeSnapshotIntoBaseFile(domain, label, parentSnapshotLocalPath, childDeltaPath, active, childTO.getPath(), volumeObjectTO, conn, timeout);
+            }
 
             QemuImgFile parent = new QemuImgFile(parentSnapshotLocalPath, QemuImg.PhysicalDiskFormat.QCOW2);
-
             List<QemuImgFile> grandChildren = mergeTreeTO.getGrandChildren().stream()
                     .map(snapshotTo -> new QemuImgFile(storagePool.getLocalPathFor(snapshotTo.getPath()), QemuImg.PhysicalDiskFormat.QCOW2))
                     .collect(Collectors.toList());
 
             logger.debug("Rebasing grandChildren [{}] into parent at [{}].", grandChildren, parentSnapshotLocalPath);
             for (QemuImgFile grandChild : grandChildren) {
+                QemuImg qemuImg = new QemuImg(timeout);
                 qemuImg.rebase(grandChild, parent, parent.getFormat().toString(), false);
             }
+            timeout = timeout - (System.currentTimeMillis() - startTime);
         }
 
         return new Answer(cmd, true, null);
     }
+
+    protected boolean isVolumeAlreadyMerged(LibvirtVMDef.DiskDef diskDef, DataTO childTO) {
+        if (diskDef.getDiskPath().contains(childTO.getPath())) {
+            return false;
+        }
+
+        for (String filePath : diskDef.getBackingStoreList()) {
+            if (filePath.contains(childTO.getPath())) {
+                return false;
+            }
+        }
+        return true;
+    }
+
+    protected LibvirtVMDef.DiskDef getDiskDef(List<LibvirtVMDef.DiskDef> disks, VolumeObjectTO volumeObjectTO, DataTO childTO, SnapshotObjectTO parentSnapshotTO) {
+        return disks.stream().filter(disk -> StringUtils.contains(disk.getDiskPath(), volumeObjectTO.getPath()) ||
+                StringUtils.contains(disk.getDiskPath(), childTO.getPath()) ||
+                StringUtils.contains(disk.getDiskPath(), parentSnapshotTO.getPath()))
+                .findFirst()
+                .orElse(null);
+    }
 }