Arik Hadas has uploaded a new change for review. Change subject: core: managed removal of memory volumes in negative flows ......................................................................
core: managed removal of memory volumes in negative flows On failure while importing VM or creating snapshot with memory, we need to remove the memory volumes that were copied/created. The remove image operation is asynchronous, i.e it is based on tasks. On negative flows such as the ones mentioned above, we need to remove the memory image in the end-action phase. Since our infrastructure for commands doesn't support scenarios where tasks are created in the end-action phase well, we used to create the tasks for the remove operation without polling them. The problem was that without polling the tasks they remained in VDSM forever. The solution is to invoke the command that removes memory volumes as a stand-alone command, and not as a child of the failed command. It means that the tasks will be created as tasks of RemoveMemoryVolumesCommand and thus its end-action methods will be called and not the ones of the failed command. Change-Id: Ib4b270ec0e1ab41cae34459dde9f9cf47b1b5bdf Bug-Url: https://bugzilla.redhat.com/1056598 Signed-off-by: Arik Hadas <[email protected]> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveMemoryVolumesCommand.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/memory/MemoryImageRemover.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/MemoryImageRemoverOnDataDomain.java M backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveMemoryVolumesParameters.java 7 files changed, 114 insertions(+), 58 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/31/23831/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java index 7911a2c..3803b9c 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java @@ -324,19 +324,15 @@ getSnapshotDao().removeMemoryFromSnapshot(snapshot.getId()); } - return removeMemoryVolumes(memoryVolume); + return removeMemoryVolumes(snapshot); } - private boolean removeMemoryVolumes(String memoryVolumes) { - RemoveMemoryVolumesParameters parameters = new RemoveMemoryVolumesParameters(memoryVolumes, getVmId()); - parameters.setParentCommand(getActionType()); - parameters.setEntityInfo(getParameters().getEntityInfo()); - parameters.setParentParameters(getParameters()); - - return getBackend().runInternalAction( + private boolean removeMemoryVolumes(Snapshot snapshot) { + VdcReturnValueBase retVal = getBackend().runInternalAction( VdcActionType.RemoveMemoryVolumes, - parameters, - ExecutionHandler.createDefaultContexForTasks(getExecutionContext())).getSucceeded(); + new RemoveMemoryVolumesParameters(snapshot.getMemoryVolume(), getVmId())); + + return retVal.getSucceeded(); } protected boolean isLiveSnapshotApplicable() { diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java index e259978..ce724af 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java @@ -13,7 +13,6 @@ import org.apache.commons.lang.StringUtils; import org.ovirt.engine.core.bll.job.ExecutionContext; import org.ovirt.engine.core.bll.job.ExecutionHandler; -import org.ovirt.engine.core.bll.memory.MemoryImageRemoverOnDataDomain; import org.ovirt.engine.core.bll.memory.MemoryUtils; import org.ovirt.engine.core.bll.network.MacPoolManager; import org.ovirt.engine.core.bll.network.VmInterfaceManager; @@ -32,6 +31,7 @@ import org.ovirt.engine.core.common.VdcObjectType; import org.ovirt.engine.core.common.action.ImportVmParameters; import org.ovirt.engine.core.common.action.MoveOrCopyImageGroupParameters; +import org.ovirt.engine.core.common.action.RemoveMemoryVolumesParameters; import org.ovirt.engine.core.common.action.VdcActionParametersBase; import org.ovirt.engine.core.common.action.VdcActionType; import org.ovirt.engine.core.common.action.VdcReturnValueBase; @@ -1120,7 +1120,7 @@ setVm(null); if (getVm() != null) { - removeVmSnapshots(getVm()); + removeVmSnapshots(); endActionOnAllImageGroups(); removeVmNetworkInterfaces(); getVmDynamicDAO().remove(getVmId()); @@ -1134,10 +1134,21 @@ } } - private void removeVmSnapshots(VM vm) { - Set<String> memoryStates = snapshotsManager.removeSnapshots(vm.getId()); - if (!memoryStates.isEmpty()) { - new MemoryImageRemoverOnDataDomain(vm, this).remove(memoryStates); + private void removeVmSnapshots() { + Guid vmId = getVmId(); + Set<String> memoryStates = snapshotsManager.removeSnapshots(vmId); + for (String memoryState : memoryStates) { + removeMemoryVolumes(memoryState, vmId); + } + } + + private void removeMemoryVolumes(String memoryVolume, Guid vmId) { + VdcReturnValueBase retVal = getBackend().runInternalAction( + VdcActionType.RemoveMemoryVolumes, + new RemoveMemoryVolumesParameters(memoryVolume, vmId)); + + if (!retVal.getSucceeded()) { + log.errorFormat("Failed to remove memory volumes: {0}", memoryVolume); } } diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveMemoryVolumesCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveMemoryVolumesCommand.java index da03968..13be472 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveMemoryVolumesCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveMemoryVolumesCommand.java @@ -4,13 +4,14 @@ import java.util.Collections; import java.util.List; -import org.ovirt.engine.core.bll.memory.HibernationVolumesRemover; +import org.ovirt.engine.core.bll.memory.MemoryImageRemoverOnDataDomain; import org.ovirt.engine.core.bll.tasks.TaskHandlerCommand; import org.ovirt.engine.core.bll.utils.PermissionSubject; import org.ovirt.engine.core.common.VdcObjectType; import org.ovirt.engine.core.common.action.RemoveMemoryVolumesParameters; import org.ovirt.engine.core.common.action.VdcActionType; import org.ovirt.engine.core.common.asynctasks.AsyncTaskCreationInfo; +import org.ovirt.engine.core.common.asynctasks.AsyncTaskType; import org.ovirt.engine.core.compat.Guid; import org.ovirt.engine.core.compat.NotImplementedException; @@ -22,8 +23,6 @@ @NonTransactiveCommandAttribute @InternalCommandAttribute public class RemoveMemoryVolumesCommand<T extends RemoveMemoryVolumesParameters> extends CommandBase<T> implements TaskHandlerCommand<T> { - /** fictive list of task IDs, used when we don't want to add tasks */ - private static final ArrayList<Guid> dummyTaskIdList = new ArrayList<>(); public RemoveMemoryVolumesCommand(T parameters) { super(parameters); @@ -35,13 +34,18 @@ @Override protected void executeCommand() { - HibernationVolumesRemover hibernationVolumesRemover = - new HibernationVolumesRemover( - getParameters().getMemoryVolumes(), + MemoryImageRemoverOnDataDomain memoryImageRemover = + new MemoryImageRemoverOnDataDomain( getParameters().getVmId(), this); - setSucceeded(hibernationVolumesRemover.remove()); + setSucceeded(memoryImageRemover.remove( + Collections.singleton(getParameters().getMemoryVolumes()))); + } + + @Override + protected AsyncTaskType getTaskType() { + return AsyncTaskType.deleteImage; } ////////////////////////////////// @@ -50,35 +54,27 @@ @Override public VdcActionType getActionType() { - return super.getActionType(); + return getParameters().getParentCommand() == VdcActionType.Unknown ? + super.getActionType() : getParameters().getParentCommand(); } - /** - * Not adding tasks - */ @Override public Guid createTask(Guid taskId, AsyncTaskCreationInfo asyncTaskCreationInfo, VdcActionType parentCommand) { - return Guid.Empty; + return super.createTask(taskId, asyncTaskCreationInfo, parentCommand); } - /** - * Not adding tasks - */ @Override public Guid createTask(Guid taskId, AsyncTaskCreationInfo asyncTaskCreationInfo, VdcActionType parentCommand, VdcObjectType vdcObjectType, Guid... entityIds) { - return Guid.Empty; + return super.createTask(taskId, asyncTaskCreationInfo, parentCommand, vdcObjectType, entityIds); } - /** - * Not adding task IDs - */ @Override public ArrayList<Guid> getTaskIdList() { - return dummyTaskIdList; + return super.getTaskIdList(); } @Override @@ -86,20 +82,14 @@ throw new NotImplementedException(); } - /** - * Not adding place holders - */ @Override public Guid persistAsyncTaskPlaceHolder() { - return Guid.Empty; + return super.persistAsyncTaskPlaceHolder(getActionType()); } - /** - * Not adding place holders - */ @Override public Guid persistAsyncTaskPlaceHolder(String taskKey) { - return Guid.Empty; + return super.persistAsyncTaskPlaceHolder(getActionType(), taskKey); } @Override 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 64a7cb0..2d30e9e 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 @@ -10,7 +10,6 @@ import org.apache.commons.lang.StringUtils; import org.ovirt.engine.core.bll.job.ExecutionContext; import org.ovirt.engine.core.bll.job.ExecutionHandler; -import org.ovirt.engine.core.bll.memory.MemoryImageRemoverOnDataDomain; import org.ovirt.engine.core.bll.network.ExternalNetworkManager; import org.ovirt.engine.core.bll.quota.QuotaConsumptionParameter; import org.ovirt.engine.core.bll.quota.QuotaStorageConsumptionParameter; @@ -25,6 +24,7 @@ import org.ovirt.engine.core.common.AuditLogType; import org.ovirt.engine.core.common.VdcObjectType; import org.ovirt.engine.core.common.action.RemoveAllVmImagesParameters; +import org.ovirt.engine.core.common.action.RemoveMemoryVolumesParameters; import org.ovirt.engine.core.common.action.RemoveVmParameters; import org.ovirt.engine.core.common.action.VdcActionType; import org.ovirt.engine.core.common.action.VdcReturnValueBase; @@ -123,11 +123,32 @@ } } - new MemoryImageRemoverOnDataDomain(getVm(), this).remove(memoryStates); + removeMemoryVolumes(); return true; } + private void removeMemoryVolumes() { + for (String memoryState : memoryStates) { + VdcReturnValueBase retVal = getBackend().runInternalAction( + VdcActionType.RemoveMemoryVolumes, + buildRemoveMemoryVolumesParameters(memoryState, getVmId())); + + if (!retVal.getSucceeded()) { + log.errorFormat("Failed to remove memory volumes whie removing vm {0} (volumes: {1})", + getVmId(), memoryState); + } + } + } + + private RemoveMemoryVolumesParameters buildRemoveMemoryVolumesParameters(String memoryState, Guid vmId) { + RemoveMemoryVolumesParameters parameters = + new RemoveMemoryVolumesParameters(memoryState, getVmId()); + parameters.setRemoveOnlyIfNotUsedAtAll(true); + + return parameters; + } + @Override protected boolean canDoAction() { if (getVm() == null) { diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/MemoryImageRemover.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/MemoryImageRemover.java index bc66680..ce17714 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/MemoryImageRemover.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/MemoryImageRemover.java @@ -56,13 +56,23 @@ if (isMemoryStateRemovable(memoryVolumes)) { return removeMemoryVolumes(memoryVolumes); } + return true; } - protected void removeMemoryVolumes(Set<String> memoryVolumes) { + /** + * Try to remove all the given memory volumes + * + * @param memoryVolumes - memory volumes to remove + * @return true if all the memory volumes were removed successfully, false otherwise + */ + protected boolean removeMemoryVolumes(Set<String> memoryVolumes) { + boolean allVolumesRemovedSucessfully = true; for (String memoryVols : memoryVolumes) { - removeMemoryVolume(memoryVols); + allVolumesRemovedSucessfully &= removeMemoryVolume(memoryVols); } + + return allVolumesRemovedSucessfully; } private boolean removeMemoryVolumes(String memVols) { diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/MemoryImageRemoverOnDataDomain.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/MemoryImageRemoverOnDataDomain.java index f1af60d..791af34 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/MemoryImageRemoverOnDataDomain.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/MemoryImageRemoverOnDataDomain.java @@ -4,24 +4,28 @@ import java.util.Set; import org.ovirt.engine.core.bll.tasks.TaskHandlerCommand; +import org.ovirt.engine.core.common.action.RemoveMemoryVolumesParameters; import org.ovirt.engine.core.common.businessentities.Disk; -import org.ovirt.engine.core.common.businessentities.VM; import org.ovirt.engine.core.common.vdscommands.DeleteImageGroupVDSCommandParameters; import org.ovirt.engine.core.compat.Guid; import org.ovirt.engine.core.dal.dbbroker.DbFacade; +import org.ovirt.engine.core.dao.SnapshotDao; public class MemoryImageRemoverOnDataDomain extends MemoryImageRemover { protected Boolean cachedPostZero; - private VM vm; + private Guid vmId; + private boolean removeOnlyIfNotUsedAtAll; - public MemoryImageRemoverOnDataDomain(VM vm, TaskHandlerCommand<?> enclosingCommand) { - super(enclosingCommand); - this.vm = vm; + public MemoryImageRemoverOnDataDomain(Guid vmId, + TaskHandlerCommand<? extends RemoveMemoryVolumesParameters> enclosingCommand) { + super(enclosingCommand, false); + this.vmId = vmId; + removeOnlyIfNotUsedAtAll = enclosingCommand.getParameters().isRemoveOnlyIfNotUsedAtAll(); } - public void remove(Set<String> memoryStates) { - removeMemoryVolumes(memoryStates); + public boolean remove(Set<String> memoryStates) { + return removeMemoryVolumes(memoryStates); } @Override @@ -40,7 +44,7 @@ if (cachedPostZero == null) { // check if one of the disks is marked with wipe_after_delete cachedPostZero = - getDbFacade().getDiskDao().getAllForVm(vm.getId()).contains( + getDbFacade().getDiskDao().getAllForVm(vmId).contains( new Object() { @Override public boolean equals(Object obj) { @@ -48,13 +52,22 @@ } }); } + return cachedPostZero; } @Override protected boolean isMemoryStateRemovable(String memoryVolume) { - return !memoryVolume.isEmpty() && - getDbFacade().getSnapshotDao().getNumOfSnapshotsByMemory(memoryVolume) == 0; + if (memoryVolume.isEmpty()) { + return false; + } + + int numOfSnapshotsUsingThisMemory = getSnapshotDao().getNumOfSnapshotsByMemory(memoryVolume); + return numOfSnapshotsUsingThisMemory == (removeOnlyIfNotUsedAtAll ? 0 : 1); + } + + protected SnapshotDao getSnapshotDao() { + return getDbFacade().getSnapshotDao(); } protected DbFacade getDbFacade() { diff --git a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveMemoryVolumesParameters.java b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveMemoryVolumesParameters.java index ca0593c..910ff17 100644 --- a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveMemoryVolumesParameters.java +++ b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveMemoryVolumesParameters.java @@ -6,6 +6,12 @@ /** comma-separated string of UUIDs representing the memory volumes */ private String memoryVolumes; private Guid vmId; + /** In the general case, we remove the memory volumes only if there is only + * one snapshot in DB that uses it because we remove the memory before + * removing the snapshots from the DB. But in some cases we first remove + * the snapshot from the DB and only then remove its memory and in that + * case we should remove the memory only if no other snapshot uses it */ + private boolean removeOnlyIfNotUsedAtAll; public RemoveMemoryVolumesParameters(String memoryVolumes, Guid vmId) { this.memoryVolumes = memoryVolumes; @@ -32,4 +38,13 @@ public void setVmId(Guid vmId) { this.vmId = vmId; } + + public boolean isRemoveOnlyIfNotUsedAtAll() { + return removeOnlyIfNotUsedAtAll; + } + + public void setRemoveOnlyIfNotUsedAtAll(boolean removeOnlyIfNotUsedAtAll) { + this.removeOnlyIfNotUsedAtAll = removeOnlyIfNotUsedAtAll; + } + } -- To view, visit http://gerrit.ovirt.org/23831 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ib4b270ec0e1ab41cae34459dde9f9cf47b1b5bdf Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: ovirt-engine-3.3 Gerrit-Owner: Arik Hadas <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
