Vered Volansky has uploaded a new change for review. Change subject: core: AddVmTemplateCommand storage allocation ......................................................................
core: AddVmTemplateCommand 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 AddVmTemplateCommand, using only existing validations. Removing old verification in this command resulted in unused old validation and validation aids, which were also removed in this patch. Change-Id: I7980510a7bb72e43e0a9dfc18460207386eb62fe Bug-Url: https://bugzilla.redhat.com/1053744 Signed-off-by: Vered Volansky <vvola...@redhat.com> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmTemplateCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmTemplateCommandTest.java 3 files changed, 117 insertions(+), 52 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/95/33695/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmTemplateCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmTemplateCommand.java index ccb80a6..56969b7 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmTemplateCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmTemplateCommand.java @@ -1,6 +1,7 @@ package org.ovirt.engine.core.bll; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.Date; import java.util.HashMap; @@ -26,7 +27,6 @@ import org.ovirt.engine.core.bll.utils.VmDeviceUtils; import org.ovirt.engine.core.bll.validator.DiskImagesValidator; import org.ovirt.engine.core.bll.validator.MultipleStorageDomainsValidator; -import org.ovirt.engine.core.bll.validator.StorageDomainValidator; import org.ovirt.engine.core.bll.validator.VmWatchdogValidator; import org.ovirt.engine.core.common.AuditLogType; import org.ovirt.engine.core.common.FeatureSupported; @@ -120,7 +120,7 @@ } if (getVm() != null) { updateVmDevices(); - updateVmDisks(); + mImages.addAll(getVmDisksFromDB()); setStoragePoolId(getVm().getStoragePoolId()); isVmInDb = true; } else if (getVdsGroup() != null && parameterMasterVm != null) { @@ -160,10 +160,10 @@ VmDeviceUtils.setVmDevices(getVm().getStaticData()); } - protected void updateVmDisks() { + protected List<DiskImage> getVmDisksFromDB() { VmHandler.updateDisksFromDb(getVm()); VmHandler.filterImageDisksForVM(getVm(), false, false, true); - mImages.addAll(getVm().getDiskList()); + return getVm().getDiskList(); } @Override @@ -438,7 +438,7 @@ return getParameters().getBaseTemplateId() != null; } - private boolean imagesRelatedChecks() { + protected boolean imagesRelatedChecks() { // images related checks if (!mImages.isEmpty()) { if (!validateVmNotDuringSnapshot()) { @@ -457,7 +457,7 @@ } MultipleStorageDomainsValidator storageDomainsValidator = - new MultipleStorageDomainsValidator(getStoragePoolId(), sourceImageDomainsImageMap.keySet()); + getStorageDomainsValidator(getStoragePoolId(), sourceImageDomainsImageMap.keySet()); if (!validate(storageDomainsValidator.allDomainsExistAndActive())) { return false; } @@ -490,26 +490,25 @@ } storageDomains.put(destImageDomain, storage); } - // update vm snapshots for storage free space check - ImagesHandler.fillImagesBySnapshots(getVm()); - - Map<StorageDomain, Integer> domainMap = - StorageDomainValidator.getSpaceRequirementsForStorageDomains( - ImagesHandler.filterImageDisks(getVm().getDiskMap().values(), true, false, true), - storageDomains, - diskInfoDestinationMap); - - for (Map.Entry<StorageDomain, Integer> entry : domainMap.entrySet()) { - if (!doesStorageDomainhaveSpaceForRequest(entry.getKey(), entry.getValue())) { - return false; - } - } + return validateSpaceRequirements(); } return true; } - protected boolean doesStorageDomainhaveSpaceForRequest(StorageDomain storageDomain, long spaceForRequest) { - return validate(new StorageDomainValidator(storageDomain).isDomainHasSpaceForRequest(spaceForRequest)); + protected boolean validateSpaceRequirements() { + // update vm snapshots for storage free space check + ImagesHandler.fillImagesBySnapshots(getVm()); + List<DiskImage> disksList = ImagesHandler.filterImageDisks(getVm().getDiskMap().values(), true, false, true); + List<DiskImage> disksListForStorageChecks = createDiskDummiesForSpaceValidations(disksList); + MultipleStorageDomainsValidator multipleSdValidator = getStorageDomainsValidator( + getVm().getStoragePoolId(), getStorageGuidSet()); + + return (validate(multipleSdValidator.allDomainsWithinThresholds()) + && validate(multipleSdValidator.allDomainsHaveSpaceForClonedDisks(disksListForStorageChecks))); + } + + protected MultipleStorageDomainsValidator getStorageDomainsValidator(Guid spId, Set<Guid> disks) { + return new MultipleStorageDomainsValidator(spId, disks); } protected boolean validateVmNotDuringSnapshot() { @@ -524,6 +523,23 @@ return destImageDomains; } + /** + * Space Validations are done using data extracted from the disks. The disks in question in this command + * don't have all the needed data, and in order not to contaminate the command's data structures, an alter + * one is created specifically for this validation - hence dummy. + * @param disksList + * @return + */ + protected List<DiskImage> createDiskDummiesForSpaceValidations(Collection<DiskImage> disksList) { + List<DiskImage> dummies = new ArrayList<>(disksList.size()); + for (DiskImage image : disksList) { + Guid targetSdId = diskInfoDestinationMap.get(image.getId()).getStorageIds().get(0); + DiskImage dummy = ImagesHandler.createDiskImageWithExcessData(image, targetSdId); + dummies.add(dummy); + } + return dummies; + } + protected void addVmTemplateToDb() { // TODO: add timezone handling setVmTemplate( diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java index 1736afc..005caff 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java @@ -15,7 +15,6 @@ import org.ovirt.engine.core.common.config.Config; import org.ovirt.engine.core.common.config.ConfigValues; import org.ovirt.engine.core.common.errors.VdcBllMessages; -import org.ovirt.engine.core.compat.Guid; import org.ovirt.engine.core.dal.dbbroker.DbFacade; public class StorageDomainValidator { @@ -262,26 +261,6 @@ return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN, storageName()); - } - - /** - * @deprecated - * This validation is replaced by hadSpaceForClonedDisks,hadSpaceForClonedDisk, hasSpaceForNewDisks and - * hasSpaceForNewDisk, according to the situation. - */ - @Deprecated - public static Map<StorageDomain, Integer> getSpaceRequirementsForStorageDomains(Collection<DiskImage> images, - Map<Guid, StorageDomain> storageDomains, Map<Guid, DiskImage> imageToDestinationDomainMap) { - Map<DiskImage, StorageDomain> spaceMap = new HashMap<DiskImage, StorageDomain>(); - for (DiskImage image : images) { - Guid storageId = imageToDestinationDomainMap.get(image.getId()).getStorageIds().get(0); - StorageDomain domain = storageDomains.get(storageId); - if (domain == null) { - domain = DbFacade.getInstance().getStorageDomainDao().get(storageId); - } - spaceMap.put(image, domain); - } - return StorageDomainValidator.getSpaceRequirementsForStorageDomains(spaceMap); } /** diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmTemplateCommandTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmTemplateCommandTest.java index 7592d93..61acd10 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmTemplateCommandTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmTemplateCommandTest.java @@ -2,10 +2,17 @@ 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.spy; import static org.mockito.Mockito.when; import static org.ovirt.engine.core.utils.MockConfigRule.mockConfig; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; import org.junit.Before; import org.junit.ClassRule; @@ -13,8 +20,13 @@ import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.runners.MockitoJUnitRunner; +import org.ovirt.engine.core.bll.validator.DiskImagesValidator; +import org.ovirt.engine.core.bll.validator.MultipleStorageDomainsValidator; import org.ovirt.engine.core.common.action.AddVmTemplateParameters; import org.ovirt.engine.core.common.businessentities.ArchitectureType; +import org.ovirt.engine.core.common.businessentities.DiskImage; +import org.ovirt.engine.core.common.businessentities.StoragePool; +import org.ovirt.engine.core.common.businessentities.StoragePoolStatus; import org.ovirt.engine.core.common.businessentities.VDSGroup; import org.ovirt.engine.core.common.businessentities.VM; import org.ovirt.engine.core.common.businessentities.VMStatus; @@ -24,35 +36,42 @@ import org.ovirt.engine.core.common.utils.SimpleDependecyInjector; import org.ovirt.engine.core.compat.Guid; import org.ovirt.engine.core.compat.Version; +import org.ovirt.engine.core.dao.StoragePoolDAO; import org.ovirt.engine.core.dao.VdsGroupDAO; import org.ovirt.engine.core.dao.VmDAO; import org.ovirt.engine.core.utils.MockConfigRule; -/** A test case for {@link AddVmTemplateCommand} */ +/** + * A test case for {@link AddVmTemplateCommand} + */ @RunWith(MockitoJUnitRunner.class) public class AddVmTemplateCommandTest { + @ClassRule + public static MockConfigRule mcr = new MockConfigRule(mockConfig(ConfigValues.VmPriorityMaxValue, 100)); private AddVmTemplateCommand<AddVmTemplateParameters> cmd; private VM vm; private VDSGroup vdsGroup; + private Guid spId; @Mock private VmDAO vmDao; - @Mock private VdsGroupDAO vdsGroupDao; - + @Mock + private StoragePoolDAO storagePoolDao; @Mock private OsRepository osRepository; - - @ClassRule - public static MockConfigRule mcr = new MockConfigRule(mockConfig(ConfigValues.VmPriorityMaxValue, 100)); + @Mock + private MultipleStorageDomainsValidator multipleSdValidator; + @Mock + private DiskImagesValidator diskImagesValidator; @Before public void setUp() { // The VM to use Guid vmId = Guid.newGuid(); Guid vdsGroupId = Guid.newGuid(); - Guid spId = Guid.newGuid(); + spId = Guid.newGuid(); vm = new VM(); vm.setId(vmId); @@ -77,7 +96,8 @@ cmd = spy(new AddVmTemplateCommand<AddVmTemplateParameters>(params) { @Override - protected void updateVmDisks() { + protected List<DiskImage> getVmDisksFromDB() { + return getDisksList(spId); } @Override @@ -108,9 +128,31 @@ public void testCanDoAction() { doReturn(true).when(cmd).validateVmNotDuringSnapshot(); vm.setStatus(VMStatus.Up); - CanDoActionTestUtils.runAndAssertCanDoActionFailure(cmd, VdcBllMessages.VMT_CANNOT_CREATE_TEMPLATE_FROM_DOWN_VM); } + + @Test + public void sufficientStorageSpace() { + setupForStorageTests(); + assertTrue(cmd.imagesRelatedChecks()); + } + + @Test + public void storageSpaceNotWithinThreshold() { + setupForStorageTests(); + doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN)). + when(multipleSdValidator).allDomainsWithinThresholds(); + assertFalse(cmd.imagesRelatedChecks()); + } + + @Test + public void insufficientStorageSpace() { + setupForStorageTests(); + doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN)). + when(multipleSdValidator).allDomainsHaveSpaceForClonedDisks(anyList()); + assertFalse(cmd.imagesRelatedChecks()); + } + @Test public void testBeanValidations() { @@ -122,4 +164,32 @@ cmd.getParameters().setName("aa-??bb"); assertFalse("Pattern-based name should not be supported for Template", cmd.validateInputs()); } + + private void setupForStorageTests() { + doReturn(true).when(cmd).validateVmNotDuringSnapshot(); + vm.setStatus(VMStatus.Down); + doReturn(multipleSdValidator).when(cmd).getStorageDomainsValidator(any(Guid.class), anySet()); + doReturn(ValidationResult.VALID).when(multipleSdValidator).allDomainsHaveSpaceForClonedDisks(anyList()); + doReturn(ValidationResult.VALID).when(multipleSdValidator).allDomainsWithinThresholds(); + doReturn(ValidationResult.VALID).when(multipleSdValidator).allDomainsExistAndActive(); + doReturn(ValidationResult.VALID).when(diskImagesValidator).diskImagesNotIllegal(); + + setupStoragePool(); + } + + private void setupStoragePool() { + StoragePool storagePool = new StoragePool(); + storagePool.setId(spId); + storagePool.setStatus(StoragePoolStatus.Up); + doReturn(storagePoolDao).when(cmd).getStoragePoolDAO(); + when(storagePoolDao.get(spId)).thenReturn(storagePool); + } + + private List<DiskImage> getDisksList(Guid spId) { + List disksList = new ArrayList(1); + DiskImage disk = new DiskImage(); + disk.setStorageIds(new ArrayList<>(Collections.singletonList(spId))); + disksList.add(disk); + return disksList; + } } -- To view, visit http://gerrit.ovirt.org/33695 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I7980510a7bb72e43e0a9dfc18460207386eb62fe 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
