Allon Mureinik has posted comments on this change. Change subject: core: WAD should be Ignored on File Domain Disks ......................................................................
Patch Set 1: Code-Review-1 (22 comments) http://gerrit.ovirt.org/#/c/30706/1//COMMIT_MSG Commit Message: Line 5: CommitDate: 2014-07-24 17:27:50 +0300 Line 6: Line 7: core: WAD should be Ignored on File Domain Disks Line 8: Line 9: Since file systems do the wiping for us, there is no need for doing "file systems do the wiping for us" is a tad inaccurate. Perhaps try something like this: "Since the file system is responsible for handling block allocation" Line 10: it again for disks on file domains. Line 11: Thus, the only case we should wipe a disk after deleting it is when Line 12: we have a disk on a block domain with 'wipeAfterDelete' = true. Line 13: http://gerrit.ovirt.org/#/c/30706/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmAndCloneImageCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmAndCloneImageCommand.java: Line 127: params.setVolumeFormat(diskImage.getVolumeFormat()); Line 128: params.setVolumeType(diskImage.getVolumeType()); Line 129: params.setUseCopyCollapse(true); Line 130: params.setSourceDomainId(srcStorageDomainId); Line 131: setStorageDomainId(srcStorageDomainId); Yikes! You'd be changing the state of the command for every call passed. Perhaps we should add a getStorageDomain(Guid id) method? Line 132: params.setWipeAfterDelete(ImagesHandler.shouldWipeAfterDelete( Line 133: diskImage.isWipeAfterDelete(), getStorageDomain().getStorageType())); Line 134: params.setEntityInfo(new EntityInfo(VdcObjectType.VM, getVmId())); Line 135: params.setParentParameters(getParameters()); http://gerrit.ovirt.org/#/c/30706/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java: Line 809: return snapshot; Line 810: } Line 811: Line 812: /** Line 813: * Decides whether an image disk should be wiped after it's being deleted or not. s/being deleted/deleted/ Line 814: * A disk on a file domain, for example, should not be wiped even if it holds 'wipeAfterDelete' = true. Line 815: * @param shouldDiskBeWipedAfterDelete the disk's 'wipeAfterDelete' property. Line 816: * @param type the disk's type. Line 817: * @return true iff the disk should be wiped after its deletion, Line 810: } Line 811: Line 812: /** Line 813: * Decides whether an image disk should be wiped after it's being deleted or not. Line 814: * A disk on a file domain, for example, should not be wiped even if it holds 'wipeAfterDelete' = true. Don't give examples - describe the entire logic, either here or in the @return clause, but not both. Line 815: * @param shouldDiskBeWipedAfterDelete the disk's 'wipeAfterDelete' property. Line 816: * @param type the disk's type. Line 817: * @return true iff the disk should be wiped after its deletion, Line 818: * i.e. only if the disk is on a block domain and holds 'wipeAfterDelete' = true. Line 811: Line 812: /** Line 813: * Decides whether an image disk should be wiped after it's being deleted or not. Line 814: * A disk on a file domain, for example, should not be wiped even if it holds 'wipeAfterDelete' = true. Line 815: * @param shouldDiskBeWipedAfterDelete the disk's 'wipeAfterDelete' property. s/the/The/ Line 816: * @param type the disk's type. Line 817: * @return true iff the disk should be wiped after its deletion, Line 818: * i.e. only if the disk is on a block domain and holds 'wipeAfterDelete' = true. Line 819: */ Line 812: /** Line 813: * Decides whether an image disk should be wiped after it's being deleted or not. Line 814: * A disk on a file domain, for example, should not be wiped even if it holds 'wipeAfterDelete' = true. Line 815: * @param shouldDiskBeWipedAfterDelete the disk's 'wipeAfterDelete' property. Line 816: * @param type the disk's type. The disk's **storage** type Line 817: * @return true iff the disk should be wiped after its deletion, Line 818: * i.e. only if the disk is on a block domain and holds 'wipeAfterDelete' = true. Line 819: */ Line 820: public static boolean shouldWipeAfterDelete (boolean shouldDiskBeWipedAfterDelete, StorageType type) { http://gerrit.ovirt.org/#/c/30706/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveMemoryVolumesCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveMemoryVolumesCommand.java: Line 36: Line 37: @Override Line 38: protected void executeCommand() { Line 39: MemoryImageRemoverOnDataDomain memoryImageRemover = Line 40: new MemoryImageRemoverOnDataDomain(getParameters().getVmId(), this); This is just a formatting change. Why is it part of this patch? Line 41: Line 42: setSucceeded(memoryImageRemover.remove( Line 43: Collections.singleton(getParameters().getMemoryVolumes()))); Line 44: } http://gerrit.ovirt.org/#/c/30706/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotSingleDiskCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotSingleDiskCommand.java: Line 47: protected VDSReturnValue mergeSnapshots() { Line 48: boolean wipeAfterDelete = ImagesHandler.shouldWipeAfterDelete( Line 49: getDiskImage().isWipeAfterDelete(), getStorageDomain().getStorageType()); Line 50: MergeSnapshotsVDSCommandParameters params = new MergeSnapshotsVDSCommandParameters(getStoragePoolId(), Line 51: getStorageDomainId(), getVmId(), getDiskImage().getId(), getDiskImage().getImageId(), I wouldn't change the getXXXId() calls - you already have a local variable, might as well use it. Line 52: getDestinationDiskImage().getImageId(), wipeAfterDelete); Line 53: return runVdsCommand(VDSCommandType.MergeSnapshots, params); Line 54: } Line 55: http://gerrit.ovirt.org/#/c/30706/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmFromImportExportCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmFromImportExportCommand.java: Line 132: } Line 133: Line 134: private void removeMemoryImages() { Line 135: new MemoryImageRemoverFromExportDomain(getVm(), this, Line 136: getParameters().getStoragePoolId(), getParameters().getStorageDomainId()).remove(); This is just a formatting change. Why is it part of the patch? Line 137: } Line 138: Line 139: @Override Line 140: public AuditLogType getAuditLogTypeValue() { http://gerrit.ovirt.org/#/c/30706/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmCommand.java: Line 327: new Predicate<Disk>() { Line 328: @Override Line 329: public boolean eval(Disk disk) { Line 330: return ImagesHandler.shouldWipeAfterDelete( Line 331: disk.isWipeAfterDelete(), getStorageDomain().getStorageType()); This is wrong. When wiping the memory volume you should care about the storage they are stored upon, not the disks. E.g., you could, theoretically, have all your disks on an NFS mount and your memory hibernated to a fast block device. Line 332: } Line 333: }).size() > 0; Line 334: Line 335: Guid taskId1 = persistAsyncTaskPlaceHolder(parentCommand, DELETE_PRIMARY_IMAGE_TASK_KEY); Line 337: // delete first image Line 338: // the next 'DeleteImageGroup' command should also take care of the image removal: Line 339: VDSReturnValue vdsRetValue = runVdsCommand( Line 340: VDSCommandType.DeleteImageGroup, Line 341: new DeleteImageGroupVDSCommandParameters(guids.get(1), post zero should be evaluated here with the given storage domain ID and the LOGICAL wipe-after-delete value from the disks in line 326. Line 342: guids.get(0), guids.get(2), postZero, false)); Line 343: Line 344: if (!vdsRetValue.getSucceeded()) { Line 345: return false; Line 353: // delete second image Line 354: // the next 'DeleteImageGroup' command should also take care of the image removal: Line 355: vdsRetValue = runVdsCommand( Line 356: VDSCommandType.DeleteImageGroup, Line 357: new DeleteImageGroupVDSCommandParameters(guids.get(1), ... and the same goes for this call. Line 358: guids.get(0), guids.get(4), postZero, false)); Line 359: Line 360: if (!vdsRetValue.getSucceeded()) { Line 361: if (startPollingTasks) { http://gerrit.ovirt.org/#/c/30706/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/CreateImagePlaceholderTaskHandler.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/CreateImagePlaceholderTaskHandler.java: Line 80: boolean wipeAfterDelete = ImagesHandler.shouldWipeAfterDelete( Line 81: DbFacade.getInstance().getDiskImageDao().get(getEnclosingCommand(). Line 82: getParameters().getDestinationImageId()).isWipeAfterDelete(), Line 83: DbFacade.getInstance().getStorageDomainDao().get(getEnclosingCommand(). Line 84: getParameters().getStorageDomainId()).getStorageType()); You have two DAO calls here, which is a shame. You can make one DAO call, get the DiskImage object, and pass its isWipeAfterDelete() and getStorageType() to ImagesHandler. Line 85: return new DeleteImageGroupVDSCommandParameters( Line 86: getEnclosingCommand().getParameters().getStoragePoolId(), Line 87: getEnclosingCommand().getParameters().getTargetStorageDomainId(), Line 88: getEnclosingCommand().getParameters().getImageGroupID(), http://gerrit.ovirt.org/#/c/30706/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/VmReplicateDiskFinishTaskHandler.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/VmReplicateDiskFinishTaskHandler.java: Line 139: boolean wipeAfterDelete = ImagesHandler.shouldWipeAfterDelete( Line 140: DbFacade.getInstance().getDiskImageDao().get(getEnclosingCommand(). Line 141: getParameters().getDestinationImageId()).isWipeAfterDelete(), Line 142: DbFacade.getInstance().getStorageDomainDao().get(getEnclosingCommand(). Line 143: getParameters().getStorageDomainId()).getStorageType()); See my comment in CreateImagePlaceholderTaskHandler - same problem here. Line 144: return new DeleteImageGroupVDSCommandParameters( Line 145: getEnclosingCommand().getParameters().getStoragePoolId(), Line 146: getEnclosingCommand().getParameters().getSourceStorageDomainId(), Line 147: getEnclosingCommand().getParameters().getImageGroupID(), http://gerrit.ovirt.org/#/c/30706/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/HibernationVolumesRemover.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/HibernationVolumesRemover.java: Line 12: private Guid vmId; Line 13: private String memoryState; Line 14: Line 15: public HibernationVolumesRemover(String memoryState, Guid vmId, Line 16: TaskHandlerCommand<?> enclosingCommand) { why? Line 17: this(memoryState, vmId, enclosingCommand, false); Line 18: } Line 19: Line 20: public HibernationVolumesRemover(String memoryState, Guid vmId, Line 18: } Line 19: Line 20: public HibernationVolumesRemover(String memoryState, Guid vmId, Line 21: TaskHandlerCommand<?> enclosingCommand, Line 22: boolean startPollingTasks) { why? Line 23: super(enclosingCommand, startPollingTasks); Line 24: this.memoryState = memoryState; Line 25: this.vmId = vmId; Line 26: } Line 49: enclosingCommand.getParameters().getParentCommand()); Line 50: } Line 51: Line 52: protected boolean isPostZero(List<Guid> guids) { Line 53: return isPostZero(getDiskDao().getAllForVm(vmId), getStorageDomainId(guids)); why stop caching? Line 54: } http://gerrit.ovirt.org/#/c/30706/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/MemoryImageRemover.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/MemoryImageRemover.java: Line 36: this.enclosingCommand = enclosingCommand; Line 37: } Line 38: Line 39: public MemoryImageRemover(TaskHandlerCommand<?> enclosingCommand, Line 40: boolean startPollingTasks) { why? Line 41: this(enclosingCommand); Line 42: this.startPollingTasks = startPollingTasks; Line 43: } Line 44: http://gerrit.ovirt.org/#/c/30706/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/MemoryImageRemoverFromExportDomain.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/MemoryImageRemoverFromExportDomain.java: Line 15: Line 16: public MemoryImageRemoverFromExportDomain(VM vm, Line 17: TaskHandlerCommand<?> enclosingCommand, Line 18: Guid storagePoolId, Line 19: Guid storageDomainId) { why? Line 20: super(enclosingCommand); Line 21: this.vm = vm; Line 22: this.storagePoolId = storagePoolId; Line 23: this.storageDomainId = storageDomainId; Line 46: guids.get(1), getStorageDomainId(guids), guids.get(4), isPostZero(guids), false); Line 47: } Line 48: Line 49: protected boolean isPostZero(List<Guid> guids) { Line 50: return isPostZero(vm.getDiskMap().values(), getStorageDomainId(guids)); why stop caching? Line 51: } http://gerrit.ovirt.org/#/c/30706/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/MemoryImageRemoverOnDataDomain.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/MemoryImageRemoverOnDataDomain.java: Line 16: private boolean removeOnlyIfNotUsedAtAll; Line 17: Line 18: public MemoryImageRemoverOnDataDomain(Guid vmId, Line 19: TaskHandlerCommand Line 20: <? extends RemoveMemoryVolumesParameters> enclosingCommand) { why? Line 21: super(enclosingCommand, false); Line 22: this.vmId = vmId; Line 23: removeOnlyIfNotUsedAtAll = enclosingCommand.getParameters().isRemoveOnlyIfNotUsedAtAll(); Line 24: } Line 39: guids.get(1), getStorageDomainId(guids), guids.get(4), isPostZero(guids), false); Line 40: } Line 41: Line 42: protected boolean isPostZero(List<Guid> guids) { Line 43: return isPostZero(getDiskDao().getAllForVm(vmId), getStorageDomainId(guids)); why stop caching? Line 44: } Line 45: Line 46: @Override Line 47: protected boolean isMemoryStateRemovable(String memoryVolume) { -- To view, visit http://gerrit.ovirt.org/30706 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie8a9932725ef861525f5102e8617938f0e0a71c7 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Idan Shaby <ish...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Idan Shaby <ish...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@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
