Arik Hadas has uploaded a new change for review. Change subject: core: remove memory image on remove snapshot ......................................................................
core: remove memory image on remove snapshot If snapshot contains memory and it is the only one that contains that memory, remove the memory volumes when removing the snapshot as well. Note that there might be cases where more than one snapshot contain the same memory: 1. when preview snapshot, the memory volume is copied from the snapshot that is being previewed to the active snapshot 2. when running vm in stateless mode, the memory volume is copied to the active snapshot 3. in the future we might support cloning a VM with its memory state (clone from snapshot + import as clone) and the memory would be copied to the cloned VM's snapshots Change-Id: Ib0b4f5306c23046326c9dbb7ef5589b50c88462d Bug-Url: https://bugzilla.redhat.com/960931 Signed-off-by: Arik Hadas <aha...@redhat.com> --- M backend/manager/dbscripts/snapshots_sp.sql M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotCommand.java M backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/SnapshotDao.java M backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/SnapshotDaoDbFacadeImpl.java M backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/SnapshotDaoTest.java 5 files changed, 150 insertions(+), 25 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/78/15678/1 diff --git a/backend/manager/dbscripts/snapshots_sp.sql b/backend/manager/dbscripts/snapshots_sp.sql index 6ea0304..30026b2 100644 --- a/backend/manager/dbscripts/snapshots_sp.sql +++ b/backend/manager/dbscripts/snapshots_sp.sql @@ -314,3 +314,16 @@ END; $procedure$ LANGUAGE plpgsql; + +Create or replace FUNCTION GetNumOfSnapshotsByMemoryVolume( + v_memory_volume VARCHAR(255)) +RETURNS SETOF BIGINT +AS $procedure$ +BEGIN + RETURN QUERY + SELECT COUNT(*) + FROM snapshots + WHERE memory_volume = v_memory_volume; +END; $procedure$ +LANGUAGE plpgsql; + 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 af83f7e..f61dfac 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 @@ -23,6 +23,7 @@ import org.ovirt.engine.core.common.action.RemoveSnapshotParameters; import org.ovirt.engine.core.common.action.VdcActionType; import org.ovirt.engine.core.common.action.VdcReturnValueBase; +import org.ovirt.engine.core.common.businessentities.Disk; import org.ovirt.engine.core.common.businessentities.DiskImage; import org.ovirt.engine.core.common.businessentities.Snapshot; import org.ovirt.engine.core.common.businessentities.Snapshot.SnapshotStatus; @@ -33,9 +34,15 @@ import org.ovirt.engine.core.common.locks.LockingGroup; import org.ovirt.engine.core.common.errors.VdcBllMessages; import org.ovirt.engine.core.common.utils.Pair; +import org.ovirt.engine.core.common.vdscommands.DeleteImageGroupVDSCommandParameters; +import org.ovirt.engine.core.common.vdscommands.VDSCommandType; +import org.ovirt.engine.core.common.vdscommands.VDSReturnValue; import org.ovirt.engine.core.compat.Guid; import org.ovirt.engine.core.dao.SnapshotDao; +import org.ovirt.engine.core.utils.GuidUtils; import org.ovirt.engine.core.utils.collections.MultiValueMapUtils; +import org.ovirt.engine.core.utils.linq.LinqUtils; +import org.ovirt.engine.core.utils.linq.Predicate; import org.ovirt.engine.core.utils.transaction.TransactionMethod; import org.ovirt.engine.core.utils.transaction.TransactionSupport; @@ -88,47 +95,102 @@ throw new VdcBLLException(VdcBllErrors.IRS_IMAGE_STATUS_ILLEGAL); } - // If the VM hasn't got any images - simply remove the snapshot. + final Snapshot snapshot = getSnapshotDao().get(getParameters().getSnapshotId()); + + boolean snapshotHasImages = hasImages(); + boolean removeSnapshotMemory = shouldRemoveMemorySnapshotVolumes(snapshot.getMemoryVolume()); + + // If the VM hasn't got any images and memory - simply remove the snapshot. // No need for locking, VDSM tasks, and all that jazz. - if (!hasImages()) { + if (!snapshotHasImages && !removeSnapshotMemory) { getSnapshotDao().remove(getParameters().getSnapshotId()); setSucceeded(true); return; } - TransactionSupport.executeInNewTransaction(new TransactionMethod<Void>() { - - @Override - public Void runInTransaction() { - Snapshot snapshot = getSnapshotDao().get(getParameters().getSnapshotId()); - getCompensationContext().snapshotEntityStatus(snapshot, snapshot.getStatus()); - getSnapshotDao().updateStatus( - getParameters().getSnapshotId(), SnapshotStatus.LOCKED); - getCompensationContext().stateChanged(); - return null; - } - }); + lockSnapshot(snapshot); freeLock(); getParameters().setEntityId(getVmId()); + if (snapshotHasImages) { + removeImages(); + } + + if (removeSnapshotMemory) { + removeMemory(snapshot); + } + + setSucceeded(true); + } + + /** + * There is a one to many relation between memory volumes and snapshots, so memory + * volumes should be removed only if the only snapshot that points to them is removed + */ + private boolean shouldRemoveMemorySnapshotVolumes(String memoryVolume) { + return !memoryVolume.isEmpty() && + getSnapshotDao().getNumOfSnapshotsByMemory(memoryVolume) == 1; + } + + private void removeMemory(final Snapshot snapshot) { + List<Guid> guids = GuidUtils.getGuidListFromString(snapshot.getMemoryVolume()); + + // get all vm disks in order to check post zero - if one of the + // disks is marked with wipe_after_delete + boolean postZero = + LinqUtils.filter(getDiskDao().getAllForVm(getVm().getId()), + new Predicate<Disk>() { + @Override + public boolean eval(Disk disk) { + return disk.isWipeAfterDelete(); + } + }).size() > 0; + + // delete first image + // the next 'DeleteImageGroup' command should also take care of the + // image removal: + VDSReturnValue vdsRetValue = runVdsCommand( + VDSCommandType.DeleteImageGroup, + new DeleteImageGroupVDSCommandParameters(guids.get(1), guids.get(0), guids.get(2), + postZero, false, getVm().getVdsGroupCompatibilityVersion().toString())); + + if (!vdsRetValue.getSucceeded()) { + log.errorFormat("Cannot remove memory dump volume for snapshot {0}", snapshot.getId()); + } + else { + Guid guid = + createTask(vdsRetValue.getCreationInfo(), VdcActionType.RemoveSnapshot, VdcObjectType.Storage, guids.get(0)); + getReturnValue().getTaskIdList().add(guid); + } + + // delete second image + // the next 'DeleteImageGroup' command should also take care of the image removal: + vdsRetValue = runVdsCommand( + VDSCommandType.DeleteImageGroup, + new DeleteImageGroupVDSCommandParameters(guids.get(1), guids.get(0), guids.get(4), + postZero, false, getVm().getVdsGroupCompatibilityVersion().toString())); + + if (!vdsRetValue.getSucceeded()) { + log.errorFormat("Cannot remove memory metadata volume for snapshot {0}", snapshot.getId()); + } + else { + Guid guid = + createTask(vdsRetValue.getCreationInfo(), VdcActionType.RemoveSnapshot, VdcObjectType.Storage, guids.get(0)); + getReturnValue().getTaskIdList().add(guid); + } + } + + private void removeImages() { for (final DiskImage source : getSourceImages()) { - // The following line is ok because we have tested in the - // candoaction that the vm + // The following line is ok because we have tested in the candoaction that the vm // is not a template and the vm is not in preview mode and that // this is not the active snapshot. DiskImage dest = getDiskImageDao().getAllSnapshotsForParent(source.getImageId()).get(0); - ImagesContainterParametersBase tempVar = new ImagesContainterParametersBase(source.getImageId(), - getVmId()); - tempVar.setDestinationImageId(dest.getImageId()); - tempVar.setEntityId(getParameters().getEntityId()); - tempVar.setParentParameters(getParameters()); - tempVar.setParentCommand(getActionType()); - ImagesContainterParametersBase p = tempVar; VdcReturnValueBase vdcReturnValue = getBackend().runInternalAction( VdcActionType.RemoveSnapshotSingleDisk, - p, + buildRemoveSnapshotSingleDiskParameters(source, dest), ExecutionHandler.createDefaultContexForTasks(getExecutionContext())); if (vdcReturnValue != null && vdcReturnValue.getInternalTaskIdList() != null) { @@ -140,7 +202,31 @@ quotasToRemoveFromCache.add(dest.getQuotaId()); QuotaManager.getInstance().removeQuotaFromCache(getStoragePoolId().getValue(), quotasToRemoveFromCache); } - setSucceeded(true); + } + + private void lockSnapshot(final Snapshot snapshot) { + TransactionSupport.executeInNewTransaction(new TransactionMethod<Void>() { + + @Override + public Void runInTransaction() { + getCompensationContext().snapshotEntityStatus(snapshot, snapshot.getStatus()); + getSnapshotDao().updateStatus( + getParameters().getSnapshotId(), SnapshotStatus.LOCKED); + getCompensationContext().stateChanged(); + return null; + } + }); + } + + private ImagesContainterParametersBase buildRemoveSnapshotSingleDiskParameters(final DiskImage source, + DiskImage dest) { + ImagesContainterParametersBase parameters = + new ImagesContainterParametersBase(source.getImageId(), getVmId()); + parameters.setDestinationImageId(dest.getImageId()); + parameters.setEntityId(getParameters().getEntityId()); + parameters.setParentParameters(getParameters()); + parameters.setParentCommand(getActionType()); + return parameters; } @Override diff --git a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/SnapshotDao.java b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/SnapshotDao.java index 200a0cb..33bde37 100644 --- a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/SnapshotDao.java +++ b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/SnapshotDao.java @@ -162,4 +162,6 @@ * @return Whether a snapshot of the given ID exists for the VM or not. */ boolean exists(Guid vmId, Guid snapshotId); + + int getNumOfSnapshotsByMemory(String memoryVolume); } diff --git a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/SnapshotDaoDbFacadeImpl.java b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/SnapshotDaoDbFacadeImpl.java index b90e975..d3e7972 100644 --- a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/SnapshotDaoDbFacadeImpl.java +++ b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/SnapshotDaoDbFacadeImpl.java @@ -213,6 +213,16 @@ parameterSource); } + @Override + public int getNumOfSnapshotsByMemory(String memoryVolume) { + MapSqlParameterSource parameterSource = getCustomMapSqlParameterSource() + .addValue("memory_volume", getNullableRepresentation(memoryVolume)); + + return getCallsHandler().executeRead("GetNumOfSnapshotsByMemoryVolume", + getIntegerMapper(), + parameterSource); + } + private String getNullableRepresentation(String memoryVolume) { return memoryVolume.isEmpty() ? null : memoryVolume; } diff --git a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/SnapshotDaoTest.java b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/SnapshotDaoTest.java index a166856..0862c6d 100644 --- a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/SnapshotDaoTest.java +++ b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/SnapshotDaoTest.java @@ -24,6 +24,10 @@ private static final Guid EXISTING_VM_ID = new Guid("77296e00-0cad-4e5a-9299-008a7b6f4355"); private static final Guid EXISTING_SNAPSHOT_ID = new Guid("a7bb24df-9fdf-4bd6-b7a9-f5ce52da0f89"); + private static final String EXISTING_MEMORY_VOLUME = + "11111111-1111-1111-1111-111111111111,22222222-2222-2222-2222-222222222222,33333333-3333-3333-3333-333333333333,44444444-4444-4444-4444-444444444444,55555555-5555-5555-5555-555555555555,66666666-6666-6666-6666-666666666666"; + private static final String NON_EXISTING_MEMORY_VOLUME = + "21111111-1111-1111-1111-111111111111,22222222-2222-2222-2222-222222222222,33333333-3333-3333-3333-333333333333,44444444-4444-4444-4444-444444444444,55555555-5555-5555-5555-555555555555,66666666-6666-6666-6666-666666666666"; private static final int TOTAL_SNAPSHOTS = 2; @Override @@ -109,6 +113,16 @@ } @Test + public void getZeroSnapshotsByMemory() { + assertEquals(dao.getNumOfSnapshotsByMemory(NON_EXISTING_MEMORY_VOLUME), 0); + } + + @Test + public void getOneSnapshotsByMemory() { + assertEquals(dao.getNumOfSnapshotsByMemory(EXISTING_MEMORY_VOLUME), 1); + } + + @Test public void getSnaphsotByTypeReturnsIdForExistingByTypeAndStatus() throws Exception { assertNotNull(dao.get(EXISTING_VM_ID, SnapshotType.REGULAR)); } -- To view, visit http://gerrit.ovirt.org/15678 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ib0b4f5306c23046326c9dbb7ef5589b50c88462d Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Arik Hadas <aha...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
