winterhazel commented on code in PR #13053:
URL: https://github.com/apache/cloudstack/pull/13053#discussion_r3223349958
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCreateDiskOnlyVMSnapshotCommandWrapper.java:
##########
@@ -146,21 +150,13 @@ protected Answer
takeDiskOnlyVmSnapshotOfStoppedVm(CreateDiskOnlyVmSnapshotComma
}
} catch (LibvirtException | QemuImgException e) {
logger.error("Exception while creating disk-only VM snapshot for
VM [{}]. Deleting leftover deltas.", vmName, e);
- for (VolumeObjectTO volumeObjectTO : volumeObjectTos) {
- Pair<Long, String> volSizeAndNewPath =
mapVolumeToSnapshotSizeAndNewVolumePath.get(volumeObjectTO.getUuid());
- PrimaryDataStoreTO primaryDataStoreTO = (PrimaryDataStoreTO)
volumeObjectTO.getDataStore();
- KVMStoragePool kvmStoragePool =
storagePoolMgr.getStoragePool(primaryDataStoreTO.getPoolType(),
primaryDataStoreTO.getUuid());
-
- if (volSizeAndNewPath == null) {
- continue;
- }
- try {
-
Files.deleteIfExists(Path.of(kvmStoragePool.getLocalPathFor(volSizeAndNewPath.second())));
- } catch (IOException ex) {
- logger.warn("Tried to delete leftover snapshot at [{}]
failed.", volSizeAndNewPath.second(), ex);
- }
- }
+ cleanupLeftoverDeltas(volumeObjectTos,
mapVolumeToSnapshotSizeAndNewVolumePath, storagePoolMgr);
return new Answer(cmd, e);
+ } catch (Exception e) {
+ logger.error("Unexpected exception while creating disk-only VM
snapshot for VM [{}]. Deleting leftover deltas.", vmName, e);
+ cleanupLeftoverDeltas(volumeObjectTos,
mapVolumeToSnapshotSizeAndNewVolumePath, storagePoolMgr);
+ return new CreateDiskOnlyVmSnapshotAnswer(cmd, false,
+ String.format("Creation of disk-only VM snapshot for VM
[%s] failed due to %s.", vmName, e.getMessage()), null);
Review Comment:
@rajiv-jain-netapp got it. Notifying operators of why there was a failure is
the right thing to do. I was just thinking about converting the two `catch`es
of this `try` to a single `catch`, as they execute pretty much the same things.
This would be done by replacing `catch (LibvirtException | QemuImgException e)`
with `catch (Exception e)` and keeping `return new
CreateDiskOnlyVmSnapshotAnswer(...` instead of `return new Answer(...`.
--
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]
