Arik Hadas has uploaded a new change for review.

Change subject: core: [cleanup] split execute method of CreateAllSnapshots
......................................................................

core: [cleanup] split execute method of CreateAllSnapshots

CreateAllSnapshotsFromVmCommand#executeVmCommand method is too long.
it's going to be even longer after adding memory snapshot, so it's a
good point to organize it by spliting the different concerns in this
method to several private methods.

Change-Id: Ie401b3aa60abba497237e02c77d8d3a77cef9a20
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/snapshots/SnapshotsManager.java
2 files changed, 33 insertions(+), 23 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/79/14179/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 76395ea..d6b1b1c 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
@@ -27,6 +27,7 @@
 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;
 import org.ovirt.engine.core.common.businessentities.Snapshot.SnapshotType;
 import org.ovirt.engine.core.common.businessentities.VM;
@@ -104,42 +105,50 @@
 
         setActionReturnValue(createdSnapshotId);
 
-        if (getDisksList().isEmpty()) {
+        addSnapshotToDB(createdSnapshotId);
+        createSnapshotsForDisks(getDisksList(), newActiveSnapshotId);
+
+        if (getTaskIdList().isEmpty()) {
             getParameters().setTaskGroupSuccess(true);
-            new SnapshotsManager().addSnapshot(createdSnapshotId,
+            incrementVmGeneration();
+        }
+        setSucceeded(true);
+    }
+
+    private Snapshot addSnapshotToDB(Guid snapshotId) {
+        if (getDisksList().isEmpty()) {
+            return new SnapshotsManager().addSnapshot(snapshotId,
                     getParameters().getDescription(),
                     SnapshotStatus.OK,
                     getParameters().getSnapshotType(),
                     getVm(),
                     true,
                     getCompensationContext());
-            // at the moment there's no need to execute vdsm Snapshot command 
for diskless snapshots,
-            // when support for ram snapshot will be introduced, this vdsm 
command should be executed
-            // for diskless snapshots as well (currently executed within 
endSuccesfully() method.
-            incrementVmGeneration();
-        } else {
-            new SnapshotsManager().addSnapshot(createdSnapshotId,
+        }
+        else {
+            return new SnapshotsManager().addSnapshot(snapshotId,
                     getParameters().getDescription(),
                     getParameters().getSnapshotType(),
                     getVm(),
                     getCompensationContext());
+        }
+    }
 
-            for (DiskImage image : getDisksList()) {
+    private void createSnapshotsForDisks(List<DiskImage> disks, Guid 
vmSnapshotId) {
+        for (DiskImage image : getDisksList()) {
 
-                VdcReturnValueBase vdcReturnValue = 
Backend.getInstance().runInternalAction(
-                                VdcActionType.CreateSnapshot,
-                                buildCreateSnapshotParameters(image, 
newActiveSnapshotId),
-                                
ExecutionHandler.createDefaultContexForTasks(getExecutionContext()));
+            VdcReturnValueBase vdcReturnValue = 
Backend.getInstance().runInternalAction(
+                            VdcActionType.CreateSnapshot,
+                            buildCreateSnapshotParameters(image, vmSnapshotId),
+                            
ExecutionHandler.createDefaultContexForTasks(getExecutionContext()));
 
-                if (vdcReturnValue.getSucceeded()) {
-                    
getTaskIdList().addAll(vdcReturnValue.getInternalTaskIdList());
-                } else {
-                    throw new 
VdcBLLException(vdcReturnValue.getFault().getError(),
-                            
"CreateAllSnapshotsFromVmCommand::executeVmCommand: Failed to create 
snapshot!");
-                }
+            if (vdcReturnValue.getSucceeded()) {
+                getTaskIdList().addAll(vdcReturnValue.getInternalTaskIdList());
+            } else {
+                throw new VdcBLLException(vdcReturnValue.getFault().getError(),
+                        "CreateAllSnapshotsFromVmCommand::executeVmCommand: 
Failed to create snapshot!");
             }
         }
-        setSucceeded(true);
     }
 
     private ImagesActionsParametersBase 
buildCreateSnapshotParameters(DiskImage image, Guid snapshotId) {
diff --git 
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/snapshots/SnapshotsManager.java
 
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/snapshots/SnapshotsManager.java
index b3d63b6..7a644f9 100644
--- 
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/snapshots/SnapshotsManager.java
+++ 
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/snapshots/SnapshotsManager.java
@@ -112,14 +112,14 @@
      *            The VM to save in configuration.
      * @param compensationContext
      *            Context for saving compensation details.
-     * @return The new snapshot's ID.
+     * @return the added snapshot
      */
-    public void addSnapshot(Guid snapshotId,
+    public Snapshot addSnapshot(Guid snapshotId,
             String description,
             SnapshotType snapshotType,
             VM vm,
             final CompensationContext compensationContext) {
-        addSnapshot(snapshotId, description, SnapshotStatus.LOCKED, 
snapshotType, vm, true, compensationContext);
+        return addSnapshot(snapshotId, description, SnapshotStatus.LOCKED, 
snapshotType, vm, true, compensationContext);
     }
 
     /**
@@ -139,6 +139,7 @@
      *            Should VM configuration be generated and saved?
      * @param compensationContext
      *            In case compensation is needed.
+     * @return the saved snapshot
      */
     public Snapshot addSnapshot(Guid snapshotId,
             String description,


--
To view, visit http://gerrit.ovirt.org/14179
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie401b3aa60abba497237e02c77d8d3a77cef9a20
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

Reply via email to