CS-3474: Transition KVM disk-only VM snapshot to Error state on delete failure

deleteVMSnapshot in KvmFileBasedStorageVmSnapshotStrategy transitioned the
snapshot to Expunging but had no exception handling, so any exception thrown
by the merge/delete helpers (e.g. an agent command timeout) left the
snapshot permanently stuck in Expunging. VMSnapshotManagerImpl.hasActiveVMSnapshotTasks
treats Expunging as active work, so this permanently blocked reboot and
further snapshot operations on the VM.

Wrap the delete body in a try/catch, mirroring the existing pattern already
used by takeVMSnapshot and revertVMSnapshot in the same class: on any
RuntimeException, transition the snapshot to Error via the existing
Expunging -> OperationFailed state machine transition before rethrowing.

Test: KvmFileBasedStorageVmSnapshotStrategyTest#testDeleteVMSnapshotMarksSnapshotFailedWhenDeleteThrowsAfterExpungeRequested
fails without this fix (no OperationFailed transition is ever fired) and
passes with it.

Author: ai-fixes[bot]

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

@@ -142,52 +142,57 @@ public boolean deleteVMSnapshot(VMSnapshot vmSnapshot) {
 
         transitStateWithoutThrow(vmSnapshotBeingDeleted, VMSnapshot.Event.ExpungeRequested);
 
-        List<VolumeObjectTO> volumeTOs = vmSnapshotHelper.getVolumeTOList(vmSnapshotBeingDeleted.getVmId());
-        List<VMSnapshotVO> snapshotChildren = vmSnapshotDao.listByParentAndStateIn(vmSnapshotBeingDeleted.getId(), VMSnapshot.State.Ready, VMSnapshot.State.Hidden);
-        PrimaryDataStoreTO nvramPrimaryDataStore = getPrimaryDataStoreForNvramCleanup(vmSnapshotBeingDeleted, volumeTOs);
-
-        long realSize = getVMSnapshotRealSize(vmSnapshotBeingDeleted);
-        int numberOfChildren = snapshotChildren.size();
-
-        List<SnapshotVO> volumeSnapshotVos = new ArrayList<>();
-        if (isCurrent && numberOfChildren == 0) {
-            volumeSnapshotVos = mergeCurrentDeltaOnSnapshot(vmSnapshotBeingDeleted, userVm, hostId, volumeTOs);
-        } else if (numberOfChildren == 0) {
-            logger.debug("Deleting VM snapshot [{}] as no snapshots/volumes depend on it.", vmSnapshot.getUuid());
-            volumeSnapshotVos = deleteSnapshot(vmSnapshotBeingDeleted, hostId);
-            mergeOldSiblingWithOldParentIfOldParentIsDead(vmSnapshotDao.findByIdIncludingRemoved(vmSnapshotBeingDeleted.getParent()), userVm, hostId, volumeTOs);
-        } else if (!isCurrent && numberOfChildren == 1) {
-            VMSnapshotVO childSnapshot = snapshotChildren.get(0);
-            volumeSnapshotVos = mergeSnapshots(vmSnapshotBeingDeleted, childSnapshot, userVm, volumeTOs, hostId);
-        }
-
-        for (SnapshotVO snapshotVO : volumeSnapshotVos) {
-            snapshotVO.setState(Snapshot.State.Destroyed);
-            snapshotDao.update(snapshotVO.getId(), snapshotVO);
-        }
+        try {
+            List<VolumeObjectTO> volumeTOs = vmSnapshotHelper.getVolumeTOList(vmSnapshotBeingDeleted.getVmId());
+            List<VMSnapshotVO> snapshotChildren = vmSnapshotDao.listByParentAndStateIn(vmSnapshotBeingDeleted.getId(), VMSnapshot.State.Ready, VMSnapshot.State.Hidden);
+            PrimaryDataStoreTO nvramPrimaryDataStore = getPrimaryDataStoreForNvramCleanup(vmSnapshotBeingDeleted, volumeTOs);
+
+            long realSize = getVMSnapshotRealSize(vmSnapshotBeingDeleted);
+            int numberOfChildren = snapshotChildren.size();
+
+            List<SnapshotVO> volumeSnapshotVos = new ArrayList<>();
+            if (isCurrent && numberOfChildren == 0) {
+                volumeSnapshotVos = mergeCurrentDeltaOnSnapshot(vmSnapshotBeingDeleted, userVm, hostId, volumeTOs);
+            } else if (numberOfChildren == 0) {
+                logger.debug("Deleting VM snapshot [{}] as no snapshots/volumes depend on it.", vmSnapshot.getUuid());
+                volumeSnapshotVos = deleteSnapshot(vmSnapshotBeingDeleted, hostId);
+                mergeOldSiblingWithOldParentIfOldParentIsDead(vmSnapshotDao.findByIdIncludingRemoved(vmSnapshotBeingDeleted.getParent()), userVm, hostId, volumeTOs);
+            } else if (!isCurrent && numberOfChildren == 1) {
+                VMSnapshotVO childSnapshot = snapshotChildren.get(0);
+                volumeSnapshotVos = mergeSnapshots(vmSnapshotBeingDeleted, childSnapshot, userVm, volumeTOs, hostId);
+            }
 
-        for (VolumeObjectTO volumeTo : volumeTOs) {
-            publishUsageEvent(EventTypes.EVENT_VM_SNAPSHOT_DELETE, vmSnapshotBeingDeleted, userVm, volumeTo);
-            virtualSize += volumeTo.getSize();
-        }
+            for (SnapshotVO snapshotVO : volumeSnapshotVos) {
+                snapshotVO.setState(Snapshot.State.Destroyed);
+                snapshotDao.update(snapshotVO.getId(), snapshotVO);
+            }
 
-        publishUsageEvent(EventTypes.EVENT_VM_SNAPSHOT_OFF_PRIMARY, vmSnapshotBeingDeleted, userVm, realSize, virtualSize);
+            for (VolumeObjectTO volumeTo : volumeTOs) {
+                publishUsageEvent(EventTypes.EVENT_VM_SNAPSHOT_DELETE, vmSnapshotBeingDeleted, userVm, volumeTo);
+                virtualSize += volumeTo.getSize();
+            }
 
-        if (numberOfChildren > 1 || (isCurrent && numberOfChildren == 1)) {
-            transitStateWithoutThrow(vmSnapshotBeingDeleted, VMSnapshot.Event.Hide);
-            return true;
-        }
+            publishUsageEvent(EventTypes.EVENT_VM_SNAPSHOT_OFF_PRIMARY, vmSnapshotBeingDeleted, userVm, realSize, virtualSize);
 
-        deleteNvramSnapshotIfNeeded(vmSnapshotBeingDeleted, hostId, nvramPrimaryDataStore);
+            if (numberOfChildren > 1 || (isCurrent && numberOfChildren == 1)) {
+                transitStateWithoutThrow(vmSnapshotBeingDeleted, VMSnapshot.Event.Hide);
+                return true;
+            }
 
-        transitStateWithoutThrow(vmSnapshotBeingDeleted, VMSnapshot.Event.OperationSucceeded);
+            deleteNvramSnapshotIfNeeded(vmSnapshotBeingDeleted, hostId, nvramPrimaryDataStore);
 
-        vmSnapshotDetailsDao.removeDetails(vmSnapshotBeingDeleted.getId());
+            transitStateWithoutThrow(vmSnapshotBeingDeleted, VMSnapshot.Event.OperationSucceeded);
 
-        vmSnapshotBeingDeleted.setRemoved(DateUtil.now());
-        vmSnapshotDao.update(vmSnapshotBeingDeleted.getId(), vmSnapshotBeingDeleted);
+            vmSnapshotDetailsDao.removeDetails(vmSnapshotBeingDeleted.getId());
 
-        return true;
+            vmSnapshotBeingDeleted.setRemoved(DateUtil.now());
+            vmSnapshotDao.update(vmSnapshotBeingDeleted.getId(), vmSnapshotBeingDeleted);
+
+            return true;
+        } catch (RuntimeException ex) {
+            transitStateWithoutThrow(vmSnapshotBeingDeleted, VMSnapshot.Event.OperationFailed);
+            throw ex;
+        }
     }
 
     @Override

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

