Maor Lipchuk has uploaded a new change for review. Change subject: core: Add storage space validation to removeSnapshot ......................................................................
core: Add storage space validation to removeSnapshot Today there is not validation in RemoveSnapshotCommand to ensure there is enough space on the domain to merge the disk snapshots. This space is needed on remove snapshot process since VDSM creates a new temporary volume, as part of the merge process, which might be large as the image actual size. The proposed fix introduces a new validation in the command which validates if each storage domain has enough free space to perform removeSnapshot. Change-Id: Ifbebc5af8e7489bccfa5d8e79695fe21c5371ec9 Bug-Url: https://bugzilla.redhat.com/864892 Signed-off-by: Maor Lipchuk <[email protected]> --- 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/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 4 files changed, 217 insertions(+), 1 deletion(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/49/11849/1 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 534a9a3..1ca583d 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 @@ -2,6 +2,7 @@ import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; @@ -12,6 +13,7 @@ import org.ovirt.engine.core.bll.quota.QuotaStorageDependent; import org.ovirt.engine.core.bll.snapshots.SnapshotsValidator; import org.ovirt.engine.core.bll.storage.StoragePoolValidator; +import org.ovirt.engine.core.bll.validator.StorageDomainValidator; import org.ovirt.engine.core.bll.validator.VmValidator; import org.ovirt.engine.core.common.AuditLogType; import org.ovirt.engine.core.common.VdcObjectType; @@ -23,12 +25,14 @@ import org.ovirt.engine.core.common.businessentities.Snapshot; import org.ovirt.engine.core.common.businessentities.Snapshot.SnapshotStatus; import org.ovirt.engine.core.common.businessentities.VMStatus; +import org.ovirt.engine.core.common.businessentities.storage_domains; import org.ovirt.engine.core.common.errors.VdcBLLException; import org.ovirt.engine.core.common.errors.VdcBllErrors; import org.ovirt.engine.core.common.locks.LockingGroup; import org.ovirt.engine.core.compat.Guid; import org.ovirt.engine.core.dal.VdcBllMessages; import org.ovirt.engine.core.dao.SnapshotDao; +import org.ovirt.engine.core.utils.MultiValueMapUtils; import org.ovirt.engine.core.utils.transaction.TransactionMethod; import org.ovirt.engine.core.utils.transaction.TransactionSupport; @@ -182,9 +186,45 @@ if (!validateImageNotActive()) { return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_CANNOT_REMOVE_ACTIVE_IMAGE); } + + if (!isEnoughSpaceToMergeSnapshots()) { + return false; + } } return true; + } + + /** + * Validates if each storage domain has enough free space to perform removeSnapshot. <BR/> + * The remove snapshot logic in VDSM includes, creating a new temporary volume which might be large as the disk + * actual size. <BR/> + * Hence, as part of the validation, we summarize 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 isEnoughSpaceToMergeSnapshots() { + for (final Guid storageDomainId : getStorageToDiskMap().keySet()) { + List<DiskImage> diskImages = getStorageToDiskMap().get(storageDomainId); + long sizeRequested = 0; + for (DiskImage diskImage : diskImages) { + sizeRequested += diskImage.getActualSize(); + } + storage_domains storageDomain = + getStorageDomainDAO().getForStoragePool(storageDomainId, getStoragePoolId()); + if (!validate(new StorageDomainValidator(storageDomain).isDomainHasSpaceForRequest(sizeRequested))) { + return false; + } + } + return true; + } + + private Map<Guid, List<DiskImage>> getStorageToDiskMap() { + Map<Guid, List<DiskImage>> storageToDisksMap = new HashMap<Guid, List<DiskImage>>(); + for (DiskImage disk : getSourceImages()) { + MultiValueMapUtils.addToMap(disk.getstorage_ids().get(0), disk, storageToDisksMap); + } + return storageToDisksMap; } @Override @@ -210,7 +250,7 @@ protected boolean validateImagesAndVMStates() { return ImagesHandler.PerformImagesChecks(getReturnValue().getCanDoActionMessages(), getVm().getStoragePoolId(), Guid.Empty, - hasImages(), hasImages(), hasImages(), hasImages(), true, true, getDiskDao().getAllForVm(getVmId())); + false, hasImages(), hasImages(), hasImages(), true, true, getDiskDao().getAllForVm(getVmId())); } protected boolean validateImageNotInTemplate() { 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 f79ec10..44e575c 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 @@ -64,6 +64,15 @@ return ValidationResult.VALID; } + public ValidationResult isDomainHasSpaceForRequestWithoutThreshold(final long requestedSize) { + if (storageDomain.getavailable_disk_size() != null && + storageDomain.getavailable_disk_size() - requestedSize < 0) { + return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_TARGET_STORAGE_DOMAIN, + storageName()); + } + return ValidationResult.VALID; + } + private static Integer getLowDiskSpaceThreshold() { return Config.<Integer> GetValue(ConfigValues.FreeSpaceCriticalLowInGB); } 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 b143162..ee94f47 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 @@ -7,26 +7,36 @@ import static org.mockito.Mockito.spy; import static org.mockito.Mockito.when; +import java.util.ArrayList; import java.util.Collections; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.Matchers; import org.mockito.Mock; import org.mockito.runners.MockitoJUnitRunner; import org.ovirt.engine.core.bll.snapshots.SnapshotsValidator; import org.ovirt.engine.core.common.action.RemoveSnapshotParameters; import org.ovirt.engine.core.common.businessentities.DiskImage; +import org.ovirt.engine.core.common.businessentities.StorageDomainStatus; +import org.ovirt.engine.core.common.businessentities.StorageDomainType; import org.ovirt.engine.core.common.businessentities.StoragePoolStatus; 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.storage_domains; import org.ovirt.engine.core.common.businessentities.storage_pool; +import org.ovirt.engine.core.common.config.ConfigValues; import org.ovirt.engine.core.compat.Guid; +import org.ovirt.engine.core.compat.NGuid; import org.ovirt.engine.core.dal.VdcBllMessages; import org.ovirt.engine.core.dao.DiskImageDAO; +import org.ovirt.engine.core.dao.StorageDomainDAO; import org.ovirt.engine.core.dao.StoragePoolDAO; import org.ovirt.engine.core.dao.VmTemplateDAO; +import org.ovirt.engine.core.utils.MockConfigRule; /** A test case for the {@link RemoveSnapshotCommand} class. */ @RunWith(MockitoJUnitRunner.class) @@ -35,8 +45,14 @@ /** The command to test */ private RemoveSnapshotCommand<RemoveSnapshotParameters> cmd; + @Rule + public MockConfigRule mcr = new MockConfigRule(); + @Mock private VmTemplateDAO vmTemplateDAO; + + @Mock + StorageDomainDAO sdDAO; @Mock private DiskImageDAO diskImageDAO; @@ -46,6 +62,11 @@ @Mock private SnapshotsValidator snapshotValidator; + + private static final Guid STORAGE_DOMAIN_ID = Guid.NewGuid(); + private static final Guid STORAGE_DOMAIN_ID2 = Guid.NewGuid(); + + private static final int USED_SPACE_GB = 4; @Before public void setUp() { @@ -58,6 +79,26 @@ doReturn(vmTemplateDAO).when(cmd).getVmTemplateDAO(); doReturn(diskImageDAO).when(cmd).getDiskImageDao(); doReturn(snapshotValidator).when(cmd).createSnapshotValidator(); + mockDAOs(cmd); + mockConfigSizeDefaults(); + } + + private void mockConfigSizeRequirements(int requiredSpaceBufferInGB) { + mcr.mockConfigValue(ConfigValues.FreeSpaceCriticalLowInGB, requiredSpaceBufferInGB); + } + + private void mockConfigSizeDefaults() { + int requiredSpaceBufferInGB = 5; + mockConfigSizeRequirements(requiredSpaceBufferInGB); + } + + private void mockDAOs(RemoveSnapshotCommand<?> cmd) { + doReturn(sdDAO).when(cmd).getStorageDomainDAO(); + } + + private void mockStorageDomainDAOGetForStoragePool(int domainSpaceGB, Guid storageDomainId) { + when(sdDAO.getForStoragePool(Matchers.<Guid> eq(storageDomainId), Matchers.<NGuid> any(NGuid.class))).thenReturn(createStorageDomain(domainSpaceGB, + storageDomainId)); } @Test @@ -82,6 +123,85 @@ public void testValidateImageNotActiveFalse() { when(diskImageDAO.get(mockSourceImage())).thenReturn(new DiskImage()); assertFalse("validation should succeed", cmd.validateImageNotActive()); + } + + @Test + public void testEnoughSpaceToMergeSnapshotsWithOneDisk() { + 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", + cmd.isEnoughSpaceToMergeSnapshots()); + } + + @Test + public void testNotEnoughSpaceToMergeSnapshotsWithOneDisk() { + when(diskImageDAO.get(mockSourceImage())).thenReturn(new DiskImage()); + mockStorageDomainDAOGetForStoragePool(7, STORAGE_DOMAIN_ID); + assertFalse("Validation should fail. Free space minus threshold should be smaller then disk size", + cmd.isEnoughSpaceToMergeSnapshots()); + } + + @Test + public void testEnoughSpaceToMergeSnapshotsWithMultipleDisk() { + ArrayList<DiskImage> imagesDisks = mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID, 4); + doReturn(imagesDisks).when(cmd).getSourceImages(); + mockStorageDomainDAOGetForStoragePool(22, STORAGE_DOMAIN_ID); + assertTrue("Validation should succeed. Free space minus threshold should be bigger then summarize all disks size", + cmd.isEnoughSpaceToMergeSnapshots()); + } + + @Test + public void testNotEnoughSpaceToMergeSnapshotsWithMultipleDisk() { + ArrayList<DiskImage> imagesDisks = mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID, 4); + System.out.println(imagesDisks); + doReturn(imagesDisks).when(cmd).getSourceImages(); + mockStorageDomainDAOGetForStoragePool(20, STORAGE_DOMAIN_ID); + assertFalse("Validation should fail. Free space minus threshold should be smaller then summarize all disks size", + cmd.isEnoughSpaceToMergeSnapshots()); + } + + @Test + public void testEnoughSpaceToMergeSnapshotsWithMultipleDiskAndDomains() { + ArrayList<DiskImage> imagesDisks = mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID, 4); + imagesDisks.addAll(mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID2, 4)); + doReturn(imagesDisks).when(cmd).getSourceImages(); + mockStorageDomainDAOGetForStoragePool(22, STORAGE_DOMAIN_ID); + mockStorageDomainDAOGetForStoragePool(22, STORAGE_DOMAIN_ID2); + assertTrue("Validation should succeed. Free space minus threshold should be bigger then summarize all disks size for each domain", + cmd.isEnoughSpaceToMergeSnapshots()); + } + + @Test + public void testNotEnoughForMultipleDiskAndDomainsFirstDomainFails() { + ArrayList<DiskImage> imagesDisks = mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID, 4); + imagesDisks.addAll(mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID2, 4)); + doReturn(imagesDisks).when(cmd).getSourceImages(); + mockStorageDomainDAOGetForStoragePool(20, STORAGE_DOMAIN_ID); + mockStorageDomainDAOGetForStoragePool(22, STORAGE_DOMAIN_ID2); + assertFalse("Validation should fail. First domain should not have enough free space for request.", + cmd.isEnoughSpaceToMergeSnapshots()); + } + + @Test + public void testNotEnoughForMultipleDiskAndDomainsSecondDomainFails() { + ArrayList<DiskImage> imagesDisks = mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID, 4); + imagesDisks.addAll(mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID2, 4)); + doReturn(imagesDisks).when(cmd).getSourceImages(); + mockStorageDomainDAOGetForStoragePool(22, STORAGE_DOMAIN_ID); + mockStorageDomainDAOGetForStoragePool(10, STORAGE_DOMAIN_ID2); + assertFalse("Validation should fail. Second domain should not have enough free space for request.", + cmd.isEnoughSpaceToMergeSnapshots()); + } + + @Test + public void testNotEnoughForMultipleDiskAndDomainsAllDomainsFail() { + ArrayList<DiskImage> imagesDisks = mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID, 4); + imagesDisks.addAll(mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID2, 4)); + doReturn(imagesDisks).when(cmd).getSourceImages(); + mockStorageDomainDAOGetForStoragePool(10, STORAGE_DOMAIN_ID); + mockStorageDomainDAOGetForStoragePool(10, STORAGE_DOMAIN_ID2); + assertFalse("Validation should fail. Second domain should not have enough free space for request.", + cmd.isEnoughSpaceToMergeSnapshots()); } @Test @@ -112,7 +232,38 @@ Guid imageId = Guid.NewGuid(); DiskImage image = new DiskImage(); image.setImageId(imageId); + ArrayList<Guid> list = new ArrayList<Guid>(); + list.add(STORAGE_DOMAIN_ID); + image.setstorage_ids(list); + image.setActualSize(4); + image.setsize(40); doReturn(Collections.singletonList(image)).when(cmd).getSourceImages(); return imageId; } + + /** Mocks a call to {@link RemoveSnapshotCommand#getSourceImages()} and returns list of images */ + private ArrayList<DiskImage> mockMultipleSourceImagesForDomain(int numberOfDisks, Guid storageDomainId, int actualDiskSize) { + ArrayList<DiskImage> listDisks = new ArrayList<DiskImage>(); + for (int index=0; index < numberOfDisks; index++) { + Guid imageId = Guid.NewGuid(); + DiskImage image = new DiskImage(); + image.setImageId(imageId); + ArrayList<Guid> list = new ArrayList<Guid>(); + list.add(storageDomainId); + image.setstorage_ids(list); + image.setActualSize(actualDiskSize); + listDisks.add(image); + } + return listDisks; + } + + protected storage_domains createStorageDomain(int availableSpace, Guid storageDomainId) { + storage_domains sd = new storage_domains(); + sd.setstorage_domain_type(StorageDomainType.Master); + sd.setstatus(StorageDomainStatus.Active); + sd.setavailable_disk_size(availableSpace); + sd.setused_disk_size(USED_SPACE_GB); + 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 1bb58fd..8581a65 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 @@ -70,6 +70,22 @@ } @Test + public void testDomainWithNotEnoughSpaceForRequestWithoutThreshold() { + validator = new StorageDomainValidator(mockStorageDomain(12, 748, StorageType.NFS)); + assertEquals("Wrong failure for not enough space for request", + VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_TARGET_STORAGE_DOMAIN, + validator.isDomainHasSpaceForRequest(13).getMessage()); + } + + @Test + public void testDomainWithEnoughSpaceForRequestWithoutThreshold() { + validator = new StorageDomainValidator(mockStorageDomain(12, 748, StorageType.NFS)); + assertEquals("Wrong failure for not enough space for request", + VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_TARGET_STORAGE_DOMAIN, + validator.isDomainHasSpaceForRequest(12).getMessage()); + } + + @Test public void testDomainWithEnoughSpaceForRequest() { validator = new StorageDomainValidator(mockStorageDomain(16, 748, StorageType.NFS)); assertTrue("Domain should have more space then threshold", validator.isDomainHasSpaceForRequest(10).isValid()); -- To view, visit http://gerrit.ovirt.org/11849 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ifbebc5af8e7489bccfa5d8e79695fe21c5371ec9 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
