Liron Ar has posted comments on this change. Change subject: core: introduce RemoveDiskSnapshotsCommand ......................................................................
Patch Set 9: (18 comments) http://gerrit.ovirt.org/#/c/26327/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotTaskHandler.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotTaskHandler.java: Line 85: @Override Line 86: public void endSuccessfully() { Line 87: if (isLastTaskHandler()) { Line 88: // Unlock on job finish Line 89: updateImagesStatus(ImageStatus.OK); 1. we should do it only on the images that you know that were OK, if the disk had images on other statuses they'd be changed to OK as well. 2. we can release volumes that we know that we are done with (it's an optimization..but maybe as merge is very long operation, it'll be better to do so). Line 90: } Line 91: endRemoveSnapshotSingleDisk(); Line 92: enclosingCommand.taskEndSuccessfully(); Line 93: enclosingCommand.getReturnValue().setSucceeded(true); Line 95: Line 96: @Override Line 97: public void endWithFailure() { Line 98: // Unlock all images since failure aborts the entire job Line 99: updateImagesStatus(ImageStatus.OK); i think that the images that we failed to merge should be marked as illegal, we can't assume they are "ok" unless we do wiser check to know what's going on with it. Line 100: endRemoveSnapshotSingleDisk(); Line 101: enclosingCommand.preventRollback(); Line 102: enclosingCommand.getReturnValue().setSucceeded(true); Line 103: } Line 113: private void updateImagesStatus(ImageStatus imageStatus) { Line 114: Guid imageGroupId = DbFacade.getInstance().getDiskImageDao().getSnapshotById(imageId).getId(); Line 115: ImagesHandler.updateAllDiskImageSnapshotsStatusWithCompensation(imageGroupId, Line 116: imageStatus, Line 117: ImageStatus.OK , see above comment, should be illegal Line 118: enclosingCommand.getCompensationContext()); Line 119: } Line 120: Line 121: @Override http://gerrit.ovirt.org/#/c/26327/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotsCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotsCommand.java: Line 29: import org.ovirt.engine.core.common.utils.Pair; Line 30: import org.ovirt.engine.core.compat.Guid; Line 31: import org.ovirt.engine.core.dao.DiskDao; Line 32: import org.ovirt.engine.core.dao.DiskImageDAO; Line 33: 1. please add nontransactiveattribute 2. please add memory locks 3. general - right now other operations won't be prevented when removing the snapshots (for example, RemoveDiskCommand). we should prevent those operations from running (also currently we could initiate concurrent removal of snapshots of the same disk which is a problem in terms of the validations (space) and i assume that also functionally). Line 34: public class RemoveDiskSnapshotsCommand<T extends RemoveDiskSnapshotsParameters> extends BaseImagesCommand<T> Line 35: implements TaskHandlerCommand<RemoveDiskSnapshotsParameters> { Line 36: Line 37: private List<DiskImage> images; Line 102: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_NOT_FOUND); Line 103: } Line 104: Line 105: DiskSnapshotsValidator diskSnapshotsValidator = createDiskSnapshotsValidator(getParameters().getImageIds()); Line 106: if (!validate(diskSnapshotsValidator.diskSnapshotsNotExist())) { as we are the only users of that validator, i'd suggest to pass to it the loaded images (getImages())) rather than re load from the db. Line 107: return false; Line 108: } Line 109: Line 110: DiskImagesValidator diskImagesValidator = createDiskImageValidator(getImages()); Line 107: return false; Line 108: } Line 109: Line 110: DiskImagesValidator diskImagesValidator = createDiskImageValidator(getImages()); Line 111: if (!validate(diskImagesValidator.diskImagesBelongToSameImageGroup())) { seems to me like this validation should be moved to the disksnapshotvalidator as well Line 112: return false; Line 113: } Line 114: Line 115: if (!validate(diskImagesValidator.diskImagesSnapshotsNotAttachedToOtherVms(false))) { Line 115: if (!validate(diskImagesValidator.diskImagesSnapshotsNotAttachedToOtherVms(false))) { Line 116: return false; Line 117: } Line 118: Line 119: if (!canRunActionOnNonManagedVm()) { any reason that we can't? atleast on hosted-engine vm? Line 120: return false; Line 121: } Line 122: Line 123: VmValidator vmValidator = createVmValidator(getVm()); Line 128: if (!validate(new StoragePoolValidator(getStoragePool()).isUp()) || Line 129: !validateVmNotDuringSnapshot() || Line 130: !validateVmNotInPreview() || Line 131: !validateSnapshotExists() || Line 132: !validate(vmValidator.vmNotHavingDeviceSnapshotsAttachedToOtherVms(false)) || this should be removed, we checked on line 115 that snapshots of that disk aren't attached to other vms. Line 133: !validateStorageDomainActive()) { Line 134: return false; Line 135: } Line 136: Line 133: !validateStorageDomainActive()) { Line 134: return false; Line 135: } Line 136: Line 137: // Check images this comment isn't that useful :) i'd suggest moving this check up Line 138: if (!validateImages()) { Line 139: return false; Line 140: } Line 141: Line 139: return false; Line 140: } Line 141: Line 142: // check that we are not deleting the vm working snapshot Line 143: if (!validateImagesNotActive()) { i'd suggest moving this check up Line 144: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_CANNOT_REMOVE_ACTIVE_IMAGE); Line 145: } Line 146: Line 147: if (!validateStorageDomainAvailableSpace()) { Line 161: protected List<SPMAsyncTaskHandler> initTaskHandlers() { Line 162: List<SPMAsyncTaskHandler> taskHandlers = new ArrayList<>(); Line 163: Line 164: for (Guid imageId : getParameters().getImageIds()) { Line 165: taskHandlers.add(new RemoveDiskSnapshotTaskHandler(this, imageId, getVmId())); perhaps we "order" the snapshots to remove by their place in the hirarechy? for example, breakage in the middle of the chain is more hazardous than breakage in the "end" of the chain that'll allow us to use all the volumes before the breakage. Line 166: } Line 167: Line 168: return taskHandlers; Line 169: } Line 174: } Line 175: Line 176: protected void updateSnapshotVmConfiguration() { Line 177: Guid imageId = getParameters().getImageIds().get(getParameters().getExecutionIndex()); Line 178: Snapshot snapshotWithoutImage = prepareSnapshotConfigWithoutImageSingleImage(getSnapshotId(), imageId); possibly we'd need to perform some sort of update also in case of failure, otherwise the snapshot might be unusable. Line 179: getSnapshotDao().update(snapshotWithoutImage); Line 180: } Line 181: Line 182: @Override Line 262: return validate(diskImagesValidator.diskImagesNotLocked()) && Line 263: validate(diskImagesValidator.diskImagesNotIllegal()); Line 264: } Line 265: Line 266: protected boolean validateImagesNotActive() { no need for that, we can check the image.isActive(). I'd suggest to have that logic in DiskSnapshotValidator.areAllSnapshots() Line 267: for (Guid imageId : getParameters().getImageIds()) { Line 268: if (getDiskImageDao().get(imageId) != null) { Line 269: return false; Line 270: } Line 292: return getImages().get(0); Line 293: } Line 294: Line 295: protected boolean isDiskPlugged() { Line 296: return getDiskDao().getAllForVm(getVmId(), true).contains(getRepresentativeImage()); it'll be better to load just the needed vm device rather than loading all the disks. Line 297: } Line 298: Line 299: @Override Line 300: public List<PermissionSubject> getPermissionCheckSubjects() { http://gerrit.ovirt.org/#/c/26327/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/DiskSnapshotsValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/DiskSnapshotsValidator.java: Line 31: } Line 32: } Line 33: Line 34: if (!disksNotExistInDbIds.isEmpty()) { Line 35: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISKS_NOT_EXIST, not sure that the message fits here Line 36: String.format("$diskIds %s", StringUtils.join(disksNotExistInDbIds, ", "))); Line 37: } Line 38: Line 39: return ValidationResult.VALID; http://gerrit.ovirt.org/#/c/26327/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java: Line 204: sumOfActualSizes += (long) image.getActualSize(); Line 205: } Line 206: Line 207: // Finding the max virtual size Line 208: long maxVirtualSize = Collections.max(diskImages, new Comparator<DiskImage>() { i'd find that during the first loop rather than iterate again. Line 209: @Override Line 210: public int compare(DiskImage diskImage1, DiskImage diskImage2) { Line 211: return diskImage1.getSize() < diskImage2.getSize() ? -1 Line 212: : diskImage1.getSize() > diskImage2.getSize() ? 1 : 0; Line 212: : diskImage1.getSize() > diskImage2.getSize() ? 1 : 0; Line 213: } Line 214: }).getSize(); Line 215: Line 216: return isDomainHasSpaceForRequest(Math.min(maxVirtualSize, sumOfActualSizes), false); shouldn't this be Math.max? (at least it seems so from the method comment) Line 217: } http://gerrit.ovirt.org/#/c/26327/9/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveDiskSnapshotsParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveDiskSnapshotsParameters.java: Line 9: public class RemoveDiskSnapshotsParameters extends ImagesContainterParametersBase implements Serializable { Line 10: Line 11: private static final long serialVersionUID = -629355522841577585L; Line 12: Line 13: private List<Guid> imageIds; is that being serialized? if so, it should fail on the json de-serialization (in case of engine restart)..concrete type is needed. Line 14: Line 15: public RemoveDiskSnapshotsParameters(Guid imageId) { Line 16: this(Arrays.asList(imageId)); Line 17: } -- To view, visit http://gerrit.ovirt.org/26327 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia714a4390d1d9b672005be30f58b7fa98b9a31cd Gerrit-PatchSet: 9 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
