Copilot commented on code in PR #14515:
URL: https://github.com/apache/cloudstack/pull/14515#discussion_r4400616903
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java:
##########
@@ -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));
+ }
Review Comment:
The new metadata-only deletion path is selected solely based on the absence
of `kvmFileBasedStorageSnapshot` volume details. If a snapshot has an NVRAM
sidecar detail (e.g. `kvmFileBasedStorageSnapshotNvram`) but no volume details,
this will bypass host-side cleanup and only remove DB metadata, potentially
leaking the NVRAM artifact on storage. Consider gating the metadata-only delete
on the absence of *any* primary-storage artifacts (volume snapshot details
*and* NVRAM detail), or explicitly checking/remediating the NVRAM detail before
taking the metadata-only path.
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java:
##########
@@ -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<VMSnapshotDetailsVO> 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());
}
Review Comment:
Given the new branching around ‘volume snapshot details’ vs ‘strategy
marker’, it would be valuable to add a unit test for the legacy/pre-marker edge
case where an `Error` snapshot has *NVRAM* detail(s) (e.g.
`kvmFileBasedStorageSnapshotNvram`) but no `kvmFileBasedStorageSnapshot` volume
details and no strategy marker. This helps lock in the intended
ownership/handling behavior and prevents accidental selection of the
metadata-only deletion path when an on-storage artifact may still exist.
##########
engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java:
##########
@@ -462,6 +470,38 @@ public void
testTakeVmSnapshotInternalFailsWhenHostLacksUefiCapabilityForUefiVm(
strategy.takeVmSnapshotInternal(vmSnapshot, Collections.emptyMap());
}
+ @Test
+ public void
testTakeVmSnapshotInternalPersistsStrategyMarkerBeforeHostValidation() throws
Exception {
+ long vmId = 10L;
+ long vmSnapshotId = 20L;
+ long hostId = 40L;
+
+ UserVmVO userVm = mock(UserVmVO.class);
+ VMSnapshotVO vmSnapshot = mock(VMSnapshotVO.class);
+
+ when(vmSnapshot.getId()).thenReturn(vmSnapshotId);
+ when(vmSnapshot.getVmId()).thenReturn(vmId);
+ when(userVm.getId()).thenReturn(vmId);
+ when(userVm.getUuid()).thenReturn("vm-uuid");
+ when(userVm.getState()).thenReturn(VirtualMachine.State.Running);
+ when(strategy.userVmDao.findById(vmId)).thenReturn(userVm);
+ when(vmSnapshotHelper.pickRunningHost(vmId)).thenReturn(hostId);
+ when(strategy.vmInstanceDetailsDao.findDetail(vmId,
ApiConstants.BootType.UEFI.toString()))
+ .thenReturn(new VMInstanceDetailVO(vmId,
ApiConstants.BootType.UEFI.toString(), "SECURE", true));
+ when(hostDetailsDao.findDetail(hostId,
Host.HOST_UEFI_ENABLE)).thenReturn(null);
+
+ try {
+ strategy.takeVmSnapshotInternal(vmSnapshot,
Collections.emptyMap());
+ } catch (CloudRuntimeException expected) {
+ // creation is expected to fail; the marker must already be
persisted
+ }
+
+ InOrder inOrder = inOrder(vmSnapshotHelper, vmSnapshotDetailsDao);
+ inOrder.verify(vmSnapshotHelper).vmSnapshotStateTransitTo(vmSnapshot,
VMSnapshot.Event.CreateRequested);
+ inOrder.verify(vmSnapshotDetailsDao).addDetail(vmSnapshotId,
"kvmFileBasedStorageSnapshotStrategy", Boolean.TRUE.toString(), false);
Review Comment:
The tests introduced several string literals for snapshot detail keys
(`\"kvmFileBasedStorageSnapshot\"`, `\"kvmFileBasedStorageSnapshotStrategy\"`).
Since the production code now defines
`KVM_FILE_BASED_STORAGE_SNAPSHOT_STRATEGY` (and already has constants for other
keys), using those constants in tests will reduce duplication and prevent
silent drift/typos if the keys change later.
##########
engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java:
##########
@@ -527,6 +567,96 @@ public void
testNotifyGuestRecoveryIssueIfNeededSendsAlert() {
verify(strategy.alertManager).sendAlert(eq(AlertManager.AlertType.ALERT_TYPE_VM_SNAPSHOT),
eq(1L), eq(2L), anyString(), anyString());
}
+ @Test
+ public void
testCanHandleReturnsHighestForSnapshotWithVolumeSnapshotDetails() {
+ VMSnapshotVO vmSnapshot =
mockVmSnapshotForCanHandle(VMSnapshot.State.Error);
+ when(vmSnapshotDetailsDao.findDetails(20L,
"kvmFileBasedStorageSnapshot"))
+ .thenReturn(List.of(new VMSnapshotDetailsVO(20L,
"kvmFileBasedStorageSnapshot", "30", true)));
+
+ assertEquals(StrategyPriority.HIGHEST, strategy.canHandle(vmSnapshot));
+
+ verify(strategy, never()).canHandle(anyLong(), any(), anyBoolean());
+ }
+
+ @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));
Review Comment:
The tests introduced several string literals for snapshot detail keys
(`\"kvmFileBasedStorageSnapshot\"`, `\"kvmFileBasedStorageSnapshotStrategy\"`).
Since the production code now defines
`KVM_FILE_BASED_STORAGE_SNAPSHOT_STRATEGY` (and already has constants for other
keys), using those constants in tests will reduce duplication and prevent
silent drift/typos if the keys change later.
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java:
##########
@@ -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");
Review Comment:
The new metadata-only deletion path is selected solely based on the absence
of `kvmFileBasedStorageSnapshot` volume details. If a snapshot has an NVRAM
sidecar detail (e.g. `kvmFileBasedStorageSnapshotNvram`) but no volume details,
this will bypass host-side cleanup and only remove DB metadata, potentially
leaking the NVRAM artifact on storage. Consider gating the metadata-only delete
on the absence of *any* primary-storage artifacts (volume snapshot details
*and* NVRAM detail), or explicitly checking/remediating the NVRAM detail before
taking the metadata-only path.
##########
engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java:
##########
@@ -527,6 +567,96 @@ public void
testNotifyGuestRecoveryIssueIfNeededSendsAlert() {
verify(strategy.alertManager).sendAlert(eq(AlertManager.AlertType.ALERT_TYPE_VM_SNAPSHOT),
eq(1L), eq(2L), anyString(), anyString());
}
+ @Test
+ public void
testCanHandleReturnsHighestForSnapshotWithVolumeSnapshotDetails() {
+ VMSnapshotVO vmSnapshot =
mockVmSnapshotForCanHandle(VMSnapshot.State.Error);
+ when(vmSnapshotDetailsDao.findDetails(20L,
"kvmFileBasedStorageSnapshot"))
+ .thenReturn(List.of(new VMSnapshotDetailsVO(20L,
"kvmFileBasedStorageSnapshot", "30", true)));
Review Comment:
The tests introduced several string literals for snapshot detail keys
(`\"kvmFileBasedStorageSnapshot\"`, `\"kvmFileBasedStorageSnapshotStrategy\"`).
Since the production code now defines
`KVM_FILE_BASED_STORAGE_SNAPSHOT_STRATEGY` (and already has constants for other
keys), using those constants in tests will reduce duplication and prevent
silent drift/typos if the keys change later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
