Allon Mureinik has posted comments on this change. Change subject: core: Added VM memory validation on snapshot ......................................................................
Patch Set 3: Code-Review+1 (15 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 getDisksListForChecks() from inside the method? 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. 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? 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? 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 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 he doesn't pass null here. 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 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() 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 279: setUpGeneralValidations(); Line 280: setUpDiskValidations(); 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()); Can we use the real values (with the eq matcher) instead of just anyList()? Line 284: CanDoActionTestUtils.runAndAssertCanDoActionFailure(cmd, VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN); Line 285: verify(multipleStorageDomainsValidator).allDomainsHaveSpaceForAllDisks(anyList(), anyList()); Line 286: } Line 287: 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 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 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 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 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 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); :-( 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
