Vered Volansky has posted comments on this change. Change subject: core: Added VM memory validation on snapshot ......................................................................
Patch Set 3: (14 comments) http://gerrit.ovirt.org/#/c/30040/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java: Line 149: if (!validate(sdValidator.allDomainsWithinThresholds())) { Line 150: return false; Line 151: } Line 152: List<DiskImage> clonedDisksList = getMemoryImageBuilder().getDisksToBeCreated(); Line 153: return validate(sdValidator.allDomainsHaveSpaceForAllDisks(newDisksList, clonedDisksList)); > why is the newDisksList a parameter? Why not just call getDisksListForCheck Done Line 154: } Line 155: Line 156: protected MemoryImageBuilder getMemoryImageBuilder() { Line 157: if (null == memoryBuilder) { Line 153: return validate(sdValidator.allDomainsHaveSpaceForAllDisks(newDisksList, clonedDisksList)); Line 154: } Line 155: Line 156: protected MemoryImageBuilder getMemoryImageBuilder() { Line 157: if (null == memoryBuilder) { > I'd flip the comparison - if (memoryBuilder == null) - seems more readable. Done Line 158: memoryBuilder = createMemoryImageBuilder(); Line 159: } Line 160: return memoryBuilder; Line 161: } Line 503: && validate(sdValidator.allDomainsExistAndActive()))) { Line 504: return false; Line 505: } Line 506: } Line 507: if (!validateSpaceRequirements(sdValidator, disksList)) { > Shouldn't this be inside the diskList.size() > 0 if? No, this was extracted in order to perform the validation in case there are no disks in the the disksList ("new" validation), yet there are memory disks ("clone" validation). Line 508: return false; Line 509: } Line 510: Line 511: if (getParameters().isSaveMemory() && Guid.Empty.equals(getStorageDomainIdForVmMemory())) { http://gerrit.ovirt.org/#/c/30040/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/LiveSnapshotMemoryImageBuilder.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/LiveSnapshotMemoryImageBuilder.java: Line 119: enclosingCommand.getTaskIdList().add(guid); Line 120: } Line 121: Line 122: private VolumeType getVolumeTypeForDomain() { Line 123: if (null == volumeTypeForDomain) { > here too - why is the null on the left side? c++ assignment bug avoidance habit. Will change if it bothers. Line 124: StorageDomainStatic sdStatic = DbFacade.getInstance().getStorageDomainStaticDao().get(storageDomainId); Line 125: volumeTypeForDomain = HibernateVmCommand.getMemoryVolumeTypeForStorageDomain(sdStatic.getStorageType()); Line 126: } Line 127: return volumeTypeForDomain; http://gerrit.ovirt.org/#/c/30040/3/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 107: private static Integer getLowDiskSpaceThreshold() { Line 108: return Config.<Integer> getValue(ConfigValues.FreeSpaceCriticalLowInGB); Line 109: } Line 110: Line 111: private double getTotalSizeForNewDisks(Collection<DiskImage> diskImages) { > should be static storageDomain is a non-static field. Line 112: double totalSizeForDisks = 0.0; Line 113: if (null != diskImages) { Line 114: for (DiskImage diskImage : diskImages) { Line 115: double sizeForDisk = diskImage.getSize(); Line 109: } Line 110: Line 111: private double getTotalSizeForNewDisks(Collection<DiskImage> diskImages) { Line 112: double totalSizeForDisks = 0.0; Line 113: if (null != diskImages) { > we shouldn't need this here - it's the caller's responsibility to make sure Caller is MultipleStorageDomianValidator. Since it operates on two lists - disks and memory. The lists are extracted from a map according to sdIds. If for one of the SDs there are no disks - there's no value in the map, and null is returned and passed here. If we pass the responsibility there there will be a lot more checks there, and then an empty list will have to be artificially passed here, which is worse than the current situation. Line 114: for (DiskImage diskImage : diskImages) { Line 115: double sizeForDisk = diskImage.getSize(); Line 116: Line 117: if (diskImage.getVolumeFormat() == VolumeFormat.COW) { Line 130: } Line 131: Line 132: private double getTotalSizeForClonedDisks(Collection<DiskImage> diskImages) { Line 133: double totalSizeForDisks = 0.0; Line 134: if (null != diskImages) { > here too yep, here too. Line 135: for (DiskImage diskImage : diskImages) { Line 136: double diskCapacity = diskImage.getSize(); Line 137: double sizeForDisk = diskCapacity; Line 138: http://gerrit.ovirt.org/#/c/30040/3/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommandTest.java: Line 277: @Test Line 278: public void testAllDomainsHaveSpaceForAllDisksFailure() { Line 279: setUpGeneralValidations(); Line 280: setUpDiskValidations(); Line 281: doReturn(Collections.EMPTY_LIST).when(cmd).getDisksList(); > Please use the generics-friendly Collections.emptyList() Done Line 282: doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN)).when(multipleStorageDomainsValidator) Line 283: .allDomainsHaveSpaceForAllDisks(anyList(), anyList()); Line 284: CanDoActionTestUtils.runAndAssertCanDoActionFailure(cmd, VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN); Line 285: verify(multipleStorageDomainsValidator).allDomainsHaveSpaceForAllDisks(anyList(), anyList()); Line 281: doReturn(Collections.EMPTY_LIST).when(cmd).getDisksList(); Line 282: doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN)).when(multipleStorageDomainsValidator) Line 283: .allDomainsHaveSpaceForAllDisks(anyList(), anyList()); Line 284: CanDoActionTestUtils.runAndAssertCanDoActionFailure(cmd, VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN); Line 285: verify(multipleStorageDomainsValidator).allDomainsHaveSpaceForAllDisks(anyList(), anyList()); > see above Done Line 286: } Line 287: Line 288: @Test Line 289: public void testAllDomainsHaveSpaceForAllDisksSuccess() { Line 288: @Test Line 289: public void testAllDomainsHaveSpaceForAllDisksSuccess() { Line 290: setUpGeneralValidations(); Line 291: setUpDiskValidations(); Line 292: doReturn(Collections.EMPTY_LIST).when(cmd).getDisksList(); > see above Done Line 293: doReturn(ValidationResult.VALID).when(multipleStorageDomainsValidator).allDomainsHaveSpaceForAllDisks(anyList(), anyList()); Line 294: CanDoActionTestUtils.runAndAssertCanDoActionSuccess(cmd); Line 295: verify(multipleStorageDomainsValidator).allDomainsHaveSpaceForAllDisks(anyList(), anyList()); Line 296: } Line 289: public void testAllDomainsHaveSpaceForAllDisksSuccess() { Line 290: setUpGeneralValidations(); Line 291: setUpDiskValidations(); Line 292: doReturn(Collections.EMPTY_LIST).when(cmd).getDisksList(); Line 293: doReturn(ValidationResult.VALID).when(multipleStorageDomainsValidator).allDomainsHaveSpaceForAllDisks(anyList(), anyList()); > see above Actually this is redundant, as it appears in setUpDiskValidations(). Line 294: CanDoActionTestUtils.runAndAssertCanDoActionSuccess(cmd); Line 295: verify(multipleStorageDomainsValidator).allDomainsHaveSpaceForAllDisks(anyList(), anyList()); Line 296: } Line 297: Line 291: setUpDiskValidations(); Line 292: doReturn(Collections.EMPTY_LIST).when(cmd).getDisksList(); Line 293: doReturn(ValidationResult.VALID).when(multipleStorageDomainsValidator).allDomainsHaveSpaceForAllDisks(anyList(), anyList()); Line 294: CanDoActionTestUtils.runAndAssertCanDoActionSuccess(cmd); Line 295: verify(multipleStorageDomainsValidator).allDomainsHaveSpaceForAllDisks(anyList(), anyList()); > see above Done Line 296: } Line 297: Line 298: private void setUpDiskValidations() { Line 299: doReturn(ValidationResult.VALID).when(diskImagesValidator).diskImagesNotLocked(); Line 299: doReturn(ValidationResult.VALID).when(diskImagesValidator).diskImagesNotLocked(); Line 300: doReturn(ValidationResult.VALID).when(diskImagesValidator).diskImagesNotIllegal(); Line 301: doReturn(ValidationResult.VALID).when(multipleStorageDomainsValidator).allDomainsExistAndActive(); Line 302: doReturn(ValidationResult.VALID).when(multipleStorageDomainsValidator).allDomainsWithinThresholds(); Line 303: doReturn(ValidationResult.VALID).when(multipleStorageDomainsValidator).allDomainsHaveSpaceForAllDisks(anyList(), anyList()); > see above Not applicable here since this is general for all tests. Line 304: } Line 305: Line 306: private void setUpGeneralValidations() { Line 307: doReturn(Boolean.TRUE).when(cmd).validateVM(vmValidator); http://gerrit.ovirt.org/#/c/30040/3/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/MultipleStorageDomainsValidatorTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/MultipleStorageDomainsValidatorTest.java: Line 149: } Line 150: Line 151: private List<DiskImage> generateDisksList(int size, List<Guid> sdIds) { Line 152: List<DiskImage> disksList = new ArrayList<>(); Line 153: ArrayList<Guid> _sdIds = new ArrayList<>(sdIds); > :-( I know, but I only had to do this for the !#$$@!#$ test, and is not worth my time thinking about an appropriate other name that will also indicate there is no difference. If it's worth your time please suggest a *good* name :) Line 154: for (int i = 0; i < size; ++i) { Line 155: DiskImage diskImage = new DiskImage(); Line 156: diskImage.setImageId(Guid.newGuid()); Line 157: diskImage.setStorageIds(_sdIds); -- To view, visit http://gerrit.ovirt.org/30040 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8fc127147bdea8737fecb034c88079521b8a8bd6 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vered Volansky <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Vered Volansky <[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
