Vered Volansky has uploaded a new change for review. Change subject: core: ImportVmTemplateCommand storage allocation ......................................................................
core: ImportVmTemplateCommand 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 ImportVmTemplateCommand, 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. Added tests to MoveOrCopyTemplateCommand.validateSpaceRequirements() in MoveMoveOrCopyTemplateCommandTest. Change-Id: I6ffc5c00905bb03c9cb3cbffa481e78c9b0ab87a Bug-Url: https://bugzilla.redhat.com/1136721 Signed-off-by: Vered Volansky <vvola...@redhat.com> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmTemplateCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageDomainCommandBase.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImportVmTemplateCommandTest.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommandTest.java 5 files changed, 95 insertions(+), 65 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/78/33478/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmTemplateCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmTemplateCommand.java index f299fb7..476de1f 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmTemplateCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmTemplateCommand.java @@ -182,23 +182,11 @@ } if (retVal && getImages() != null && !getImages().isEmpty() && !isImagesAlreadyOnTarget()) { - Map<StorageDomain, Integer> domainMap = getSpaceRequirementsForStorageDomains( - new ArrayList<DiskImage>(getVmTemplate().getDiskImageMap().values())); - if (domainMap.isEmpty()) { - int sz = 0; - if (getVmTemplate().getDiskImageMap() != null) { - for (DiskImage image : getVmTemplate().getDiskImageMap().values()) { - sz += image.getSize(); - } - } - domainMap.put(getStorageDomain(), sz); - } - for (Map.Entry<StorageDomain, Integer> entry : domainMap.entrySet()) { - if (!doesStorageDomainhaveSpaceForRequest(entry.getKey(), entry.getValue())) { - return false; - } + if (!validateSpaceRequirements(getImages())) { + return false; } } + if (retVal) { retVal = validateMacAddress(Entities.<VmNic, VmNetworkInterface> upcast(getVmTemplate().getInterfaces())); } diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommand.java index 9afa129..5d294a5 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommand.java @@ -355,15 +355,6 @@ return getStorageDomainDAO().getForStoragePool(domainId, getStoragePool().getId()); } - protected Map<StorageDomain, Integer> getSpaceRequirementsForStorageDomains(Collection<DiskImage> images) { - Map<DiskImage, StorageDomain> spaceMap = new HashMap<DiskImage, StorageDomain>(); - for (DiskImage image : images) { - StorageDomain domain = getStorageDomain(imageToDestinationDomainMap.get(image.getId())); - spaceMap.put(image, domain); - } - return StorageDomainValidator.getSpaceRequirementsForStorageDomains(spaceMap); - } - /** * 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, as alter diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageDomainCommandBase.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageDomainCommandBase.java index dd0b155..b69c308 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageDomainCommandBase.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageDomainCommandBase.java @@ -14,7 +14,6 @@ import org.ovirt.engine.core.bll.context.CommandContext; import org.ovirt.engine.core.bll.context.CompensationContext; import org.ovirt.engine.core.bll.utils.PermissionSubject; -import org.ovirt.engine.core.bll.validator.StorageDomainValidator; import org.ovirt.engine.core.common.VdcObjectType; import org.ovirt.engine.core.common.action.StorageDomainParametersBase; import org.ovirt.engine.core.common.businessentities.LUN_storage_server_connection_map; @@ -119,10 +118,6 @@ } } return returnValue; - } - - protected boolean doesStorageDomainhaveSpaceForRequest(StorageDomain storageDomain, long sizeRequested) { - return validate(new StorageDomainValidator(storageDomain).isDomainHasSpaceForRequest(sizeRequested)); } protected boolean isNotLocalData(final boolean isInternal) { diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImportVmTemplateCommandTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImportVmTemplateCommandTest.java index 9298fee..38e7b8d 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImportVmTemplateCommandTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImportVmTemplateCommandTest.java @@ -4,8 +4,8 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyList; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; @@ -21,7 +21,6 @@ import javax.validation.ConstraintViolation; -import org.junit.Rule; import org.junit.Test; import org.ovirt.engine.core.bll.context.EngineContext; import org.ovirt.engine.core.common.action.ImportVmTemplateParameters; @@ -37,7 +36,6 @@ import org.ovirt.engine.core.common.businessentities.VmTemplate; import org.ovirt.engine.core.common.businessentities.VolumeFormat; import org.ovirt.engine.core.common.businessentities.VolumeType; -import org.ovirt.engine.core.common.config.ConfigValues; import org.ovirt.engine.core.common.errors.VdcBllMessages; import org.ovirt.engine.core.common.queries.VdcQueryParametersBase; import org.ovirt.engine.core.common.queries.VdcQueryReturnValue; @@ -48,30 +46,17 @@ import org.ovirt.engine.core.dao.StorageDomainStaticDAO; import org.ovirt.engine.core.dao.StoragePoolDAO; import org.ovirt.engine.core.dao.VmTemplateDAO; -import org.ovirt.engine.core.utils.MockConfigRule; import org.springframework.util.Assert; public class ImportVmTemplateCommandTest { - @Rule - public MockConfigRule mcr = new MockConfigRule(); - @Test public void insufficientDiskSpace() { - final int lotsOfSpaceRequired = 1073741824; - final ImportVmTemplateCommand c = - setupDiskSpaceTest(lotsOfSpaceRequired); - assertFalse(c.canDoAction()); - assertTrue(c.getReturnValue() - .getCanDoActionMessages() - .contains(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN.toString())); - } - - @Test - public void sufficientDiskSpace() { - final ImportVmTemplateCommand c = - setupDiskSpaceTest(0); - assertTrue(c.canDoAction()); + // The following is enough since the validation is mocked out anyway. Just want to make sure the flow in CDA is correct. + // Full test for the scenarios is done in the inherited class. + final ImportVmTemplateCommand command = setupVolumeFormatAndTypeTest(VolumeFormat.RAW, VolumeType.Preallocated, StorageType.NFS); + doReturn(false).when(command).validateSpaceRequirements(anyList()); + assertFalse(command.canDoAction()); } @Test @@ -115,14 +100,14 @@ VolumeType volumeType, StorageType storageType) { CanDoActionTestUtils.runAndAssertCanDoActionSuccess( - setupVolumeFormatAndTypeTest(volumeFormat, volumeType, storageType, 0)); + setupVolumeFormatAndTypeTest(volumeFormat, volumeType, storageType)); } private void assertInvalidVolumeInfoCombination(VolumeFormat volumeFormat, VolumeType volumeType, StorageType storageType) { CanDoActionTestUtils.runAndAssertCanDoActionFailure( - setupVolumeFormatAndTypeTest(volumeFormat, volumeType, storageType, 0), + setupVolumeFormatAndTypeTest(volumeFormat, volumeType, storageType), VdcBllMessages.ACTION_TYPE_FAILED_DISK_CONFIGURATION_NOT_SUPPORTED); } @@ -140,12 +125,8 @@ private ImportVmTemplateCommand setupVolumeFormatAndTypeTest( VolumeFormat volumeFormat, VolumeType volumeType, - StorageType storageType, - int freeSpaceCritical) { - mcr.mockConfigValue(ConfigValues.FreeSpaceCriticalLowInGB, freeSpaceCritical); - - ImportVmTemplateCommand command = - spy(new ImportVmTemplateCommand(createParameters())); + StorageType storageType) { + ImportVmTemplateCommand command = spy(new ImportVmTemplateCommand(createParameters())); Backend backend = mock(Backend.class); doReturn(backend).when(command).getBackend(); @@ -159,6 +140,7 @@ mockStorageDomains(command); doReturn(true).when(command).setAndValidateDiskProfiles(); doReturn(true).when(command).setAndValidateCpuProfile(); + doReturn(true).when(command).validateSpaceRequirements(anyList()); return command; } @@ -223,13 +205,6 @@ when(dao.get(any(Guid.class))).thenReturn(domain); doReturn(dao).when(command).getStorageDomainStaticDAO(); - } - - private ImportVmTemplateCommand setupDiskSpaceTest(final int extraDiskSpaceRequired) { - return setupVolumeFormatAndTypeTest(VolumeFormat.RAW, - VolumeType.Preallocated, - StorageType.NFS, - extraDiskSpaceRequired); } protected ImportVmTemplateParameters createParameters() { diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommandTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommandTest.java index 93627f2..1dbf171 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommandTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommandTest.java @@ -10,16 +10,22 @@ 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.Mockito.doAnswer; import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import org.junit.Before; import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; import org.mockito.invocation.InvocationOnMock; +import org.mockito.runners.MockitoJUnitRunner; import org.mockito.stubbing.Answer; +import org.ovirt.engine.core.bll.validator.MultipleStorageDomainsValidator; import org.ovirt.engine.core.common.action.MoveOrCopyParameters; import org.ovirt.engine.core.common.businessentities.DiskImage; import org.ovirt.engine.core.common.businessentities.StorageDomain; @@ -29,9 +35,13 @@ import org.ovirt.engine.core.common.vdscommands.VDSReturnValue; import org.ovirt.engine.core.compat.Guid; +@RunWith(MockitoJUnitRunner.class) public class MoveOrCopyTemplateCommandTest { private Guid[] diskIds; + + @Mock + private MultipleStorageDomainsValidator multipleSdValidator; @Before public void setup() { @@ -91,6 +101,67 @@ verify(cmd, times(2)).runVdsCommand(any(VDSCommandType.class), any(GetImagesListVDSCommandParameters.class)); } + @Test + public void sufficientStorageSpaceWithCollapse() { + MoveOrCopyParameters parameters = createParameters(false); + parameters.setCopyCollapse(true); + final MoveOrCopyTemplateCommand<MoveOrCopyParameters> command = setupSpaceTests(parameters); + assertTrue(command.validateSpaceRequirements(anyList())); + verify(multipleSdValidator).allDomainsHaveSpaceForClonedDisks(anyList()); + verify(multipleSdValidator, never()).allDomainsHaveSpaceForDisksWithSnapshots(anyList()); + verify(multipleSdValidator, never()).allDomainsHaveSpaceForNewDisks(anyList()); + } + + @Test + public void sufficientStorageSpaceWithSnapshots() { + MoveOrCopyParameters parameters = createParameters(false); + parameters.setCopyCollapse(false); + final MoveOrCopyTemplateCommand<MoveOrCopyParameters> command = setupSpaceTests(parameters); + assertTrue(command.validateSpaceRequirements(anyList())); + verify(multipleSdValidator, never()).allDomainsHaveSpaceForClonedDisks(anyList()); + verify(multipleSdValidator).allDomainsHaveSpaceForDisksWithSnapshots(anyList()); + verify(multipleSdValidator, never()).allDomainsHaveSpaceForNewDisks(anyList()); + } + + @Test + public void lowThreshold() { + MoveOrCopyParameters parameters = createParameters(false); + parameters.setCopyCollapse(true); + final MoveOrCopyTemplateCommand<MoveOrCopyParameters> command = setupSpaceTests(parameters); + doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN)). + when(multipleSdValidator).allDomainsWithinThresholds(); + assertFalse(command.validateSpaceRequirements(anyList())); + verify(multipleSdValidator, never()).allDomainsHaveSpaceForClonedDisks(anyList()); + verify(multipleSdValidator, never()).allDomainsHaveSpaceForDisksWithSnapshots(anyList()); + verify(multipleSdValidator, never()).allDomainsHaveSpaceForNewDisks(anyList()); + } + + @Test + public void insufficientStorageSpaceWithCollapse() { + MoveOrCopyParameters parameters = createParameters(false); + parameters.setCopyCollapse(true); + final MoveOrCopyTemplateCommand<MoveOrCopyParameters> command = setupSpaceTests(parameters); + doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN)). + when(multipleSdValidator).allDomainsHaveSpaceForClonedDisks(anyList()); + assertFalse(command.validateSpaceRequirements(anyList())); + verify(multipleSdValidator).allDomainsHaveSpaceForClonedDisks(anyList()); + verify(multipleSdValidator, never()).allDomainsHaveSpaceForDisksWithSnapshots(anyList()); + verify(multipleSdValidator, never()).allDomainsHaveSpaceForNewDisks(anyList()); + } + + @Test + public void insufficientStorageSpaceWithSnapshots() { + MoveOrCopyParameters parameters = createParameters(false); + final MoveOrCopyTemplateCommand<MoveOrCopyParameters> command = setupSpaceTests(parameters); + parameters.setCopyCollapse(false); + doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN)). + when(multipleSdValidator).allDomainsHaveSpaceForDisksWithSnapshots(anyList()); + assertFalse(command.validateSpaceRequirements(anyList())); + verify(multipleSdValidator, never()).allDomainsHaveSpaceForClonedDisks(anyList()); + verify(multipleSdValidator).allDomainsHaveSpaceForDisksWithSnapshots(anyList()); + verify(multipleSdValidator, never()).allDomainsHaveSpaceForNewDisks(anyList()); + } + protected MoveOrCopyParameters createParameters(boolean moveToSameStorageDomain) { Guid targetStorageDomainId = Guid.newGuid(); Map<Guid, Guid> imageToDestinationDomainMap = new HashMap<>(); @@ -137,6 +208,16 @@ return cmd; } + private MoveOrCopyTemplateCommand<MoveOrCopyParameters> setupSpaceTests(MoveOrCopyParameters parameters) { + MoveOrCopyTemplateCommand<MoveOrCopyParameters> cmd = spy(new MoveOrCopyTemplateCommand<MoveOrCopyParameters>(parameters)); + doReturn(multipleSdValidator).when(cmd).createMultipleStorageDomainsValidator(anyList()); + doReturn(ValidationResult.VALID).when(multipleSdValidator).allDomainsHaveSpaceForClonedDisks(anyList()); + doReturn(ValidationResult.VALID).when(multipleSdValidator).allDomainsHaveSpaceForDisksWithSnapshots(anyList()); + doReturn(ValidationResult.VALID).when(multipleSdValidator).allDomainsWithinThresholds(); + doReturn(ValidationResult.VALID).when(multipleSdValidator).allDomainsExistAndActive(); + return cmd; + } + private List<DiskImage> createImageList() { List<DiskImage> disks = new ArrayList<>(); for (Guid id : diskIds) { -- To view, visit http://gerrit.ovirt.org/33478 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I6ffc5c00905bb03c9cb3cbffa481e78c9b0ab87a 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
