Allon Mureinik has posted comments on this change. Change subject: core: Fix AddVm faulty storage allocation checks ......................................................................
Patch Set 1: Code-Review-1 (17 comments) http://gerrit.ovirt.org/#/c/26734/1//COMMIT_MSG Commit Message: Line 8: Line 9: Fix AddVmCommand and its descendants storage allocation checks. Line 10: Amended and added relevant tests. Line 11: Note that the tests here rely on the fact that StorageDomainValidator is Line 12: well tested, and only simulate it's output, then testing the commands s/it's/its/ Line 13: according to it. Line 14: Line 15: Change-Id: Iff4ad246934b3b94f21ae602067033347c913780 Line 16: Bug-Url: https://bugzilla.redhat.com/917682 http://gerrit.ovirt.org/#/c/26734/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 299: return super.buildAndCheckDestStorageDomains(); Line 300: } Line 301: Line 302: protected boolean validateFreeSpace(StorageDomainValidator storageDomainValidator, List<DiskImage> disksList) Line 303: { please take it up a line Line 304: for (DiskImage diskImage : disksList) { Line 305: List<DiskImage> snapshots = getAllImageSnapshots(diskImage); Line 306: diskImage.getSnapshots().addAll(snapshots); Line 307: } http://gerrit.ovirt.org/#/c/26734/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java: Line 138: return retValue; Line 139: } Line 140: Line 141: protected boolean validateFreeSpace(StorageDomainValidator storageDomainValidator, List<DiskImage> disksList) Line 142: { please take it up a line Line 143: for (DiskImage diskImage : disksList) { Line 144: List<DiskImage> snapshots = getAllImageSnapshots(diskImage); Line 145: diskImage.getSnapshots().addAll(snapshots); Line 146: } http://gerrit.ovirt.org/#/c/26734/1/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmCommandTest.java: Line 136: assertTrue("canDoAction failed for the wrong reason", Line 137: cmd.getReturnValue() Line 138: .getCanDoActionMessages() Line 139: .contains(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_TARGET_STORAGE_DOMAIN.toString())); Line 140: please remove this empty line Line 141: } Line 142: Line 143: private void mockOsRepository() { Line 144: SimpleDependecyInjector.getInstance().bind(OsRepository.class, osRepository); Line 233: @Test Line 234: public void validateSpaceAndThreshold() { Line 235: AddVmCommand<VmManagementParametersBase> command = setupCanAddVmTests(0, 0); Line 236: doReturn(ValidationResult.VALID).when(storageDomainValidator).isDomainWithinThresholds(); Line 237: doReturn(ValidationResult.VALID).when(storageDomainValidator).hasSpaceForNewDisks(any(List.class)); s/any(List.class)/anyList/ Line 238: doReturn(storageDomainValidator).when(command).createStorageDomainValidator(any(StorageDomain.class)); Line 239: assertTrue(command.validateSpaceRequirements()); Line 240: verify(storageDomainValidator, times(TOTAL_NUM_DOMAINS)).hasSpaceForNewDisks(any(List.class)); Line 241: verify(storageDomainValidator, never()).hasSpaceForClonedDisks(any(List.class)); Line 236: doReturn(ValidationResult.VALID).when(storageDomainValidator).isDomainWithinThresholds(); Line 237: doReturn(ValidationResult.VALID).when(storageDomainValidator).hasSpaceForNewDisks(any(List.class)); Line 238: doReturn(storageDomainValidator).when(command).createStorageDomainValidator(any(StorageDomain.class)); Line 239: assertTrue(command.validateSpaceRequirements()); Line 240: verify(storageDomainValidator, times(TOTAL_NUM_DOMAINS)).hasSpaceForNewDisks(any(List.class)); s/any(List.class)/anyList/ Line 241: verify(storageDomainValidator, never()).hasSpaceForClonedDisks(any(List.class)); Line 242: } Line 243: Line 244: @Test Line 237: doReturn(ValidationResult.VALID).when(storageDomainValidator).hasSpaceForNewDisks(any(List.class)); Line 238: doReturn(storageDomainValidator).when(command).createStorageDomainValidator(any(StorageDomain.class)); Line 239: assertTrue(command.validateSpaceRequirements()); Line 240: verify(storageDomainValidator, times(TOTAL_NUM_DOMAINS)).hasSpaceForNewDisks(any(List.class)); Line 241: verify(storageDomainValidator, never()).hasSpaceForClonedDisks(any(List.class)); s/any(List.class)/anyList/ Line 242: } Line 243: Line 244: @Test Line 245: public void validateSpaceNotEnough() throws Exception { Line 245: public void validateSpaceNotEnough() throws Exception { Line 246: AddVmCommand<VmManagementParametersBase> command = setupCanAddVmTests(0, 0); Line 247: doReturn(ValidationResult.VALID).when(storageDomainValidator).isDomainWithinThresholds(); Line 248: doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_TARGET_STORAGE_DOMAIN)). Line 249: when(storageDomainValidator).hasSpaceForNewDisks(any(List.class)); s/any(List.class)/anyList/ Line 250: doReturn(storageDomainValidator).when(command).createStorageDomainValidator(any(StorageDomain.class)); Line 251: assertFalse(command.validateSpaceRequirements()); Line 252: verify(storageDomainValidator).hasSpaceForNewDisks(any(List.class)); Line 253: verify(storageDomainValidator, never()).hasSpaceForClonedDisks(any(List.class)); Line 248: doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_TARGET_STORAGE_DOMAIN)). Line 249: when(storageDomainValidator).hasSpaceForNewDisks(any(List.class)); Line 250: doReturn(storageDomainValidator).when(command).createStorageDomainValidator(any(StorageDomain.class)); Line 251: assertFalse(command.validateSpaceRequirements()); Line 252: verify(storageDomainValidator).hasSpaceForNewDisks(any(List.class)); s/any(List.class)/anyList/ Line 253: verify(storageDomainValidator, never()).hasSpaceForClonedDisks(any(List.class)); Line 254: } Line 255: Line 256: @Test Line 249: when(storageDomainValidator).hasSpaceForNewDisks(any(List.class)); Line 250: doReturn(storageDomainValidator).when(command).createStorageDomainValidator(any(StorageDomain.class)); Line 251: assertFalse(command.validateSpaceRequirements()); Line 252: verify(storageDomainValidator).hasSpaceForNewDisks(any(List.class)); Line 253: verify(storageDomainValidator, never()).hasSpaceForClonedDisks(any(List.class)); s/any(List.class)/anyList/ Line 254: } Line 255: Line 256: @Test Line 257: public void validateSpaceNotWithinThreshold() throws Exception { Line 352: return cmd; Line 353: } Line 354: Line 355: private void initializeVmDAOMock(VM vm) { Line 356: when(vmDAO.get(Matchers.<Guid>any(Guid.class))).thenReturn(vm); why? Line 357: } Line 358: Line 359: private AddVmCommand<VmManagementParametersBase> setupCanAddVmTests(final int domainSizeGB, Line 360: final int sizeRequired) { Line 423: when(vmTemplateDAO.get(Matchers.<Guid> any(Guid.class))).thenReturn(createVmTemplate()); Line 424: } Line 425: Line 426: private void mockVdsGroupDAOReturnVdsGroup() { Line 427: when(vdsGroupDao.get(Matchers.<Guid>any(Guid.class))).thenReturn(createVdsGroup()); why? Line 428: } Line 429: Line 430: private VmTemplate createVmTemplate() { Line 431: if (vmTemplate == null) { Line 555: command.storageToDisksMap.put(STORAGE_DOMAIN_ID_1, generateDisksList(NUM_DISKS_STORAGE_DOMAIN_1)); Line 556: command.storageToDisksMap.put(STORAGE_DOMAIN_ID_2, generateDisksList(NUM_DISKS_STORAGE_DOMAIN_2)); Line 557: } Line 558: Line 559: static private List<DiskImage> generateDisksList(int size) { private static Line 560: List<DiskImage> disksList = new ArrayList<>(); Line 561: for (int i = 0; i < size; ++i) { Line 562: DiskImage diskImage = new DiskImage(); Line 563: diskImage.setImageId(Guid.newGuid()); Line 588: } Line 589: return disksList; Line 590: } Line 591: Line 592: protected void mockGetAllSnapshots(AddVmFromTemplateCommand<AddVmFromTemplateParameters> command) { should be private Line 593: doAnswer(new Answer<List<DiskImage>>() { Line 594: @Override Line 595: public List<DiskImage> answer(InvocationOnMock invocation) throws Throwable { Line 596: Object[] args = invocation.getArguments(); http://gerrit.ovirt.org/#/c/26734/1/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmFromSnapshotCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmFromSnapshotCommandTest.java: Line 28: import org.ovirt.engine.core.compat.Guid; Line 29: Line 30: @RunWith(MockitoJUnitRunner.class) Line 31: public class AddVmFromSnapshotCommandTest extends AddVmCommandTest{ Line 32: for the entire class: s/any(List.class)/anyList()/ Line 33: /** Line 34: * The command under test. Line 35: */ Line 36: protected AddVmFromSnapshotCommand<AddVmFromSnapshotParameters> command; http://gerrit.ovirt.org/#/c/26734/1/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommandTest.java: Line 1: package org.ovirt.engine.core.bll; Line 2: Line 3: import static junit.framework.Assert.assertFalse; Line 4: import static junit.framework.Assert.assertTrue; Line 5: import static org.mockito.Matchers.any; General: use anyList instead of any Line 6: import static org.mockito.Mockito.doAnswer; Line 7: import static org.mockito.Mockito.doReturn; Line 8: import static org.mockito.Mockito.mock; Line 9: import static org.mockito.Mockito.never; Line 27: import org.ovirt.engine.core.common.errors.VdcBllMessages; Line 28: import org.ovirt.engine.core.compat.Guid; Line 29: Line 30: @RunWith(MockitoJUnitRunner.class) Line 31: public class AddVmFromTemplateCommandTest extends AddVmCommandTest{ add space before "{" Line 32: Line 33: /** Line 34: * The command under test. Line 35: */ -- To view, visit http://gerrit.ovirt.org/26734 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iff4ad246934b3b94f21ae602067033347c913780 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vered Volansky <vvola...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@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
