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..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,8 @@ 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 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 (!hasRelevantVmSnapshotDetails(vmSnapshotBeingDeleted)) { + return deleteVMSnapshotMetadataOnly(vmSnapshotBeingDeleted); + } + + UserVmVO userVm = userVmDao.findById(vmSnapshot.getVmId()); Long hostId = pickHostForNvramSidecarCleanup(vmSnapshotBeingDeleted, userVm, "delete"); validateHostSupportsNvramSidecarCleanup(vmSnapshotBeingDeleted, hostId, "delete"); long virtualSize = 0; @@ -322,16 +327,47 @@ private void mergeOldSiblingWithOldParentIfOldParentIsDead(VMSnapshotVO oldParen transitStateWithoutThrow(oldParent, VMSnapshot.Event.OperationSucceeded); } + 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); + + vmSnapshotDetailsDao.removeDetails(vmSnapshot.getId()); + + vmSnapshot.setRemoved(DateUtil.now()); + vmSnapshotDao.update(vmSnapshot.getId(), vmSnapshot); + + return true; + } + + 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; + } + @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 (hasKVMFileBasedStorageSnapshotDetailInVmSnapshotDetails(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 +559,7 @@ protected VMSnapshot takeVmSnapshotInternal(VMSnapshot vmSnapshot, Map 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); + when(vmSnapshot.getVmId()).thenReturn(10L); + when(vmSnapshot.getUuid()).thenReturn("vm-snapshot"); + when(vmSnapshot.getState()).thenReturn(state); + when(vmSnapshot.getType()).thenReturn(VMSnapshot.Type.Disk); + return vmSnapshot; + } + private void mockCandidateHostScope(long vmId, long hostId) { long poolId = 50L; long clusterId = 60L;