slavkap commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r782155384



##########
File path: 
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java
##########
@@ -1585,17 +1607,214 @@ public Answer createSnapshot(final CreateObjectCommand 
cmd) {
                         s_logger.debug("Failed to manage snapshot: " + result);
                         return new CreateObjectAnswer("Failed to manage 
snapshot: " + result);
                     }
+                } else {
+                    snapshotPath = 
getSnapshotPathInPrimaryStorage(primaryPool.getLocalPath(), snapshotName);
+                    String copyResult = 
copySnapshotToPrimaryStorageDir(primaryPool, diskPath, snapshotPath, volume);
+                    validateCopyResult(copyResult, snapshotPath);
                 }
             }
 
             final SnapshotObjectTO newSnapshot = new SnapshotObjectTO();
-            // NOTE: sort of hack, we'd better just put snapshtoName
-            newSnapshot.setPath(disk.getPath() + File.separator + 
snapshotName);
+
+            newSnapshot.setPath(snapshotPath);
             return new CreateObjectAnswer(newSnapshot);
-        } catch (final LibvirtException e) {
-            s_logger.debug("Failed to manage snapshot: ", e);
-            return new CreateObjectAnswer("Failed to manage snapshot: " + 
e.toString());
+        } catch (CloudRuntimeException | LibvirtException | IOException ex) {
+            String errorMsg = String.format("Failed take snapshot for volume 
[%s], in VM [%s], due to [%s].", volume, vmName, ex.getMessage());
+            s_logger.error(errorMsg, ex);
+            return new CreateObjectAnswer(errorMsg);
+        }
+    }
+
+    protected void validateCopyResult(String copyResult, String snapshotPath) 
throws CloudRuntimeException, IOException {
+        if (copyResult == null) {
+            return;
+        }
+
+        Files.deleteIfExists(Paths.get(snapshotPath));
+        throw new CloudRuntimeException(copyResult);
+    }
+
+    /**
+     * 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;
+     * @throws LibvirtException
+     */
+    protected void mergeSnapshotIntoBaseFile(Domain vm, String diskLabel, 
String baseFilePath, String snapshotName, VolumeObjectTO volume,
+            Connect conn) throws LibvirtException {
+        boolean isLibvirtSupportingFlagDeleteOnCommandVirshBlockcommit = 
LibvirtUtilitiesHelper.isLibvirtSupportingFlagDeleteOnCommandVirshBlockcommit(conn);
+        String vmName = vm.getName();
+        String mergeCommand = String.format(COMMAND_MERGE_SNAPSHOT, vmName, 
diskLabel, baseFilePath, isLibvirtSupportingFlagDeleteOnCommandVirshBlockcommit 
? "--delete" : "");
+        String mergeResult = Script.runSimpleBashScript(mergeCommand);
+
+        if (mergeResult == null) {
+            s_logger.debug(String.format("Successfully merged snapshot [%s] 
into VM [%s] %s 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);
+
+        s_logger.warn(String.format("%s VM XML: [%s].", errorMsg, 
vm.getXMLDesc(0)));
+        throw new CloudRuntimeException(errorMsg);
+    }
+
+    /**
+     * Manually deletes the unused snapshot file.<br/>
+     * This method is necessary due to Libvirt created the tag '--delete' on 
command 'virsh blockcommit' on version <b>1.2.9</b>, however it was only 
implemented on version
+     *  <b>6.0.0</b>.
+     * @param snapshotPath The unused snapshot file to manually delete.
+     */
+    protected boolean manuallyDeleteUnusedSnapshotFile(boolean 
isLibvirtSupportingFlagDeleteOnCommandVirshBlockcommit, String snapshotPath) {

Review comment:
       maybe it doesn't have to be `boolean`
   ```suggestion
       protected void manuallyDeleteUnusedSnapshotFile(boolean 
isLibvirtSupportingFlagDeleteOnCommandVirshBlockcommit, String snapshotPath) {
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to