Vered Volansky has uploaded a new change for review. Change subject: core: ImportVmCommand storage allocation checks ......................................................................
core: ImportVmCommand storage allocation checks 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 ImportVmCommand, while added new space allocation validation to storage domain validators regarding snapshoted disks to be cloned with the spnapshots. Tests were amended accordingly. Change-Id: Ifbb1d985f9afa476452d1d2b78be1fd18c128c8f Bug-Url: https://bugzilla.redhat.com/1053746 Signed-off-by: Vered Volansky <vvola...@redhat.com> --- 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/ImportVmCommand.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/validator/MultipleStorageDomainsValidator.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/ImportVmCommandTest.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/MultipleStorageDomainsValidatorTest.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/StorageDomainValidatorFreeSpaceTest.java M backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DiskImageBase.java 9 files changed, 322 insertions(+), 62 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/98/32898/1 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 8bfaf4e..d29ed85 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 @@ -816,4 +816,11 @@ } return snapshot; } + + public static DiskImage createDiskImageWithExcessData(DiskImage diskImage, Guid sdId) { + DiskImage dummy = DiskImage.copyOf(diskImage); + dummy.setStorageIds(new ArrayList<Guid>(Collections.singletonList(sdId))); + dummy.getSnapshots().addAll(ImagesHandler.getAllImageSnapshots(dummy.getImageId())); + return dummy; + } } diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java index 79b902f..11bbfcc 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java @@ -417,16 +417,8 @@ } if (!isImagesAlreadyOnTarget()) { - Map<StorageDomain, Integer> domainMap = getSpaceRequirementsForStorageDomains(imageList); - - if (!setDomainsForMemoryImages(domainMap)) { - return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_NO_SUITABLE_DOMAIN_FOUND); - } - - for (Map.Entry<StorageDomain, Integer> entry : domainMap.entrySet()) { - if (!doesStorageDomainhaveSpaceForRequest(entry.getKey(), entry.getValue())) { - return false; - } + if (!handleDestStorageDomains()) { + return false; } } @@ -457,16 +449,32 @@ return true; } + protected boolean handleDestStorageDomains() { + List<DiskImage> dummiesDisksList = createDiskDummiesForSpaceValidations(imageList); + if (getParameters().getCopyCollapse()) { + Snapshot activeSnapshot = getActiveSnapshot(); + if (activeSnapshot != null && activeSnapshot.containsMemory()) { + //Checking space for memory volume of the active image (if there is one) + StorageDomain storageDomain = updateStorageDomainInMemoryVolumes(dummiesDisksList); + if (storageDomain == null) { + return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_NO_SUITABLE_DOMAIN_FOUND); + } + } + } else { //Check space for all the snapshot's memory volumes + if (!updateDomainsForMemoryImages(dummiesDisksList)) { + return false; + } + } + return validateSpaceRequirements(dummiesDisksList); + } + /** - * This method fills the given map of domain to the required size for storing memory images - * within it, and also update the memory volume in each snapshot that has memory volume with - * the right storage pool and storage domain where it is going to be imported. + * For each snapshot that has memory volume, this method updates the memory volume with + * the storage pool and storage domain it's going to be imported to. * - * @param domain2requiredSize - * Maps domain to size required for storing memory volumes in it * @return true if we managed to assign storage domain for every memory volume, false otherwise */ - private boolean setDomainsForMemoryImages(Map<StorageDomain, Integer> domain2requiredSize) { + private boolean updateDomainsForMemoryImages(List<DiskImage> disksList) { Map<String, String> handledMemoryVolumes = new HashMap<String, String>(); for (Snapshot snapshot : getVm().getSnapshots()) { String memoryVolume = snapshot.getMemoryVolume(); @@ -480,17 +488,10 @@ continue; } - VM vm = getVmFromSnapshot(snapshot); - int requiredSizeForMemory = (int) Math.ceil((vm.getTotalMemorySizeInBytes() + - MemoryUtils.META_DATA_SIZE_IN_BYTES) * 1.0 / BYTES_IN_GB); - StorageDomain storageDomain = VmHandler.findStorageDomainForMemory( - getParameters().getStoragePoolId(), requiredSizeForMemory, domain2requiredSize); + StorageDomain storageDomain = updateStorageDomainInMemoryVolumes(disksList); if (storageDomain == null) { - return false; + return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_NO_SUITABLE_DOMAIN_FOUND); } - int requiredSizeInDomainIncludingMemoryVolumes = domain2requiredSize.containsKey(storageDomain) ? - domain2requiredSize.get(storageDomain) + requiredSizeForMemory : requiredSizeForMemory; - domain2requiredSize.put(storageDomain, requiredSizeInDomainIncludingMemoryVolumes); String modifiedMemoryVolume = MemoryUtils.changeStorageDomainAndPoolInMemoryState( memoryVolume, storageDomain.getId(), getParameters().getStoragePoolId()); // replace the volume representation with the one with the correct domain & pool @@ -499,6 +500,13 @@ handledMemoryVolumes.put(memoryVolume, modifiedMemoryVolume); } return true; + } + + private StorageDomain updateStorageDomainInMemoryVolumes(List<DiskImage> disksList) { + List<DiskImage> memoryDisksList = MemoryUtils.createDiskDummies(getVm().getTotalMemorySizeInBytes(), MemoryUtils.META_DATA_SIZE_IN_BYTES); + StorageDomain storageDomain = VmHandler.findStorageDomainForMemory(getParameters().getStoragePoolId(), memoryDisksList); + disksList.addAll(memoryDisksList); + return storageDomain; } /** @@ -959,12 +967,20 @@ } private String getMemoryVolumeFromActiveSnapshotInExportDomain() { + Snapshot activeSnapshot = getActiveSnapshot(); + if (activeSnapshot != null) { + return activeSnapshot.getMemoryVolume(); + } + return StringUtils.EMPTY; + } + + private Snapshot getActiveSnapshot() { for (Snapshot snapshot : getVm().getSnapshots()) { if (snapshot.getType() == SnapshotType.ACTIVE) - return snapshot.getMemoryVolume(); + return snapshot; } log.warnFormat("VM {0} doesn't have active snapshot in export domain", getVmId()); - return StringUtils.EMPTY; + return null; } /** 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 727c329..f47c63e 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 @@ -16,6 +16,7 @@ import org.ovirt.engine.core.bll.storage.StorageDomainCommandBase; import org.ovirt.engine.core.bll.utils.PermissionSubject; import org.ovirt.engine.core.bll.utils.VmDeviceUtils; +import org.ovirt.engine.core.bll.validator.MultipleStorageDomainsValidator; import org.ovirt.engine.core.bll.validator.StorageDomainValidator; import org.ovirt.engine.core.common.AuditLogType; import org.ovirt.engine.core.common.VdcObjectType; @@ -360,6 +361,42 @@ 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 + * one is created specifically fo this validation - hence dummy. + * @param disksList + * @return + */ + protected List<DiskImage> createDiskDummiesForSpaceValidations(List<DiskImage> disksList) { + List<DiskImage> dummies = new ArrayList<>(disksList.size()); + for (DiskImage image : disksList) { + Guid targetSdId = imageToDestinationDomainMap.get(image.getId()); + DiskImage dummy = ImagesHandler.createDiskImageWithExcessData(image, targetSdId); + dummies.add(dummy); + } + return dummies; + } + + protected boolean validateSpaceRequirements(List<DiskImage> disksList) { + MultipleStorageDomainsValidator sdValidator = createMultipleStorageDomainsValidator(disksList); + if (!validate(sdValidator.allDomainsExistAndActive()) + || !validate(sdValidator.allDomainsWithinThresholds())) { + return false; + } + + if (getParameters().getCopyCollapse()) { + return validate(sdValidator.allDomainsHaveSpaceForClonedDisks(disksList)); + } + + return validate(sdValidator.allDomainsHaveSpaceForDisksWithSnapshots(disksList)); + } + + protected MultipleStorageDomainsValidator createMultipleStorageDomainsValidator( List<DiskImage> disksList) { + return new MultipleStorageDomainsValidator(getStoragePoolId(), + ImagesHandler.getAllStorageIdsForImageIds(disksList)); + } + protected void ensureDomainMap(Collection<DiskImage> images, Guid defaultDomainId) { if (imageToDestinationDomainMap == null) { imageToDestinationDomainMap = new HashMap<Guid, Guid>(); 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 84407f9..cb72ce2 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 @@ -100,6 +100,23 @@ * 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 allDomainsHaveSpaceForClonedDisks(List<DiskImage> disksList) { + final Map<Guid, List<DiskImage>> disksMap = getDomainsDisksMap(disksList); + + return validOrFirstFailure(new ValidatorPredicate() { + @Override + public ValidationResult evaluate(Map.Entry<Guid, StorageDomainValidator> entry) { + Guid sdId = entry.getKey(); + List<DiskImage> disksList = disksMap.get(sdId); + return getStorageDomainValidator(entry).hasSpaceForClonedDisks(disksList); + } + }); + } + + /** + * 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 allDomainsHaveSpaceForAllDisks(List<DiskImage> newDisksList, List<DiskImage> clonedDisksList) { final Map<Guid, List<DiskImage>> domainsNewDisksMap = getDomainsDisksMap(newDisksList); final Map<Guid, List<DiskImage>> domainsClonedDisksMap = getDomainsDisksMap(clonedDisksList); @@ -115,6 +132,23 @@ }); } + /** + * 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 allDomainsHaveSpaceForDisksWithSnapshots(List<DiskImage> disksList) { + final Map<Guid, List<DiskImage>> disksMap = getDomainsDisksMap(disksList); + + return validOrFirstFailure(new ValidatorPredicate() { + @Override + public ValidationResult evaluate(Map.Entry<Guid, StorageDomainValidator> entry) { + Guid sdId = entry.getKey(); + List<DiskImage> disksList = disksMap.get(sdId); + return getStorageDomainValidator(entry).hasSpaceForDisksWithSnapshots(disksList); + } + }); + } + /** @return The lazy-loaded validator for the given map entry */ protected StorageDomainValidator getStorageDomainValidator(Map.Entry<Guid, StorageDomainValidator> entry) { if (entry.getValue() == null) { 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 d7552de..1736afc 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 @@ -131,11 +131,10 @@ * */ private double getTotalSizeForNewDisks(Collection<DiskImage> diskImages) { - double totalSizeForDisks = 0.0; - if (diskImages != null) { - for (DiskImage diskImage : diskImages) { - double sizeForDisk = diskImage.getSize(); - + return getTotalSizeForDisksByMethod(diskImages, new SizeAssessment() { + @Override + public double getSizeForDisk(DiskImage diskImage) { + double sizeForDisk = diskImage.getCapacity(); if (diskImage.getVolumeFormat() == VolumeFormat.COW) { if (storageDomain.getStorageType().isFileDomain()) { sizeForDisk = EMPTY_QCOW_HEADER_SIZE; @@ -145,10 +144,9 @@ } else if (diskImage.getVolumeType() == VolumeType.Sparse) { sizeForDisk = EMPTY_QCOW_HEADER_SIZE; } - totalSizeForDisks += sizeForDisk; + return sizeForDisk; } - } - return totalSizeForDisks; + }); } /** @@ -166,25 +164,53 @@ * * */ private double getTotalSizeForClonedDisks(Collection<DiskImage> diskImages) { - double totalSizeForDisks = 0.0; - if (diskImages != null) { - for (DiskImage diskImage : diskImages) { - double diskCapacity = diskImage.getSize(); - double sizeForDisk = diskCapacity; - + return getTotalSizeForDisksByMethod(diskImages, new SizeAssessment() { + @Override + public double getSizeForDisk(DiskImage diskImage) { + double sizeForDisk = diskImage.getCapacity(); if ((storageDomain.getStorageType().isFileDomain() && diskImage.getVolumeType() == VolumeType.Sparse) || storageDomain.getStorageType().isBlockDomain() && diskImage.getVolumeFormat() == VolumeFormat.COW) { - double usedSapce = diskImage.getActualDiskWithSnapshotsSizeInBytes(); - sizeForDisk = Math.min(diskCapacity, usedSapce); + double usedSpace = diskImage.getActualDiskWithSnapshotsSizeInBytes(); + sizeForDisk = Math.min(diskImage.getCapacity(), usedSpace); } if (diskImage.getVolumeFormat() == VolumeFormat.COW) { sizeForDisk = Math.ceil(QCOW_OVERHEAD_FACTOR * sizeForDisk); } - totalSizeForDisks += sizeForDisk; + return sizeForDisk; } - } - return totalSizeForDisks; + }); + } + + /** + * Verify there's enough space in the storage domain for creating cloned DiskImages with snapshots without collapse. + * Space should be allocated according to the volumes type and format, and allocation policy, + * according to the following table: + * + * | File Domain | Block Domain + * -----|-----------------------------------------|---------------- + * qcow | 1.1 * used space |1.1 * used space + * -----|-----------------------------------------|---------------- + * raw | preallocated: disk capacity |disk capacity + * | sparse: used space | + * + * */ + private double getTotalSizeForDisksWithSnapshots(Collection<DiskImage> diskImages) { + return getTotalSizeForDisksByMethod(diskImages, new SizeAssessment() { + @Override + public double getSizeForDisk(DiskImage diskImage) { + double sizeForDisk = diskImage.getCapacity(); + if ((storageDomain.getStorageType().isFileDomain() && diskImage.getVolumeType() == VolumeType.Sparse) + || diskImage.getVolumeFormat() == VolumeFormat.COW) { + sizeForDisk = diskImage.getActualDiskWithSnapshotsSizeInBytes(); + } + + if (diskImage.getVolumeFormat() == VolumeFormat.COW) { + sizeForDisk = Math.ceil(QCOW_OVERHEAD_FACTOR * sizeForDisk); + } + return sizeForDisk; + } + }); } public ValidationResult hasSpaceForNewDisks(Collection<DiskImage> diskImages) { @@ -201,6 +227,13 @@ return validateRequiredSpace(availableSize, totalSizeForDisks); } + public ValidationResult hasSpaceForDisksWithSnapshots(Collection<DiskImage> diskImages) { + double availableSize = storageDomain.getAvailableDiskSizeInBytes(); + double totalSizeForDisks = getTotalSizeForDisksWithSnapshots(diskImages); + + return validateRequiredSpace(availableSize, totalSizeForDisks); + } + public ValidationResult hasSpaceForAllDisks(Collection<DiskImage> newDiskImages, Collection<DiskImage> clonedDiskImages) { double availableSize = storageDomain.getAvailableDiskSizeInBytes(); double totalSizeForNewDisks = getTotalSizeForNewDisks(newDiskImages); @@ -208,6 +241,10 @@ double totalSizeForDisks = totalSizeForNewDisks + totalSizeForClonedDisks; return validateRequiredSpace(availableSize, totalSizeForDisks); + } + + public ValidationResult hasSpaceForDiskWithSnapshots(DiskImage diskImage) { + return hasSpaceForDisksWithSnapshots(Collections.singleton(diskImage)); } public ValidationResult hasSpaceForClonedDisk(DiskImage diskImage) { @@ -289,4 +326,25 @@ return isDomainHasSpaceForRequest(Math.min(maxVirtualSize, sumOfActualSizes), false); } + + /** + * Validates all the storage domains by a given predicate. + * + * @return {@link ValidationResult#VALID} if all the domains are OK, or the + * first validation error if they aren't. + */ + private double getTotalSizeForDisksByMethod(Collection<DiskImage> diskImages, SizeAssessment sizeAssessment) { + double totalSizeForDisks = 0.0; + if (diskImages != null) { + for (DiskImage diskImage : diskImages) { + double sizeForDisk = sizeAssessment.getSizeForDisk(diskImage); + totalSizeForDisks += sizeForDisk; + } + } + return totalSizeForDisks; + } + + private static interface SizeAssessment { + public double getSizeForDisk(DiskImage diskImage); + } } diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImportVmCommandTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImportVmCommandTest.java index e2a9ef3..6326ad3 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImportVmCommandTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImportVmCommandTest.java @@ -6,10 +6,13 @@ 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.doNothing; 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 static org.mockito.Mockito.when; import static org.ovirt.engine.core.utils.MockConfigRule.mockConfig; @@ -30,12 +33,14 @@ 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.ImportVmParameters; import org.ovirt.engine.core.common.businessentities.BusinessEntitiesDefinitions; import org.ovirt.engine.core.common.businessentities.Disk; import org.ovirt.engine.core.common.businessentities.DiskImage; import org.ovirt.engine.core.common.businessentities.DiskInterface; import org.ovirt.engine.core.common.businessentities.DisplayType; +import org.ovirt.engine.core.common.businessentities.ImageStatus; import org.ovirt.engine.core.common.businessentities.Snapshot; import org.ovirt.engine.core.common.businessentities.StorageDomain; import org.ovirt.engine.core.common.businessentities.StorageDomainStatus; @@ -70,6 +75,9 @@ @Mock OsRepository osRepository; + @Mock + private MultipleStorageDomainsValidator multipleSdValidator; + @Before public void setUp() { // init the injector with the osRepository instance @@ -84,26 +92,52 @@ } @Test - public void insufficientDiskSpace() { - final int lotsOfSpace = 1073741824; - final ImportVmCommand<ImportVmParameters> c = setupDiskSpaceTest(lotsOfSpace); - assertFalse(c.canDoAction()); - assertTrue(c.getReturnValue() - .getCanDoActionMessages() - .contains(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN.toString())); + public void insufficientDiskSpaceWithCollapse() { + final ImportVmCommand<ImportVmParameters> command = setupDiskSpaceTest(createParameters()); + doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN)). + when(multipleSdValidator).allDomainsHaveSpaceForClonedDisks(anyList()); + CanDoActionTestUtils.runAndAssertCanDoActionFailure(command, + VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN); + verify(multipleSdValidator).allDomainsHaveSpaceForClonedDisks(anyList()); + verify(multipleSdValidator, never()).allDomainsHaveSpaceForDisksWithSnapshots(anyList()); + verify(multipleSdValidator, never()).allDomainsHaveSpaceForNewDisks(anyList()); + } + + @Test + public void insufficientDiskSpaceWithSnapshots() { + ImportVmParameters parameters = createParameters(); + final ImportVmCommand<ImportVmParameters> command = setupDiskSpaceTest(parameters); + parameters.setCopyCollapse(false); + doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN)). + when(multipleSdValidator).allDomainsHaveSpaceForDisksWithSnapshots(anyList()); + CanDoActionTestUtils.runAndAssertCanDoActionFailure(command, + VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN); + verify(multipleSdValidator, never()).allDomainsHaveSpaceForClonedDisks(anyList()); + verify(multipleSdValidator).allDomainsHaveSpaceForDisksWithSnapshots(anyList()); + verify(multipleSdValidator, never()).allDomainsHaveSpaceForNewDisks(anyList()); } @Test public void sufficientDiskSpace() { - final int extraDiskSpaceRequired = 0; - final ImportVmCommand<ImportVmParameters> c = setupDiskSpaceTest(extraDiskSpaceRequired); - assertTrue(c.canDoAction()); + final ImportVmCommand<ImportVmParameters> command = setupDiskSpaceTest(createParameters()); + CanDoActionTestUtils.runAndAssertCanDoActionSuccess(command); + verify(multipleSdValidator).allDomainsHaveSpaceForClonedDisks(anyList()); + verify(multipleSdValidator, never()).allDomainsHaveSpaceForDisksWithSnapshots(anyList()); + verify(multipleSdValidator, never()).allDomainsHaveSpaceForNewDisks(anyList()); } - private ImportVmCommand<ImportVmParameters> setupDiskSpaceTest(final int diskSpaceRequired) { - mcr.mockConfigValue(ConfigValues.FreeSpaceCriticalLowInGB, diskSpaceRequired); + @Test + public void lowThresholdStorageSpace() { + final ImportVmCommand<ImportVmParameters> command = setupDiskSpaceTest(createParameters()); + doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN)). + when(multipleSdValidator).allDomainsWithinThresholds(); + CanDoActionTestUtils.runAndAssertCanDoActionFailure(command, + VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN); + } - ImportVmCommand<ImportVmParameters> cmd = spy(new ImportVmCommand<ImportVmParameters>(createParameters())); + private ImportVmCommand<ImportVmParameters> setupDiskSpaceTest(ImportVmParameters parameters) { + ImportVmCommand<ImportVmParameters> cmd = spy(new ImportVmCommand<ImportVmParameters>(parameters)); + parameters.setCopyCollapse(true); doReturn(true).when(cmd).validateNoDuplicateVm(); doReturn(true).when(cmd).validateVdsCluster(); doReturn(true).when(cmd).validateUsbPolicy(); @@ -120,6 +154,17 @@ doReturn(new StoragePool()).when(cmd).getStoragePool(); doReturn(new VDSGroup()).when(cmd).getVdsGroup(); + ArrayList<Guid> sdIds = new ArrayList<Guid>(Collections.singletonList(Guid.newGuid())); + for (DiskImage image : parameters.getVm().getImages()) { + image.setStorageIds(sdIds); + } + + doReturn(mockCreateDiskDummiesForSpaceValidations()).when(cmd).createDiskDummiesForSpaceValidations(anyList()); + 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; } @@ -169,6 +214,20 @@ sd.setStatus(StorageDomainStatus.Active); sd.setAvailableDiskSize(2); return sd; + } + + protected List<DiskImage> mockCreateDiskDummiesForSpaceValidations() { + List<DiskImage> disksList = new ArrayList<>(); + for (int i = 0; i < 3; ++i) { + DiskImage diskImage = new DiskImage(); + diskImage.setActive(false); + diskImage.setId(Guid.newGuid()); + diskImage.setImageId(Guid.newGuid()); + diskImage.setParentId(Guid.newGuid()); + diskImage.setImageStatus(ImageStatus.OK); + disksList.add(diskImage); + } + return disksList; } @Test @@ -325,7 +384,7 @@ @Test public void testValidateClusterSupportForVirtioScsi() { - ImportVmCommand<ImportVmParameters> cmd = setupDiskSpaceTest(0); + ImportVmCommand<ImportVmParameters> cmd = setupDiskSpaceTest(createParameters()); cmd.getParameters().getVm().getDiskMap().values().iterator().next().setDiskInterface(DiskInterface.VirtIO_SCSI); cmd.getVdsGroup().setcompatibility_version(Version.v3_2); CanDoActionTestUtils.runAndAssertCanDoActionFailure(cmd, diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/MultipleStorageDomainsValidatorTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/MultipleStorageDomainsValidatorTest.java index 56249af..1d0d0ef 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/MultipleStorageDomainsValidatorTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/MultipleStorageDomainsValidatorTest.java @@ -145,6 +145,36 @@ } @Test + public void testAllDomainsHaveSpaceForClonedDisksSuccess(){ + List<Guid> sdIds = Arrays.asList(sdId1, sdId2); + List<DiskImage> disksList = generateDisksList(NUM_DISKS, sdIds); + + StorageDomainValidator storageDomainValidator = mock(StorageDomainValidator.class); + doReturn(ValidationResult.VALID).when(storageDomainValidator).hasSpaceForClonedDisks(anyList()); + doReturn(storageDomainValidator).when(validator).getStorageDomainValidator(any(Map.Entry.class)); + + assertTrue(validator.allDomainsHaveSpaceForClonedDisks(disksList).isValid()); + verify(storageDomainValidator, times(NUM_DOMAINS)).hasSpaceForClonedDisks(anyList()); + } + + @Test + public void testAllDomainsHaveSpaceForClonedDisksFail(){ + List<Guid> sdIds = Arrays.asList(sdId1, sdId2); + List<DiskImage> disksList = generateDisksList(NUM_DISKS, sdIds); + + StorageDomainValidator storageDomainValidator = mock(StorageDomainValidator.class); + doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN)). + when(storageDomainValidator).hasSpaceForClonedDisks(anyList()); + doReturn(storageDomainValidator).when(validator).getStorageDomainValidator(any(Map.Entry.class)); + + ValidationResult result = validator.allDomainsHaveSpaceForClonedDisks(disksList); + assertFalse(result.isValid()); + assertEquals("Wrong validation error", + VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN, + result.getMessage()); + } + + @Test public void testAllDomainsHaveSpaceForAllDisksSuccess(){ List<Guid> sdIdsForNew = Arrays.asList(sdId1, sdId2); List<Guid> sdIdsForCloned = Arrays.asList(sdId2, sdId3); diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/StorageDomainValidatorFreeSpaceTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/StorageDomainValidatorFreeSpaceTest.java index eead8f8..c53f3cb 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/StorageDomainValidatorFreeSpaceTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/StorageDomainValidatorFreeSpaceTest.java @@ -25,15 +25,18 @@ private boolean isValidForCloned; private boolean isValidForNew; + private boolean isValidForSnapshots; public StorageDomainValidatorFreeSpaceTest(DiskImage disk, StorageDomain sd, boolean isValidForCloned, - boolean isValidForNew) { + boolean isValidForNew, + boolean isValidForSnapshots) { this.disk = disk; this.sd = sd; this.isValidForCloned = isValidForCloned; this.isValidForNew = isValidForNew; + this.isValidForSnapshots = isValidForSnapshots; } @Parameters @@ -60,7 +63,9 @@ params.add(new Object[] { disk, sd, volumeFormat == VolumeFormat.RAW && volumeType == VolumeType.Sparse, - volumeFormat == VolumeFormat.COW || volumeType == VolumeType.Sparse }); + volumeFormat == VolumeFormat.COW || volumeType == VolumeType.Sparse, + volumeFormat == VolumeFormat.RAW && volumeType == VolumeType.Sparse + }); } } } @@ -70,6 +75,14 @@ } @Test + public void testValidateDiskWithSnapshots() { + StorageDomainValidator sdValidator = new StorageDomainValidator(sd); + assertEquals(disk.getVolumeFormat() + ", " + disk.getVolumeType() + ", " + sd.getStorageType(), + isValidForSnapshots, + sdValidator.hasSpaceForDiskWithSnapshots(disk).isValid()); + } + + @Test public void testValidateClonedDisk() { StorageDomainValidator sdValidator = new StorageDomainValidator(sd); assertEquals(disk.getVolumeFormat() + ", " + disk.getVolumeType() + ", " + sd.getStorageType(), diff --git a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DiskImageBase.java b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DiskImageBase.java index 9258685..c44810c 100644 --- a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DiskImageBase.java +++ b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DiskImageBase.java @@ -2,6 +2,7 @@ import javax.validation.Valid; +import org.codehaus.jackson.annotate.JsonIgnore; import org.ovirt.engine.core.compat.Guid; public class DiskImageBase extends Disk { @@ -59,6 +60,11 @@ getImage().setSize(size); } + @JsonIgnore + public long getCapacity() { + return getSize(); + } + /** * disk size in GB */ -- To view, visit http://gerrit.ovirt.org/32898 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ifbb1d985f9afa476452d1d2b78be1fd18c128c8f 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
