Fix rollback disk snapshots on instance snapshot failure

Author: sureshanaparti

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

@@ -19,6 +19,7 @@
 package org.apache.cloudstack.storage.vmsnapshot;
 
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
@@ -111,7 +112,7 @@ public VMSnapshot takeVMSnapshot(VMSnapshot vmSnapshot) {
         FreezeThawVMAnswer freezeAnswer = null;
         FreezeThawVMCommand thawCmd = null;
         FreezeThawVMAnswer thawAnswer = null;
-        List<SnapshotInfo> forRollback = new ArrayList<>();
+        Map<Long, SnapshotInfo> volumeToSnapshotInfoMapForRollback = new HashMap<>();
         long startFreeze = 0;
         try {
             vmSnapshotHelper.vmSnapshotStateTransitTo(vmSnapshotVO, VMSnapshot.Event.CreateRequested);
@@ -165,7 +166,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, volumeToSnapshotInfoMapForRollback, vol);
 
                     if (snapInfo == null) {
                         thawAnswer = (FreezeThawVMAnswer) agentMgr.send(hostId, thawCmd);
@@ -222,7 +223,7 @@ public VMSnapshot takeVMSnapshot(VMSnapshot vmSnapshot) {
                 }
             }
             if (!result) {
-                for (SnapshotInfo snapshotInfo : forRollback) {
+                for (SnapshotInfo snapshotInfo : volumeToSnapshotInfoMapForRollback.values()) {
                     rollbackDiskSnapshot(snapshotInfo);
                 }
                 try {
@@ -388,10 +389,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 +441,7 @@ protected void revertDiskSnapshot(VMSnapshot vmSnapshot) {
         }
     }
 
-    protected SnapshotInfo createDiskSnapshot(VMSnapshot vmSnapshot, List<SnapshotInfo> forRollback, VolumeInfo vol) {
+    protected SnapshotInfo createDiskSnapshot(VMSnapshot vmSnapshot, Map<Long, SnapshotInfo> volumeToSnapshotInfoMapForRollback, 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,6 +455,7 @@ 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());
+        volumeToSnapshotInfoMapForRollback.put(vol.getId(), snapshotInfo);
         SnapshotStrategy snapshotStrategy = storageStrategyFactory.getSnapshotStrategy(snapshotInfo, SnapshotOperation.TAKE);
         if (snapshotStrategy == null) {
             throw new CloudRuntimeException("Could not find strategy for snapshot uuid:" + snapshotInfo.getUuid());
@@ -456,7 +464,7 @@ protected SnapshotInfo createDiskSnapshot(VMSnapshot vmSnapshot, List<SnapshotIn
         if (snapshotInfo == null) {
             throw new CloudRuntimeException("Failed to create snapshot");
         } else {
-          forRollback.add(snapshotInfo);
+            volumeToSnapshotInfoMapForRollback.put(vol.getId(), 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

@@ -24,7 +24,9 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Date;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.UUID;
 
 import javax.inject.Inject;
@@ -153,7 +155,7 @@ public void setUp() throws Exception {
     @Test
     public void testCreateDiskSnapshotBasedOnStrategy() throws Exception {
         VMSnapshotVO vmSnapshot = Mockito.mock(VMSnapshotVO.class);
-        List<SnapshotInfo> forRollback = new ArrayList<>();
+        Map<Long, SnapshotInfo> volumeToSnapshotInfoMapForRollback = new HashMap<>();
         VolumeInfo vol = Mockito.mock(VolumeInfo.class);
         SnapshotInfo snapshotInfo = Mockito.mock(SnapshotInfo.class);
         SnapshotStrategy strategy = Mockito.mock(SnapshotStrategy.class);
@@ -177,7 +179,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, volumeToSnapshotInfoMapForRollback, vol);
         assertNotNull(info);
     }