Vered Volansky has uploaded a new change for review. Change subject: core: Reorganize sd space validations in AddVm ......................................................................
core: Reorganize sd space validations in AddVm This is the first of two patches meant to fix faulty storage space allocations validations in the different add VM commands. This patch keeps the current (faulty) validations and reorganizes them in methods that can be overridden in the derived classes when needed. It also adds two new threshold tests - verifying the AddVmCommand behaviour to the simulated StorageDomainValidator.isDomainWithinThresholds . Change-Id: I482709d7edb0333a6aa7b9ab723eea7f4c5a00d3 Related-to: https://bugzilla.redhat.com/show_bug.cgi?id=917682 https://bugzilla.redhat.com/show_bug.cgi?id=1053742 Signed-off-by: Vered Volansky <vvola...@redhat.com> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmCommandTest.java 2 files changed, 100 insertions(+), 29 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/33/26733/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java index 457fa7f..40619b7 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java @@ -263,40 +263,34 @@ protected boolean canAddVm(ArrayList<String> reasons, Collection<StorageDomain> destStorages) { VmStatic vmStaticFromParams = getParameters().getVmStaticData(); - boolean returnValue = + boolean canAddVm = canAddVm(reasons, vmStaticFromParams.getName(), getStoragePoolId(), vmStaticFromParams.getPriority()); - if (returnValue) { + if (canAddVm) { List<ValidationError> validationErrors = validateCustomProperties(vmStaticFromParams); if (!validationErrors.isEmpty()) { VmPropertiesUtils.getInstance().handleCustomPropertiesError(validationErrors, reasons); - returnValue = false; + return false; } } // check that template image and vm are on the same storage pool - if (returnValue - && shouldCheckSpaceInStorageDomains()) { + if (shouldCheckSpaceInStorageDomains()) { if (!getStoragePoolId().equals(getStoragePoolIdFromSourceImageContainer())) { reasons.add(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_POOL_NOT_MATCH.toString()); - returnValue = false; - } else { - for (StorageDomain domain : destStorages) { - StorageDomainValidator storageDomainValidator = new StorageDomainValidator(domain); - if (!validate(storageDomainValidator.isDomainExistAndActive())) { - return false; - } - if (!validate(storageDomainValidator.isDomainWithinThresholds()) - || !validate(storageDomainValidator.isDomainHasSpaceForRequest(getNeededDiskSize(domain.getId())))) { - return false; - } - } + return false; + } + for (StorageDomain domain : destStorages) { + StorageDomainValidator storageDomainValidator = new StorageDomainValidator(domain); + if (!validate(storageDomainValidator.isDomainExistAndActive())) { + return false; + } + } + if (!validateSpaceRequirements()) { + return false; } } - if (returnValue) { - returnValue = isDedicatedVdsOnSameCluster(vmStaticFromParams); - } - return returnValue; + return isDedicatedVdsOnSameCluster(vmStaticFromParams); } protected boolean shouldCheckSpaceInStorageDomains() { @@ -337,6 +331,35 @@ return returnValue; } + /** + * Check if destination storage has enough space + * @return + */ + protected boolean validateSpaceRequirements() { + for (Map.Entry<Guid, List<DiskImage>> sdImageEntry : storageToDisksMap.entrySet()) { + StorageDomain destStorageDomain = destStorages.get(sdImageEntry.getKey()); + StorageDomainValidator storageDomainValidator = createStorageDomainValidator(destStorageDomain); + if (!validateDomainsThreshold(storageDomainValidator) || + !validateFreeSpace(storageDomainValidator, destStorageDomain)) { + return false; + } + } + return true; + } + + protected StorageDomainValidator createStorageDomainValidator(StorageDomain storageDomain) { + return new StorageDomainValidator(storageDomain); + } + + private boolean validateDomainsThreshold(StorageDomainValidator storageDomainValidator){ + return validate(storageDomainValidator.isDomainWithinThresholds()); + } + + protected boolean validateFreeSpace(StorageDomainValidator storageDomainValidator, StorageDomain domain) + { + return validate(storageDomainValidator.isDomainHasSpaceForRequest(getNeededDiskSize(domain.getId()))); + } + protected boolean checkSingleQxlDisplay() { if (!getParameters().getVmStaticData().getSingleQxlPci()) { return true; diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmCommandTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmCommandTest.java index a7a19a5..1c8f84f 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmCommandTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmCommandTest.java @@ -8,6 +8,7 @@ import static org.mockito.Matchers.anyString; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.when; @@ -28,6 +29,7 @@ import org.mockito.stubbing.Answer; import org.ovirt.engine.core.bll.interfaces.BackendInternal; import org.ovirt.engine.core.bll.snapshots.SnapshotsValidator; +import org.ovirt.engine.core.bll.validator.StorageDomainValidator; import org.ovirt.engine.core.common.action.AddVmFromSnapshotParameters; import org.ovirt.engine.core.common.action.AddVmFromTemplateParameters; import org.ovirt.engine.core.common.action.VmManagementParametersBase; @@ -64,6 +66,10 @@ @SuppressWarnings("serial") public class AddVmCommandTest { + private static final Guid STORAGE_DOMAIN_ID_1 = Guid.newGuid(); + private static final Guid STORAGE_DOMAIN_ID_2 = Guid.newGuid(); + private static final int NUM_OF_DISKS_1 = 3; + private static final int NUM_OF_DISKS_2 = 3; private static final int REQUIRED_DISK_SIZE_GB = 10; private static final int AVAILABLE_SPACE_GB = 11; private static final int USED_SPACE_GB = 4; @@ -72,6 +78,7 @@ private static final Guid STORAGE_DOMAIN_ID = Guid.newGuid(); private VmTemplate vmTemplate = null; private VDSGroup vdsGroup = null; + private StorageDomainValidator storageDomainValidator; @Rule public MockConfigRule mcr = new MockConfigRule(); @@ -216,6 +223,23 @@ assertTrue("isVirtioScsiEnabled hasn't been defaulted to true on cluster >= 3.3.", cmd.isVirtioScsiEnabled()); } + @Test + public void ValidateSpaceAndThreshold() { + AddVmCommand<VmManagementParametersBase> command = setupCanAddVmTests(0, 0); + assertTrue(command.validateSpaceRequirements()); + } + + @Test + public void ValidateSpaceNotWithinThreshold() throws Exception { + AddVmCommand<VmManagementParametersBase> command = setupCanAddVmTests(0, 0); + storageDomainValidator = mock(StorageDomainValidator.class); + doReturn((new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_TARGET_STORAGE_DOMAIN))). + when(storageDomainValidator).isDomainWithinThresholds(); + doReturn(storageDomainValidator).when(command).createStorageDomainValidator(any(StorageDomain.class)); + assertFalse(command.validateSpaceRequirements()); + } + + private void mockDisplayTypes(int osId, Version clusterVersion) { Map<Integer, Map<Version, List<DisplayType>>> displayTypeMap = new HashMap<>(); displayTypeMap.put(osId, new HashMap<Version, List<DisplayType>>()); @@ -291,14 +315,6 @@ mockDAOs(cmd); doReturn(snapshotDao).when(cmd).getSnapshotDao(); mockBackend(cmd); - return cmd; - } - - private AddVmFromTemplateCommand<AddVmFromTemplateParameters> setupCanAddVmFromTemplateTests(final int domainSizeGB, - final int sizeRequired) { - VM vm = initializeMock(domainSizeGB, sizeRequired); - AddVmFromTemplateCommand<AddVmFromTemplateParameters> cmd = createVmFromTemplateCommand(vm); - initCommandMethods(cmd); return cmd; } @@ -506,9 +522,41 @@ mockDAOs(cmd); mockBackend(cmd); doReturn(new VDSGroup()).when(cmd).getVdsGroup(); + generateStorageToDisksMap(cmd); + initDestSDs(cmd); + storageDomainValidator = mock(StorageDomainValidator.class); + doReturn(ValidationResult.VALID).when(storageDomainValidator).isDomainWithinThresholds(); + doReturn(ValidationResult.VALID).when(storageDomainValidator).isDomainHasSpaceForRequest(any(int.class)); + doReturn(storageDomainValidator).when(cmd).createStorageDomainValidator(any(StorageDomain.class)); return cmd; } + private void generateStorageToDisksMap(AddVmCommand<VmManagementParametersBase> command) { + command.storageToDisksMap = new HashMap<Guid, List<DiskImage>>(); + command.storageToDisksMap.put(STORAGE_DOMAIN_ID_1, generateDisksList(NUM_OF_DISKS_1)); + command.storageToDisksMap.put(STORAGE_DOMAIN_ID_2, generateDisksList(NUM_OF_DISKS_2)); + } + + private List<DiskImage> generateDisksList(int size) { + List<DiskImage> disksList = new ArrayList<>(); + for (int i = 0; i < size; ++i) { + DiskImage diskImage = new DiskImage(); + diskImage.setImageId(Guid.newGuid()); + disksList.add(diskImage); + } + return disksList; + } + + private void initDestSDs(AddVmCommand<VmManagementParametersBase> command) { + StorageDomain sd1 = new StorageDomain(); + StorageDomain sd2 = new StorageDomain(); + sd1.setId(STORAGE_DOMAIN_ID_1); + sd2.setId(STORAGE_DOMAIN_ID_2); + command.destStorages.put(STORAGE_DOMAIN_ID_1, sd1); + command.destStorages.put(STORAGE_DOMAIN_ID_2, sd2); + } + + private <T extends VmManagementParametersBase> void mockUninterestingMethods(AddVmCommand<T> spy) { doReturn(true).when(spy).isVmNameValidLength(Matchers.<VM> any(VM.class)); doReturn(false).when(spy).isVmWithSameNameExists(anyString()); -- To view, visit http://gerrit.ovirt.org/26733 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I482709d7edb0333a6aa7b9ab723eea7f4c5a00d3 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vered Volansky <vvola...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
