Vered Volansky has uploaded a new change for review. Change subject: core: AddVmPoolWithVmsCommand storage allocation ......................................................................
core: AddVmPoolWithVmsCommand storage allocation This patch is a part of a series of patches, adding storage allocation validations to the system when they're missing, and replacing old verification usage with unified, new, correct and tested verification. This patch did this for AddVmPoolWithVmsCommand, using only existing validations. Previously the storage allocation check was wrongfully in CommonVmPoolWithVmsCommand, and is now extracted to AddVmPoolWithVmsCommand only. Change-Id: I3120acf7eb5c71be6ade437522174a7bf62ea52a Bug-Url: https://bugzilla.redhat.com/1143888 Signed-off-by: Vered Volansky <[email protected]> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmPoolWithVmsCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommonVmPoolWithVmsCommand.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmPoolWithVmsCommandTest.java 3 files changed, 73 insertions(+), 28 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/94/33694/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmPoolWithVmsCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmPoolWithVmsCommand.java index a71555b..a051ef8 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmPoolWithVmsCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmPoolWithVmsCommand.java @@ -5,16 +5,19 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Set; import org.apache.commons.lang.StringUtils; import org.ovirt.engine.core.bll.quota.QuotaConsumptionParameter; import org.ovirt.engine.core.bll.quota.QuotaSanityParameter; import org.ovirt.engine.core.bll.quota.QuotaVdsDependent; import org.ovirt.engine.core.bll.utils.PermissionSubject; +import org.ovirt.engine.core.bll.validator.MultipleStorageDomainsValidator; import org.ovirt.engine.core.common.AuditLogType; import org.ovirt.engine.core.common.VdcObjectType; import org.ovirt.engine.core.common.action.AddVmPoolWithVmsParameters; import org.ovirt.engine.core.common.businessentities.ActionGroup; +import org.ovirt.engine.core.common.businessentities.DiskImage; import org.ovirt.engine.core.common.businessentities.VmPool; import org.ovirt.engine.core.common.errors.VdcBllMessages; import org.ovirt.engine.core.common.validation.group.CreateEntity; @@ -120,4 +123,27 @@ addValidationGroup(CreateEntity.class); return super.getValidationGroups(); } + + @Override + public boolean checkDestDomains() { + return super.checkDestDomains() && validateSpaceRequirements(); + } + + protected boolean validateSpaceRequirements() { + int numOfVms = getParameters().getVmsCount(); + List<DiskImage> disksList = new ArrayList<>(); + // Number of added disks multiplies by the vms number + for (int i = 0; i < numOfVms; ++i) { + disksList.addAll(diskInfoDestinationMap.values()); + } + Guid spId = getVmTemplate().getStoragePoolId(); + Set<Guid> sdIds = destStorages.keySet(); + MultipleStorageDomainsValidator storageDomainsValidator = getStorageDomainsValidator(spId, sdIds); + return validate(storageDomainsValidator.allDomainsWithinThresholds()) + && validate(storageDomainsValidator.allDomainsHaveSpaceForNewDisks(disksList)); + } + + protected MultipleStorageDomainsValidator getStorageDomainsValidator(Guid spId, Set<Guid> sdIds) { + return new MultipleStorageDomainsValidator(spId, sdIds); + } } diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommonVmPoolWithVmsCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommonVmPoolWithVmsCommand.java index f3d1a38..97bed22 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommonVmPoolWithVmsCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommonVmPoolWithVmsCommand.java @@ -14,7 +14,6 @@ import org.ovirt.engine.core.bll.quota.QuotaStorageConsumptionParameter; import org.ovirt.engine.core.bll.quota.QuotaStorageDependent; import org.ovirt.engine.core.bll.storage.StoragePoolValidator; -import org.ovirt.engine.core.bll.validator.StorageDomainValidator; import org.ovirt.engine.core.common.AuditLogType; import org.ovirt.engine.core.common.FeatureSupported; import org.ovirt.engine.core.common.VdcObjectType; @@ -305,7 +304,7 @@ if (!setAndValidateCpuProfile()) { return false; } - return checkFreeSpaceAndTypeOnDestDomains(); + return checkDestDomains(); } protected boolean verifyAddVM() { @@ -353,8 +352,7 @@ return true; } - public boolean checkFreeSpaceAndTypeOnDestDomains() { - boolean retValue = true; + public boolean checkDestDomains() { List<Guid> validDomains = new ArrayList<Guid>(); for (DiskImage diskImage : diskInfoDestinationMap.values()) { Guid domainId = diskImage.getStorageIds().get(0); @@ -364,29 +362,18 @@ StorageDomain domain = destStorages.get(domainId); if (domain == null) { domain = getStorageDomainDAO().getForStoragePool(domainId, getVmTemplate().getStoragePoolId()); + destStorages.put(domainId, domain); } - int numOfDisksOnDomain = 0; if (storageToDisksMap.containsKey(domainId)) { - numOfDisksOnDomain = storageToDisksMap.get(domainId).size(); - } - if (numOfDisksOnDomain > 0) { - if (domain.getStorageDomainType() == StorageDomainType.ImportExport) { - addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_TYPE_ILLEGAL); - retValue = false; - break; - } - if (!doesStorageDomainhaveSpaceForRequest(domain, numOfDisksOnDomain - * getBlockSparseInitSizeInGB() * getParameters().getVmsCount())) { - return false; + int numOfDisksOnDomain = storageToDisksMap.get(domainId).size(); + if (numOfDisksOnDomain > 0 + && (domain.getStorageDomainType() == StorageDomainType.ImportExport)) { + return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_TYPE_ILLEGAL); } } validDomains.add(domainId); } - return retValue; - } - - protected boolean doesStorageDomainhaveSpaceForRequest(StorageDomain storageDomain, long sizeRequested) { - return validate(new StorageDomainValidator(storageDomain).isDomainHasSpaceForRequest(sizeRequested)); + return true; } private int getBlockSparseInitSizeInGB() { diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmPoolWithVmsCommandTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmPoolWithVmsCommandTest.java index ad0362c..5f17533 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmPoolWithVmsCommandTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmPoolWithVmsCommandTest.java @@ -2,14 +2,28 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyList; +import static org.mockito.Matchers.anySet; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.runners.MockitoJUnitRunner; +import org.ovirt.engine.core.bll.validator.MultipleStorageDomainsValidator; import org.ovirt.engine.core.common.action.AddVmPoolWithVmsParameters; -import org.ovirt.engine.core.common.config.ConfigValues; import org.ovirt.engine.core.common.errors.VdcBllMessages; +import org.ovirt.engine.core.compat.Guid; +@RunWith(MockitoJUnitRunner.class) public class AddVmPoolWithVmsCommandTest extends CommonVmPoolWithVmsCommandTestAbstract { + + @Mock + private MultipleStorageDomainsValidator multipleSdValidator; @SuppressWarnings("serial") @Override @@ -27,30 +41,42 @@ @Test public void validateCanDoAction() { + setupForStorageTests(); assertTrue(command.canDoAction()); } @Test - public void validateFreeSpaceOnDestinationDomains() { - assertTrue(command.checkFreeSpaceAndTypeOnDestDomains()); + public void validateSufficientSpaceOnDestinationDomains() { + setupForStorageTests(); + assertTrue(command.checkDestDomains()); + verify(multipleSdValidator).allDomainsWithinThresholds(); + verify(multipleSdValidator).allDomainsHaveSpaceForNewDisks(anyList()); } @Test - public void validateMultiDisksWithNotEnoughSpaceOnDomains() { - mcr.mockConfigValue(ConfigValues.FreeSpaceCriticalLowInGB, 95); + public void validateInsufficientSpaceOnDomains() { + setupForStorageTests(); + doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN)). + when(multipleSdValidator).allDomainsHaveSpaceForNewDisks(anyList()); assertFalse(command.canDoAction()); assertTrue(command.getReturnValue() .getCanDoActionMessages() .contains(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN.toString())); + verify(multipleSdValidator).allDomainsWithinThresholds(); + verify(multipleSdValidator).allDomainsHaveSpaceForNewDisks(anyList()); } @Test - public void validateNoFreeSpaceOnDomains() { - mcr.mockConfigValue(ConfigValues.FreeSpaceCriticalLowInGB, 100); + public void validateDomainNotWithinThreshold() { + setupForStorageTests(); + doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN)). + when(multipleSdValidator).allDomainsWithinThresholds(); assertFalse(command.canDoAction()); assertTrue(command.getReturnValue() .getCanDoActionMessages() .contains(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN.toString())); + verify(multipleSdValidator).allDomainsWithinThresholds(); + verify(multipleSdValidator, never()).allDomainsHaveSpaceForNewDisks(anyList()); } @Test @@ -65,4 +91,10 @@ public void validateBeanValidations() { assertTrue(command.validateInputs()); } + + private void setupForStorageTests() { + doReturn(multipleSdValidator).when((AddVmPoolWithVmsCommand) command).getStorageDomainsValidator(any(Guid.class), anySet()); + doReturn(ValidationResult.VALID).when(multipleSdValidator).allDomainsWithinThresholds(); + doReturn(ValidationResult.VALID).when(multipleSdValidator).allDomainsHaveSpaceForNewDisks(anyList()); + } } -- To view, visit http://gerrit.ovirt.org/33694 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I3120acf7eb5c71be6ade437522174a7bf62ea52a Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: ovirt-engine-3.5 Gerrit-Owner: Vered Volansky <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
