JoaoJandre commented on code in PR #14515:
URL: https://github.com/apache/cloudstack/pull/14515#discussion_r4404172696
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java:
##########
@@ -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<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
(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());
Review Comment:
I'm not sure this fallback is good approach. Aren't the other changes enough?
My fear is that we eventually try to operate a VM snapshot which is not from
this strategy.
--
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]
