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/AbstractDiskVmCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AttachDiskToVmCommand.java 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 10 files changed, 123 insertions(+), 25 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/58/20458/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractDiskVmCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractDiskVmCommand.java index ed433f7..b056cf0 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractDiskVmCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractDiskVmCommand.java @@ -133,9 +133,16 @@ return true; } + /** + * loads the disk info for the active snapshot, for luns the lun disk will be returned. + */ + protected Disk loadActiveDisk(Guid diskId) { + return getDiskDao().get(diskId); + } + protected Disk loadDisk(Guid diskId) { if (getParameters().getSnapshotId() == null) { - return getDiskDao().get(diskId); + return loadActiveDisk(diskId); } else { return getDiskImageDao().getDiskSnapshotForVmSnapshot(diskId, getParameters().getSnapshotId()); } diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AttachDiskToVmCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AttachDiskToVmCommand.java index cdf8a6e..0255b19 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AttachDiskToVmCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AttachDiskToVmCommand.java @@ -64,14 +64,21 @@ } boolean isImageDisk = disk.getDiskStorageType() == DiskStorageType.IMAGE; - if (isImageDisk && ((DiskImage) disk).getImageStatus() == ImageStatus.ILLEGAL) { - return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_ILLEGAL_DISK_OPERATION); - } - if (isImageDisk && ((DiskImage) disk).getImageStatus() == ImageStatus.LOCKED) { - addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_DISKS_LOCKED); - addCanDoActionMessage(String.format("$%1$s %2$s", "diskAliases", disk.getDiskAlias())); - return false; + if (isImageDisk) { + //TODO : this load and check of the active disk will be removed + // as update script for previous version for the status illegal will be introduced. + Disk activeDisk = loadActiveDisk(disk.getId()); + + if (((DiskImage) activeDisk).getImageStatus() == ImageStatus.ILLEGAL) { + return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_ILLEGAL_DISK_OPERATION); + } + + if (((DiskImage) disk).getImageStatus() == ImageStatus.LOCKED) { + addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_DISKS_LOCKED); + addCanDoActionMessage(String.format("$%1$s %2$s", "diskAliases", disk.getDiskAlias())); + return false; + } } if (!isVmExist() || !isVmInUpPausedDownStatus()) { 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 394ec03..8f3c5e7 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..15963f8 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,34 @@ 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 { + updateAllDiskImageSnapshotsStatus(diskId, status); + } + } + + protected static void updateAllDiskImageSnapshotsStatus(final Guid diskId, + final ImageStatus status) { + 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..9cf1b38 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,20 @@ } @Override + protected void lockImage() { + ImagesHandler.updateAllDiskImageSnapshotsStatusWithCompensation(getRelevantDiskImage().getId(), + ImageStatus.LOCKED, + getRelevantDiskImage().getImageStatus(), + getCompensationContext()); + } + + @Override + protected void unLockImage() { + ImagesHandler.updateAllDiskImageSnapshotsStatus(getRelevantDiskImage().getId(), + ImageStatus.OK); + } + + @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 e1546a3..d6f5da5 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 @@ -290,7 +290,10 @@ getImageStorageDomainMapDao().remove( new ImageStorageDomainMapId(getParameters().getImageId(), getParameters().getStorageDomainId())); - unLockImage(); + ImagesHandler.updateAllDiskImageSnapshotsStatusWithCompensation(getRelevantDiskImage().getId(), + getRelevantDiskImage().getImageStatus(), + null, + null); return null; }}); } @@ -313,8 +316,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/20458 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ibac8daf6a9e970859a6759d84d7849e7f84e0d79 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: ovirt-engine-3.3 Gerrit-Owner: Liron Ar <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
