Vered Volansky has uploaded a new change for review. Change subject: core: Merge snapshots storage allocation ......................................................................
core: Merge snapshots 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 RemoveSnapshotCommand and RemoveDiskSnapshotsCommand. This also resulted in removing tests of old/buggy verification. Change-Id: I8081eaf60af50ad8d894454244fb709f4a08d5c6 Bug-Url: https://bugzilla.redhat.com/1053733 Signed-off-by: Vered Volansky <[email protected]> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotsCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotCommand.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/RemoveSnapshotCommandTest.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/StorageDomainValidatorTest.java 6 files changed, 57 insertions(+), 110 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/96/33696/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotsCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotsCommand.java index 34f408c..365a097 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotsCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotsCommand.java @@ -264,7 +264,11 @@ } protected boolean validateStorageDomainAvailableSpace() { - return validate(getStorageDomainValidator().hasSpaceForRemovingDiskSnapshots(getImages())); + // What should be checked here is that there's enough space for removing a set of disk snapshots consecutively. + // Worst-case scenario when merging a snapshot in terms of space, is the outcome volume, along with the not-yet-deleted volumes. + // The following implementation does just that. In this case only snapshots are passed to the validation + // (as opposed to the whole chain). + return validate(getStorageDomainValidator().hasSpaceForClonedDisks(getImages())); } protected DiskImagesValidator createDiskImageValidator(List<DiskImage> disksList) { diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotCommand.java index d9aa8d1..9b9ff16 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotCommand.java @@ -1,11 +1,10 @@ package org.ovirt.engine.core.bll; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; -import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Map.Entry; import org.apache.commons.lang.StringUtils; import org.ovirt.engine.core.bll.quota.QuotaConsumptionParameter; @@ -16,7 +15,7 @@ import org.ovirt.engine.core.bll.tasks.CommandCoordinatorUtil; import org.ovirt.engine.core.bll.tasks.interfaces.CommandCallBack; import org.ovirt.engine.core.bll.validator.DiskImagesValidator; -import org.ovirt.engine.core.bll.validator.StorageDomainValidator; +import org.ovirt.engine.core.bll.validator.MultipleStorageDomainsValidator; import org.ovirt.engine.core.bll.validator.VmValidator; import org.ovirt.engine.core.common.AuditLogType; import org.ovirt.engine.core.common.FeatureSupported; @@ -34,7 +33,6 @@ import org.ovirt.engine.core.common.businessentities.DiskImage; import org.ovirt.engine.core.common.businessentities.Snapshot; import org.ovirt.engine.core.common.businessentities.Snapshot.SnapshotStatus; -import org.ovirt.engine.core.common.businessentities.StorageDomain; import org.ovirt.engine.core.common.businessentities.VM; import org.ovirt.engine.core.common.errors.VdcBLLException; import org.ovirt.engine.core.common.errors.VdcBllErrors; @@ -44,7 +42,6 @@ import org.ovirt.engine.core.compat.Guid; import org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogDirector; import org.ovirt.engine.core.dao.SnapshotDao; -import org.ovirt.engine.core.utils.collections.MultiValueMapUtils; import org.ovirt.engine.core.utils.transaction.TransactionMethod; import org.ovirt.engine.core.utils.transaction.TransactionSupport; @@ -336,43 +333,26 @@ /** * Validates the storage domains. * - * Each domain is validated for status and for enough free space to perform removeSnapshot. <BR/> + * Each domain is validated for status, threshold and for enough free space to perform removeSnapshot. * The remove snapshot logic in VDSM includes creating a new temporary volume which might be as large as the disk's - * actual size. <BR/> + * actual size. * Hence, as part of the validation, we sum up all the disks virtual sizes, for each storage domain. * * @return True if there is enough space in all relevant storage domains. False otherwise. */ protected boolean validateStorageDomains() { - for (final Entry<Guid, List<DiskImage>> storageToDiskEntry : getStorageToDiskMap().entrySet()) { - Guid storageDomainId = storageToDiskEntry.getKey(); - StorageDomain storageDomain = - getStorageDomainDAO().getForStoragePool(storageDomainId, getStoragePoolId()); - StorageDomainValidator validator = new StorageDomainValidator(storageDomain); - - if (!validate(validator.isDomainExistAndActive())) { - return false; - } - - List<DiskImage> diskImages = storageToDiskEntry.getValue(); - long sizeRequested = 0L; - for (DiskImage diskImage : diskImages) { - sizeRequested += diskImage.getActualSize(); - } - - if (!validate(validator.isDomainHasSpaceForRequest(sizeRequested, false))) { - return false; - } - } - return true; + MultipleStorageDomainsValidator storageDomainsValidator = getStorageDomainsValidator(getStoragePoolId(), getStorageDomainsIds()); + return validate(storageDomainsValidator.allDomainsExistAndActive()) + && validate(storageDomainsValidator.allDomainsWithinThresholds()) + && validate(storageDomainsValidator.allDomainsHaveSpaceForClonedDisks(getSourceImages())); } - private Map<Guid, List<DiskImage>> getStorageToDiskMap() { - Map<Guid, List<DiskImage>> storageToDisksMap = new HashMap<Guid, List<DiskImage>>(); - for (DiskImage disk : getSourceImages()) { - MultiValueMapUtils.addToMap(disk.getStorageIds().get(0), disk, storageToDisksMap); - } - return storageToDisksMap; + protected Collection<Guid> getStorageDomainsIds() { + return ImagesHandler.getAllStorageIdsForImageIds(getSourceImages()); + } + + protected MultipleStorageDomainsValidator getStorageDomainsValidator(Guid spId, Collection<Guid> sdIds) { + return new MultipleStorageDomainsValidator(spId, sdIds); } @Override 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 d00cc88..7582cb5 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 @@ -159,7 +159,7 @@ } /** @return The DAO object used to retrieve storage domains */ - protected StorageDomainDAO getStorageDomainDAO() { + public StorageDomainDAO getStorageDomainDAO() { return DbFacade.getInstance().getStorageDomainDao(); } 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 005caff..ce1f87b 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 @@ -286,27 +286,6 @@ } /** - * Calculates the requires space for removing a set of disk snapshots consequently. - * The calculation takes into account that the required space (for each snapshots merge) - * couldn't be larger than either of disk's virtual size and sum of the actual sizes. - * I.e. the minimum between values. - * - * @param diskImages - * @return the required space - */ - public ValidationResult hasSpaceForRemovingDiskSnapshots(Collection<DiskImage> diskImages) { - // Sums the actual sizes and finds the max virtual size - long sumOfActualSizes = 0L; - long maxVirtualSize = 0L; - for (DiskImage image : diskImages) { - sumOfActualSizes += (long) image.getActualSize(); - maxVirtualSize = Math.max(maxVirtualSize, image.getSize()); - } - - 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 diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RemoveSnapshotCommandTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RemoveSnapshotCommandTest.java index b2549a1..8db6b62 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RemoveSnapshotCommandTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RemoveSnapshotCommandTest.java @@ -4,14 +4,18 @@ import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyBoolean; +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.Arrays; import java.util.Collections; +import java.util.HashSet; import java.util.List; +import java.util.Set; import org.junit.Before; import org.junit.Rule; @@ -20,6 +24,7 @@ import org.mockito.Mock; import org.mockito.runners.MockitoJUnitRunner; import org.ovirt.engine.core.bll.snapshots.SnapshotsValidator; +import org.ovirt.engine.core.bll.validator.MultipleStorageDomainsValidator; import org.ovirt.engine.core.bll.validator.VmValidator; import org.ovirt.engine.core.common.action.RemoveSnapshotParameters; import org.ovirt.engine.core.common.businessentities.DiskImage; @@ -30,9 +35,11 @@ import org.ovirt.engine.core.common.businessentities.StorageDomainType; import org.ovirt.engine.core.common.businessentities.StoragePool; import org.ovirt.engine.core.common.businessentities.StoragePoolStatus; +import org.ovirt.engine.core.common.businessentities.StorageType; import org.ovirt.engine.core.common.businessentities.VM; import org.ovirt.engine.core.common.businessentities.VMStatus; import org.ovirt.engine.core.common.businessentities.VmTemplate; +import org.ovirt.engine.core.common.businessentities.VolumeFormat; import org.ovirt.engine.core.common.config.ConfigValues; import org.ovirt.engine.core.common.errors.VdcBllMessages; import org.ovirt.engine.core.compat.Guid; @@ -74,11 +81,13 @@ private VmValidator vmValidator; + private MultipleStorageDomainsValidator storageDomainsValidator; + private static final Guid STORAGE_DOMAIN_ID = Guid.newGuid(); private static final Guid STORAGE_DOMAIN_ID2 = Guid.newGuid(); private static final Guid STORAGE_POOLD_ID = Guid.newGuid(); - private static final int USED_SPACE_GB = 4; + //private static final int USED_SPACE_GB = 4; private static final int IMAGE_ACTUAL_SIZE_GB = 4; @Before @@ -102,6 +111,7 @@ snapshotValidator = spy(new SnapshotsValidator()); doReturn(snapshotValidator).when(cmd).createSnapshotValidator(); mockConfigSizeDefaults(); + spySdValidator(); } private void mockVm() { @@ -134,6 +144,24 @@ storageDomainId)); } + private void spySdValidatorForOneDomain() { + Set<Guid> sdIds = new HashSet<>(Arrays.asList(STORAGE_DOMAIN_ID)); + spySdValidator(sdIds); + } + + private void spySdValidator() { + Set<Guid> sdIds = new HashSet<>(Arrays.asList(STORAGE_DOMAIN_ID, STORAGE_DOMAIN_ID2)); + spySdValidator(sdIds); + } + + private void spySdValidator(Set<Guid> sdIds) { + storageDomainsValidator = spy(new MultipleStorageDomainsValidator(STORAGE_POOLD_ID, sdIds)); + doReturn(storageDomainsValidator).when(cmd).getStorageDomainsValidator(any(Guid.class), anySet()); + doReturn(ValidationResult.VALID).when(storageDomainsValidator).allDomainsExistAndActive(); + doReturn(sdDAO).when(storageDomainsValidator).getStorageDomainDAO(); + doReturn(sdIds).when(cmd).getStorageDomainsIds(); + } + @Test public void testValidateImageNotInTemplateTrue() { when(vmTemplateDAO.get(mockSourceImage())).thenReturn(null); @@ -160,6 +188,7 @@ @Test public void testEnoughSpaceToMergeSnapshotsWithOneDisk() { + spySdValidatorForOneDomain(); when(diskImageDAO.get(mockSourceImage())).thenReturn(new DiskImage()); mockStorageDomainDAOGetForStoragePool(10, STORAGE_DOMAIN_ID); assertTrue("Validation should succeed. Free space minus threshold should be bigger then disk size", @@ -168,6 +197,7 @@ @Test public void testNotEnoughSpaceToMergeSnapshotsWithOneDisk() { + spySdValidatorForOneDomain(); when(diskImageDAO.get(mockSourceImage())).thenReturn(new DiskImage()); mockStorageDomainDAOGetForStoragePool(3, STORAGE_DOMAIN_ID); assertFalse("Validation should fail. Free space minus threshold should be smaller then disk size", @@ -176,6 +206,7 @@ @Test public void testEnoughSpaceToMergeSnapshotsWithMultipleDisk() { + spySdValidatorForOneDomain(); List<DiskImage> imagesDisks = mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID, IMAGE_ACTUAL_SIZE_GB); doReturn(imagesDisks).when(cmd).getSourceImages(); mockStorageDomainDAOGetForStoragePool(22, STORAGE_DOMAIN_ID); @@ -185,6 +216,7 @@ @Test public void testNotEnoughSpaceToMergeSnapshotsWithMultipleDisk() { + spySdValidatorForOneDomain(); List<DiskImage> imagesDisks = mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID, IMAGE_ACTUAL_SIZE_GB); doReturn(imagesDisks).when(cmd).getSourceImages(); mockStorageDomainDAOGetForStoragePool(15, STORAGE_DOMAIN_ID); @@ -315,6 +347,9 @@ list.add(storageDomainId); image.setStorageIds(list); image.setActualSize(actualDiskSize); + image.setSizeInGigabytes(actualDiskSize); + image.setvolumeFormat(VolumeFormat.COW); + image.getSnapshots().add(image); listDisks.add(image); } return listDisks; @@ -325,7 +360,7 @@ sd.setStorageDomainType(StorageDomainType.Master); sd.setStatus(StorageDomainStatus.Active); sd.setAvailableDiskSize(availableSpace); - sd.setUsedDiskSize(USED_SPACE_GB); + sd.setStorageType(StorageType.ISCSI); sd.setStoragePoolId(STORAGE_POOLD_ID); sd.setId(storageDomainId); return sd; diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/StorageDomainValidatorTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/StorageDomainValidatorTest.java index 42e8f7f..f3d27cd 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/StorageDomainValidatorTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/StorageDomainValidatorTest.java @@ -7,15 +7,12 @@ import org.junit.Before; import org.junit.ClassRule; import org.junit.Test; -import org.ovirt.engine.core.common.businessentities.DiskImage; import org.ovirt.engine.core.common.businessentities.StorageDomain; import org.ovirt.engine.core.common.businessentities.StorageDomainStatus; import org.ovirt.engine.core.common.businessentities.StorageType; import org.ovirt.engine.core.common.config.ConfigValues; import org.ovirt.engine.core.common.errors.VdcBllMessages; import org.ovirt.engine.core.utils.MockConfigRule; - -import java.util.Arrays; /** * A test case for the {@link StorageDomainValidator} class. @@ -103,46 +100,6 @@ assertTrue("Domain should have more space then threshold", validator.isDomainWithinThresholds().isValid()); } - @Test - public void testDomainWithEnoughSizeToRemoveDiskSnapshotsSumOfActualSizes() { - validator = new StorageDomainValidator(mockStorageDomain(1024, 0, StorageType.NFS)); - DiskImage image1 = mockDiskImage(128, 1024); - DiskImage image2 = mockDiskImage(256, 2048); - - assertTrue("Domain should have enough space for merging the snapshots", - validator.hasSpaceForRemovingDiskSnapshots(Arrays.asList(image1, image2)).isValid()); - } - - @Test - public void testDomainWithEnoughSizeToRemoveDiskSnapshotsMaxVirtualSize() { - validator = new StorageDomainValidator(mockStorageDomain(1024, 0, StorageType.NFS)); - DiskImage image1 = mockDiskImage(1024, 1024); - DiskImage image2 = mockDiskImage(256, 1024); - - assertTrue("Domain should have enough space for merging the snapshots", - validator.hasSpaceForRemovingDiskSnapshots(Arrays.asList(image1, image2)).isValid()); - } - - @Test - public void testDomainWithNotEnoughSizeToRemoveDiskSnapshotsSumOfActualSizes() { - validator = new StorageDomainValidator(mockStorageDomain(1024, 0, StorageType.NFS)); - DiskImage image1 = mockDiskImage(1024, 1512); - DiskImage image2 = mockDiskImage(256, 1512); - - assertTrue("Domain should not have enough space for merging the snapshots", - !validator.hasSpaceForRemovingDiskSnapshots(Arrays.asList(image1, image2)).isValid()); - } - - @Test - public void testDomainWithNotEnoughSizeToRemoveDiskSnapshotsMaxVirtualSize() { - validator = new StorageDomainValidator(mockStorageDomain(512, 0, StorageType.NFS)); - DiskImage image1 = mockDiskImage(768, 1024); - DiskImage image2 = mockDiskImage(768, 1024); - - assertTrue("Domain should not have enough space for merging the snapshots", - !validator.hasSpaceForRemovingDiskSnapshots(Arrays.asList(image1, image2)).isValid()); - } - private static StorageDomain mockStorageDomain(int availableSize, int usedSize, StorageType storageType) { StorageDomain sd = new StorageDomain(); sd.setAvailableDiskSize(availableSize); @@ -150,13 +107,5 @@ sd.setStatus(StorageDomainStatus.Active); sd.setStorageType(storageType); return sd; - } - - private static DiskImage mockDiskImage(long actualSize, long virtualSize) { - DiskImage image = new DiskImage(); - image.setActualSize(actualSize); - image.setSize(virtualSize); - - return image; } } -- To view, visit http://gerrit.ovirt.org/33696 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I8081eaf60af50ad8d894454244fb709f4a08d5c6 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
