JoaoJandre commented on code in PR #14466:
URL: https://github.com/apache/cloudstack/pull/14466#discussion_r4362282232
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java:
##########
@@ -153,18 +168,21 @@ public boolean deleteVMSnapshot(VMSnapshot vmSnapshot) {
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());
+ logger.debug("Deleting VM snapshot [{}] as no snapshots/volumes
depend on it.", vmSnapshotBeingDeleted.getUuid());
volumeSnapshotVos = deleteSnapshot(vmSnapshotBeingDeleted, hostId);
-
mergeOldSiblingWithOldParentIfOldParentIsDead(vmSnapshotDao.findByIdIncludingRemoved(vmSnapshotBeingDeleted.getParent()),
userVm, hostId, volumeTOs);
+ VMSnapshotVO oldParent =
vmSnapshotDao.findByIdIncludingRemoved(vmSnapshotBeingDeleted.getParent());
+ try {
+ mergeOldSiblingWithOldParentIfOldParentIsDead(oldParent,
userVm, hostId, volumeTOs);
+ } catch (CloudRuntimeException ex) {
+ logger.warn("Deleted VM snapshot [{}], but failed to merge its
hidden parent [{}] with its remaining child; the parent will be kept hidden on
storage.",
+ vmSnapshotBeingDeleted.getUuid(), oldParent.getUuid(),
ex);
Review Comment:
Could you explain this part @abh1sar ? We get an error and continue as if
everything went ok?
By the PR's logic, wouldn't it be best to only set the snapshots that were
acutally merged as removed?
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMergeDiskOnlyVMSnapshotCommandWrapper.java:
##########
@@ -60,51 +91,103 @@ public Answer execute(MergeDiskOnlyVmSnapshotCommand
command, LibvirtComputingRe
return mergeDiskOnlySnapshotsForRunningVM(command,
serverResource);
}
return mergeDiskOnlySnapshotsForStoppedVM(command, serverResource);
- } catch (LibvirtException | QemuImgException | CloudRuntimeException
ex) {
- return new Answer(command, ex);
+ } catch (LibvirtException | CloudRuntimeException ex) {
+ logger.error("Failed to merge disk-only snapshots of VM [{}]
before any volume was merged.", command.getVmName(), ex);
+ return new MergeDiskOnlyVmSnapshotAnswer(command, false,
ex.getMessage(), new ArrayList<>());
}
}
- protected Answer
mergeDiskOnlySnapshotsForStoppedVM(MergeDiskOnlyVmSnapshotCommand cmd,
LibvirtComputingResource resource) throws QemuImgException, LibvirtException {
- QemuImg qemuImg = new QemuImg(resource.getCmdsTimeout());
- KVMStoragePoolManager storageManager = resource.getStoragePoolMgr();
+ /**
+ * Returns how long, in seconds, the host may spend merging. When the
management server informs the command's wait, a safety margin is subtracted
+ * from it so that the jobs can be cancelled and the answer delivered
before the management server stops waiting. Otherwise, the given fallback is
used.
+ */
+ protected int getMergeTimeoutInSeconds(MergeDiskOnlyVmSnapshotCommand cmd,
int fallbackInSeconds) {
Review Comment:
Since the MGMT server always waits double the wait time, is this needed?
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java:
##########
@@ -153,18 +168,21 @@ public boolean deleteVMSnapshot(VMSnapshot vmSnapshot) {
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());
+ logger.debug("Deleting VM snapshot [{}] as no snapshots/volumes
depend on it.", vmSnapshotBeingDeleted.getUuid());
volumeSnapshotVos = deleteSnapshot(vmSnapshotBeingDeleted, hostId);
-
mergeOldSiblingWithOldParentIfOldParentIsDead(vmSnapshotDao.findByIdIncludingRemoved(vmSnapshotBeingDeleted.getParent()),
userVm, hostId, volumeTOs);
+ VMSnapshotVO oldParent =
vmSnapshotDao.findByIdIncludingRemoved(vmSnapshotBeingDeleted.getParent());
+ try {
+ mergeOldSiblingWithOldParentIfOldParentIsDead(oldParent,
userVm, hostId, volumeTOs);
+ } catch (CloudRuntimeException ex) {
+ logger.warn("Deleted VM snapshot [{}], but failed to merge its
hidden parent [{}] with its remaining child; the parent will be kept hidden on
storage.",
+ vmSnapshotBeingDeleted.getUuid(), oldParent.getUuid(),
ex);
+ }
} 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);
- }
+ markVolumeSnapshotsAsDestroyed(volumeSnapshotVos);
Review Comment:
this is being done inside mergeOldSiblingWithOldParentIfOldParentIsDead and
also here.
--
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]
