Vered Volansky has uploaded a new change for review. Change subject: core: Fix storage allocation check when merging a snapshot ......................................................................
core: Fix storage allocation check when merging a snapshot When merging a snapshot we need extra available space for the snapshot being removed, plus this snapshot's child (the one on top of the deleted one). This patch takes the child-snapshot's size into consideration. Change-Id: I41213884ed0ca9ddf8354ab7be58f032a6d90673 Bug-Url: https://bugzilla.redhat.com/1178010 Bug-Url: https://bugzilla.redhat.com/1053733 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/RemoveDiskSnapshotsCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotCommand.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotsCommandTest.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RemoveSnapshotCommandTest.java 5 files changed, 91 insertions(+), 145 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/55/37255/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 403f660..4f4614a 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 @@ -873,4 +873,20 @@ } return diskDummies; } + + public static List<DiskImage> getSnapshotsDummiesForStorageAllocations(Collection<DiskImage> originalDisks) { + List<DiskImage> diskDummies = new ArrayList<>(); + for (DiskImage snapshot : originalDisks) { + DiskImage clone = DiskImage.copyOf(snapshot); + // Add the child snapshot into which the deleted snapshot is going to be merged to the + // DiskImage for StorageDomainValidator to handle + List<DiskImage> snapshots = DbFacade.getInstance().getDiskImageDao().getAllSnapshotsForParent(clone.getImageId()); + clone.getSnapshots().clear(); + clone.getSnapshots().add(clone); // Add the clone itself since snapshots should contain the entire chain. + clone.getSnapshots().addAll(snapshots); + diskDummies.add(clone); + } + return diskDummies; + } + } 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 f7f2e97..9a43752 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 @@ -417,7 +417,8 @@ // 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())); + List<DiskImage> disksList = ImagesHandler.getSnapshotsDummiesForStorageAllocations(getImages()); + return validate(getStorageDomainValidator().hasSpaceForClonedDisks(disksList)); } 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 eab311a..a0fca1a 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 @@ -341,10 +341,15 @@ * @return True if there is enough space in all relevant storage domains. False otherwise. */ protected boolean validateStorageDomains() { + List<DiskImage> disksList = getSnapshotsDummiesForStorageAllocations(); MultipleStorageDomainsValidator storageDomainsValidator = getStorageDomainsValidator(getStoragePoolId(), getStorageDomainsIds()); return validate(storageDomainsValidator.allDomainsExistAndActive()) && validate(storageDomainsValidator.allDomainsWithinThresholds()) - && validate(storageDomainsValidator.allDomainsHaveSpaceForClonedDisks(getSourceImages())); + && validate(storageDomainsValidator.allDomainsHaveSpaceForClonedDisks(disksList)); + } + + protected List<DiskImage> getSnapshotsDummiesForStorageAllocations() { + return ImagesHandler.getSnapshotsDummiesForStorageAllocations(getSourceImages()); } protected Collection<Guid> getStorageDomainsIds() { diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotsCommandTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotsCommandTest.java index 4b2b3fb..9f67033 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotsCommandTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RemoveDiskSnapshotsCommandTest.java @@ -191,6 +191,7 @@ doReturn(true).when(cmd).isLiveMergeSupported(); doReturn(ValidationResult.VALID).when(vmValidator).vmQualifiedForSnapshotMerge(); doReturn(ValidationResult.VALID).when(vmValidator).vmHostCanLiveMerge(); + doReturn(true).when(cmd).validateStorageDomainAvailableSpace(); CanDoActionTestUtils.runAndAssertCanDoActionSuccess(cmd); } } 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 8db6b62..f7eca43 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,6 +4,7 @@ import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyBoolean; +import static org.mockito.Matchers.anyList; import static org.mockito.Matchers.anySet; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.spy; @@ -30,16 +31,11 @@ import org.ovirt.engine.core.common.businessentities.DiskImage; import org.ovirt.engine.core.common.businessentities.Snapshot; import org.ovirt.engine.core.common.businessentities.Snapshot.SnapshotType; -import org.ovirt.engine.core.common.businessentities.StorageDomain; -import org.ovirt.engine.core.common.businessentities.StorageDomainStatus; -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; @@ -55,7 +51,9 @@ @RunWith(MockitoJUnitRunner.class) public class RemoveSnapshotCommandTest { - /** The command to test */ + /** + * The command to test + */ private RemoveSnapshotCommand<RemoveSnapshotParameters> cmd; @Rule @@ -85,10 +83,7 @@ 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 IMAGE_ACTUAL_SIZE_GB = 4; + private static final Guid STORAGE_POOL_ID = Guid.newGuid(); @Before public void setUp() { @@ -106,7 +101,7 @@ vmValidator = spy(new VmValidator(cmd.getVm())); doReturn(ValidationResult.VALID).when(vmValidator).vmNotHavingDeviceSnapshotsAttachedToOtherVms(anyBoolean()); doReturn(vmValidator).when(cmd).createVmValidator(any(VM.class)); - doReturn(STORAGE_POOLD_ID).when(cmd).getStoragePoolId(); + doReturn(STORAGE_POOL_ID).when(cmd).getStoragePoolId(); mockSnapshot(SnapshotType.REGULAR); snapshotValidator = spy(new SnapshotsValidator()); doReturn(snapshotValidator).when(cmd).createSnapshotValidator(); @@ -118,7 +113,7 @@ VM vm = new VM(); vm.setId(Guid.newGuid()); vm.setStatus(VMStatus.Down); - vm.setStoragePoolId(STORAGE_POOLD_ID); + vm.setStoragePoolId(STORAGE_POOL_ID); vm.setVdsGroupCompatibilityVersion(Version.v3_5); doReturn(vm).when(cmd).getVm(); } @@ -139,38 +134,27 @@ mockConfigSizeRequirements(requiredSpaceBufferInGB); } - private void mockStorageDomainDAOGetForStoragePool(int domainSpaceGB, Guid storageDomainId) { - when(sdDAO.getForStoragePool(storageDomainId, STORAGE_POOLD_ID)).thenReturn(createStorageDomain(domainSpaceGB, - 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)); + storageDomainsValidator = spy(new MultipleStorageDomainsValidator(STORAGE_POOL_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(); + doReturn(ValidationResult.VALID).when(storageDomainsValidator).allDomainsExistAndActive(); + doReturn(ValidationResult.VALID).when(storageDomainsValidator).allDomainsWithinThresholds(); + doReturn(ValidationResult.VALID).when(storageDomainsValidator).allDomainsHaveSpaceForClonedDisks(anyList()); } @Test public void testValidateImageNotInTemplateTrue() { - when(vmTemplateDAO.get(mockSourceImage())).thenReturn(null); + when(vmTemplateDAO.get(mockSourceImageAndGetId())).thenReturn(null); assertTrue("validation should succeed", cmd.validateImageNotInTemplate()); } @Test public void testValidateImageNotInTemplateFalse() { - when(vmTemplateDAO.get(mockSourceImage())).thenReturn(new VmTemplate()); + when(vmTemplateDAO.get(mockSourceImageAndGetId())).thenReturn(new VmTemplate()); assertFalse("validation should succeed", cmd.validateImageNotInTemplate()); } @@ -187,90 +171,32 @@ } @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", - cmd.validateStorageDomains()); + public void testCanDoActionEnoughSpace() { + prepareForVmValidatorTests(); + spySdValidator(); + cmd.getVm().setStatus(VMStatus.Up); + doReturn(ValidationResult.VALID).when(vmValidator).vmHostCanLiveMerge(); + + mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID); + CanDoActionTestUtils.runAndAssertCanDoActionSuccess(cmd); } @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", - cmd.validateStorageDomains()); - } + public void testCanDoActionNotEnoughSpace() { + prepareForVmValidatorTests(); + spySdValidator(); + cmd.getVm().setStatus(VMStatus.Up); + doReturn(ValidationResult.VALID).when(vmValidator).vmHostCanLiveMerge(); - @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); - assertTrue("Validation should succeed. Free space minus threshold should be bigger then summarize all disks size", - cmd.validateStorageDomains()); - } - - @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); - assertFalse("Validation should fail. Free space minus threshold should be smaller then summarize all disks size", - cmd.validateStorageDomains()); - } - - @Test - public void testEnoughSpaceToMergeSnapshotsWithMultipleDiskAndDomains() { - List<DiskImage> imagesDisks = mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID, IMAGE_ACTUAL_SIZE_GB); - imagesDisks.addAll(mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID2, IMAGE_ACTUAL_SIZE_GB)); - 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.validateStorageDomains()); - } - - @Test - public void testNotEnoughForMultipleDiskAndDomainsFirstDomainFails() { - List<DiskImage> imagesDisks = mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID, IMAGE_ACTUAL_SIZE_GB); - imagesDisks.addAll(mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID2, IMAGE_ACTUAL_SIZE_GB)); - doReturn(imagesDisks).when(cmd).getSourceImages(); - mockStorageDomainDAOGetForStoragePool(15, STORAGE_DOMAIN_ID); - mockStorageDomainDAOGetForStoragePool(22, STORAGE_DOMAIN_ID2); - assertFalse("Validation should fail. First domain should not have enough free space for request.", - cmd.validateStorageDomains()); - } - - @Test - public void testNotEnoughForMultipleDiskAndDomainsSecondDomainFails() { - List<DiskImage> imagesDisks = mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID, IMAGE_ACTUAL_SIZE_GB); - imagesDisks.addAll(mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID2, IMAGE_ACTUAL_SIZE_GB)); - 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.validateStorageDomains()); - } - - @Test - public void testNotEnoughForMultipleDiskAndDomainsAllDomainsFail() { - List<DiskImage> imagesDisks = mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID, IMAGE_ACTUAL_SIZE_GB); - imagesDisks.addAll(mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID2, IMAGE_ACTUAL_SIZE_GB)); - 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.validateStorageDomains()); + List<DiskImage> imagesDisks = mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID); + when(storageDomainsValidator.allDomainsHaveSpaceForClonedDisks(imagesDisks)).thenReturn( + new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN)); + CanDoActionTestUtils.runAndAssertCanDoActionFailure(cmd, VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN); } private void prepareForVmValidatorTests() { StoragePool sp = new StoragePool(); - sp.setId(STORAGE_POOLD_ID); + sp.setId(STORAGE_POOL_ID); sp.setStatus(StoragePoolStatus.Up); cmd.setSnapshotName("someSnapshot"); @@ -278,7 +204,7 @@ doReturn(ValidationResult.VALID).when(snapshotValidator).vmNotInPreview(any(Guid.class)); doReturn(ValidationResult.VALID).when(snapshotValidator).snapshotExists(any(Guid.class), any(Guid.class)); doReturn(true).when(cmd).validateImages(); - doReturn(sp).when(spDao).get(STORAGE_POOLD_ID); + doReturn(sp).when(spDao).get(STORAGE_POOL_ID); doReturn(Collections.emptyList()).when(cmd).getSourceImages(); } @@ -313,6 +239,7 @@ CanDoActionTestUtils.runAndAssertCanDoActionFailure(cmd, VdcBllMessages.ACTION_TYPE_FAILED_VM_IS_NOT_DOWN_OR_UP); } + @Test public void vmHasPluggedDdeviceSnapshotsAttachedToOtherVms() { prepareForVmValidatorTests(); @@ -322,47 +249,43 @@ VdcBllMessages.ACTION_TYPE_FAILED_VM_DISK_SNAPSHOT_IS_ATTACHED_TO_ANOTHER_VM); } - /** Mocks a call to {@link RemoveSnapshotCommand#getSourceImages()} and returns its image guid */ - private Guid mockSourceImage() { - Guid imageId = Guid.newGuid(); + /** + * Mocks a call to {@link RemoveSnapshotCommand#getSourceImages()} and returns its image guid + */ + private Guid mockSourceImageAndGetId() { + return mockSourceImage().getImageId(); + } + + private DiskImage createDiskImage(Guid storageDomainId) { DiskImage image = new DiskImage(); - image.setImageId(imageId); - ArrayList<Guid> list = new ArrayList<Guid>(); - list.add(STORAGE_DOMAIN_ID); - image.setStorageIds(list); - image.setActualSize(IMAGE_ACTUAL_SIZE_GB); - image.setSize(40); + image.setImageId(Guid.newGuid()); + ArrayList<Guid> sdIds = new ArrayList<>(Arrays.asList(storageDomainId)); + image.setStorageIds(sdIds); + return image; + } + + /** + * Mocks a call to {@link RemoveSnapshotCommand#getSourceImages()} and returns the DiskImage + */ + private DiskImage mockSourceImage() { + DiskImage image = createDiskImage(STORAGE_DOMAIN_ID); doReturn(Collections.singletonList(image)).when(cmd).getSourceImages(); - return imageId; + when(diskImageDAO.get(image.getImageId())).thenReturn(image); + + return image; } - /** Mocks a call to {@link RemoveSnapshotCommand#getSourceImages()} and returns list of images */ - private static List<DiskImage> mockMultipleSourceImagesForDomain(int numberOfDisks, Guid storageDomainId, int actualDiskSize) { - List<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.setStorageIds(list); - image.setActualSize(actualDiskSize); - image.setSizeInGigabytes(actualDiskSize); - image.setvolumeFormat(VolumeFormat.COW); - image.getSnapshots().add(image); - listDisks.add(image); + /** + * Mocks a call to {@link RemoveSnapshotCommand#getSourceImages()} and returns list of images + */ + private List<DiskImage> mockMultipleSourceImagesForDomain(int numberOfDisks, Guid storageDomainId) { + List<DiskImage> disksList = new ArrayList<>(numberOfDisks); + for (int index = 0; index < numberOfDisks; index++) { + DiskImage image =createDiskImage(storageDomainId); + disksList.add(image); } - return listDisks; - } - - private static StorageDomain createStorageDomain(int availableSpace, Guid storageDomainId) { - StorageDomain sd = new StorageDomain(); - sd.setStorageDomainType(StorageDomainType.Master); - sd.setStatus(StorageDomainStatus.Active); - sd.setAvailableDiskSize(availableSpace); - sd.setStorageType(StorageType.ISCSI); - sd.setStoragePoolId(STORAGE_POOLD_ID); - sd.setId(storageDomainId); - return sd; + doReturn(disksList).when(cmd).getSourceImages(); + doReturn(disksList).when(cmd).getSnapshotsDummiesForStorageAllocations(); + return disksList; } } -- To view, visit http://gerrit.ovirt.org/37255 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I41213884ed0ca9ddf8354ab7be58f032a6d90673 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
