diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java index 79e2a9722039..b9aa997cfbdc 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java +++ b/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 volumeTOs = vmSnapshotHelper.getVolumeTOList(vmSnapshotBeingDeleted.getVmId()); - List 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 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 volumeTOs = vmSnapshotHelper.getVolumeTOList(vmSnapshotBeingDeleted.getVmId()); + List 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 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 diff --git a/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java b/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java index 35b1f9ff65bf..8bfe846984ad 100644 --- a/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java +++ b/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;