Vered Volansky has uploaded a new change for review. Change subject: core: Storage allocation validation fix on new VMs ......................................................................
core: Storage allocation validation fix on new VMs In the current situation, allocation checks are done on the template disk, which may be of the wrong storage type/format. Updating the image's data to support the allocation validations. Change-Id: I13f8585a04157a55528ad4331455c3f156bdb84b Bug-Url: https://bugzilla.redhat.com/1179688 Bug-Url: https://bugzilla.redhat.com/1178021 Bug-Url: https://bugzilla.redhat.com/1179690 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/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/main/java/org/ovirt/engine/core/bll/ImagesHandler.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/MultipleStorageDomainsValidator.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmCommandTest.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmPoolWithVmsCommandTest.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/CommonVmPoolWithVmsCommandTestAbstract.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateVmPoolWithVmsCommandTest.java 9 files changed, 111 insertions(+), 103 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/86/36686/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 24e2f33..e72ad45 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 @@ -380,9 +380,13 @@ return validate(storageDomainValidator.isDomainWithinThresholds()); } + /** + * This validation is for thin provisioning, when done differently on other commands, this method should be overridden. + */ protected boolean validateFreeSpace(StorageDomainValidator storageDomainValidator, List<DiskImage> disksList) { - return validate(storageDomainValidator.hasSpaceForNewDisks(disksList)); + Collection<DiskImage> disks = ImagesHandler.getDisksDummiesForStorageAllocations(disksList); + return validate(storageDomainValidator.hasSpaceForNewDisks(disks)); } protected boolean checkSingleQxlDisplay() { 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 60c6db0..a71555b 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,22 +5,17 @@ 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.businessentities.VolumeFormat; -import org.ovirt.engine.core.common.businessentities.VolumeType; import org.ovirt.engine.core.common.errors.VdcBllMessages; import org.ovirt.engine.core.common.validation.group.CreateEntity; import org.ovirt.engine.core.compat.Guid; @@ -124,43 +119,5 @@ protected List<Class<?>> getValidationGroups() { addValidationGroup(CreateEntity.class); return super.getValidationGroups(); - } - - @Override - public boolean checkDestDomains() { - return super.checkDestDomains() && validateSpaceRequirements(); - } - - protected boolean validateSpaceRequirements() { - int numOfVms = getParameters().getVmsCount(); - List<DiskImage> diskdummies = getDiskList(); - 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); - } - - private List<DiskImage> getDiskList() { - List<DiskImage> disksList = new ArrayList<>(); - for (DiskImage diskImage : diskInfoDestinationMap.values()) { - DiskImage clone = DiskImage.copyOf(diskImage); - // At this point the disks are the template's, which could have another volume type/format - // This change is for storage allocation validations, "real" override for these values is done in CreateSnapshotCommand. - clone.setVolumeType(VolumeType.Sparse); - clone.setvolumeFormat(VolumeFormat.COW); - disksList.add(clone); - } - return disksList; } } 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 8f78f5f..9909240 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 @@ -1,9 +1,11 @@ package org.ovirt.engine.core.bll; import java.util.ArrayList; +import java.util.Collection; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Set; import org.ovirt.engine.core.bll.context.CommandContext; import org.ovirt.engine.core.bll.job.ExecutionContext; @@ -14,6 +16,8 @@ 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.storage.MultipleStorageDomainsValidator; +import org.ovirt.engine.core.bll.validator.storage.StoragePoolValidator; import org.ovirt.engine.core.common.AuditLogType; import org.ovirt.engine.core.common.FeatureSupported; import org.ovirt.engine.core.common.VdcObjectType; @@ -374,7 +378,24 @@ } validDomains.add(domainId); } - return true; + + return validateSpaceRequirements(); + } + + protected boolean validateSpaceRequirements() { + int numOfVms = getParameters().getVmsCount(); + Collection<DiskImage> diskDummies = ImagesHandler.getDisksDummiesForStorageAllocations(diskInfoDestinationMap.values()); + Collection<DiskImage> disks = new ArrayList<>(numOfVms * diskDummies.size()); + // Number of added disks multiplies by the vms number + for (int i = 0; i < numOfVms; ++i) { + disks.addAll(diskDummies); + } + + Guid spId = getVmTemplate().getStoragePoolId(); + Set<Guid> sdIds = destStorages.keySet(); + MultipleStorageDomainsValidator storageDomainsValidator = getStorageDomainsValidator(spId, sdIds); + return validate(storageDomainsValidator.allDomainsWithinThresholds()) + && validate(storageDomainsValidator.allDomainsHaveSpaceForNewDisks(disks)); } private int getBlockSparseInitSizeInGB() { @@ -422,4 +443,8 @@ return list; } + 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/ImagesHandler.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java index 7dd7488..15cd687 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java @@ -855,4 +855,21 @@ description.put(ImagesHandler.DISK_DESCRIPTION, diskDescription != null ? diskDescription : ""); return JsonHelper.mapToJson(description, false); } + + + /** + * This method is used for storage allocation validations, where the disks are the template's, + * which could have another volume type/format than the target disk volume type/format, which is not yet created. + * "Real" override for these values is done in CreateSnapshotCommand, when creating the new DiskImages. + */ + public static Collection<DiskImage> getDisksDummiesForStorageAllocations(Collection<DiskImage> originalDisks) { + Collection<DiskImage> diskDummies = new HashSet<>(); + for (DiskImage diskImage : originalDisks) { + DiskImage clone = DiskImage.copyOf(diskImage); + clone.setVolumeType(VolumeType.Sparse); + clone.setvolumeFormat(VolumeFormat.COW); + diskDummies.add(clone); + } + return diskDummies; + } } diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/MultipleStorageDomainsValidator.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/MultipleStorageDomainsValidator.java index 7582cb5..5ffaf8e 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/MultipleStorageDomainsValidator.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/MultipleStorageDomainsValidator.java @@ -83,7 +83,7 @@ * Validates that all the domains have enough space for the request * @return {@link ValidationResult#VALID} if all the domains have enough free space, or a {@link ValidationResult} with the first low-on-space domain encountered. */ - public ValidationResult allDomainsHaveSpaceForNewDisks(List<DiskImage> disksList) { + public ValidationResult allDomainsHaveSpaceForNewDisks(Collection<DiskImage> disksList) { final Map<Guid, List<DiskImage>> disksMap = getDomainsDisksMap(disksList); return validOrFirstFailure(new ValidatorPredicate() { 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 4a000f4..e52cd68 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 @@ -535,12 +535,13 @@ } private static DiskImage createDiskImage(int size) { - DiskImage img = new DiskImage(); - img.setSizeInGigabytes(size); - img.setActualSize(size); - img.setId(Guid.newGuid()); - img.setStorageIds(new ArrayList<Guid>(Arrays.asList(STORAGE_DOMAIN_ID_1))); - return img; + DiskImage diskImage = new DiskImage(); + diskImage.setSizeInGigabytes(size); + diskImage.setActualSize(size); + diskImage.setId(Guid.newGuid()); + diskImage.setImageId(Guid.newGuid()); + diskImage.setStorageIds(new ArrayList<Guid>(Arrays.asList(STORAGE_DOMAIN_ID_1))); + return diskImage; } protected StorageDomain createStorageDomain(int availableSpace) { @@ -628,8 +629,7 @@ private static 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()); + DiskImage diskImage = createDiskImage(REQUIRED_DISK_SIZE_GB); disksList.add(diskImage); } return disksList; 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 5f17533..55f1539 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 @@ -1,14 +1,7 @@ package org.ovirt.engine.core.bll; -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; @@ -16,8 +9,6 @@ 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.errors.VdcBllMessages; -import org.ovirt.engine.core.compat.Guid; @RunWith(MockitoJUnitRunner.class) public class AddVmPoolWithVmsCommandTest extends CommonVmPoolWithVmsCommandTestAbstract { @@ -46,40 +37,6 @@ } @Test - public void validateSufficientSpaceOnDestinationDomains() { - setupForStorageTests(); - assertTrue(command.checkDestDomains()); - verify(multipleSdValidator).allDomainsWithinThresholds(); - verify(multipleSdValidator).allDomainsHaveSpaceForNewDisks(anyList()); - } - - @Test - 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 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 public void validatePatternBasedPoolName() { String patternBaseName = "aa-??bb"; command.getParameters().getVmStaticData().setName(patternBaseName); @@ -90,11 +47,5 @@ @Test 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()); } } diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/CommonVmPoolWithVmsCommandTestAbstract.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/CommonVmPoolWithVmsCommandTestAbstract.java index 400b58e..12a3eea 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/CommonVmPoolWithVmsCommandTestAbstract.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/CommonVmPoolWithVmsCommandTestAbstract.java @@ -1,8 +1,14 @@ package org.ovirt.engine.core.bll; +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.Matchers.eq; import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.ovirt.engine.core.utils.MockConfigRule.mockConfig; @@ -15,10 +21,12 @@ import org.junit.Before; import org.junit.Rule; +import org.junit.Test; import org.mockito.Matchers; import org.mockito.Mock; import org.mockito.MockitoAnnotations; import org.ovirt.engine.core.bll.interfaces.BackendInternal; +import org.ovirt.engine.core.bll.validator.storage.MultipleStorageDomainsValidator; import org.ovirt.engine.core.common.action.AddVmPoolWithVmsParameters; import org.ovirt.engine.core.common.businessentities.ArchitectureType; import org.ovirt.engine.core.common.businessentities.DiskImage; @@ -35,6 +43,7 @@ import org.ovirt.engine.core.common.businessentities.VmTemplate; import org.ovirt.engine.core.common.businessentities.network.VmNetworkInterface; import org.ovirt.engine.core.common.config.ConfigValues; +import org.ovirt.engine.core.common.errors.VdcBllMessages; import org.ovirt.engine.core.common.interfaces.VDSBrokerFrontend; import org.ovirt.engine.core.common.osinfo.OsRepository; import org.ovirt.engine.core.common.vdscommands.VDSCommandType; @@ -110,12 +119,49 @@ @Mock protected StorageDomainDAO storageDomainDAO; + @Mock + private MultipleStorageDomainsValidator multipleSdValidator; + /** * The command under test. */ protected CommonVmPoolWithVmsCommand<AddVmPoolWithVmsParameters> command; protected abstract CommonVmPoolWithVmsCommand<AddVmPoolWithVmsParameters> createCommand(); + + @Test + public void validateSufficientSpaceOnDestinationDomains() { + setupForStorageTests(); + assertTrue(command.checkDestDomains()); + verify(multipleSdValidator).allDomainsWithinThresholds(); + verify(multipleSdValidator).allDomainsHaveSpaceForNewDisks(anyList()); + } + + @Test + 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 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()); + } @Before public void setupMocks() { @@ -338,4 +384,11 @@ mockVmNetworkInterfaceDao(); mockStoragePoolDAO(); } + + + protected void setupForStorageTests() { + doReturn(multipleSdValidator).when(command).getStorageDomainsValidator(any(Guid.class), anySet()); + doReturn(ValidationResult.VALID).when(multipleSdValidator).allDomainsWithinThresholds(); + doReturn(ValidationResult.VALID).when(multipleSdValidator).allDomainsHaveSpaceForNewDisks(anyList()); + } } diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateVmPoolWithVmsCommandTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateVmPoolWithVmsCommandTest.java index c29eaf5..5c21820 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateVmPoolWithVmsCommandTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateVmPoolWithVmsCommandTest.java @@ -25,6 +25,7 @@ @Test public void validateCanDoAction() { mockVMPoolDAO(); + setupForStorageTests(); assertTrue(command.canDoAction()); } -- To view, visit http://gerrit.ovirt.org/36686 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I13f8585a04157a55528ad4331455c3f156bdb84b Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: ovirt-engine-3.5 Gerrit-Owner: Vered Volansky <vvola...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
