Fix rollback disk snapshots on instance snapshot failure (#12949)

Author: sureshanaparti

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

@@ -111,7 +111,7 @@ public VMSnapshot takeVMSnapshot(VMSnapshot vmSnapshot) {
         FreezeThawVMAnswer freezeAnswer = null;
         FreezeThawVMCommand thawCmd = null;
         FreezeThawVMAnswer thawAnswer = null;
-        List<SnapshotInfo> forRollback = new ArrayList<>();
+        List<SnapshotInfo> snapshotsForRollback = new ArrayList<>();
         long startFreeze = 0;
         try {
             vmSnapshotHelper.vmSnapshotStateTransitTo(vmSnapshotVO, VMSnapshot.Event.CreateRequested);
@@ -165,7 +165,7 @@ public VMSnapshot takeVMSnapshot(VMSnapshot vmSnapshot) {
                 logger.info("The virtual machine is frozen");
                 for (VolumeInfo vol : vinfos) {
                     long startSnapshtot = System.nanoTime();
-                    SnapshotInfo snapInfo = createDiskSnapshot(vmSnapshot, forRollback, vol);
+                    SnapshotInfo snapInfo = createDiskSnapshot(vmSnapshot, snapshotsForRollback, vol);
 
                     if (snapInfo == null) {
                         thawAnswer = (FreezeThawVMAnswer) agentMgr.send(hostId, thawCmd);
@@ -222,7 +222,7 @@ public VMSnapshot takeVMSnapshot(VMSnapshot vmSnapshot) {
                 }
             }
             if (!result) {
-                for (SnapshotInfo snapshotInfo : forRollback) {
+                for (SnapshotInfo snapshotInfo : snapshotsForRollback) {
                     rollbackDiskSnapshot(snapshotInfo);
                 }
                 try {
@@ -388,10 +388,16 @@ private long elapsedTime(long startTime) {
 
     //Rollback if one of disks snapshot fails
     protected void rollbackDiskSnapshot(SnapshotInfo snapshotInfo) {
+        if (snapshotInfo == null) {
+            return;
+        }
         Long snapshotID = snapshotInfo.getId();
         SnapshotVO snapshot = snapshotDao.findById(snapshotID);
+        if (snapshot == null) {
+            return;
+        }
         deleteSnapshotByStrategy(snapshot);
-        logger.debug("Rollback is executed: deleting snapshot with id:" + snapshotID);
+        logger.debug("Rollback is executed: deleting snapshot with id: {}", snapshotID);
     }
 
     protected void deleteSnapshotByStrategy(SnapshotVO snapshot) {
@@ -434,7 +440,7 @@ protected void revertDiskSnapshot(VMSnapshot vmSnapshot) {
         }
     }
 
-    protected SnapshotInfo createDiskSnapshot(VMSnapshot vmSnapshot, List<SnapshotInfo> forRollback, VolumeInfo vol) {
+    protected SnapshotInfo createDiskSnapshot(VMSnapshot vmSnapshot, List<SnapshotInfo> snapshotsForRollback, VolumeInfo vol) {
         String snapshotName = vmSnapshot.getId() + "_" + vol.getUuid();
         SnapshotVO snapshot = new SnapshotVO(vol.getDataCenterId(), vol.getAccountId(), vol.getDomainId(), vol.getId(), vol.getDiskOfferingId(),
                               snapshotName, (short) Snapshot.Type.GROUP.ordinal(),  Snapshot.Type.GROUP.name(),  vol.getSize(), vol.getMinIops(),  vol.getMaxIops(), Hypervisor.HypervisorType.KVM, null);
@@ -448,15 +454,14 @@ protected SnapshotInfo createDiskSnapshot(VMSnapshot vmSnapshot, List<SnapshotIn
         vol.addPayload(setPayload(vol, snapshot, quiescevm));
         SnapshotInfo snapshotInfo = snapshotDataFactory.getSnapshot(snapshot.getId(), vol.getDataStore());
         snapshotInfo.addPayload(vol.getpayload());
+        snapshotsForRollback.add(snapshotInfo);
         SnapshotStrategy snapshotStrategy = storageStrategyFactory.getSnapshotStrategy(snapshotInfo, SnapshotOperation.TAKE);
         if (snapshotStrategy == null) {
             throw new CloudRuntimeException("Could not find strategy for snapshot uuid:" + snapshotInfo.getUuid());
         }
         snapshotInfo = snapshotStrategy.takeSnapshot(snapshotInfo);
         if (snapshotInfo == null) {
             throw new CloudRuntimeException("Failed to create snapshot");
-        } else {
-          forRollback.add(snapshotInfo);
         }
         vmSnapshotDetailsDao.persist(new VMSnapshotDetailsVO(vmSnapshot.getId(), STORAGE_SNAPSHOT, String.valueOf(snapshot.getId()), true));
         snapshotInfo.markBackedUp();

engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/VMSnapshotStrategyKVMTest.java

@@ -153,7 +153,7 @@ public void setUp() throws Exception {
     @Test
     public void testCreateDiskSnapshotBasedOnStrategy() throws Exception {
         VMSnapshotVO vmSnapshot = Mockito.mock(VMSnapshotVO.class);
-        List<SnapshotInfo> forRollback = new ArrayList<>();
+        List<SnapshotInfo> snapshotsForRollback = new ArrayList<>();
         VolumeInfo vol = Mockito.mock(VolumeInfo.class);
         SnapshotInfo snapshotInfo = Mockito.mock(SnapshotInfo.class);
         SnapshotStrategy strategy = Mockito.mock(SnapshotStrategy.class);
@@ -177,7 +177,7 @@ public void testCreateDiskSnapshotBasedOnStrategy() throws Exception {
         VMSnapshotDetailsVO vmDetails = new VMSnapshotDetailsVO(vmSnapshot.getId(), volUuid, String.valueOf(snapshot.getId()), false);
         when(vmSnapshotDetailsDao.persist(any())).thenReturn(vmDetails);
 
-        info =  vmStrategy.createDiskSnapshot(vmSnapshot, forRollback, vol);
+        info =  vmStrategy.createDiskSnapshot(vmSnapshot, snapshotsForRollback, vol);
         assertNotNull(info);
     }