Allon Mureinik has uploaded a new change for review. Change subject: core: Add ImagesHandler.filterEditableImageDisks ......................................................................
core: Add ImagesHandler.filterEditableImageDisks In the current implementation, an image that can be snapshoted is a non-sharable image. Most of the commands CDAs refer to images the commands can manipulate in some way - which means they must be allowed to snapshot. This patch contains: * A new method, filterEditableImageDisks, as syntactic sugar on top of filterImagesHandler. * Tests to show it's completely equivalent to: filterImageDisks(disks, false, true) filterImageDisks(disks, true, false) filterImageDisks(disks, true, true) * Using the new method wherever it improves commands' readability. Change-Id: I9f766309911d27503dbbb80a07fcc776d9068cd2 Signed-off-by: Allon Mureinik <[email protected]> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromSnapshotCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmTemplateCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmCommand.java 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/RemoveAllVmImagesCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmFromImportExportCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmPoolCommandBase.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmRunHandler.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImagesHandlerTest.java 10 files changed, 47 insertions(+), 19 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/20/12320/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromSnapshotCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromSnapshotCommand.java index 047f8c4..4374312 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromSnapshotCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromSnapshotCommand.java @@ -185,9 +185,7 @@ protected Collection<DiskImage> getDiskImagesFromConfiguration() { if (diskImagesFromConfiguration == null) { diskImagesFromConfiguration = - ImagesHandler.filterImageDisks(vmFromConfiguration.getDiskMap().values(), - false, - true); + ImagesHandler.filterEditableImageDisks(vmFromConfiguration.getDiskMap().values()); } return diskImagesFromConfiguration; } diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmTemplateCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmTemplateCommand.java index 7acb0c9..1328390 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmTemplateCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmTemplateCommand.java @@ -254,7 +254,7 @@ Map<StorageDomain, Integer> domainMap = StorageDomainValidator.getSpaceRequirementsForStorageDomains( - ImagesHandler.filterImageDisks(getVm().getDiskMap().values(), true, false), + ImagesHandler.filterEditableImageDisks(getVm().getDiskMap().values()), storageDomains, diskInfoDestinationMap); for (Map.Entry<StorageDomain, Integer> entry : domainMap.entrySet()) { diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmCommand.java index 78adb84..64b9af5 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmCommand.java @@ -278,7 +278,7 @@ private Collection<DiskImage> getDisksBasedOnImage() { if (disksImages == null) { - disksImages = ImagesHandler.filterImageDisks(getVm().getDiskMap().values(), true, false); + disksImages = ImagesHandler.filterEditableImageDisks(getVm().getDiskMap().values()); } return disksImages; } 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 23b78a2..3775187 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 @@ -440,7 +440,7 @@ Collection<? extends Disk> diskImageList) { boolean returnValue = true; - List<DiskImage> images = filterImageDisks(diskImageList, true, false); + List<DiskImage> images = filterEditableImageDisks(diskImageList); if (returnValue && checkImagesLocked) { returnValue = checkImagesLocked(messages, images); } @@ -486,10 +486,10 @@ private static List<DiskImage> getImages(Guid vmId, Collection<? extends Disk> diskImageList) { if (diskImageList == null) { - return filterImageDisks(DbFacade.getInstance().getDiskDao().getAllForVm(vmId), true, false); + return filterEditableImageDisks(DbFacade.getInstance().getDiskDao().getAllForVm(vmId)); } - return filterImageDisks(diskImageList, true, false); + return filterEditableImageDisks(diskImageList); } private static boolean checkDiskImages(List<String> messages, @@ -614,6 +614,10 @@ return diskImages; } + public static List<DiskImage> filterEditableImageDisks(Collection<? extends Disk> listOfDisks) { + return filterImageDisks(listOfDisks, true, true); + } + public static List<LunDisk> filterDiskBasedOnLuns(Collection<Disk> listOfDisks) { List<LunDisk> lunDisks = new ArrayList<LunDisk>(); for (Disk disk : listOfDisks) { diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveAllVmImagesCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveAllVmImagesCommand.java index 5ad2f8e..044eca5 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveAllVmImagesCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveAllVmImagesCommand.java @@ -37,9 +37,7 @@ List<DiskImage> images = getParameters().Images; if (images == null) { images = - ImagesHandler.filterImageDisks(DbFacade.getInstance().getDiskDao().getAllForVm(getVmId()), - true, - false); + ImagesHandler.filterEditableImageDisks(DbFacade.getInstance().getDiskDao().getAllForVm(getVmId())); } for (DiskImage image : images) { if (Boolean.TRUE.equals(image.getActive())) { diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmCommand.java index 2e05a1b..84886a8 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmCommand.java @@ -156,7 +156,7 @@ } Collection<Disk> vmDisks = getVm().getDiskMap().values(); - List<DiskImage> vmImages = ImagesHandler.filterImageDisks(vmDisks, true, false); + List<DiskImage> vmImages = ImagesHandler.filterEditableImageDisks(vmDisks); if (!vmImages.isEmpty()) { Set<Guid> storageIds = ImagesHandler.getAllStorageIdsForImageIds(vmImages); MultipleStorageDomainsValidator storageValidator = new MultipleStorageDomainsValidator(getVm().getStoragePoolId(), storageIds); @@ -303,9 +303,7 @@ VmHandler.updateDisksFromDb(getVm()); // Get all disk images for VM (VM should not have any image disk associated with it). - List<DiskImage> diskImages = ImagesHandler.filterImageDisks(getVm().getDiskMap().values(), - true, - false); + List<DiskImage> diskImages = ImagesHandler.filterEditableImageDisks(getVm().getDiskMap().values()); // If the VM still has disk images related to it, change their status to Illegal. if (!diskImages.isEmpty()) { diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmFromImportExportCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmFromImportExportCommand.java index 1520949..6ec6710 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmFromImportExportCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmFromImportExportCommand.java @@ -12,11 +12,11 @@ import org.ovirt.engine.core.common.action.RemoveVmFromImportExportParamenters; import org.ovirt.engine.core.common.action.VdcActionType; 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.StorageDomainType; import org.ovirt.engine.core.common.businessentities.VM; import org.ovirt.engine.core.common.businessentities.VMStatus; -import org.ovirt.engine.core.common.businessentities.StorageDomain; import org.ovirt.engine.core.common.queries.GetAllFromExportDomainQueryParameters; import org.ovirt.engine.core.common.queries.VdcQueryReturnValue; import org.ovirt.engine.core.common.queries.VdcQueryType; @@ -79,7 +79,7 @@ protected void executeVmCommand() { removeVmInSpm(getParameters().getStoragePoolId(), getVmId(), getParameters().getStorageDomainId()); List<DiskImage> images = - ImagesHandler.filterImageDisks(getVm().getDiskMap().values(), true, false); + ImagesHandler.filterEditableImageDisks(getVm().getDiskMap().values()); for (DiskImage image : images) { image.setStorageIds(new ArrayList<Guid>(Arrays.asList(getParameters().getStorageDomainId()))); image.setStoragePoolId(getParameters().getStoragePoolId()); diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmPoolCommandBase.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmPoolCommandBase.java index c497180..bf20606 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmPoolCommandBase.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmPoolCommandBase.java @@ -184,7 +184,7 @@ } List<Disk> disks = DbFacade.getInstance().getDiskDao().getAllForVm(vmId); - List<DiskImage> vmImages = ImagesHandler.filterImageDisks(disks, true, true); + List<DiskImage> vmImages = ImagesHandler.filterEditableImageDisks(disks); VM vm = DbFacade.getInstance().getVmDao().get(vmId); storage_pool sp = DbFacade.getInstance().getStoragePoolDao().get(vm.getStoragePoolId()); diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmRunHandler.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmRunHandler.java index 4ab9ad7..feee7eb 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmRunHandler.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmRunHandler.java @@ -123,7 +123,7 @@ } } - List<DiskImage> vmImages = ImagesHandler.filterImageDisks(vmDisks, true, false); + List<DiskImage> vmImages = ImagesHandler.filterEditableImageDisks(vmDisks); if (retValue && !vmImages.isEmpty()) { storage_pool sp = getStoragePoolDAO().get(vm.getStoragePoolId()); ValidationResult spUpResult = new StoragePoolValidator(sp).isUp(); diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImagesHandlerTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImagesHandlerTest.java index 911683a..e6d0bc7 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImagesHandlerTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImagesHandlerTest.java @@ -5,11 +5,14 @@ import java.util.ArrayList; import java.util.Arrays; +import java.util.List; import java.util.Set; import org.junit.Before; import org.junit.Test; +import org.ovirt.engine.core.common.businessentities.Disk; import org.ovirt.engine.core.common.businessentities.DiskImage; +import org.ovirt.engine.core.common.businessentities.LunDisk; import org.ovirt.engine.core.compat.Guid; /** A test case for {@link ImagesHandler} */ @@ -21,11 +24,17 @@ /** The disks to use for testing */ private DiskImage disk1; private DiskImage disk2; + private LunDisk lunDisk; @Before public void setUp() { disk1 = new DiskImage(); + disk1.setShareable(true); + disk2 = new DiskImage(); + disk2.setShareable(false); + + lunDisk = new LunDisk(); } @Test @@ -65,4 +74,25 @@ assertEquals("Wrong number of Guids returned", 3, result.size()); assertTrue("Wrong Guids returned", result.containsAll(Arrays.asList(sdId1, sdId2, sdIdShared))); } + + @Test + public void testFilterImages() { + List<Disk> allDisks = Arrays.asList(lunDisk, disk1, disk2); + + List<DiskImage> result = ImagesHandler.filterImageDisks(allDisks, false, false); + assertEquals("Wrong number of disks filtered", 2, result.size()); + assertTrue("Wrong disks filtered", result.containsAll(Arrays.asList(disk1, disk2))); + + result = ImagesHandler.filterImageDisks(allDisks, true, false); + assertEquals("Wrong number of disks filtered", 1, result.size()); + assertTrue("Wrong disks filtered", result.contains(disk2)); + + result = ImagesHandler.filterImageDisks(allDisks, false, true); + assertEquals("Wrong number of disks filtered", 1, result.size()); + assertTrue("Wrong disks filtered", result.contains(disk2)); + + result = ImagesHandler.filterEditableImageDisks(allDisks); + assertEquals("Wrong number of disks filtered", 1, result.size()); + assertTrue("Wrong disks filtered", result.contains(disk2)); + } } -- To view, visit http://gerrit.ovirt.org/12320 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I9f766309911d27503dbbb80a07fcc776d9068cd2 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Allon Mureinik <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
