Arik Hadas has uploaded a new change for review. Change subject: core: revert the existing solution for bz 878041 ......................................................................
core: revert the existing solution for bz 878041 Before having the auto-start VMs runner job, the problem of HA VMs that could not be restarted when failing during live snapshot operation was solved by comparing the state of the VM at the end of the live snapshot operation with its state when the command started, and if we saw that the VM went down unintentionally we ran it at the end of the live snapshot command. That solution was wrong since it was specific to live snapshot operation and it required to put large portion of the code in endVmCommand method inside new transaction scope. Now that we lock the VM for the entire live snapshot operation and we have the auto-start VMs runner job that keep trying to restart failed HA VM while it is locked, which is a more general and clean solution that solves this case as well, we can revert the previous solution. Change-Id: I242a2278c9c23139026b58564ee35967e5258bfe 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/common/src/main/java/org/ovirt/engine/core/common/action/CreateAllSnapshotsFromVmParameters.java 2 files changed, 41 insertions(+), 85 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/63/19163/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 cb5846c..45432ec 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 @@ -8,7 +8,6 @@ import org.apache.commons.lang.exception.ExceptionUtils; import org.ovirt.engine.core.bll.job.ExecutionContext; -import org.ovirt.engine.core.bll.job.ExecutionHandler; import org.ovirt.engine.core.bll.memory.LiveSnapshotMemoryImageBuilder; import org.ovirt.engine.core.bll.memory.MemoryImageBuilder; import org.ovirt.engine.core.bll.memory.NullableMemoryImageBuilder; @@ -28,7 +27,6 @@ import org.ovirt.engine.core.common.VdcObjectType; import org.ovirt.engine.core.common.action.CreateAllSnapshotsFromVmParameters; import org.ovirt.engine.core.common.action.ImagesActionsParametersBase; -import org.ovirt.engine.core.common.action.RunVmParams; import org.ovirt.engine.core.common.action.VdcActionParametersBase; import org.ovirt.engine.core.common.action.VdcActionType; import org.ovirt.engine.core.common.action.VdcReturnValueBase; @@ -42,7 +40,6 @@ import org.ovirt.engine.core.common.businessentities.StorageDomain; import org.ovirt.engine.core.common.businessentities.VM; import org.ovirt.engine.core.common.businessentities.VMStatus; -import org.ovirt.engine.core.common.businessentities.VmExitStatus; import org.ovirt.engine.core.common.config.Config; import org.ovirt.engine.core.common.config.ConfigValues; import org.ovirt.engine.core.common.errors.VdcBLLException; @@ -120,7 +117,6 @@ protected void executeVmCommand() { Guid createdSnapshotId = getSnapshotDao().getId(getVmId(), SnapshotType.ACTIVE); getParameters().setSnapshotType(determineSnapshotType()); - getParameters().setInitialVmStatus(getVm().getStatus()); getSnapshotDao().updateId(createdSnapshotId, newActiveSnapshotId); @@ -215,66 +211,48 @@ @Override protected void endVmCommand() { - // The following code must be executed in an inner transaction to make the changes visible - // to the RunVm command that might occur afterwards - TransactionSupport.executeInNewTransaction(new TransactionMethod<Void>() { - - @Override - public Void runInTransaction() { - boolean taskGroupSucceeded = getParameters().getTaskGroupSuccess(); - Snapshot createdSnapshot = getSnapshotDao().get(getVmId(), getParameters().getSnapshotType(), SnapshotStatus.LOCKED); - // if the snapshot was not created the command should be - // handled as a failure - if (createdSnapshot == null) { - taskGroupSucceeded = false; - } - if (taskGroupSucceeded) { - getSnapshotDao().updateStatus(createdSnapshot.getId(), SnapshotStatus.OK); - - if (isLiveSnapshotApplicable()) { - performLiveSnapshot(createdSnapshot); - } else { - // If the created snapshot contains memory, remove the memory volumes as - // they are not in use since no live snapshot was created - if (!createdSnapshot.getMemoryVolume().isEmpty()) { - logMemorySavingFailed(); - if (!removeMemoryFromSnapshot(createdSnapshot, true)) { - log.errorFormat("Failed to remove unused memory {0} of snapshot {1}", - createdSnapshot.getMemoryVolume(), createdSnapshot.getId()); - } - } - } - } else { - if (createdSnapshot != null) { - revertToActiveSnapshot(createdSnapshot.getId()); - // If the removed snapshot contained memory, remove the memory volumes - // Note that the memory volumes might not have been created - if (!removeMemoryFromSnapshot(createdSnapshot, false)) { - log.warnFormat("Failed to remove memory {0} of snapshot {1}", - createdSnapshot.getMemoryVolume(), createdSnapshot.getId()); - } - } else { - log.warnFormat("No snapshot was created for VM {0} which is in LOCKED status", getVmId()); - } - } - - incrementVmGeneration(); - - endActionOnDisks(); - setSucceeded(taskGroupSucceeded); - getReturnValue().setEndActionTryAgain(false); - return null; - } - }); - - // if the VM is HA + it went down during the snapshot creation process because of an error, run it - // (during the snapshot creation process the VM couldn't be started (rerun)) - if (getVm() != null && getVm().isAutoStartup() && isVmDownUnintentionally() && - getParameters().getInitialVmStatus() != VMStatus.Down) { - Backend.getInstance().runInternalAction(VdcActionType.RunVm, - new RunVmParams(getVmId()), - ExecutionHandler.createInternalJobContext()); + boolean taskGroupSucceeded = getParameters().getTaskGroupSuccess(); + Snapshot createdSnapshot = getSnapshotDao().get(getVmId(), getParameters().getSnapshotType(), SnapshotStatus.LOCKED); + // if the snapshot was not created the command should be + // handled as a failure + if (createdSnapshot == null) { + taskGroupSucceeded = false; } + if (taskGroupSucceeded) { + getSnapshotDao().updateStatus(createdSnapshot.getId(), SnapshotStatus.OK); + + if (isLiveSnapshotApplicable()) { + performLiveSnapshot(createdSnapshot); + } else { + // If the created snapshot contains memory, remove the memory volumes as + // they are not in use since no live snapshot was created + if (!createdSnapshot.getMemoryVolume().isEmpty()) { + logMemorySavingFailed(); + if (!removeMemoryFromSnapshot(createdSnapshot, true)) { + log.errorFormat("Failed to remove unused memory {0} of snapshot {1}", + createdSnapshot.getMemoryVolume(), createdSnapshot.getId()); + } + } + } + } else { + if (createdSnapshot != null) { + revertToActiveSnapshot(createdSnapshot.getId()); + // If the removed snapshot contained memory, remove the memory volumes + // Note that the memory volumes might not have been created + if (!removeMemoryFromSnapshot(createdSnapshot, false)) { + log.warnFormat("Failed to remove memory {0} of snapshot {1}", + createdSnapshot.getMemoryVolume(), createdSnapshot.getId()); + } + } else { + log.warnFormat("No snapshot was created for VM {0} which is in LOCKED status", getVmId()); + } + } + + incrementVmGeneration(); + + endActionOnDisks(); + setSucceeded(taskGroupSucceeded); + getReturnValue().setEndActionTryAgain(false); } private void logMemorySavingFailed() { @@ -313,14 +291,6 @@ }); return sortedList; - } - - /** - * returns true if the VM is down because of an error, otherwise false - */ - private boolean isVmDownUnintentionally() { - VM vm = getVmDAO().get(getVmId()); - return vm.getExitStatus() == VmExitStatus.Error && vm.isDown(); } /** diff --git a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/CreateAllSnapshotsFromVmParameters.java b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/CreateAllSnapshotsFromVmParameters.java index 5e607fd..d445c77 100644 --- a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/CreateAllSnapshotsFromVmParameters.java +++ b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/CreateAllSnapshotsFromVmParameters.java @@ -5,7 +5,6 @@ import org.hibernate.validator.constraints.NotEmpty; import org.ovirt.engine.core.common.businessentities.BusinessEntitiesDefinitions; import org.ovirt.engine.core.common.businessentities.Snapshot.SnapshotType; -import org.ovirt.engine.core.common.businessentities.VMStatus; import org.ovirt.engine.core.common.validation.annotation.ValidDescription; import org.ovirt.engine.core.common.validation.group.CreateEntity; import org.ovirt.engine.core.compat.Guid; @@ -21,11 +20,6 @@ private String description; private boolean needsLocking; - /** - * Used to store the vm status when the command start, will be used to check if the vm went down during the - * execution - */ - private VMStatus initialVmStatus; /** Used to indicate the type of snapshot to take */ private SnapshotType snapshotType; @@ -54,14 +48,6 @@ public SnapshotType getSnapshotType() { return snapshotType; - } - - public void setInitialVmStatus(VMStatus vmStatus) { - this.initialVmStatus = vmStatus; - } - - public VMStatus getInitialVmStatus() { - return initialVmStatus; } /** -- To view, visit http://gerrit.ovirt.org/19163 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I242a2278c9c23139026b58564ee35967e5258bfe Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Arik Hadas <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
