Allon Mureinik has posted comments on this change. Change subject: core: Verifying storage space for AddDiskCommand ......................................................................
Patch Set 2: Code-Review-1 (9 comments) The general notion is fine, there are a few implementation gaps that need fixing. .................................................... Commit Message Line 4: Commit: Vered Volansky <vvola...@redhat.com> Line 5: CommitDate: 2013-12-04 14:31:27 +0200 Line 6: Line 7: core: Verifying storage space for AddDiskCommand Line 8: I'd perhaps add a comment that this is the first in a series of patches to fix the storage validations throughout the commands. Line 9: Added hasSpaceForNewDisk(s) in StorageDomainValidator. Added test - Line 10: StorageDomainValidatorFreeSpaceTest. Line 11: Applied use in AddDiskCommand (former use is buggy). Line 12: Line 8: Line 9: Added hasSpaceForNewDisk(s) in StorageDomainValidator. Added test - Line 10: StorageDomainValidatorFreeSpaceTest. Line 11: Applied use in AddDiskCommand (former use is buggy). Line 12: I'd also add a paragraph on what you did in AddDiskCommandTest. Although the change is completely A-OK, it isn't straight forward, and you should explain that this patch turns in into a more correct unit test by treating the validator as a black box. Line 13: Bug-url: https://bugzilla.redhat.com/960934 Line 14: Change-Id: I1a33502683ec77fba09efffba1438beb552082f7 .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java Line 154 Line 155 Line 156 Line 157 Line 158 You removed the only call to hasFreeSpace(StorageDomain), so now it too should be removed. After removing that, you should also remove doesStorageDomainhaveSpaceForRequest(StorageDomain) and isStorageDomainWithinThresholds(StorageDomain). Having said that, I'm guessing we'd still want to keep the low disk space validation of isStorageDomainWithingThresholds IN ADDITION to the proper check you introduced. .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java Line 80: private static Integer getLowDiskSpaceThreshold() { Line 81: return Config.<Integer> GetValue(ConfigValues.FreeSpaceCriticalLowInGB); Line 82: } Line 83: Line 84: public ValidationResult hasSpaceForNewDisks(List<DiskImage> diskImages) { This declaration should be weakened to Collection<DiskImage>, or maybe even Iterable<DiskImage> Line 85: double availableSize = storageDomain.getAvailableDiskSizeInBytes(); Line 86: double totalSizeForDisks = 0.0; Line 87: Line 88: for (DiskImage diskImage : diskImages) { Line 104: return validateRequiredSpace(availableSize, totalSizeForDisks); Line 105: } Line 106: Line 107: public ValidationResult hasSpaceForNewDisk(DiskImage diskImage) { Line 108: return hasSpaceForNewDisks(Collections.singletonList(diskImage)); once the definition is weakened, this could be Collections.singleton(diskImage), which AFAIK is slightly more efficient. Line 109: } Line 110: Line 111: private ValidationResult validateRequiredSpace(double availableSize, double sizeToCompare) { Line 112: if (availableSize >= sizeToCompare) { Line 113: return ValidationResult.VALID; Line 114: } Line 115: Line 116: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_TARGET_STORAGE_DOMAIN, Line 117: storageName()); BTW: This is an extremely useless message. Lets remember to fix it up after we done with the entire series and move all the commands to this proper validation. Line 118: } Line 119: Line 120: public static Map<StorageDomain, Integer> getSpaceRequirementsForStorageDomains(Collection<DiskImage> images, Line 121: Map<Guid, StorageDomain> storageDomains, Map<Guid, DiskImage> imageToDestinationDomainMap) { .................................................... File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddDiskToVmCommandTest.java Line 365: when(snapshotsValidator.vmNotInPreview(any(Guid.class))).thenReturn(ValidationResult.VALID); Line 366: return snapshotsValidator; Line 367: } Line 368: Line 369: private StorageDomainValidator mockStorageDomainValidatorWithoutSpace() { This should be static. Line 370: StorageDomainValidator storageDomainValidator = mock(StorageDomainValidator.class); Line 371: when(storageDomainValidator.hasSpaceForNewDisk(any(DiskImage.class))).thenReturn( Line 372: new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_TARGET_STORAGE_DOMAIN)); Line 373: when(storageDomainValidator.isDomainExistAndActive()).thenReturn(ValidationResult.VALID); Line 373: when(storageDomainValidator.isDomainExistAndActive()).thenReturn(ValidationResult.VALID); Line 374: return storageDomainValidator; Line 375: } Line 376: Line 377: private StorageDomainValidator mockStorageDomainValidatorWithSpace() { This too Line 378: StorageDomainValidator storageDomainValidator = mock(StorageDomainValidator.class); Line 379: when(storageDomainValidator.hasSpaceForNewDisk(any(DiskImage.class))).thenReturn(ValidationResult.VALID); Line 380: when(storageDomainValidator.isDomainExistAndActive()).thenReturn(ValidationResult.VALID); Line 381: return storageDomainValidator; .................................................... File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/StorageDomainValidatorFreeSpaceTest.java Line 25: private boolean isValidForNew; Line 26: Line 27: public StorageDomainValidatorFreeSpaceTest(DiskImage disk, Line 28: StorageDomain sd, Line 29: boolean isValidForNew) { Are you sure this is formatted correctly? Line 30: this.disk = disk; Line 31: this.sd = sd; Line 32: this.isValidForNew = isValidForNew; Line 33: } -- To view, visit http://gerrit.ovirt.org/15377 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1a33502683ec77fba09efffba1438beb552082f7 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vered Volansky <vvola...@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: Sergey Gotliv <sgot...@redhat.com> Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com> 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
