From 678ed7414cd8cfb8c870f5db2cf47a9100757e56 Mon Sep 17 00:00:00 2001 From: "ai-fixes[bot]" <321365310+ai-fixes[bot]@users.noreply.github.com> Date: Wed, 30 Sep 2026 10:42:41 +0000 Subject: [PATCH] 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. --- ...KvmFileBasedStorageVmSnapshotStrategy.java | 81 ++++++++++--------- ...ileBasedStorageVmSnapshotStrategyTest.java | 26 ++++++ 2 files changed, 69 insertions(+), 38 deletions(-) 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;