Daniel Erez has uploaded a new change for review. Change subject: core: DiskImageDAO - introduce getAllSnapshotsForLeaf ......................................................................
core: DiskImageDAO - introduce getAllSnapshotsForLeaf Introducing a recursive stored procedure for retrieving a snapshot chain by a leaf image. It is required for ensuring data integrity when querying GetAllDisksByVmId (i.e. avoid race while removing a disk/etc). Change-Id: Ib6068be44fd5e5c47eed2064d87f506d3e3be444 Bug-Url: https://bugzilla.redhat.com/1113087 Signed-off-by: Daniel Erez <de...@redhat.com> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmAndCloneImageCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllDisksByVmIdQuery.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/MoveOrCopyDiskCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfDataUpdater.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfHelper.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateVmDisksCommand.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommandTest.java M backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskImageDAO.java M backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskImageDAODbFacadeImpl.java M backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/DiskImageDAOTest.java M packaging/dbscripts/disk_images_sp.sql 13 files changed, 63 insertions(+), 24 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/00/31500/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmAndCloneImageCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmAndCloneImageCommand.java index 8104612..ae9e442 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmAndCloneImageCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmAndCloneImageCommand.java @@ -303,7 +303,7 @@ } protected List<DiskImage> getAllImageSnapshots(DiskImage diskImage) { - return ImagesHandler.getAllImageSnapshots(diskImage.getImageId(), diskImage.getImageTemplateId()); + return ImagesHandler.getAllImageSnapshots(diskImage.getImageId()); } /** diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java index 87dee58..aaee8d4 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java @@ -162,7 +162,7 @@ } protected List<DiskImage> getAllImageSnapshots(DiskImage diskImage) { - return ImagesHandler.getAllImageSnapshots(diskImage.getImageId(), diskImage.getImageTemplateId()); + return ImagesHandler.getAllImageSnapshots(diskImage.getImageId()); } @Override diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllDisksByVmIdQuery.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllDisksByVmIdQuery.java index b3f1592..9024a9b 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllDisksByVmIdQuery.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllDisksByVmIdQuery.java @@ -46,7 +46,7 @@ } protected List<DiskImage> getAllImageSnapshots(DiskImage diskImage) { - return ImagesHandler.getAllImageSnapshots(diskImage.getImageId(), diskImage.getImageTemplateId()); + return ImagesHandler.getAllImageSnapshots(diskImage.getImageId()); } private Map<Guid, VmDevice> getDisksVmDeviceMap() { 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 e925f4c..d9353c5 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 @@ -364,18 +364,10 @@ * be trybackfrom image * * @param imageId - * @param imageTemplateId * @return */ - public static ArrayList<DiskImage> getAllImageSnapshots(Guid imageId, Guid imageTemplateId) { - ArrayList<DiskImage> snapshots = new ArrayList<DiskImage>(); - Guid curImage = imageId; - while (!imageTemplateId.equals(curImage) && !curImage.equals(Guid.Empty)) { - DiskImage curDiskImage = DbFacade.getInstance().getDiskImageDao().getSnapshotById(curImage); - snapshots.add(curDiskImage); - curImage = curDiskImage.getParentId(); - } - return snapshots; + public static List<DiskImage> getAllImageSnapshots(Guid imageId) { + return DbFacade.getInstance().getDiskImageDao().getAllSnapshotsForLeaf(imageId); } public static String cdPathWindowsToLinux(String windowsPath, Guid storagePoolId, Guid vdsId) { @@ -520,8 +512,7 @@ if (disk.getDiskStorageType() == DiskStorageType.IMAGE) { DiskImage diskImage = (DiskImage) disk; diskImage.getSnapshots().addAll( - ImagesHandler.getAllImageSnapshots(diskImage.getImageId(), - diskImage.getImageTemplateId())); + ImagesHandler.getAllImageSnapshots(diskImage.getImageId())); } } } diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java index ad7b83d..eedc36b 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java @@ -168,8 +168,8 @@ return false; } - private List<DiskImage> getAllImageSnapshots() { - return ImagesHandler.getAllImageSnapshots(getImage().getImageId(), getImage().getImageTemplateId()); + protected List<DiskImage> getAllImageSnapshots() { + return ImagesHandler.getAllImageSnapshots(getImage().getImageId()); } protected boolean checkIfNeedToBeOverride() { diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfDataUpdater.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfDataUpdater.java index 7b4af53..105cfcb 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfDataUpdater.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfDataUpdater.java @@ -490,9 +490,8 @@ } } - protected ArrayList<DiskImage> getAllImageSnapshots(DiskImage diskImage) { - return ImagesHandler.getAllImageSnapshots(diskImage.getImageId(), - diskImage.getImageTemplateId()); + protected List<DiskImage> getAllImageSnapshots(DiskImage diskImage) { + return ImagesHandler.getAllImageSnapshots(diskImage.getImageId()); } protected String generateVmMetadata(VM vm, ArrayList<DiskImage> AllVmImages) { diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfHelper.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfHelper.java index 58f646c..47c6ca7 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfHelper.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfHelper.java @@ -101,8 +101,7 @@ List<DiskImage> filteredDisks = ImagesHandler.filterImageDisks(vm.getDiskList(), false, true, true); for (DiskImage diskImage : filteredDisks) { - List<DiskImage> images = ImagesHandler.getAllImageSnapshots(diskImage.getImageId(), - diskImage.getImageTemplateId()); + List<DiskImage> images = ImagesHandler.getAllImageSnapshots(diskImage.getImageId()); AllVmImages.addAll(images); } diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateVmDisksCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateVmDisksCommand.java index 0de7a35..dcead5b 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateVmDisksCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateVmDisksCommand.java @@ -409,9 +409,8 @@ } for (DiskImage diskImage : disksList) { - Guid templateId = diskImage.getImageTemplateId(); List<DiskImage> allImageSnapshots = - ImagesHandler.getAllImageSnapshots(diskImage.getImageId(), templateId); + ImagesHandler.getAllImageSnapshots(diskImage.getImageId()); diskImage.getSnapshots().addAll(allImageSnapshots); } diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommandTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommandTest.java index c5850fd..9afc2b0 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommandTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommandTest.java @@ -135,6 +135,7 @@ initSrcStorageDomain(); initDestStorageDomain(); doReturn(vmDeviceDao).when(command).getVmDeviceDAO(); + doReturn(new ArrayList<DiskImage>()).when(command).getAllImageSnapshots(); assertFalse(command.canDoAction()); assertTrue(command.getReturnValue() .getCanDoActionMessages() @@ -183,6 +184,7 @@ initSrcStorageDomain(); initDestStorageDomain(); doReturn(mockStorageDomainValidatorWithoutSpace()).when(command).createStorageDomainValidator(); + doReturn(new ArrayList<DiskImage>()).when(command).getAllImageSnapshots(); CanDoActionTestUtils.runAndAssertCanDoActionFailure(command, VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_STORAGE_DOMAIN); } @@ -194,6 +196,7 @@ initSrcStorageDomain(); initDestStorageDomain(); doReturn(mockStorageDomainValidatorWithSpace()).when(command).createStorageDomainValidator(); + doReturn(new ArrayList<DiskImage>()).when(command).getAllImageSnapshots(); CanDoActionTestUtils.runAndAssertCanDoActionSuccess(command); } diff --git a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskImageDAO.java b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskImageDAO.java index 1dbbd0d..f0a4787 100644 --- a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskImageDAO.java +++ b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskImageDAO.java @@ -31,6 +31,16 @@ List<DiskImage> getAllSnapshotsForParent(Guid id); /** + * Retrieves all snapshots by a given image id + * (ordered by the images chain). + * + * @param id + * the parent id + * @return the list of snapshots + */ + List<DiskImage> getAllSnapshotsForLeaf(Guid id); + + /** * Retrieves all snapshots associated with the given storage domain. * * @param id diff --git a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskImageDAODbFacadeImpl.java b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskImageDAODbFacadeImpl.java index 281fca2..7368bd2 100644 --- a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskImageDAODbFacadeImpl.java +++ b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskImageDAODbFacadeImpl.java @@ -51,6 +51,15 @@ } @Override + public List<DiskImage> getAllSnapshotsForLeaf(Guid id) { + MapSqlParameterSource parameterSource = getCustomMapSqlParameterSource() + .addValue("image_guid", id); + return getCallsHandler().executeReadList("GetSnapshotByLeafGuid", + DiskImageRowMapper.instance, + parameterSource); + } + + @Override public List<DiskImage> getAllSnapshotsForStorageDomain(Guid id) { MapSqlParameterSource parameterSource = getCustomMapSqlParameterSource() .addValue("storage_domain_id", id); diff --git a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/DiskImageDAOTest.java b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/DiskImageDAOTest.java index 64ebaed..664950b 100644 --- a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/DiskImageDAOTest.java +++ b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/DiskImageDAOTest.java @@ -148,4 +148,11 @@ assertNotNull(result); assertTrue(result.isEmpty()); } + + @Test + public void testGetAllSnapshotsForLeaf() { + List<DiskImage> images = dao.getAllSnapshotsForLeaf(FixturesTool.IMAGE_ID); + + assertFalse(images.isEmpty()); + } } diff --git a/packaging/dbscripts/disk_images_sp.sql b/packaging/dbscripts/disk_images_sp.sql index 3254c34..d2d7904 100644 --- a/packaging/dbscripts/disk_images_sp.sql +++ b/packaging/dbscripts/disk_images_sp.sql @@ -88,6 +88,28 @@ +Create or replace FUNCTION GetSnapshotByLeafGuid(v_image_guid UUID) +RETURNS SETOF images_storage_domain_view STABLE + AS $procedure$ +BEGIN + RETURN QUERY WITH RECURSIVE image_list AS ( + SELECT * + FROM images_storage_domain_view + WHERE image_guid = v_image_guid + UNION ALL + SELECT images_storage_domain_view.* + FROM images_storage_domain_view + JOIN image_list ON + image_list.parentid = images_storage_domain_view.image_guid AND + image_list.image_group_id = images_storage_domain_view.image_group_id + ) + SELECT * + FROM image_list; +END; $procedure$ +LANGUAGE plpgsql; + + + Create or replace FUNCTION GetVmImageByImageGuid(v_image_guid UUID) RETURNS SETOF vm_images_view STABLE -- To view, visit http://gerrit.ovirt.org/31500 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ib6068be44fd5e5c47eed2064d87f506d3e3be444 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Daniel Erez <de...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
