Idan Shaby has posted comments on this change. Change subject: core: WAD should be Ignored on File Domain Disks ......................................................................
Patch Set 1: (15 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 someth Done 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); > Regardless - you're COPYING a disk. WAD should be calculated according to t Done 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/ Done 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 @ret Done 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/ Done 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 Done 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? Done 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/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. Done 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/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. Done 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. Done 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? Done 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? Done Line 23: super(enclosingCommand, startPollingTasks); Line 24: this.memoryState = memoryState; Line 25: this.vmId = vmId; Line 26: } 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? Done 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? Done Line 20: super(enclosingCommand); Line 21: this.vm = vm; Line 22: this.storagePoolId = storagePoolId; Line 23: this.storageDomainId = storageDomainId; 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? Done Line 21: super(enclosingCommand, false); Line 22: this.vmId = vmId; Line 23: removeOnlyIfNotUsedAtAll = enclosingCommand.getParameters().isRemoveOnlyIfNotUsedAtAll(); Line 24: } -- 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
