Liron Ar has uploaded a new change for review. Change subject: core: adding support for db lock of all disk snapshots ......................................................................
core: adding support for db lock of all disk snapshots This change is "part" of http://gerrit.ovirt.org/#/c/17679/ and is separated just for review sake. This patch adds "support" for locking all disk snapshots/volumes as part of the being added support for attaching disk snapshots to vms. The added method in ImagesHandler can be used to lock all the disk snapshots, while the current "lock" method locks the given volume only. The need is for this change is to prevent race conditions, provide ability to lock only the needed snapshots of a disk and to have correct UI indication of the current state. Example use cases: 1. When removing a disk, all the disk snapshots should be locked and have its status reverted (otherwise they will be displayed as "OK" for vms that they are attached to). 2. When extending a disk with snapshots, only the active snapshot should be locked as other snapshots can be used as attached disk snapshots in other vms. 3. When moving a disk all the disk snapshots should be locked, as when the copy of the data ends the source would be deleted. Change-Id: Ibac8daf6a9e970859a6759d84d7849e7f84e0d79 Signed-off-by: Liron Aravot <[email protected]> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/BaseImagesCommand.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/MoveImageGroupCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveImageCommand.java M backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDao.java M backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDaoDbFacadeImpl.java M backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/ImageDaoTest.java M packaging/dbscripts/images_sp.sql 8 files changed, 98 insertions(+), 17 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/08/20308/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/BaseImagesCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/BaseImagesCommand.java index 067fc29..906ba6b 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/BaseImagesCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/BaseImagesCommand.java @@ -23,8 +23,6 @@ import org.ovirt.engine.core.dao.DiskImageDynamicDAO; import org.ovirt.engine.core.dao.ImageDao; import org.ovirt.engine.core.dao.SnapshotDao; -import org.ovirt.engine.core.utils.transaction.TransactionMethod; -import org.ovirt.engine.core.utils.transaction.TransactionSupport; /** * Base class for all image handling commands @@ -283,18 +281,6 @@ protected void lockImage() { setImageStatus(ImageStatus.LOCKED); - } - - protected void lockImageWithCompensation() { - final DiskImage diskImage = getRelevantDiskImage(); - TransactionSupport.executeInNewTransaction(new TransactionMethod<Void>() { - @Override - public Void runInTransaction() { - getCompensationContext().snapshotEntityStatus(diskImage.getImage()); - getCompensationContext().stateChanged(); - setImageStatus(ImageStatus.LOCKED); - return null; - }}); } protected void unLockImage() { 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 d375aa0..8208af7 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 @@ -10,6 +10,7 @@ import java.util.Set; import org.apache.commons.lang.StringUtils; +import org.ovirt.engine.core.bll.context.CompensationContext; import org.ovirt.engine.core.bll.storage.StorageHelperDirector; import org.ovirt.engine.core.bll.utils.VmDeviceUtils; import org.ovirt.engine.core.bll.validator.StorageDomainValidator; @@ -42,10 +43,13 @@ import org.ovirt.engine.core.common.vdscommands.VDSCommandType; import org.ovirt.engine.core.common.vdscommands.VdsAndPoolIDVDSParametersBase; import org.ovirt.engine.core.compat.Guid; +import org.ovirt.engine.core.compat.TransactionScopeOption; import org.ovirt.engine.core.dal.dbbroker.DbFacade; import org.ovirt.engine.core.utils.collections.MultiValueMapUtils; import org.ovirt.engine.core.utils.log.Log; import org.ovirt.engine.core.utils.log.LogFactory; +import org.ovirt.engine.core.utils.transaction.TransactionMethod; +import org.ovirt.engine.core.utils.transaction.TransactionSupport; public final class ImagesHandler { public static final String DISK = "_Disk"; @@ -640,4 +644,29 @@ DbFacade.getInstance().getImageDao().updateStatus(imageId, imageStatus); } + protected static void updateAllDiskImageSnapshotsStatusWithCompensation(final Guid diskId, + final ImageStatus status, + ImageStatus statusForCompensation, + final CompensationContext compensationContext) { + + if (compensationContext != null) { + List<DiskImage> diskSnapshots = + DbFacade.getInstance().getDiskImageDao().getAllSnapshotsForImageGroup(diskId); + for (DiskImage diskSnapshot : diskSnapshots) { + diskSnapshot.setImageStatus(statusForCompensation); + compensationContext.snapshotEntityStatus(diskSnapshot.getImage()); + } + + TransactionSupport.executeInScope(TransactionScopeOption.Required, new TransactionMethod<Void>() { + @Override + public Void runInTransaction() { + DbFacade.getInstance().getImageDao().updateStatusOfImagesByImageGroupId(diskId, status); + compensationContext.stateChanged(); + return null; + } + }); + } else { + DbFacade.getInstance().getImageDao().updateStatusOfImagesByImageGroupId(diskId, status); + } + } } diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveImageGroupCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveImageGroupCommand.java index 0fabe97..0bd47e3 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveImageGroupCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveImageGroupCommand.java @@ -9,6 +9,7 @@ import org.ovirt.engine.core.common.action.VdcReturnValueBase; import org.ovirt.engine.core.common.asynctasks.EntityInfo; import org.ovirt.engine.core.common.businessentities.ImageDbOperationScope; +import org.ovirt.engine.core.common.businessentities.ImageStatus; import org.ovirt.engine.core.compat.Guid; import org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogDirector; import org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogableBase; @@ -45,6 +46,22 @@ } @Override + protected void lockImage() { + ImagesHandler.updateAllDiskImageSnapshotsStatusWithCompensation(getRelevantDiskImage().getId(), + ImageStatus.LOCKED, + ImageStatus.OK, + getCompensationContext()); + } + + @Override + protected void unLockImage() { + ImagesHandler.updateAllDiskImageSnapshotsStatusWithCompensation(getRelevantDiskImage().getId(), + ImageStatus.OK, + null, + null); + } + + @Override protected void endSuccessfully() { removeImage(getParameters().getSourceDomainId()); super.endSuccessfully(); diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveImageCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveImageCommand.java index 250df9e..db58dbd 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveImageCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveImageCommand.java @@ -318,7 +318,10 @@ getImageStorageDomainMapDao().remove( new ImageStorageDomainMapId(getParameters().getImageId(), getParameters().getStorageDomainId())); - unLockImage(); + ImagesHandler.updateAllDiskImageSnapshotsStatusWithCompensation(getRelevantDiskImage().getId(), + ImageStatus.OK, + null, + null); return null; }}); } @@ -341,8 +344,10 @@ if (getParameters().isShouldLockImage()) { // the image status should be set to ILLEGAL, so that in case compensation runs the image status will // be revert to be ILLEGAL, as we can't tell whether the task started on vdsm side or not. - getDiskImage().setImageStatus(ImageStatus.ILLEGAL); - lockImageWithCompensation(); + ImagesHandler.updateAllDiskImageSnapshotsStatusWithCompensation(getRelevantDiskImage().getId(), + ImageStatus.LOCKED, + ImageStatus.ILLEGAL, + getCompensationContext()); } return runVdsCommand(VDSCommandType.DeleteImageGroup, new DeleteImageGroupVDSCommandParameters(getDiskImage().getStoragePoolId(), diff --git a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDao.java b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDao.java index 28f6d39..1503382 100644 --- a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDao.java +++ b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDao.java @@ -8,6 +8,8 @@ * <code>ImageDAO</code> defines a type for performing CRUD operations on instances of {@link Image}. */ public interface ImageDao extends GenericDao<Image, Guid>, StatusAwareDao<Guid, ImageStatus> { + public void updateStatusOfImagesByImageGroupId(Guid imageGroupId, ImageStatus status); + public void updateQuotaForImageAndSnapshots(Guid imageGroupId, Guid quotaId); public void updateImageVmSnapshotId(Guid id, Guid vmSnapshotId); diff --git a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDaoDbFacadeImpl.java b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDaoDbFacadeImpl.java index 1d6883e..e3c1286 100644 --- a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDaoDbFacadeImpl.java +++ b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDaoDbFacadeImpl.java @@ -99,6 +99,14 @@ } @Override + public void updateStatusOfImagesByImageGroupId(Guid imageGroupId, ImageStatus status) { + MapSqlParameterSource parameterSource = getCustomMapSqlParameterSource() + .addValue("image_group_id", imageGroupId) + .addValue("status", status); + getCallsHandler().executeModification("UpdateStatusOfImagesByImageGroupId", parameterSource); + } + + @Override public void updateQuotaForImageAndSnapshots(Guid imageGroupId, Guid quotaId) { MapSqlParameterSource parameterSource = getCustomMapSqlParameterSource() .addValue("image_group_id", imageGroupId) diff --git a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/ImageDaoTest.java b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/ImageDaoTest.java index d051818..f47e154 100644 --- a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/ImageDaoTest.java +++ b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/ImageDaoTest.java @@ -1,10 +1,14 @@ package org.ovirt.engine.core.dao; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.fail; +import java.util.List; + import org.junit.Test; +import org.ovirt.engine.core.common.businessentities.DiskImage; import org.ovirt.engine.core.common.businessentities.Image; import org.ovirt.engine.core.common.businessentities.ImageStatus; import org.ovirt.engine.core.common.businessentities.VolumeFormat; @@ -104,4 +108,20 @@ // check that the new quota is the inserted one assertEquals("quota wasn't changed", quotaId, FixturesTool.DEFAULT_QUOTA_GENERAL); } + + @Test + public void updateStatusOfImagesByImageGroupId() { + Image image = dao.get(EXISTING_IMAGE_ID); + List<DiskImage> snapshots = dbFacade.getDiskImageDao().getAllSnapshotsForImageGroup(image.getDiskId()); + assertFalse(snapshots.size() == 1); + for (DiskImage diskImage : snapshots) { + assertFalse(ImageStatus.LOCKED == diskImage.getImageStatus()); + } + dao.updateStatusOfImagesByImageGroupId(image.getDiskId(), ImageStatus.LOCKED); + snapshots = dbFacade.getDiskImageDao().getAllSnapshotsForImageGroup(image.getDiskId()); + + for (DiskImage diskImage : snapshots) { + assertEquals(ImageStatus.LOCKED, diskImage.getImageStatus()); + } + } } diff --git a/packaging/dbscripts/images_sp.sql b/packaging/dbscripts/images_sp.sql index ce0532a..7a46c39 100644 --- a/packaging/dbscripts/images_sp.sql +++ b/packaging/dbscripts/images_sp.sql @@ -73,6 +73,20 @@ +Create or replace FUNCTION UpdateStatusOfImagesByImageGroupId( + v_image_group_id UUID, + v_status INTEGER) +RETURNS VOID +AS $procedure$ +BEGIN + UPDATE images + SET imageStatus = v_status + WHERE image_group_id = v_image_group_id; +END; $procedure$ +LANGUAGE plpgsql; + + + Create or replace FUNCTION UpdateImageVmSnapshotId( v_image_id UUID, v_vm_snapshot_id UUID) -- To view, visit http://gerrit.ovirt.org/20308 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ibac8daf6a9e970859a6759d84d7849e7f84e0d79 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liron Ar <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
