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 parent. This patch adds the parent's size into consideration. Change-Id: I41213884ed0ca9ddf8354ab7be58f032a6d90673 Bug-Url: https://bugzilla.redhat.com/1178010 Signed-off-by: Vered Volansky <vvola...@redhat.com> --- 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/RemoveSnapshotCommandTest.java 3 files changed, 44 insertions(+), 14 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/14/37014/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 4e4f5e1..d34bccc 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,12 @@ // 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> snapshots = getImages(); + List<DiskImage> snapshotsAndParents = new ArrayList<>(snapshots); + for (DiskImage snapshot : snapshots) { + snapshotsAndParents.add(getDiskImageDao().getSnapshotById(snapshot.getParentId())); + } + return validate(getStorageDomainValidator().hasSpaceForClonedDisks(snapshotsAndParents)); } 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 788da9d..7672d89 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,26 @@ * @return True if there is enough space in all relevant storage domains. False otherwise. */ protected boolean validateStorageDomains() { + List<DiskImage> disksList = getVolumesForStorageAllocations(); MultipleStorageDomainsValidator storageDomainsValidator = getStorageDomainsValidator(getStoragePoolId(), getStorageDomainsIds()); return validate(storageDomainsValidator.allDomainsExistAndActive()) && validate(storageDomainsValidator.allDomainsWithinThresholds()) - && validate(storageDomainsValidator.allDomainsHaveSpaceForClonedDisks(getSourceImages())); + && validate(storageDomainsValidator.allDomainsHaveSpaceForClonedDisks(disksList)); + } + + /** + * The relevant images for storage allocations are the snapshot volume(s) and the snapshot's parent. + * Since they are merged on snapshot removal, at some point the needed space is their sum, hence both + * should be validated for storage space. + * @return + */ + protected List<DiskImage> getVolumesForStorageAllocations() { + List<DiskImage> snapshots = getSourceImages(); + List<DiskImage> snapshotsAndParents = new ArrayList<>(snapshots); + for (DiskImage snapshot : snapshots) { + snapshotsAndParents.add(getDiskImageDao().getSnapshotById(snapshot.getParentId())); + } + return snapshotsAndParents; } protected Collection<Guid> getStorageDomainsIds() { 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 84c9c59..322bb55 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 @@ -164,13 +164,13 @@ @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()); } @@ -189,7 +189,10 @@ @Test public void testEnoughSpaceToMergeSnapshotsWithOneDisk() { spySdValidatorForOneDomain(); - when(diskImageDAO.get(mockSourceImage())).thenReturn(new DiskImage()); + List<DiskImage> imagesDisks = Collections.singletonList(mockSourceImage()); + doReturn(imagesDisks).when(cmd).getVolumesForStorageAllocations(); + Guid imageId = imagesDisks.get(0).getImageId(); + when(diskImageDAO.get(imageId)).thenReturn(new DiskImage()); mockStorageDomainDAOGetForStoragePool(10, STORAGE_DOMAIN_ID); assertTrue("Validation should succeed. Free space minus threshold should be bigger then disk size", cmd.validateStorageDomains()); @@ -198,7 +201,8 @@ @Test public void testNotEnoughSpaceToMergeSnapshotsWithOneDisk() { spySdValidatorForOneDomain(); - when(diskImageDAO.get(mockSourceImage())).thenReturn(new DiskImage()); + DiskImage diskImage = mockSourceImage(); + when(diskImageDAO.get(diskImage.getImageId())).thenReturn(diskImage); mockStorageDomainDAOGetForStoragePool(3, STORAGE_DOMAIN_ID); assertFalse("Validation should fail. Free space minus threshold should be smaller then disk size", cmd.validateStorageDomains()); @@ -208,7 +212,7 @@ public void testEnoughSpaceToMergeSnapshotsWithMultipleDisk() { spySdValidatorForOneDomain(); List<DiskImage> imagesDisks = mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID, IMAGE_ACTUAL_SIZE_GB); - doReturn(imagesDisks).when(cmd).getSourceImages(); + doReturn(imagesDisks).when(cmd).getVolumesForStorageAllocations(); mockStorageDomainDAOGetForStoragePool(22, STORAGE_DOMAIN_ID); assertTrue("Validation should succeed. Free space minus threshold should be bigger then summarize all disks size", cmd.validateStorageDomains()); @@ -218,7 +222,7 @@ public void testNotEnoughSpaceToMergeSnapshotsWithMultipleDisk() { spySdValidatorForOneDomain(); List<DiskImage> imagesDisks = mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID, IMAGE_ACTUAL_SIZE_GB); - doReturn(imagesDisks).when(cmd).getSourceImages(); + doReturn(imagesDisks).when(cmd).getVolumesForStorageAllocations(); mockStorageDomainDAOGetForStoragePool(15, STORAGE_DOMAIN_ID); assertFalse("Validation should fail. Free space minus threshold should be smaller then summarize all disks size", cmd.validateStorageDomains()); @@ -228,7 +232,7 @@ 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(); + doReturn(imagesDisks).when(cmd).getVolumesForStorageAllocations(); 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", @@ -239,7 +243,7 @@ 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(); + doReturn(imagesDisks).when(cmd).getVolumesForStorageAllocations(); mockStorageDomainDAOGetForStoragePool(15, STORAGE_DOMAIN_ID); mockStorageDomainDAOGetForStoragePool(22, STORAGE_DOMAIN_ID2); assertFalse("Validation should fail. First domain should not have enough free space for request.", @@ -250,7 +254,7 @@ 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(); + doReturn(imagesDisks).when(cmd).getVolumesForStorageAllocations(); mockStorageDomainDAOGetForStoragePool(22, STORAGE_DOMAIN_ID); mockStorageDomainDAOGetForStoragePool(10, STORAGE_DOMAIN_ID2); assertFalse("Validation should fail. Second domain should not have enough free space for request.", @@ -261,7 +265,7 @@ 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(); + doReturn(imagesDisks).when(cmd).getVolumesForStorageAllocations(); mockStorageDomainDAOGetForStoragePool(10, STORAGE_DOMAIN_ID); mockStorageDomainDAOGetForStoragePool(10, STORAGE_DOMAIN_ID2); assertFalse("Validation should fail. Second domain should not have enough free space for request.", @@ -323,7 +327,12 @@ } /** Mocks a call to {@link RemoveSnapshotCommand#getSourceImages()} and returns its image guid */ - private Guid mockSourceImage() { + private Guid mockSourceImageAndGetId() { + return mockSourceImage().getImageId(); + } + + /** Mocks a call to {@link RemoveSnapshotCommand#getSourceImages()} and returns the DiskImage */ + private DiskImage mockSourceImage() { Guid imageId = Guid.newGuid(); DiskImage image = new DiskImage(); image.setImageId(imageId); @@ -333,7 +342,7 @@ image.setActualSize(IMAGE_ACTUAL_SIZE_GB); image.setSize(40); doReturn(Collections.singletonList(image)).when(cmd).getSourceImages(); - return imageId; + return image; } /** Mocks a call to {@link RemoveSnapshotCommand#getSourceImages()} and returns list of images */ -- To view, visit http://gerrit.ovirt.org/37014 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I41213884ed0ca9ddf8354ab7be58f032a6d90673 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vered Volansky <vvola...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
