From a768e890b255c82bb927f75df0f1213d40227d74 Mon Sep 17 00:00:00 2001 From: Suresh Kumar Anaparti Date: Wed, 28 Oct 2026 17:43:57 +0530 Subject: [PATCH 1/2] Fix instance snapshot with KvmFileBasedStorageVmSnapshotStrategy --- ...KvmFileBasedStorageVmSnapshotStrategy.java | 43 +++++- ...ileBasedStorageVmSnapshotStrategyTest.java | 130 ++++++++++++++++++ 2 files changed, 168 insertions(+), 5 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..2047cea2261b 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 @@ -86,6 +86,7 @@ public class KvmFileBasedStorageVmSnapshotStrategy extends StorageVMSnapshotStra private static final List supportedStoragePoolTypes = List.of(Storage.StoragePoolType.Filesystem, Storage.StoragePoolType.NetworkFilesystem, Storage.StoragePoolType.SharedMountPoint); private static final String KVM_FILE_BASED_STORAGE_SNAPSHOT_NVRAM = "kvmFileBasedStorageSnapshotNvram"; + protected static final String KVM_FILE_BASED_STORAGE_SNAPSHOT_STRATEGY = "kvmFileBasedStorageSnapshotStrategy"; @Inject protected SnapshotDataStoreDao snapshotDataStoreDao; @@ -133,8 +134,12 @@ public VMSnapshot takeVMSnapshot(VMSnapshot vmSnapshot) { @Override public boolean deleteVMSnapshot(VMSnapshot vmSnapshot) { logger.info("Starting VM snapshot delete process for snapshot [{}].", vmSnapshot.getUuid()); - UserVmVO userVm = userVmDao.findById(vmSnapshot.getVmId()); VMSnapshotVO vmSnapshotBeingDeleted = (VMSnapshotVO) vmSnapshot; + if (!hasVolumeSnapshotDetails(vmSnapshotBeingDeleted)) { + return deleteVMSnapshotWithoutVolumeSnapshots(vmSnapshotBeingDeleted); + } + + UserVmVO userVm = userVmDao.findById(vmSnapshot.getVmId()); Long hostId = pickHostForNvramSidecarCleanup(vmSnapshotBeingDeleted, userVm, "delete"); validateHostSupportsNvramSidecarCleanup(vmSnapshotBeingDeleted, hostId, "delete"); long virtualSize = 0; @@ -322,16 +327,43 @@ private void mergeOldSiblingWithOldParentIfOldParentIsDead(VMSnapshotVO oldParen transitStateWithoutThrow(oldParent, VMSnapshot.Event.OperationSucceeded); } + protected boolean deleteVMSnapshotWithoutVolumeSnapshots(VMSnapshotVO vmSnapshot) { + logger.info("VM snapshot [{}] has no volume snapshots on primary storage; removing only its metadata.", vmSnapshot.getUuid()); + + transitStateWithoutThrow(vmSnapshot, VMSnapshot.Event.ExpungeRequested); + transitStateWithoutThrow(vmSnapshot, VMSnapshot.Event.OperationSucceeded); + + vmSnapshotDetailsDao.removeDetails(vmSnapshot.getId()); + + vmSnapshot.setRemoved(DateUtil.now()); + vmSnapshotDao.update(vmSnapshot.getId(), vmSnapshot); + + return true; + } + + protected boolean hasVolumeSnapshotDetails(VMSnapshot vmSnapshot) { + return CollectionUtils.isNotEmpty(vmSnapshotDetailsDao.findDetails(vmSnapshot.getId(), KVM_FILE_BASED_STORAGE_SNAPSHOT)); + } + + protected boolean isCreatedByThisStrategy(VMSnapshot vmSnapshot) { + return vmSnapshotDetailsDao.findDetail(vmSnapshot.getId(), KVM_FILE_BASED_STORAGE_SNAPSHOT_STRATEGY) != null; + } + @Override public StrategyPriority canHandle(VMSnapshot vmSnapshot) { - if (!VMSnapshot.State.Allocated.equals(vmSnapshot.getState())) { - List vmSnapshotDetails = vmSnapshotDetailsDao.findDetails(vmSnapshot.getId(), KVM_FILE_BASED_STORAGE_SNAPSHOT); - if (CollectionUtils.isEmpty(vmSnapshotDetails)) { + VMSnapshot.State state = vmSnapshot.getState(); + if (!VMSnapshot.State.Allocated.equals(state)) { + if (hasVolumeSnapshotDetails(vmSnapshot) || isCreatedByThisStrategy(vmSnapshot)) { + return StrategyPriority.HIGHEST; + } + if (!VMSnapshot.State.Error.equals(state)) { logger.debug("KVM file based storage VM snapshot strategy cannot handle [{}] as it is not a KVM file based storage VM snapshot.", vmSnapshot.getUuid()); return StrategyPriority.CANT_HANDLE; } - return StrategyPriority.HIGHEST; + // Error rows created before the strategy marker existed carry no detail at all; fall back to the creation eligibility check. + logger.debug("VM snapshot [{}] is in Error state without any KVM file based storage details; checking whether this strategy would have created it.", + vmSnapshot.getUuid()); } long vmId = vmSnapshot.getVmId(); @@ -523,6 +555,7 @@ protected VMSnapshot takeVmSnapshotInternal(VMSnapshot vmSnapshot, Map Date: Wed, 28 Oct 2026 18:37:44 +0530 Subject: [PATCH 2/2] review changes --- ...KvmFileBasedStorageVmSnapshotStrategy.java | 18 ++-- ...ileBasedStorageVmSnapshotStrategyTest.java | 100 +++++++++++++----- 2 files changed, 85 insertions(+), 33 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 2047cea2261b..0bc70849bc9a 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 @@ -85,7 +85,7 @@ public class KvmFileBasedStorageVmSnapshotStrategy extends StorageVMSnapshotStrategy { private static final List supportedStoragePoolTypes = List.of(Storage.StoragePoolType.Filesystem, Storage.StoragePoolType.NetworkFilesystem, Storage.StoragePoolType.SharedMountPoint); - private static final String KVM_FILE_BASED_STORAGE_SNAPSHOT_NVRAM = "kvmFileBasedStorageSnapshotNvram"; + protected static final String KVM_FILE_BASED_STORAGE_SNAPSHOT_NVRAM = "kvmFileBasedStorageSnapshotNvram"; protected static final String KVM_FILE_BASED_STORAGE_SNAPSHOT_STRATEGY = "kvmFileBasedStorageSnapshotStrategy"; @Inject @@ -135,8 +135,8 @@ public VMSnapshot takeVMSnapshot(VMSnapshot vmSnapshot) { public boolean deleteVMSnapshot(VMSnapshot vmSnapshot) { logger.info("Starting VM snapshot delete process for snapshot [{}].", vmSnapshot.getUuid()); VMSnapshotVO vmSnapshotBeingDeleted = (VMSnapshotVO) vmSnapshot; - if (!hasVolumeSnapshotDetails(vmSnapshotBeingDeleted)) { - return deleteVMSnapshotWithoutVolumeSnapshots(vmSnapshotBeingDeleted); + if (!hasRelevantVmSnapshotDetails(vmSnapshotBeingDeleted)) { + return deleteVMSnapshotMetadataOnly(vmSnapshotBeingDeleted); } UserVmVO userVm = userVmDao.findById(vmSnapshot.getVmId()); @@ -327,8 +327,8 @@ private void mergeOldSiblingWithOldParentIfOldParentIsDead(VMSnapshotVO oldParen transitStateWithoutThrow(oldParent, VMSnapshot.Event.OperationSucceeded); } - protected boolean deleteVMSnapshotWithoutVolumeSnapshots(VMSnapshotVO vmSnapshot) { - logger.info("VM snapshot [{}] has no volume snapshots on primary storage; removing only its metadata.", vmSnapshot.getUuid()); + protected boolean deleteVMSnapshotMetadataOnly(VMSnapshotVO vmSnapshot) { + logger.info("VM snapshot [{}] has no volume snapshots or NVRAM sidecar on primary storage; removing only its metadata.", vmSnapshot.getUuid()); transitStateWithoutThrow(vmSnapshot, VMSnapshot.Event.ExpungeRequested); transitStateWithoutThrow(vmSnapshot, VMSnapshot.Event.OperationSucceeded); @@ -341,10 +341,14 @@ protected boolean deleteVMSnapshotWithoutVolumeSnapshots(VMSnapshotVO vmSnapshot return true; } - protected boolean hasVolumeSnapshotDetails(VMSnapshot vmSnapshot) { + protected boolean hasKVMFileBasedStorageSnapshotDetailInVmSnapshotDetails(VMSnapshot vmSnapshot) { return CollectionUtils.isNotEmpty(vmSnapshotDetailsDao.findDetails(vmSnapshot.getId(), KVM_FILE_BASED_STORAGE_SNAPSHOT)); } + protected boolean hasRelevantVmSnapshotDetails(VMSnapshotVO vmSnapshot) { + return hasKVMFileBasedStorageSnapshotDetailInVmSnapshotDetails(vmSnapshot) || StringUtils.isNotBlank(getNvramSnapshotPath(vmSnapshot)); + } + protected boolean isCreatedByThisStrategy(VMSnapshot vmSnapshot) { return vmSnapshotDetailsDao.findDetail(vmSnapshot.getId(), KVM_FILE_BASED_STORAGE_SNAPSHOT_STRATEGY) != null; } @@ -353,7 +357,7 @@ protected boolean isCreatedByThisStrategy(VMSnapshot vmSnapshot) { public StrategyPriority canHandle(VMSnapshot vmSnapshot) { VMSnapshot.State state = vmSnapshot.getState(); if (!VMSnapshot.State.Allocated.equals(state)) { - if (hasVolumeSnapshotDetails(vmSnapshot) || isCreatedByThisStrategy(vmSnapshot)) { + if (hasKVMFileBasedStorageSnapshotDetailInVmSnapshotDetails(vmSnapshot) || isCreatedByThisStrategy(vmSnapshot)) { return StrategyPriority.HIGHEST; } if (!VMSnapshot.State.Error.equals(state)) { 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 8261f83def3e..3cc6a6421542 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 @@ -18,6 +18,9 @@ */ package org.apache.cloudstack.storage.vmsnapshot; +import static org.apache.cloudstack.storage.vmsnapshot.KvmFileBasedStorageVmSnapshotStrategy.KVM_FILE_BASED_STORAGE_SNAPSHOT; +import static org.apache.cloudstack.storage.vmsnapshot.KvmFileBasedStorageVmSnapshotStrategy.KVM_FILE_BASED_STORAGE_SNAPSHOT_NVRAM; +import static org.apache.cloudstack.storage.vmsnapshot.KvmFileBasedStorageVmSnapshotStrategy.KVM_FILE_BASED_STORAGE_SNAPSHOT_STRATEGY; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; @@ -148,7 +151,7 @@ public void testProcessCreateVmSnapshotAnswerPersistsNvramPath() throws Exceptio method.setAccessible(true); method.invoke(strategy, vmSnapshot, Collections.emptyMap(), answer, userVm, vmSnapshotVO, 0L, null); - verify(vmSnapshotDetailsDao).addDetail(42L, "kvmFileBasedStorageSnapshotNvram", "nvram/42.fd", false); + verify(vmSnapshotDetailsDao).addDetail(42L, KVM_FILE_BASED_STORAGE_SNAPSHOT_NVRAM, "nvram/42.fd", false); } @Test @@ -164,8 +167,8 @@ public void testRevertVMSnapshotPassesNvramPathToAgentCommand() { SnapshotDataStoreVO snapshotDataStoreVO = mock(SnapshotDataStoreVO.class); SnapshotInfo snapshotInfo = mock(SnapshotInfo.class); SnapshotObjectTO snapshotObjectTO = mock(SnapshotObjectTO.class); - VMSnapshotDetailsVO volumeSnapshotDetail = new VMSnapshotDetailsVO(snapshotId, "kvmFileBasedStorageSnapshot", String.valueOf(snapshotId), true); - VMSnapshotDetailsVO nvramDetail = new VMSnapshotDetailsVO(snapshotId, "kvmFileBasedStorageSnapshotNvram", "nvram/42.fd", false); + VMSnapshotDetailsVO volumeSnapshotDetail = new VMSnapshotDetailsVO(snapshotId, KVM_FILE_BASED_STORAGE_SNAPSHOT, String.valueOf(snapshotId), true); + VMSnapshotDetailsVO nvramDetail = new VMSnapshotDetailsVO(snapshotId, KVM_FILE_BASED_STORAGE_SNAPSHOT_NVRAM, "nvram/42.fd", false); when(vmSnapshot.getVmId()).thenReturn(vmId); when(vmSnapshot.getId()).thenReturn(snapshotId); @@ -183,8 +186,8 @@ public void testRevertVMSnapshotPassesNvramPathToAgentCommand() { .thenReturn(new DetailVO(hostId, Host.HOST_UEFI_ENABLE, Boolean.TRUE.toString())); when(hostDetailsDao.findDetail(hostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM)) .thenReturn(new DetailVO(hostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM, Boolean.TRUE.toString())); - when(vmSnapshotDetailsDao.findDetails(snapshotId, "kvmFileBasedStorageSnapshot")).thenReturn(List.of(volumeSnapshotDetail)); - when(vmSnapshotDetailsDao.findDetail(snapshotId, "kvmFileBasedStorageSnapshotNvram")).thenReturn(nvramDetail); + when(vmSnapshotDetailsDao.findDetails(snapshotId, KVM_FILE_BASED_STORAGE_SNAPSHOT)).thenReturn(List.of(volumeSnapshotDetail)); + when(vmSnapshotDetailsDao.findDetail(snapshotId, KVM_FILE_BASED_STORAGE_SNAPSHOT_NVRAM)).thenReturn(nvramDetail); when(snapshotDataStoreDao.findOneBySnapshotAndDatastoreRole(snapshotId, DataStoreRole.Primary)).thenReturn(snapshotDataStoreVO); when(snapshotDataStoreVO.getSnapshotId()).thenReturn(snapshotId); when(snapshotDataStoreVO.getDataStoreId()).thenReturn(dataStoreId); @@ -279,12 +282,12 @@ public void testDeleteNvramSnapshotIfNeededPassesPrimaryDataStoreToAgentCommand( long hostId = 40L; VMSnapshotVO vmSnapshot = mock(VMSnapshotVO.class); - VMSnapshotDetailsVO nvramDetail = new VMSnapshotDetailsVO(20L, "kvmFileBasedStorageSnapshotNvram", "nvram/42.fd", false); + VMSnapshotDetailsVO nvramDetail = new VMSnapshotDetailsVO(20L, KVM_FILE_BASED_STORAGE_SNAPSHOT_NVRAM, "nvram/42.fd", false); PrimaryDataStoreTO primaryDataStore = mock(PrimaryDataStoreTO.class); when(vmSnapshot.getId()).thenReturn(20L); when(vmSnapshot.getUuid()).thenReturn("vm-snapshot"); - when(vmSnapshotDetailsDao.findDetail(20L, "kvmFileBasedStorageSnapshotNvram")).thenReturn(nvramDetail); + when(vmSnapshotDetailsDao.findDetail(20L, KVM_FILE_BASED_STORAGE_SNAPSHOT_NVRAM)).thenReturn(nvramDetail); when(hostDetailsDao.findDetail(hostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM)) .thenReturn(new DetailVO(hostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM, Boolean.TRUE.toString())); when(agentMgr.easySend(eq(hostId), any())).thenReturn(new Answer(null, true, null)); @@ -308,11 +311,11 @@ public void testValidateHostSupportsNvramSidecarCleanupFailsWhenHostLacksNvramAw long hostId = 40L; VMSnapshotVO vmSnapshot = mock(VMSnapshotVO.class); - VMSnapshotDetailsVO nvramDetail = new VMSnapshotDetailsVO(20L, "kvmFileBasedStorageSnapshotNvram", "nvram/42.fd", false); + VMSnapshotDetailsVO nvramDetail = new VMSnapshotDetailsVO(20L, KVM_FILE_BASED_STORAGE_SNAPSHOT_NVRAM, "nvram/42.fd", false); when(vmSnapshot.getId()).thenReturn(20L); when(vmSnapshot.getUuid()).thenReturn("vm-snapshot"); - when(vmSnapshotDetailsDao.findDetail(20L, "kvmFileBasedStorageSnapshotNvram")).thenReturn(nvramDetail); + when(vmSnapshotDetailsDao.findDetail(20L, KVM_FILE_BASED_STORAGE_SNAPSHOT_NVRAM)).thenReturn(nvramDetail); when(hostDetailsDao.findDetail(hostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM)).thenReturn(null); try { @@ -349,11 +352,11 @@ public void testGetPrimaryDataStoreForNvramCleanupPrefersRootSnapshotPrimaryData SnapshotInfo rootSnapshotInfo = mock(SnapshotInfo.class); SnapshotObjectTO rootSnapshotObjectTo = mock(SnapshotObjectTO.class); VolumeObjectTO rootSnapshotVolume = mock(VolumeObjectTO.class); - VMSnapshotDetailsVO volumeSnapshotDetail = new VMSnapshotDetailsVO(vmSnapshotId, "kvmFileBasedStorageSnapshot", String.valueOf(rootSnapshotId), true); + VMSnapshotDetailsVO volumeSnapshotDetail = new VMSnapshotDetailsVO(vmSnapshotId, KVM_FILE_BASED_STORAGE_SNAPSHOT, String.valueOf(rootSnapshotId), true); when(vmSnapshot.getId()).thenReturn(vmSnapshotId); when(vmSnapshot.getUuid()).thenReturn("vm-snapshot"); - when(vmSnapshotDetailsDao.findDetails(vmSnapshotId, "kvmFileBasedStorageSnapshot")).thenReturn(List.of(volumeSnapshotDetail)); + when(vmSnapshotDetailsDao.findDetails(vmSnapshotId, KVM_FILE_BASED_STORAGE_SNAPSHOT)).thenReturn(List.of(volumeSnapshotDetail)); when(snapshotDataStoreDao.findOneBySnapshotAndDatastoreRole(rootSnapshotId, DataStoreRole.Primary)).thenReturn(rootSnapshotDataStore); when(rootSnapshotDataStore.getSnapshotId()).thenReturn(rootSnapshotId); when(rootSnapshotDataStore.getDataStoreId()).thenReturn(dataStoreId); @@ -431,8 +434,8 @@ public void testDeleteVMSnapshotFailsWhenHostLacksNvramAwareCleanupCapabilityFor UserVmVO userVm = mock(UserVmVO.class); VMSnapshotVO vmSnapshot = mock(VMSnapshotVO.class); - VMSnapshotDetailsVO volumeSnapshotDetail = new VMSnapshotDetailsVO(vmSnapshotId, "kvmFileBasedStorageSnapshot", "30", true); - VMSnapshotDetailsVO nvramDetail = new VMSnapshotDetailsVO(vmSnapshotId, "kvmFileBasedStorageSnapshotNvram", "nvram/42.fd", false); + VMSnapshotDetailsVO volumeSnapshotDetail = new VMSnapshotDetailsVO(vmSnapshotId, KVM_FILE_BASED_STORAGE_SNAPSHOT, "30", true); + VMSnapshotDetailsVO nvramDetail = new VMSnapshotDetailsVO(vmSnapshotId, KVM_FILE_BASED_STORAGE_SNAPSHOT_NVRAM, "nvram/42.fd", false); when(vmSnapshot.getVmId()).thenReturn(vmId); when(vmSnapshot.getId()).thenReturn(vmSnapshotId); @@ -440,8 +443,8 @@ public void testDeleteVMSnapshotFailsWhenHostLacksNvramAwareCleanupCapabilityFor when(userVm.getState()).thenReturn(VirtualMachine.State.Running); when(strategy.userVmDao.findById(vmId)).thenReturn(userVm); when(vmSnapshotHelper.pickRunningHost(vmId)).thenReturn(hostId); - when(vmSnapshotDetailsDao.findDetails(vmSnapshotId, "kvmFileBasedStorageSnapshot")).thenReturn(List.of(volumeSnapshotDetail)); - when(vmSnapshotDetailsDao.findDetail(vmSnapshotId, "kvmFileBasedStorageSnapshotNvram")).thenReturn(nvramDetail); + when(vmSnapshotDetailsDao.findDetails(vmSnapshotId, KVM_FILE_BASED_STORAGE_SNAPSHOT)).thenReturn(List.of(volumeSnapshotDetail)); + when(vmSnapshotDetailsDao.findDetail(vmSnapshotId, KVM_FILE_BASED_STORAGE_SNAPSHOT_NVRAM)).thenReturn(nvramDetail); when(hostDetailsDao.findDetail(hostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM)).thenReturn(null); strategy.deleteVMSnapshot(vmSnapshot); @@ -498,7 +501,7 @@ public void testTakeVmSnapshotInternalPersistsStrategyMarkerBeforeHostValidation InOrder inOrder = inOrder(vmSnapshotHelper, vmSnapshotDetailsDao); inOrder.verify(vmSnapshotHelper).vmSnapshotStateTransitTo(vmSnapshot, VMSnapshot.Event.CreateRequested); - inOrder.verify(vmSnapshotDetailsDao).addDetail(vmSnapshotId, "kvmFileBasedStorageSnapshotStrategy", Boolean.TRUE.toString(), false); + inOrder.verify(vmSnapshotDetailsDao).addDetail(vmSnapshotId, KVM_FILE_BASED_STORAGE_SNAPSHOT_STRATEGY, Boolean.TRUE.toString(), false); verifyNoInteractions(agentMgr); } @@ -570,8 +573,8 @@ public void testNotifyGuestRecoveryIssueIfNeededSendsAlert() { @Test public void testCanHandleReturnsHighestForSnapshotWithVolumeSnapshotDetails() { VMSnapshotVO vmSnapshot = mockVmSnapshotForCanHandle(VMSnapshot.State.Error); - when(vmSnapshotDetailsDao.findDetails(20L, "kvmFileBasedStorageSnapshot")) - .thenReturn(List.of(new VMSnapshotDetailsVO(20L, "kvmFileBasedStorageSnapshot", "30", true))); + when(vmSnapshotDetailsDao.findDetails(20L, KVM_FILE_BASED_STORAGE_SNAPSHOT)) + .thenReturn(List.of(new VMSnapshotDetailsVO(20L, KVM_FILE_BASED_STORAGE_SNAPSHOT, "30", true))); assertEquals(StrategyPriority.HIGHEST, strategy.canHandle(vmSnapshot)); @@ -581,9 +584,9 @@ public void testCanHandleReturnsHighestForSnapshotWithVolumeSnapshotDetails() { @Test public void testCanHandleReturnsHighestForErrorSnapshotWithStrategyMarker() { VMSnapshotVO vmSnapshot = mockVmSnapshotForCanHandle(VMSnapshot.State.Error); - when(vmSnapshotDetailsDao.findDetails(20L, "kvmFileBasedStorageSnapshot")).thenReturn(List.of()); - when(vmSnapshotDetailsDao.findDetail(20L, "kvmFileBasedStorageSnapshotStrategy")) - .thenReturn(new VMSnapshotDetailsVO(20L, "kvmFileBasedStorageSnapshotStrategy", Boolean.TRUE.toString(), false)); + when(vmSnapshotDetailsDao.findDetails(20L, KVM_FILE_BASED_STORAGE_SNAPSHOT)).thenReturn(List.of()); + when(vmSnapshotDetailsDao.findDetail(20L, KVM_FILE_BASED_STORAGE_SNAPSHOT_STRATEGY)) + .thenReturn(new VMSnapshotDetailsVO(20L, KVM_FILE_BASED_STORAGE_SNAPSHOT_STRATEGY, Boolean.TRUE.toString(), false)); assertEquals(StrategyPriority.HIGHEST, strategy.canHandle(vmSnapshot)); @@ -593,7 +596,7 @@ public void testCanHandleReturnsHighestForErrorSnapshotWithStrategyMarker() { @Test public void testCanHandleReturnsCantHandleForReadySnapshotWithoutVolumeSnapshotDetails() { VMSnapshotVO vmSnapshot = mockVmSnapshotForCanHandle(VMSnapshot.State.Ready); - when(vmSnapshotDetailsDao.findDetails(20L, "kvmFileBasedStorageSnapshot")).thenReturn(List.of()); + when(vmSnapshotDetailsDao.findDetails(20L, KVM_FILE_BASED_STORAGE_SNAPSHOT)).thenReturn(List.of()); assertEquals(StrategyPriority.CANT_HANDLE, strategy.canHandle(vmSnapshot)); @@ -603,7 +606,7 @@ public void testCanHandleReturnsCantHandleForReadySnapshotWithoutVolumeSnapshotD @Test public void testCanHandleChecksCreationEligibilityForErrorSnapshotWithoutVolumeSnapshotDetails() { VMSnapshotVO vmSnapshot = mockVmSnapshotForCanHandle(VMSnapshot.State.Error); - when(vmSnapshotDetailsDao.findDetails(20L, "kvmFileBasedStorageSnapshot")).thenReturn(List.of()); + when(vmSnapshotDetailsDao.findDetails(20L, KVM_FILE_BASED_STORAGE_SNAPSHOT)).thenReturn(List.of()); doReturn(StrategyPriority.HIGHEST).when(strategy).canHandle(10L, null, false); assertEquals(StrategyPriority.HIGHEST, strategy.canHandle(vmSnapshot)); @@ -614,14 +617,14 @@ public void testCanHandleChecksCreationEligibilityForErrorSnapshotWithoutVolumeS @Test public void testCanHandleReturnsCantHandleForErrorSnapshotWhenVmIsNotEligible() { VMSnapshotVO vmSnapshot = mockVmSnapshotForCanHandle(VMSnapshot.State.Error); - when(vmSnapshotDetailsDao.findDetails(20L, "kvmFileBasedStorageSnapshot")).thenReturn(List.of()); + when(vmSnapshotDetailsDao.findDetails(20L, KVM_FILE_BASED_STORAGE_SNAPSHOT)).thenReturn(List.of()); doReturn(StrategyPriority.CANT_HANDLE).when(strategy).canHandle(10L, null, false); assertEquals(StrategyPriority.CANT_HANDLE, strategy.canHandle(vmSnapshot)); } @Test - public void testDeleteVMSnapshotRemovesOnlyMetadataWhenSnapshotHasNoVolumeSnapshots() throws Exception { + public void testDeleteVMSnapshotRemovesOnlyMetadataWhenSnapshotHasNoPrimaryStorageArtifacts() throws Exception { long vmId = 10L; long vmSnapshotId = 20L; @@ -629,7 +632,8 @@ public void testDeleteVMSnapshotRemovesOnlyMetadataWhenSnapshotHasNoVolumeSnapsh when(vmSnapshot.getVmId()).thenReturn(vmId); when(vmSnapshot.getId()).thenReturn(vmSnapshotId); when(vmSnapshot.getUuid()).thenReturn("vm-snapshot"); - when(vmSnapshotDetailsDao.findDetails(vmSnapshotId, "kvmFileBasedStorageSnapshot")).thenReturn(List.of()); + when(vmSnapshotDetailsDao.findDetails(vmSnapshotId, KVM_FILE_BASED_STORAGE_SNAPSHOT)).thenReturn(List.of()); + when(vmSnapshotDetailsDao.findDetail(vmSnapshotId, KVM_FILE_BASED_STORAGE_SNAPSHOT_NVRAM)).thenReturn(null); assertTrue(strategy.deleteVMSnapshot(vmSnapshot)); @@ -647,6 +651,50 @@ public void testDeleteVMSnapshotRemovesOnlyMetadataWhenSnapshotHasNoVolumeSnapsh verify(strategy, never()).publishUsageEvent(anyString(), any(VMSnapshot.class), any(UserVm.class), anyLong(), anyLong()); } + @Test + public void testDeleteVMSnapshotCleansUpNvramSidecarOnHostWhenSnapshotHasNoVolumeSnapshotDetails() throws Exception { + long vmId = 10L; + long vmSnapshotId = 20L; + long hostId = 40L; + + UserVmVO userVm = mock(UserVmVO.class); + VMSnapshotVO vmSnapshot = mock(VMSnapshotVO.class); + VolumeObjectTO rootVolume = mock(VolumeObjectTO.class); + PrimaryDataStoreTO primaryDataStore = mock(PrimaryDataStoreTO.class); + VMSnapshotDetailsVO nvramDetail = new VMSnapshotDetailsVO(vmSnapshotId, KVM_FILE_BASED_STORAGE_SNAPSHOT_NVRAM, "nvram/42.fd", false); + + when(vmSnapshot.getVmId()).thenReturn(vmId); + when(vmSnapshot.getId()).thenReturn(vmSnapshotId); + when(vmSnapshot.getUuid()).thenReturn("vm-snapshot"); + when(vmSnapshot.getState()).thenReturn(VMSnapshot.State.Error); + when(vmSnapshot.getCurrent()).thenReturn(false); + when(userVm.getState()).thenReturn(VirtualMachine.State.Running); + when(strategy.userVmDao.findById(vmId)).thenReturn(userVm); + when(vmSnapshotHelper.pickRunningHost(vmId)).thenReturn(hostId); + when(rootVolume.getVolumeType()).thenReturn(Volume.Type.ROOT); + when(rootVolume.getDataStore()).thenReturn(primaryDataStore); + when(vmSnapshotHelper.getVolumeTOList(vmId)).thenReturn(List.of(rootVolume)); + when(vmSnapshotDetailsDao.findDetails(vmSnapshotId, KVM_FILE_BASED_STORAGE_SNAPSHOT)).thenReturn(List.of()); + when(vmSnapshotDetailsDao.findDetail(vmSnapshotId, KVM_FILE_BASED_STORAGE_SNAPSHOT_STRATEGY)).thenReturn(null); + when(vmSnapshotDetailsDao.findDetail(vmSnapshotId, KVM_FILE_BASED_STORAGE_SNAPSHOT_NVRAM)).thenReturn(nvramDetail); + when(hostDetailsDao.findDetail(hostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM)) + .thenReturn(new DetailVO(hostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM, Boolean.TRUE.toString())); + when(agentMgr.easySend(eq(hostId), any())).thenReturn(new Answer(null, true, null)); + + assertTrue(strategy.deleteVMSnapshot(vmSnapshot)); + + verify(strategy, never()).deleteVMSnapshotMetadataOnly(any(VMSnapshotVO.class)); + verify(agentMgr).easySend(eq(hostId), argThat(command -> command instanceof DeleteDiskOnlyVmSnapshotCommand + && ((DeleteDiskOnlyVmSnapshotCommand) command).getSnapshots().isEmpty() + && "nvram/42.fd".equals(((DeleteDiskOnlyVmSnapshotCommand) command).getNvramSnapshotPath()) + && ((DeleteDiskOnlyVmSnapshotCommand) command).getPrimaryDataStore() == null)); + verify(agentMgr).easySend(eq(hostId), argThat(command -> command instanceof DeleteDiskOnlyVmSnapshotCommand + && "nvram/42.fd".equals(((DeleteDiskOnlyVmSnapshotCommand) command).getNvramSnapshotPath()) + && ((DeleteDiskOnlyVmSnapshotCommand) command).getPrimaryDataStore() == primaryDataStore)); + verify(vmSnapshotDetailsDao).removeDetails(vmSnapshotId); + verify(vmSnapshotDao).update(vmSnapshotId, vmSnapshot); + } + private VMSnapshotVO mockVmSnapshotForCanHandle(VMSnapshot.State state) { VMSnapshotVO vmSnapshot = mock(VMSnapshotVO.class); when(vmSnapshot.getId()).thenReturn(20L);