Liron Ar has posted comments on this change. Change subject: core: avoid broken status on snapshot removal ......................................................................
Patch Set 6: (3 comments) General - we need to think clarify what's the behavior in case of snapshot with memory. I'd consider leaving the broken status to indicate that the snapshot isn't the "same" as it was created (or maybe rename it to "partial") and maybe allow to perform operations on it. http://gerrit.ovirt.org/#/c/27611/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotCommand.java: Line 194: List<String> failedToRemoveDisks = new ArrayList<>(); Line 195: Line 196: // Remove successfully deleted images from the snapshot Line 197: for (VdcActionParametersBase parametersBase : getParameters().getImagesParameters()) { Line 198: ImagesContainterParametersBase imagesParams = (ImagesContainterParametersBase) parametersBase; in case of snapshot with memory, seems that we might get here with other type of parameters and fail on the casting. Line 199: Line 200: if (imagesParams.getTaskGroupSuccess()) { Line 201: Snapshot snapshotWithoutImage = ImagesHandler.prepareSnapshotConfigWithoutImageSingleImage( Line 202: getParameters().getSnapshotId(), imagesParams.getImageId()); Line 199: Line 200: if (imagesParams.getTaskGroupSuccess()) { Line 201: Snapshot snapshotWithoutImage = ImagesHandler.prepareSnapshotConfigWithoutImageSingleImage( Line 202: getParameters().getSnapshotId(), imagesParams.getImageId()); Line 203: getSnapshotDao().update(snapshotWithoutImage); suggestion - we can improve things a bit here and perform only one update after all were removed, Line 204: } Line 205: else { Line 206: log.errorFormat("Could not delete image {0} from snapshot {1}", Line 207: getParameters().getSnapshotId(), imagesParams.getImageId()); Line 206: log.errorFormat("Could not delete image {0} from snapshot {1}", Line 207: getParameters().getSnapshotId(), imagesParams.getImageId()); Line 208: Line 209: DiskImage diskImage = getDiskImageDao().getSnapshotById(imagesParams.getImageId()); Line 210: failedToRemoveDisks.add(diskImage.getDiskAlias()); i'd add this also to the logging so it'll be easier to track.. "image 111 of disk 33343.." Line 211: } Line 212: } Line 213: Line 214: if (!failedToRemoveDisks.isEmpty()) { -- To view, visit http://gerrit.ovirt.org/27611 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2a15034185ef30f53fbeaa46440cd92954d39eee Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Daniel Erez <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