@@ -439,6 +439,32 @@ public void testDeleteVMSnapshotFailsWhenHostLacksNvramAwareCleanupCapabilityFor
         strategy.deleteVMSnapshot(vmSnapshot);
     }
 
+    @Test(expected = CloudRuntimeException.class)
+    public void testDeleteVMSnapshotMarksSnapshotFailedWhenDeleteThrowsAfterExpungeRequested() throws Exception {
+        long vmId = 10L;
+        long vmSnapshotId = 20L;
+        long hostId = 40L;
+
+        UserVmVO userVm = mock(UserVmVO.class);
+        VMSnapshotVO vmSnapshot = mock(VMSnapshotVO.class);
+
+        when(vmSnapshot.getVmId()).thenReturn(vmId);
+        when(vmSnapshot.getId()).thenReturn(vmSnapshotId);
+        when(vmSnapshot.getUuid()).thenReturn("vm-snapshot");
+        when(userVm.getState()).thenReturn(VirtualMachine.State.Running);
+        when(strategy.userVmDao.findById(vmId)).thenReturn(userVm);
+        when(vmSnapshotHelper.pickRunningHost(vmId)).thenReturn(hostId);
+        when(vmSnapshotHelper.getVolumeTOList(vmId)).thenThrow(new CloudRuntimeException("Communication failure with host, command timed out"));
+
+        try {
+            strategy.deleteVMSnapshot(vmSnapshot);
+        } finally {
+            InOrder inOrder = inOrder(vmSnapshotHelper);
+            inOrder.verify(vmSnapshotHelper).vmSnapshotStateTransitTo(vmSnapshot, VMSnapshot.Event.ExpungeRequested);
+            inOrder.verify(vmSnapshotHelper).vmSnapshotStateTransitTo(vmSnapshot, VMSnapshot.Event.OperationFailed);
+        }
+    }
+
     @Test(expected = CloudRuntimeException.class)
     public void testTakeVmSnapshotInternalFailsWhenHostLacksUefiCapabilityForUefiVm() throws Exception {
         long vmId = 10L;