You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by ed...@apache.org on 2013/10/23 21:22:11 UTC

[2/2] git commit: updated refs/heads/master to 0ed7ebd

Squashed & merged commit of the following:

commit c9ee0d12e191e803fb341f3f96e95ca434a36f6c
Author: Wei Zhou <w....@leaseweb.com>
Date:   Wed Oct 23 16:55:10 2013 +0200

    CLOUDSTACK-4931, CLOUDSTACK-4937: setDetails to user VMs only
    (cherry picked from commit a94acc5a43aeaf5f18f1912e2653a92f6041a6e5)

commit fe1586c71377bc6d219db2dcf088c40b65dd1fc4
Author: Anthony Xu <an...@citrix.com>
Date:   Tue Oct 22 11:20:27 2013 -0700

    CLOUDSTACK-4649:
            vm sync tracks the pv driver version for xenserver

     Anthony

commit 56a218f66eda540b4b4b04030ee71fc6863f8532
Author: Anthony Xu <an...@citrix.com>
Date:   Mon Oct 21 16:10:07 2013 -0700

    CLOUDSTACK-4649:
        xs 6.1/6.2 introduce the new virtual platform, so there are two virtual platforms, windows PV driver version must match virtual platforms,
    this patch tracks PV driver versions in vm details and template details.

    Anthony

commit 4e85d28c678a6f96b5b70d8d33fc60f9d1ea3df6
Author: Laszlo Hornyak <la...@gmail.com>
Date:   Mon Oct 21 21:17:33 2013 +0200

    removed unused static field

    - s_httpClientManager was not used

    Signed-off-by: Laszlo Hornyak <la...@gmail.com>

commit d4121fa26023db236f7396cea455ef090672ae9a
Author: Chris Suich <ch...@netapp.com>
Date:   Tue Oct 22 10:45:22 2013 -0400

    Updated DataMotionServiceImpl and ApiResponseHelper based on review feedback.

commit aaf026e1e4204d405bcda2ae4f1a01b1d0f7e7cb
Author: Chris Suich <ch...@netapp.com>
Date:   Thu Oct 17 14:27:12 2013 -0400

    Added context to strategy sorting error responses
    Added TODOs for DRYing out pickStrategy() overloading

commit a221f4aa3fb2ddc255bc35cf753f98f88f5bf44e
Author: Chris Suich <ch...@netapp.com>
Date:   Wed Oct 16 09:57:28 2013 -0400

    Updated inefficient strategy sorting/selection
    Removed unnecessary canRevertSnapshot from PrimaryDataStoreDriver
    Other general cleaup and fixes from reviews

commit 7d58949c6a1b7e853e891b59387a9620e8cd7a91
Author: Chris Suich <ch...@netapp.com>
Date:   Mon Oct 14 14:01:22 2013 -0400

    Added volume snapshot revert capability to SnapshotResponse
    Updated UI to hide/show snapshot revert action per snapshot

Signed-off-by: Edison Su <su...@gmail.com>


Project: http://git-wip-us.apache.org/repos/asf/cloudstack/repo
Commit: http://git-wip-us.apache.org/repos/asf/cloudstack/commit/0ed7ebd7
Tree: http://git-wip-us.apache.org/repos/asf/cloudstack/tree/0ed7ebd7
Diff: http://git-wip-us.apache.org/repos/asf/cloudstack/diff/0ed7ebd7

Branch: refs/heads/master
Commit: 0ed7ebd7e72988d091f129d37fd37acac3bd661f
Parents: 4b530c8
Author: Chris Suich <ch...@netapp.com>
Authored: Wed Oct 23 15:17:24 2013 -0400
Committer: Edison Su <su...@gmail.com>
Committed: Wed Oct 23 12:21:43 2013 -0700

----------------------------------------------------------------------
 .../org/apache/cloudstack/api/ApiConstants.java |   4 +-
 .../api/response/SnapshotResponse.java          |  25 ++-
 .../subsystem/api/storage/SnapshotInfo.java     |   2 +
 .../subsystem/api/storage/SnapshotStrategy.java |   9 +-
 .../subsystem/api/storage/StrategyPriority.java |  86 +++-----
 .../api/storage/StrategyPriorityTest.java       |  90 +++++---
 .../com/cloud/vm/VirtualMachineManagerImpl.java | 208 ++++++++++---------
 .../storage/motion/DataMotionServiceImpl.java   |  38 ++--
 .../test/FakePrimaryDataStoreDriver.java        |   7 +-
 .../cloudstack/storage/test/SnapshotTest.java   |  56 +++--
 .../storage/snapshot/SnapshotObject.java        |  39 ++--
 .../snapshot/XenserverSnapshotStrategy.java     |   9 +-
 .../CloudStackPrimaryDataStoreDriverImpl.java   |  49 +++--
 .../SamplePrimaryDataStoreDriverImpl.java       |  22 +-
 .../driver/SolidfirePrimaryDataStoreDriver.java |  73 ++++---
 server/src/com/cloud/api/ApiResponseHelper.java |  22 +-
 .../storage/snapshot/SnapshotManagerImpl.java   |  57 ++---
 ui/scripts/storage.js                           |   2 +-
 18 files changed, 441 insertions(+), 357 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/cloudstack/blob/0ed7ebd7/api/src/org/apache/cloudstack/api/ApiConstants.java
----------------------------------------------------------------------
diff --git a/api/src/org/apache/cloudstack/api/ApiConstants.java b/api/src/org/apache/cloudstack/api/ApiConstants.java
index c75e6a0..dbf6685 100755
--- a/api/src/org/apache/cloudstack/api/ApiConstants.java
+++ b/api/src/org/apache/cloudstack/api/ApiConstants.java
@@ -34,6 +34,7 @@ public class ApiConstants {
     public static final String BYTES_READ_RATE = "bytesreadrate";
     public static final String BYTES_WRITE_RATE = "byteswriterate";
     public static final String CATEGORY = "category";
+    public static final String CAN_REVERT = "canrevert";
     public static final String CERTIFICATE = "certificate";
     public static final String PRIVATE_KEY = "privatekey";
     public static final String DOMAIN_SUFFIX = "domainsuffix";
@@ -187,6 +188,7 @@ public class ApiConstants {
     public static final String REQUIRES_HVM = "requireshvm";
     public static final String RESOURCE_TYPE = "resourcetype";
     public static final String RESPONSE = "response";
+    public static final String REVERTABLE = "revertable";
     public static final String QUERY_FILTER = "queryfilter";
     public static final String SCHEDULE = "schedule";
     public static final String SCOPE = "scope";
@@ -249,7 +251,7 @@ public class ApiConstants {
     public static final String IS_VOLATILE = "isvolatile";
     public static final String VOLUME_ID = "volumeid";
     public static final String ZONE_ID = "zoneid";
-    public static final String ZONE_NAME = "zonename";    
+    public static final String ZONE_NAME = "zonename";
     public static final String NETWORK_TYPE = "networktype";
     public static final String PAGE = "page";
     public static final String PAGE_SIZE = "pagesize";

http://git-wip-us.apache.org/repos/asf/cloudstack/blob/0ed7ebd7/api/src/org/apache/cloudstack/api/response/SnapshotResponse.java
----------------------------------------------------------------------
diff --git a/api/src/org/apache/cloudstack/api/response/SnapshotResponse.java b/api/src/org/apache/cloudstack/api/response/SnapshotResponse.java
index e9cb109..7c2b4a9 100644
--- a/api/src/org/apache/cloudstack/api/response/SnapshotResponse.java
+++ b/api/src/org/apache/cloudstack/api/response/SnapshotResponse.java
@@ -26,18 +26,8 @@ import org.apache.cloudstack.api.EntityReference;
 import com.cloud.serializer.Param;
 import com.cloud.storage.Snapshot;
 import com.google.gson.annotations.SerializedName;
-import com.cloud.serializer.Param;
-import com.cloud.storage.Snapshot;
-import com.google.gson.annotations.SerializedName;
-import org.apache.cloudstack.api.ApiConstants;
-import org.apache.cloudstack.api.BaseResponse;
-import org.apache.cloudstack.api.EntityReference;
-
-import java.util.Date;
-import java.util.List;
 
 @EntityReference(value=Snapshot.class)
-@SuppressWarnings("unused")
 public class SnapshotResponse extends BaseResponse implements ControlledEntityResponse {
     @SerializedName(ApiConstants.ID)
     @Param(description = "ID of the snapshot")
@@ -100,6 +90,9 @@ public class SnapshotResponse extends BaseResponse implements ControlledEntityRe
     @SerializedName(ApiConstants.TAGS)  @Param(description="the list of resource tags associated with snapshot", responseObject = ResourceTagResponse.class)
     private List<ResourceTagResponse> tags;
 
+    @SerializedName(ApiConstants.REVERTABLE)
+    @Param(description="indicates whether the underlying storage supports reverting the volume to this snapshot")
+    private boolean revertable;
 
     @Override
     public String getObjectId() {
@@ -118,6 +111,7 @@ public class SnapshotResponse extends BaseResponse implements ControlledEntityRe
         return accountName;
     }
 
+    @Override
     public void setAccountName(String accountName) {
         this.accountName = accountName;
     }
@@ -131,6 +125,7 @@ public class SnapshotResponse extends BaseResponse implements ControlledEntityRe
         this.domainId = domainId;
     }
 
+    @Override
     public void setDomainName(String domainName) {
         this.domainName = domainName;
     }
@@ -180,8 +175,16 @@ public class SnapshotResponse extends BaseResponse implements ControlledEntityRe
     public void setZoneId(String zoneId) {
         this.zoneId = zoneId;
     }
- 
+
     public void setTags(List<ResourceTagResponse> tags) {
         this.tags = tags;
     }
+
+    public boolean isRevertable() {
+        return revertable;
+    }
+
+    public void setRevertable(boolean revertable) {
+        this.revertable = revertable;
+    }
 }

http://git-wip-us.apache.org/repos/asf/cloudstack/blob/0ed7ebd7/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotInfo.java
----------------------------------------------------------------------
diff --git a/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotInfo.java b/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotInfo.java
index 8d6b760..a0ef7dd 100644
--- a/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotInfo.java
+++ b/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotInfo.java
@@ -32,4 +32,6 @@ public interface SnapshotInfo extends DataObject, Snapshot {
     Long getDataCenterId();
 
     ObjectInDataStoreStateMachine.State getStatus();
+
+    boolean isRevertable();
 }

http://git-wip-us.apache.org/repos/asf/cloudstack/blob/0ed7ebd7/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotStrategy.java
----------------------------------------------------------------------
diff --git a/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotStrategy.java b/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotStrategy.java
index e4cecb6..3436d16 100644
--- a/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotStrategy.java
+++ b/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotStrategy.java
@@ -19,6 +19,13 @@ package org.apache.cloudstack.engine.subsystem.api.storage;
 import com.cloud.storage.Snapshot;
 
 public interface SnapshotStrategy {
+    enum SnapshotOperation {
+        TAKE,
+        BACKUP,
+        DELETE,
+        REVERT
+    }
+
     SnapshotInfo takeSnapshot(SnapshotInfo snapshot);
 
     SnapshotInfo backupSnapshot(SnapshotInfo snapshot);
@@ -27,5 +34,5 @@ public interface SnapshotStrategy {
 
     boolean revertSnapshot(Long snapshotId);
 
-    StrategyPriority.Priority canHandle(Snapshot snapshot);
+    StrategyPriority.Priority canHandle(Snapshot snapshot, SnapshotOperation op);
 }

http://git-wip-us.apache.org/repos/asf/cloudstack/blob/0ed7ebd7/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriority.java
----------------------------------------------------------------------
diff --git a/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriority.java b/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriority.java
index e930d10..05159dd 100644
--- a/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriority.java
+++ b/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriority.java
@@ -16,11 +16,11 @@
 // under the License.
 package org.apache.cloudstack.engine.subsystem.api.storage;
 
-import java.util.Collections;
-import java.util.Comparator;
 import java.util.List;
 import java.util.Map;
 
+import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotStrategy.SnapshotOperation;
+
 import com.cloud.host.Host;
 import com.cloud.storage.Snapshot;
 
@@ -33,67 +33,49 @@ public class StrategyPriority {
         HIGHEST
     }
 
-    public static void sortStrategies(List<SnapshotStrategy> strategies, Snapshot snapshot) {
-        Collections.sort(strategies, new SnapshotStrategyComparator(snapshot));
-    }
-
-    public static void sortStrategies(List<DataMotionStrategy> strategies, DataObject srcData, DataObject destData) {
-        Collections.sort(strategies, new DataMotionStrategyComparator(srcData, destData));
-    }
-
-    public static void sortStrategies(List<DataMotionStrategy> strategies, Map<VolumeInfo, DataStore> volumeMap, Host srcHost, Host destHost) {
-        Collections.sort(strategies, new DataMotionStrategyHostComparator(volumeMap, srcHost, destHost));
-    }
-
-    static class SnapshotStrategyComparator implements Comparator<SnapshotStrategy> {
+    public static SnapshotStrategy pickStrategy(List<SnapshotStrategy> strategies, Snapshot snapshot, SnapshotOperation op) {
+        Priority highestPriority = Priority.CANT_HANDLE;
+        SnapshotStrategy strategyToUse = null;
 
-        Snapshot snapshot;
-
-        public SnapshotStrategyComparator(Snapshot snapshot) {
-            this.snapshot = snapshot;
+        for (SnapshotStrategy strategy : strategies) {
+            Priority priority = strategy.canHandle(snapshot, op);
+            if (priority.ordinal() > highestPriority.ordinal()) {
+                highestPriority = priority;
+                strategyToUse = strategy;
+            }
         }
 
-        @Override
-        public int compare(SnapshotStrategy o1, SnapshotStrategy o2) {
-            int i1 = o1.canHandle(snapshot).ordinal();
-            int i2 = o2.canHandle(snapshot).ordinal();
-            return Integer.compare(i2, i1);
-        }
+        return strategyToUse;
     }
 
-    static class DataMotionStrategyComparator implements Comparator<DataMotionStrategy> {
-
-        DataObject srcData, destData;
-
-        public DataMotionStrategyComparator(DataObject srcData, DataObject destData) {
-            this.srcData = srcData;
-            this.destData = destData;
+    // TODO DRY this out by consolidating methods
+    public static DataMotionStrategy pickStrategy(List<DataMotionStrategy> strategies, DataObject srcData, DataObject destData) {
+        Priority highestPriority = Priority.CANT_HANDLE;
+        DataMotionStrategy strategyToUse = null;
+
+        for (DataMotionStrategy strategy : strategies) {
+            Priority priority = strategy.canHandle(srcData, destData);
+            if (priority.ordinal() > highestPriority.ordinal()) {
+                highestPriority = priority;
+                strategyToUse = strategy;
+            }
         }
 
-        @Override
-        public int compare(DataMotionStrategy o1, DataMotionStrategy o2) {
-            int i1 = o1.canHandle(srcData, destData).ordinal();
-            int i2 = o2.canHandle(srcData, destData).ordinal();
-            return Integer.compare(i2, i1);
-        }
+        return strategyToUse;
     }
 
-    static class DataMotionStrategyHostComparator implements Comparator<DataMotionStrategy> {
-
-        Host srcHost, destHost;
-        Map<VolumeInfo, DataStore> volumeMap;
+    public static DataMotionStrategy pickStrategy(List<DataMotionStrategy> strategies, Map<VolumeInfo, DataStore> volumeMap, Host srcHost, Host destHost) {
+        Priority highestPriority = Priority.CANT_HANDLE;
+        DataMotionStrategy strategyToUse = null;
 
-        public DataMotionStrategyHostComparator(Map<VolumeInfo, DataStore> volumeMap, Host srcHost, Host destHost) {
-            this.volumeMap = volumeMap;
-            this.srcHost = srcHost;
-            this.destHost = destHost;
+        for (DataMotionStrategy strategy : strategies) {
+            Priority priority = strategy.canHandle(volumeMap, srcHost, destHost);
+            if (priority.ordinal() > highestPriority.ordinal()) {
+                highestPriority = priority;
+                strategyToUse = strategy;
+            }
         }
 
-        @Override
-        public int compare(DataMotionStrategy o1, DataMotionStrategy o2) {
-            int i1 = o1.canHandle(volumeMap, srcHost, destHost).ordinal();
-            int i2 = o2.canHandle(volumeMap, srcHost, destHost).ordinal();
-            return Integer.compare(i2, i1);
-        }
+        return strategyToUse;
     }
 }

http://git-wip-us.apache.org/repos/asf/cloudstack/blob/0ed7ebd7/engine/api/test/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriorityTest.java
----------------------------------------------------------------------
diff --git a/engine/api/test/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriorityTest.java b/engine/api/test/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriorityTest.java
index 3d75279..e18e660 100644
--- a/engine/api/test/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriorityTest.java
+++ b/engine/api/test/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriorityTest.java
@@ -17,10 +17,10 @@
 package org.apache.cloudstack.engine.subsystem.api.storage;
 
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
 
+import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotStrategy.SnapshotOperation;
 import org.apache.cloudstack.engine.subsystem.api.storage.StrategyPriority.Priority;
 import org.junit.Test;
 
@@ -43,22 +43,34 @@ public class StrategyPriorityTest {
         SnapshotStrategy pluginStrategy = mock(SnapshotStrategy.class);
         SnapshotStrategy highestStrategy = mock(SnapshotStrategy.class);
 
-        doReturn(Priority.CANT_HANDLE).when(cantHandleStrategy).canHandle(any(Snapshot.class));
-        doReturn(Priority.DEFAULT).when(defaultStrategy).canHandle(any(Snapshot.class));
-        doReturn(Priority.HYPERVISOR).when(hyperStrategy).canHandle(any(Snapshot.class));
-        doReturn(Priority.PLUGIN).when(pluginStrategy).canHandle(any(Snapshot.class));
-        doReturn(Priority.HIGHEST).when(highestStrategy).canHandle(any(Snapshot.class));
+        doReturn(Priority.CANT_HANDLE).when(cantHandleStrategy).canHandle(any(Snapshot.class), any(SnapshotOperation.class));
+        doReturn(Priority.DEFAULT).when(defaultStrategy).canHandle(any(Snapshot.class), any(SnapshotOperation.class));
+        doReturn(Priority.HYPERVISOR).when(hyperStrategy).canHandle(any(Snapshot.class), any(SnapshotOperation.class));
+        doReturn(Priority.PLUGIN).when(pluginStrategy).canHandle(any(Snapshot.class), any(SnapshotOperation.class));
+        doReturn(Priority.HIGHEST).when(highestStrategy).canHandle(any(Snapshot.class), any(SnapshotOperation.class));
 
         List<SnapshotStrategy> strategies = new ArrayList<SnapshotStrategy>(5);
-        strategies.addAll(Arrays.asList(defaultStrategy, pluginStrategy, hyperStrategy, cantHandleStrategy, highestStrategy));
+        SnapshotStrategy strategy = null;
 
-        StrategyPriority.sortStrategies(strategies, mock(Snapshot.class));
+        strategies.add(cantHandleStrategy);
+        strategy = StrategyPriority.pickStrategy(strategies, mock(Snapshot.class), SnapshotOperation.TAKE);
+        assertEquals("A strategy was found when it shouldn't have been.", null, strategy);
 
-        assertEquals("Highest was not 1st.", highestStrategy, strategies.get(0));
-        assertEquals("Plugin was not 2nd.", pluginStrategy, strategies.get(1));
-        assertEquals("Hypervisor was not 3rd.", hyperStrategy, strategies.get(2));
-        assertEquals("Default was not 4th.", defaultStrategy, strategies.get(3));
-        assertEquals("Can't Handle was not 5th.", cantHandleStrategy, strategies.get(4));
+        strategies.add(defaultStrategy);
+        strategy = StrategyPriority.pickStrategy(strategies, mock(Snapshot.class), SnapshotOperation.TAKE);
+        assertEquals("Default strategy was not picked.", defaultStrategy, strategy);
+
+        strategies.add(hyperStrategy);
+        strategy = StrategyPriority.pickStrategy(strategies, mock(Snapshot.class), SnapshotOperation.TAKE);
+        assertEquals("Hypervisor strategy was not picked.", hyperStrategy, strategy);
+
+        strategies.add(pluginStrategy);
+        strategy = StrategyPriority.pickStrategy(strategies, mock(Snapshot.class), SnapshotOperation.TAKE);
+        assertEquals("Plugin strategy was not picked.", pluginStrategy, strategy);
+
+        strategies.add(highestStrategy);
+        strategy = StrategyPriority.pickStrategy(strategies, mock(Snapshot.class), SnapshotOperation.TAKE);
+        assertEquals("Highest strategy was not picked.", highestStrategy, strategy);
     }
 
     @Test
@@ -76,15 +88,27 @@ public class StrategyPriorityTest {
         doReturn(Priority.HIGHEST).when(highestStrategy).canHandle(any(DataObject.class), any(DataObject.class));
 
         List<DataMotionStrategy> strategies = new ArrayList<DataMotionStrategy>(5);
-        strategies.addAll(Arrays.asList(defaultStrategy, pluginStrategy, hyperStrategy, cantHandleStrategy, highestStrategy));
+        DataMotionStrategy strategy = null;
+
+        strategies.add(cantHandleStrategy);
+        strategy = StrategyPriority.pickStrategy(strategies, mock(DataObject.class), mock(DataObject.class));
+        assertEquals("A strategy was found when it shouldn't have been.", null, strategy);
+
+        strategies.add(defaultStrategy);
+        strategy = StrategyPriority.pickStrategy(strategies, mock(DataObject.class), mock(DataObject.class));
+        assertEquals("Default strategy was not picked.", defaultStrategy, strategy);
 
-        StrategyPriority.sortStrategies(strategies, mock(DataObject.class), mock(DataObject.class));
+        strategies.add(hyperStrategy);
+        strategy = StrategyPriority.pickStrategy(strategies, mock(DataObject.class), mock(DataObject.class));
+        assertEquals("Hypervisor strategy was not picked.", hyperStrategy, strategy);
 
-        assertEquals("Highest was not 1st.", highestStrategy, strategies.get(0));
-        assertEquals("Plugin was not 2nd.", pluginStrategy, strategies.get(1));
-        assertEquals("Hypervisor was not 3rd.", hyperStrategy, strategies.get(2));
-        assertEquals("Default was not 4th.", defaultStrategy, strategies.get(3));
-        assertEquals("Can't Handle was not 5th.", cantHandleStrategy, strategies.get(4));
+        strategies.add(pluginStrategy);
+        strategy = StrategyPriority.pickStrategy(strategies, mock(DataObject.class), mock(DataObject.class));
+        assertEquals("Plugin strategy was not picked.", pluginStrategy, strategy);
+
+        strategies.add(highestStrategy);
+        strategy = StrategyPriority.pickStrategy(strategies, mock(DataObject.class), mock(DataObject.class));
+        assertEquals("Highest strategy was not picked.", highestStrategy, strategy);
     }
 
     @Test
@@ -103,14 +127,26 @@ public class StrategyPriorityTest {
         doReturn(Priority.HIGHEST).when(highestStrategy).canHandle(any(Map.class), any(Host.class), any(Host.class));
 
         List<DataMotionStrategy> strategies = new ArrayList<DataMotionStrategy>(5);
-        strategies.addAll(Arrays.asList(defaultStrategy, pluginStrategy, hyperStrategy, cantHandleStrategy, highestStrategy));
+        DataMotionStrategy strategy = null;
+
+        strategies.add(cantHandleStrategy);
+        strategy = StrategyPriority.pickStrategy(strategies, mock(Map.class), mock(Host.class), mock(Host.class));
+        assertEquals("A strategy was found when it shouldn't have been.", null, strategy);
+
+        strategies.add(defaultStrategy);
+        strategy = StrategyPriority.pickStrategy(strategies, mock(Map.class), mock(Host.class), mock(Host.class));
+        assertEquals("Default strategy was not picked.", defaultStrategy, strategy);
+
+        strategies.add(hyperStrategy);
+        strategy = StrategyPriority.pickStrategy(strategies, mock(Map.class), mock(Host.class), mock(Host.class));
+        assertEquals("Hypervisor strategy was not picked.", hyperStrategy, strategy);
 
-        StrategyPriority.sortStrategies(strategies, mock(Map.class), mock(Host.class), mock(Host.class));
+        strategies.add(pluginStrategy);
+        strategy = StrategyPriority.pickStrategy(strategies, mock(Map.class), mock(Host.class), mock(Host.class));
+        assertEquals("Plugin strategy was not picked.", pluginStrategy, strategy);
 
-        assertEquals("Highest was not 1st.", highestStrategy, strategies.get(0));
-        assertEquals("Plugin was not 2nd.", pluginStrategy, strategies.get(1));
-        assertEquals("Hypervisor was not 3rd.", hyperStrategy, strategies.get(2));
-        assertEquals("Default was not 4th.", defaultStrategy, strategies.get(3));
-        assertEquals("Can't Handle was not 5th.", cantHandleStrategy, strategies.get(4));
+        strategies.add(highestStrategy);
+        strategy = StrategyPriority.pickStrategy(strategies, mock(Map.class), mock(Host.class), mock(Host.class));
+        assertEquals("Highest strategy was not picked.", highestStrategy, strategy);
     }
 }

http://git-wip-us.apache.org/repos/asf/cloudstack/blob/0ed7ebd7/engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java
----------------------------------------------------------------------
diff --git a/engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java b/engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java
index c2242c7..b4cb53f 100755
--- a/engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java
+++ b/engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java
@@ -35,8 +35,6 @@ import javax.ejb.Local;
 import javax.inject.Inject;
 import javax.naming.ConfigurationException;
 
-import org.apache.log4j.Logger;
-
 import org.apache.cloudstack.affinity.dao.AffinityGroupVMMapDao;
 import org.apache.cloudstack.context.CallContext;
 import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService;
@@ -50,8 +48,8 @@ import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
 import org.apache.cloudstack.managed.context.ManagedContextRunnable;
 import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
 import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
-import org.apache.cloudstack.utils.identity.ManagementServerNode;
 import org.apache.cloudstack.storage.to.VolumeObjectTO;
+import org.apache.cloudstack.utils.identity.ManagementServerNode;
 import org.apache.log4j.Logger;
 
 import com.cloud.agent.AgentManager;
@@ -86,7 +84,6 @@ import com.cloud.agent.api.UnPlugNicCommand;
 import com.cloud.agent.api.to.DiskTO;
 import com.cloud.agent.api.to.NicTO;
 import com.cloud.agent.api.to.VirtualMachineTO;
-import com.cloud.agent.api.to.VolumeTO;
 import com.cloud.agent.manager.Commands;
 import com.cloud.agent.manager.allocator.HostAllocator;
 import com.cloud.alert.AlertManager;
@@ -283,20 +280,20 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
 
     static final ConfigKey<Integer> StartRetry = new ConfigKey<Integer>(Integer.class, "start.retry", "Advanced", "10", "Number of times to retry create and start commands", true);
     static final ConfigKey<Integer> VmOpWaitInterval = new ConfigKey<Integer>("Advanced", Integer.class, "vm.op.wait.interval", "120",
-        "Time (in seconds) to wait before checking if a previous operation has succeeded", true);
+            "Time (in seconds) to wait before checking if a previous operation has succeeded", true);
 
     static final ConfigKey<Integer> VmOpLockStateRetry = new ConfigKey<Integer>("Advanced", Integer.class, "vm.op.lock.state.retry", "5",
-        "Times to retry locking the state of a VM for operations, -1 means forever", true);
+            "Times to retry locking the state of a VM for operations, -1 means forever", true);
     static final ConfigKey<Long> VmOpCleanupInterval = new ConfigKey<Long>("Advanced", Long.class, "vm.op.cleanup.interval", "86400",
-        "Interval to run the thread that cleans up the vm operations (in seconds)", false);
+            "Interval to run the thread that cleans up the vm operations (in seconds)", false);
     static final ConfigKey<Long> VmOpCleanupWait = new ConfigKey<Long>("Advanced", Long.class, "vm.op.cleanup.wait", "3600",
-        "Time (in seconds) to wait before cleanuping up any vm work items", true);
+            "Time (in seconds) to wait before cleanuping up any vm work items", true);
     static final ConfigKey<Long> VmOpCancelInterval = new ConfigKey<Long>("Advanced", Long.class, "vm.op.cancel.interval", "3600",
-        "Time (in seconds) to wait before cancelling a operation", false);
+            "Time (in seconds) to wait before cancelling a operation", false);
     static final ConfigKey<Boolean> VmDestroyForcestop = new ConfigKey<Boolean>("Advanced", Boolean.class, "vm.destroy.forcestop", "false",
-        "On destroy, force-stop takes this value ", true);
+            "On destroy, force-stop takes this value ", true);
     static final ConfigKey<Integer> ClusterDeltaSyncInterval = new ConfigKey<Integer>("Advanced", Integer.class, "sync.interval", "60", "Cluster Delta sync interval in seconds",
-        false);
+            false);
 
     ScheduledExecutorService _executor = null;
 
@@ -312,8 +309,8 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
     @Override
     @DB
     public void allocate(String vmInstanceName, VirtualMachineTemplate template, ServiceOffering serviceOffering, Pair<? extends DiskOffering, Long> rootDiskOffering,
-        LinkedHashMap<? extends DiskOffering, Long> dataDiskOfferings, LinkedHashMap<? extends Network, ? extends NicProfile> auxiliaryNetworks, DeploymentPlan plan,
-        HypervisorType hyperType) throws InsufficientCapacityException {
+            LinkedHashMap<? extends DiskOffering, Long> dataDiskOfferings, LinkedHashMap<? extends Network, ? extends NicProfile> auxiliaryNetworks, DeploymentPlan plan,
+            HypervisorType hyperType) throws InsufficientCapacityException {
 
         VMInstanceVO vm = _vmDao.findVMByInstanceName(vmInstanceName);
         Account owner = _entityMgr.findById(Account.class, vm.getAccountId());
@@ -372,7 +369,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
 
     @Override
     public void allocate(String vmInstanceName, VirtualMachineTemplate template, ServiceOffering serviceOffering, LinkedHashMap<? extends Network, ? extends NicProfile> networks,
-        DeploymentPlan plan, HypervisorType hyperType) throws InsufficientCapacityException {
+            DeploymentPlan plan, HypervisorType hyperType) throws InsufficientCapacityException {
         allocate(vmInstanceName, template, serviceOffering, new Pair<DiskOffering, Long>(serviceOffering, null), null, networks, plan, hyperType);
     }
 
@@ -552,7 +549,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
 
     @DB
     protected Ternary<VMInstanceVO, ReservationContext, ItWorkVO> changeToStartState(VirtualMachineGuru vmGuru, VMInstanceVO vm, User caller, Account account)
-        throws ConcurrentOperationException {
+            throws ConcurrentOperationException {
         long vmId = vm.getId();
 
         ItWorkVO work = new ItWorkVO(UUID.randomUUID().toString(), _nodeId, State.Starting, vm.getType(), vm.getId());
@@ -645,13 +642,13 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
 
     @Override
     public void advanceStart(String vmUuid, Map<VirtualMachineProfile.Param, Object> params) throws InsufficientCapacityException, ConcurrentOperationException,
-        ResourceUnavailableException {
+    ResourceUnavailableException {
         advanceStart(vmUuid, params, null);
     }
 
     @Override
     public void advanceStart(String vmUuid, Map<VirtualMachineProfile.Param, Object> params, DeploymentPlan planToDeploy) throws InsufficientCapacityException,
-        ConcurrentOperationException, ResourceUnavailableException {
+    ConcurrentOperationException, ResourceUnavailableException {
         CallContext cctxt = CallContext.current();
         Account account = cctxt.getCallingAccount();
         User caller = cctxt.getCallingUser();
@@ -680,10 +677,10 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
         if (planToDeploy != null && planToDeploy.getDataCenterId() != 0) {
             if (s_logger.isDebugEnabled()) {
                 s_logger.debug("advanceStart: DeploymentPlan is provided, using dcId:" + planToDeploy.getDataCenterId() + ", podId: " + planToDeploy.getPodId() + ", clusterId: " +
-                               planToDeploy.getClusterId() + ", hostId: " + planToDeploy.getHostId() + ", poolId: " + planToDeploy.getPoolId());
+                        planToDeploy.getClusterId() + ", hostId: " + planToDeploy.getHostId() + ", poolId: " + planToDeploy.getPoolId());
             }
             plan = new DataCenterDeployment(planToDeploy.getDataCenterId(), planToDeploy.getPodId(), planToDeploy.getClusterId(), planToDeploy.getHostId(),
-                planToDeploy.getPoolId(), planToDeploy.getPhysicalNetworkId(), ctx);
+                    planToDeploy.getPoolId(), planToDeploy.getPhysicalNetworkId(), ctx);
         }
 
         HypervisorGuru hvGuru = _hvGuruMgr.getGuru(vm.getHypervisorType());
@@ -741,20 +738,20 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
                                         // planner
                                         if (s_logger.isDebugEnabled()) {
                                             s_logger.debug("Cannot satisfy the deployment plan passed in since the ready Root volume is in different cluster. volume's cluster: " +
-                                                           rootVolClusterId + ", cluster specified: " + clusterIdSpecified);
+                                                    rootVolClusterId + ", cluster specified: " + clusterIdSpecified);
                                         }
                                         throw new ResourceUnavailableException(
-                                            "Root volume is ready in different cluster, Deployment plan provided cannot be satisfied, unable to create a deployment for " + vm,
-                                            Cluster.class, clusterIdSpecified);
+                                                "Root volume is ready in different cluster, Deployment plan provided cannot be satisfied, unable to create a deployment for " + vm,
+                                                Cluster.class, clusterIdSpecified);
                                     }
                                 }
                                 plan = new DataCenterDeployment(planToDeploy.getDataCenterId(), planToDeploy.getPodId(), planToDeploy.getClusterId(), planToDeploy.getHostId(),
-                                    vol.getPoolId(), null, ctx);
+                                        vol.getPoolId(), null, ctx);
                             } else {
                                 plan = new DataCenterDeployment(rootVolDcId, rootVolPodId, rootVolClusterId, null, vol.getPoolId(), null, ctx);
                                 if (s_logger.isDebugEnabled()) {
                                     s_logger.debug(vol + " is READY, changing deployment plan to use this pool's dcId: " + rootVolDcId + " , podId: " + rootVolPodId +
-                                                   " , and clusterId: " + rootVolClusterId);
+                                            " , and clusterId: " + rootVolClusterId);
                                 }
                                 planChangedByVolume = true;
                             }
@@ -781,7 +778,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
                         continue;
                     }
                     throw new InsufficientServerCapacityException("Unable to create a deployment for " + vmProfile, DataCenter.class, plan.getDataCenterId(),
-                        areAffinityGroupsAssociated(vmProfile));
+                            areAffinityGroupsAssociated(vmProfile));
                 }
 
                 if (dest != null) {
@@ -796,7 +793,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
                 ClusterDetailsVO cluster_detail_ram = _clusterDetailsDao.findDetail(cluster_id, "memoryOvercommitRatio");
                 //storing the value of overcommit in the vm_details table for doing a capacity check in case the cluster overcommit ratio is changed.
                 if (_uservmDetailsDao.findDetail(vm.getId(), "cpuOvercommitRatio") == null &&
-                    ((Float.parseFloat(cluster_detail_cpu.getValue()) > 1f || Float.parseFloat(cluster_detail_ram.getValue()) > 1f))) {
+                        ((Float.parseFloat(cluster_detail_cpu.getValue()) > 1f || Float.parseFloat(cluster_detail_ram.getValue()) > 1f))) {
                     UserVmDetailVO vmDetail_cpu = new UserVmDetailVO(vm.getId(), "cpuOvercommitRatio", cluster_detail_cpu.getValue());
                     UserVmDetailVO vmDetail_ram = new UserVmDetailVO(vm.getId(), "memoryOvercommitRatio", cluster_detail_ram.getValue());
                     _uservmDetailsDao.persist(vmDetail_cpu);
@@ -867,7 +864,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
                         }
                         if (vmGuru.finalizeStart(vmProfile, destHostId, cmds, ctx)) {
                             syncDiskChainChange(startAnswer);
-                            
+
                             if (!changeState(vm, Event.OperationSucceeded, destHostId, work, Step.Done)) {
                                 throw new ConcurrentOperationException("Unable to transition to a new state.");
                             }
@@ -884,7 +881,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
 
                             StopCommand cmd = new StopCommand(vm, getExecuteInSequence());
                             StopAnswer answer = (StopAnswer)_agentMgr.easySend(destHostId, cmd);
-                           if ( answer != null ) {
+                            if ( answer != null ) {
                                 String hypervisortoolsversion = answer.getHypervisorToolsVersion();
                                 if (hypervisortoolsversion != null) {
                                     if (vm.getType() == VirtualMachine.Type.User) {
@@ -969,17 +966,17 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
             throw new CloudRuntimeException("Unable to start instance '" + vm.getHostName() + "' (" + vm.getUuid() + "), see management server log for details");
         }
     }
-    
+
     private void syncDiskChainChange(StartAnswer answer) {
-    	VirtualMachineTO vmSpec = answer.getVirtualMachine();
-    	
-    	for(DiskTO disk : vmSpec.getDisks()) {
-    		if(disk.getType() != Volume.Type.ISO) {
-	    		VolumeObjectTO vol = (VolumeObjectTO)disk.getData();
-	    		
-	    		volumeMgr.updateVolumeDiskChain(vol.getId(), vol.getPath(), vol.getChainInfo());
-    		}
-    	}
+        VirtualMachineTO vmSpec = answer.getVirtualMachine();
+
+        for(DiskTO disk : vmSpec.getDisks()) {
+            if(disk.getType() != Volume.Type.ISO) {
+                VolumeObjectTO vol = (VolumeObjectTO)disk.getData();
+
+                volumeMgr.updateVolumeDiskChain(vol.getId(), vol.getPath(), vol.getChainInfo());
+            }
+        }
     }
 
     @Override
@@ -1563,8 +1560,8 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
                 _networkMgr.rollbackNicForMigration(vmSrc, profile);
 
                 _alertMgr.sendAlert(alertType, fromHost.getDataCenterId(), fromHost.getPodId(), "Unable to migrate vm " + vm.getInstanceName() + " from host " +
-                                                                                                fromHost.getName() + " in zone " + dest.getDataCenter().getName() + " and pod " +
-                                                                                                dest.getPod().getName(), "Migrate Command failed.  Please check logs.");
+                        fromHost.getName() + " in zone " + dest.getDataCenter().getName() + " and pod " +
+                        dest.getPod().getName(), "Migrate Command failed.  Please check logs.");
                 try {
                     _agentMgr.send(dstHostId, new Commands(cleanup(vm)), null);
                 } catch (AgentUnavailableException ae) {
@@ -1597,8 +1594,8 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
                 if (_poolHostDao.findByPoolHost(pool.getId(), host.getId()) == null || pool.isLocal() != diskOffering.getUseLocalStorage()) {
                     // Cannot find a pool for the volume. Throw an exception.
                     throw new CloudRuntimeException("Cannot migrate volume " + volume + " to storage pool " + pool + " while migrating vm to host " + host +
-                                                    ". Either the pool is not accessible from the " +
-                                                    "host or because of the offering with which the volume is created it cannot be placed on " + "the given pool.");
+                            ". Either the pool is not accessible from the " +
+                            "host or because of the offering with which the volume is created it cannot be placed on " + "the given pool.");
                 } else if (pool.getId() == currentPool.getId()) {
                     // If the pool to migrate too is the same as current pool, remove the volume from the list of
                     // volumes to be migrated.
@@ -1630,7 +1627,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
                 if (!currentPoolAvailable && !volumeToPool.containsKey(volume)) {
                     // Cannot find a pool for the volume. Throw an exception.
                     throw new CloudRuntimeException("Cannot find a storage pool which is available for volume " + volume + " while migrating virtual machine " +
-                                                    profile.getVirtualMachine() + " to host " + host);
+                            profile.getVirtualMachine() + " to host " + host);
                 }
             }
         }
@@ -1666,7 +1663,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
 
     @Override
     public void migrateWithStorage(String vmUuid, long srcHostId, long destHostId, Map<Volume, StoragePool> volumeToPool) throws ResourceUnavailableException,
-        ConcurrentOperationException {
+    ConcurrentOperationException {
         VMInstanceVO vm = _vmDao.findByUuid(vmUuid);
 
         HostVO srcHost = _hostDao.findById(srcHostId);
@@ -1686,7 +1683,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
         // a vm and not migrating a vm with storage.
         if (volumeToPool.isEmpty()) {
             throw new InvalidParameterValueException("Migration of the vm " + vm + "from host " + srcHost + " to destination host " + destHost +
-                                                     " doesn't involve migrating the volumes.");
+                    " doesn't involve migrating the volumes.");
         }
 
         short alertType = AlertManager.ALERT_TYPE_USERVM_MIGRATE;
@@ -1739,8 +1736,8 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
             if (!migrated) {
                 s_logger.info("Migration was unsuccessful.  Cleaning up: " + vm);
                 _alertMgr.sendAlert(alertType, srcHost.getDataCenterId(), srcHost.getPodId(), "Unable to migrate vm " + vm.getInstanceName() + " from host " + srcHost.getName() +
-                                                                                              " in zone " + dc.getName() + " and pod " + dc.getName(),
-                    "Migrate Command failed.  Please check logs.");
+                        " in zone " + dc.getName() + " and pod " + dc.getName(),
+                        "Migrate Command failed.  Please check logs.");
                 try {
                     _agentMgr.send(destHostId, new Commands(cleanup(vm.getInstanceName())), null);
                     stateTransitTo(vm, Event.OperationFailed, srcHostId);
@@ -1908,7 +1905,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
 
     @Override
     public void advanceReboot(String vmUuid, Map<VirtualMachineProfile.Param, Object> params) throws InsufficientCapacityException, ConcurrentOperationException,
-        ResourceUnavailableException {
+    ResourceUnavailableException {
         VMInstanceVO vm = _vmDao.findByUuid(vmUuid);
 
         DataCenter dc = _entityMgr.findById(DataCenter.class, vm.getDataCenterId());
@@ -1957,7 +1954,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
 
             // sync VM Snapshots related transient states
             List<VMSnapshotVO> vmSnapshotsInTrasientStates = _vmSnapshotDao.listByInstanceId(vm.getId(), VMSnapshot.State.Expunging, VMSnapshot.State.Reverting,
-                VMSnapshot.State.Creating);
+                    VMSnapshot.State.Creating);
             if (vmSnapshotsInTrasientStates.size() > 1) {
                 s_logger.info("Found vm " + vm.getInstanceName() + " with VM snapshots in transient states, needs to sync VM snapshot state");
                 if (!_vmSnapshotMgr.syncVMSnapshot(vm, hostId)) {
@@ -2071,7 +2068,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
 
     public void fullSync(final long clusterId, Map<String, Ternary<String, State, String>> newStates) {
         if (newStates==null)
-        	return;
+            return;
         Map<Long, AgentVmInfo> infos = convertToInfos(newStates);
         Set<VMInstanceVO> set_vms = Collections.synchronizedSet(new HashSet<VMInstanceVO>());
         set_vms.addAll(_vmDao.listByClusterId(clusterId));
@@ -2082,7 +2079,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
 
             // sync VM Snapshots related transient states
             List<VMSnapshotVO> vmSnapshotsInExpungingStates = _vmSnapshotDao.listByInstanceId(vm.getId(), VMSnapshot.State.Expunging, VMSnapshot.State.Creating,
-                VMSnapshot.State.Reverting);
+                    VMSnapshot.State.Reverting);
             if (vmSnapshotsInExpungingStates.size() > 0) {
                 s_logger.info("Found vm " + vm.getInstanceName() + " in state. " + vm.getState() + ", needs to sync VM snapshot state");
                 Long hostId = null;
@@ -2100,9 +2097,9 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
             }
 
             if ((info == null && (vm.getState() == State.Running || vm.getState() == State.Starting)) ||
-                (info != null && (info.state == State.Running && vm.getState() == State.Starting))) {
+                    (info != null && (info.state == State.Running && vm.getState() == State.Starting))) {
                 s_logger.info("Found vm " + vm.getInstanceName() + " in inconsistent state. " + vm.getState() + " on CS while " + (info == null ? "Stopped" : "Running") +
-                              " on agent");
+                        " on agent");
                 info = new AgentVmInfo(vm.getInstanceName(), vm, State.Stopped);
 
                 // Bug 13850- grab outstanding work item if any for this VM state so that we mark it as DONE after we change VM state, else it will remain pending
@@ -2139,7 +2136,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
                     e.printStackTrace();
                 }
             } else if (info != null &&
-                       (vm.getState() == State.Stopped || vm.getState() == State.Stopping || vm.isRemoved() || vm.getState() == State.Destroyed || vm.getState() == State.Expunging)) {
+                    (vm.getState() == State.Stopped || vm.getState() == State.Stopping || vm.isRemoved() || vm.getState() == State.Destroyed || vm.getState() == State.Expunging)) {
                 Host host = _hostDao.findByGuid(info.getHostUuid());
                 if (host != null) {
                     s_logger.warn("Stopping a VM which is stopped/stopping/destroyed/expunging " + info.name);
@@ -2158,19 +2155,19 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
                     }
                 }
             } else
-            // host id can change
-            if (info != null && vm.getState() == State.Running) {
-                // check for host id changes
-                Host host = _hostDao.findByGuid(info.getHostUuid());
-                if (host != null && (vm.getHostId() == null || host.getId() != vm.getHostId())) {
-                    s_logger.info("Found vm " + vm.getInstanceName() + " with inconsistent host in db, new host is " + host.getId());
-                    try {
-                        stateTransitTo(vm, VirtualMachine.Event.AgentReportMigrated, host.getId());
-                    } catch (NoTransitionException e) {
-                        s_logger.warn(e.getMessage());
+                // host id can change
+                if (info != null && vm.getState() == State.Running) {
+                    // check for host id changes
+                    Host host = _hostDao.findByGuid(info.getHostUuid());
+                    if (host != null && (vm.getHostId() == null || host.getId() != vm.getHostId())) {
+                        s_logger.info("Found vm " + vm.getInstanceName() + " with inconsistent host in db, new host is " + host.getId());
+                        try {
+                            stateTransitTo(vm, VirtualMachine.Event.AgentReportMigrated, host.getId());
+                        } catch (NoTransitionException e) {
+                            s_logger.warn(e.getMessage());
+                        }
                     }
                 }
-            }
             /* else if(info == null && vm.getState() == State.Stopping) { //Handling CS-13376
                         s_logger.warn("Marking the VM as Stopped as it was still stopping on the CS" +vm.getName());
                         vm.setState(State.Stopped); // Setting the VM as stopped on the DB and clearing it from the host
@@ -2213,13 +2210,13 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
             VMInstanceVO vm = _vmDao.findVMByInstanceName(name);
             if (vm != null) {
                 map.put(vm.getId(), new AgentVmInfo(entry.getKey(), vm, entry.getValue().second(),
-                		entry.getValue().first(), entry.getValue().third()));
+                        entry.getValue().first(), entry.getValue().third()));
                 is_alien_vm = false;
             }
             // alien VMs
             if (is_alien_vm) {
-                map.put(alien_vm_count--, new AgentVmInfo(entry.getKey(), null, entry.getValue().second(), 
-                		entry.getValue().first(), entry.getValue().third()));
+                map.put(alien_vm_count--, new AgentVmInfo(entry.getKey(), null, entry.getValue().second(),
+                        entry.getValue().first(), entry.getValue().third()));
                 s_logger.warn("Found an alien VM " + entry.getKey());
             }
         }
@@ -2297,8 +2294,17 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
 
             String hostDesc = "name: " + hostVO.getName() + " (id:" + hostVO.getId() + "), availability zone: " + dcVO.getName() + ", pod: " + podVO.getName();
             _alertMgr.sendAlert(alertType, vm.getDataCenterId(), vm.getPodIdToDeployIn(), "VM (name: " + vm.getInstanceName() + ", id: " + vm.getId() + ") stopped on host " +
-                                                                                          hostDesc + " due to storage failure",
-                "Virtual Machine " + vm.getInstanceName() + " (id: " + vm.getId() + ") running on host [" + vm.getHostId() + "] stopped due to storage failure.");
+                    hostDesc + " due to storage failure",
+                    "Virtual Machine " + vm.getInstanceName() + " (id: " + vm.getId() + ") running on host [" + vm.getHostId() + "] stopped due to storage failure.");
+        }
+        // track hypervsion tools version
+        if( info.hvtoolsversion != null && !info.hvtoolsversion.isEmpty() ) {
+            if (vm.getType() == VirtualMachine.Type.User) {
+                UserVmVO userVm = _userVmDao.findById(vm.getId());
+                _userVmDao.loadDetails(userVm);
+                userVm.setDetail("hypervisortoolsversion",  info.hvtoolsversion);
+                _userVmDao.saveDetails(userVm);
+            }
         }
         // track hypervsion tools version
         if( info.hvtoolsversion != null && !info.hvtoolsversion.isEmpty() ) {
@@ -2314,7 +2320,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
             if (serverState == State.Starting) {
                 if (vm.getHostId() != null && vm.getHostId() != hostId) {
                     s_logger.info("CloudStack is starting VM on host " + vm.getHostId() + ", but status report comes from a different host " + hostId +
-                                  ", skip status sync for vm: " + vm.getInstanceName());
+                            ", skip status sync for vm: " + vm.getInstanceName());
                     return null;
                 }
             }
@@ -2339,7 +2345,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
             if (serverState == State.Starting) {
                 if (vm.getHostId() != null && vm.getHostId() != hostId) {
                     s_logger.info("CloudStack is starting VM on host " + vm.getHostId() + ", but status report comes from a different host " + hostId +
-                                  ", skip status sync for vm: " + vm.getInstanceName());
+                            ", skip status sync for vm: " + vm.getInstanceName());
                     return null;
                 }
             }
@@ -2355,7 +2361,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
                     if (vm.getHostId() == null || hostId != vm.getHostId()) {
                         if (s_logger.isDebugEnabled()) {
                             s_logger.debug("detected host change when VM " + vm + " is at running state, VM could be live-migrated externally from host " + vm.getHostId() +
-                                           " to host " + hostId);
+                                    " to host " + hostId);
                         }
 
                         stateTransitTo(vm, VirtualMachine.Event.AgentReportMigrated, hostId);
@@ -2453,7 +2459,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
     }
 
     private void ensureVmRunningContext(long hostId, VMInstanceVO vm, Event cause) throws OperationTimedoutException, ResourceUnavailableException, NoTransitionException,
-        InsufficientAddressCapacityException {
+    InsufficientAddressCapacityException {
         VirtualMachineGuru vmGuru = getVmGuru(vm);
 
         s_logger.debug("VM state is starting on full sync so updating it to running");
@@ -2482,7 +2488,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
         for (NicVO nic : nics) {
             Network network = _networkModel.getNetwork(nic.getNetworkId());
             NicProfile nicProfile = new NicProfile(nic, network, nic.getBroadcastUri(), nic.getIsolationUri(), null, _networkModel.isSecurityGroupSupportedInNetwork(network),
-                _networkModel.getNetworkTag(profile.getHypervisorType(), network));
+                    _networkModel.getNetworkTag(profile.getHypervisorType(), network));
             profile.addNic(nicProfile);
         }
 
@@ -2685,13 +2691,13 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
             this.vm = vm;
             this.hostUuid = host;
             this.hvtoolsversion= hvtoolsversion;
-            
+
         }
-        
+
         public AgentVmInfo(String name, VMInstanceVO vm, State state, String host) {
-        	this(name, vm, state, host, null);           
+            this(name, vm, state, host, null);
         }
-        
+
         public AgentVmInfo(String name, VMInstanceVO vm, State state) {
             this(name, vm, state, null, null);
         }
@@ -2699,7 +2705,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
         public String getHostUuid() {
             return hostUuid;
         }
-        
+
         public String getHvtoolsversion() {
             return hvtoolsversion;
         }
@@ -2721,7 +2727,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
         if (!(vmInstance.getState().equals(State.Stopped) || vmInstance.getState().equals(State.Running))) {
             s_logger.warn("Unable to upgrade virtual machine " + vmInstance.toString() + " in state " + vmInstance.getState());
             throw new InvalidParameterValueException("Unable to upgrade virtual machine " + vmInstance.toString() + " " + " in state " + vmInstance.getState() +
-                                                     "; make sure the virtual machine is stopped/running");
+                    "; make sure the virtual machine is stopped/running");
         }
 
         // Check if the service offering being upgraded to is what the VM is already running with
@@ -2731,7 +2737,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
             }
 
             throw new InvalidParameterValueException("Not upgrading vm " + vmInstance.toString() + " since it already " + "has the requested service offering (" +
-                                                     newServiceOffering.getName() + ")");
+                    newServiceOffering.getName() + ")");
         }
 
         ServiceOfferingVO currentServiceOffering = _offeringDao.findByIdIncludingRemoved(vmInstance.getServiceOfferingId());
@@ -2749,8 +2755,8 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
         // offering
         if (currentServiceOffering.getUseLocalStorage() != newServiceOffering.getUseLocalStorage()) {
             throw new InvalidParameterValueException("Unable to upgrade virtual machine " + vmInstance.toString() +
-                                                     ", cannot switch between local storage and shared storage service offerings.  Current offering " + "useLocalStorage=" +
-                                                     currentServiceOffering.getUseLocalStorage() + ", target offering useLocalStorage=" + newServiceOffering.getUseLocalStorage());
+                    ", cannot switch between local storage and shared storage service offerings.  Current offering " + "useLocalStorage=" +
+                    currentServiceOffering.getUseLocalStorage() + ", target offering useLocalStorage=" + newServiceOffering.getUseLocalStorage());
         }
 
         // if vm is a system vm, check if it is a system service offering, if yes return with error as it cannot be used for user vms
@@ -2761,7 +2767,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
         // Check that there are enough resources to upgrade the service offering
         if (!isVirtualMachineUpgradable(vmInstance, newServiceOffering)) {
             throw new InvalidParameterValueException("Unable to upgrade virtual machine, not enough resources available " + "for an offering of " + newServiceOffering.getCpu() +
-                                                     " cpu(s) at " + newServiceOffering.getSpeed() + " Mhz, and " + newServiceOffering.getRamSize() + " MB of memory");
+                    " cpu(s) at " + newServiceOffering.getSpeed() + " Mhz, and " + newServiceOffering.getRamSize() + " MB of memory");
         }
 
         // Check that the service offering being upgraded to has all the tags of the current service offering
@@ -2769,8 +2775,8 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
         List<String> newTags = StringUtils.csvTagsToList(newServiceOffering.getTags());
         if (!newTags.containsAll(currentTags)) {
             throw new InvalidParameterValueException("Unable to upgrade virtual machine; the new service offering " + "does not have all the tags of the " +
-                                                     "current service offering. Current service offering tags: " + currentTags + "; " + "new service " + "offering tags: " +
-                                                     newTags);
+                    "current service offering. Current service offering tags: " + currentTags + "; " + "new service " + "offering tags: " +
+                    newTags);
         }
     }
 
@@ -2787,7 +2793,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
 
     @Override
     public NicProfile addVmToNetwork(VirtualMachine vm, Network network, NicProfile requested) throws ConcurrentOperationException, ResourceUnavailableException,
-        InsufficientCapacityException {
+    InsufficientCapacityException {
         CallContext cctx = CallContext.current();
 
         s_logger.debug("Adding vm " + vm + " to network " + network + "; requested nic profile " + requested);
@@ -2823,7 +2829,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
                     long isDefault = (nic.isDefaultNic()) ? 1 : 0;
                     // insert nic's Id into DB as resource_name
                     UsageEventUtils.publishUsageEvent(EventTypes.EVENT_NETWORK_OFFERING_ASSIGN, vmVO.getAccountId(), vmVO.getDataCenterId(), vmVO.getId(),
-                        Long.toString(nic.getId()), network.getNetworkOfferingId(), null, isDefault, VirtualMachine.class.getName(), vmVO.getUuid());
+                            Long.toString(nic.getId()), network.getNetworkOfferingId(), null, isDefault, VirtualMachine.class.getName(), vmVO.getUuid());
                     return nic;
                 } else {
                     s_logger.warn("Failed to plug nic to the vm " + vm + " in network " + network);
@@ -2879,7 +2885,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
         }
 
         NicProfile nicProfile = new NicProfile(nic, network, nic.getBroadcastUri(), nic.getIsolationUri(), _networkModel.getNetworkRate(network.getId(), vm.getId()),
-            _networkModel.isSecurityGroupSupportedInNetwork(network), _networkModel.getNetworkTag(vmProfile.getVirtualMachine().getHypervisorType(), network));
+                _networkModel.isSecurityGroupSupportedInNetwork(network), _networkModel.getNetworkTag(vmProfile.getVirtualMachine().getHypervisorType(), network));
 
         //1) Unplug the nic
         if (vm.getState() == State.Running) {
@@ -2890,7 +2896,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
                 s_logger.debug("Nic is unplugged successfully for vm " + vm + " in network " + network);
                 long isDefault = (nic.isDefaultNic()) ? 1 : 0;
                 UsageEventUtils.publishUsageEvent(EventTypes.EVENT_NETWORK_OFFERING_REMOVE, vm.getAccountId(), vm.getDataCenterId(), vm.getId(), Long.toString(nic.getId()),
-                    network.getNetworkOfferingId(), null, isDefault, VirtualMachine.class.getName(), vm.getUuid());
+                        network.getNetworkOfferingId(), null, isDefault, VirtualMachine.class.getName(), vm.getUuid());
             } else {
                 s_logger.warn("Failed to unplug nic for the vm " + vm + " from network " + network);
                 return false;
@@ -2962,7 +2968,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
 
         try {
             NicProfile nicProfile = new NicProfile(nic, network, nic.getBroadcastUri(), nic.getIsolationUri(), _networkModel.getNetworkRate(network.getId(), vm.getId()),
-                _networkModel.isSecurityGroupSupportedInNetwork(network), _networkModel.getNetworkTag(vmProfile.getVirtualMachine().getHypervisorType(), network));
+                    _networkModel.isSecurityGroupSupportedInNetwork(network), _networkModel.getNetworkTag(vmProfile.getVirtualMachine().getHypervisorType(), network));
 
             //1) Unplug the nic
             if (vm.getState() == State.Running) {
@@ -2999,7 +3005,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
 
     @Override
     public void findHostAndMigrate(String vmUuid, Long newSvcOfferingId, ExcludeList excludes) throws InsufficientCapacityException, ConcurrentOperationException,
-        ResourceUnavailableException {
+    ResourceUnavailableException {
 
         VMInstanceVO vm = _vmDao.findByUuid(vmUuid);
         if (vm == null) {
@@ -3184,8 +3190,8 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
                 s_logger.info("Migration was unsuccessful.  Cleaning up: " + vm);
 
                 _alertMgr.sendAlert(alertType, fromHost.getDataCenterId(), fromHost.getPodId(), "Unable to migrate vm " + vm.getInstanceName() + " from host " +
-                                                                                                fromHost.getName() + " in zone " + dest.getDataCenter().getName() + " and pod " +
-                                                                                                dest.getPod().getName(), "Migrate Command failed.  Please check logs.");
+                        fromHost.getName() + " in zone " + dest.getDataCenter().getName() + " and pod " +
+                        dest.getPod().getName(), "Migrate Command failed.  Please check logs.");
                 try {
                     _agentMgr.send(dstHostId, new Commands(cleanup(vm.getInstanceName())), null);
                 } catch (AgentUnavailableException ae) {
@@ -3205,7 +3211,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
     }
 
     public boolean plugNic(Network network, NicTO nic, VirtualMachineTO vm, ReservationContext context, DeployDestination dest) throws ConcurrentOperationException,
-        ResourceUnavailableException, InsufficientCapacityException {
+    ResourceUnavailableException, InsufficientCapacityException {
         boolean result = true;
 
         VMInstanceVO router = _vmDao.findById(vm.getId());
@@ -3228,14 +3234,14 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
             s_logger.warn("Unable to apply PlugNic, vm " + router + " is not in the right state " + router.getState());
 
             throw new ResourceUnavailableException("Unable to apply PlugNic on the backend," + " vm " + vm + " is not in the right state", DataCenter.class,
-                router.getDataCenterId());
+                    router.getDataCenterId());
         }
 
         return result;
     }
 
     public boolean unplugNic(Network network, NicTO nic, VirtualMachineTO vm, ReservationContext context, DeployDestination dest) throws ConcurrentOperationException,
-        ResourceUnavailableException {
+    ResourceUnavailableException {
 
         boolean result = true;
         VMInstanceVO router = _vmDao.findById(vm.getId());
@@ -3261,7 +3267,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
             s_logger.warn("Unable to apply unplug nic, Vm " + router + " is not in the right state " + router.getState());
 
             throw new ResourceUnavailableException("Unable to apply unplug nic on the backend," + " vm " + router + " is not in the right state", DataCenter.class,
-                router.getDataCenterId());
+                    router.getDataCenterId());
         }
 
         return result;
@@ -3269,7 +3275,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
 
     @Override
     public VMInstanceVO reConfigureVm(String vmUuid, ServiceOffering oldServiceOffering, boolean reconfiguringOnExistingHost) throws ResourceUnavailableException,
-        ConcurrentOperationException {
+    ConcurrentOperationException {
         VMInstanceVO vm = _vmDao.findByUuid(vmUuid);
 
         long newServiceofferingId = vm.getServiceOfferingId();
@@ -3280,7 +3286,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
         Float cpuOvercommitRatio = CapacityManager.CpuOverprovisioningFactor.valueIn(hostVo.getClusterId());
         long minMemory = (long)(newServiceOffering.getRamSize() / memoryOvercommitRatio);
         ScaleVmCommand reconfigureCmd = new ScaleVmCommand(vm.getInstanceName(), newServiceOffering.getCpu(), (int)(newServiceOffering.getSpeed() / cpuOvercommitRatio),
-            newServiceOffering.getSpeed(), minMemory * 1024L * 1024L, newServiceOffering.getRamSize() * 1024L * 1024L, newServiceOffering.getLimitCpuUse());
+                newServiceOffering.getSpeed(), minMemory * 1024L * 1024L, newServiceOffering.getRamSize() * 1024L * 1024L, newServiceOffering.getLimitCpuUse());
 
         Long dstHostId = vm.getHostId();
         ItWorkVO work = new ItWorkVO(UUID.randomUUID().toString(), _nodeId, State.Running, vm.getType(), vm.getId());

http://git-wip-us.apache.org/repos/asf/cloudstack/blob/0ed7ebd7/engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java
----------------------------------------------------------------------
diff --git a/engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java b/engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java
index 2d31320..213b47a 100644
--- a/engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java
+++ b/engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java
@@ -18,6 +18,7 @@
  */
 package org.apache.cloudstack.storage.motion;
 
+import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 
@@ -29,13 +30,13 @@ import org.apache.cloudstack.engine.subsystem.api.storage.DataMotionStrategy;
 import org.apache.cloudstack.engine.subsystem.api.storage.DataObject;
 import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
 import org.apache.cloudstack.engine.subsystem.api.storage.StrategyPriority;
-import org.apache.cloudstack.engine.subsystem.api.storage.StrategyPriority.Priority;
 import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
 import org.apache.cloudstack.framework.async.AsyncCompletionCallback;
 import org.springframework.stereotype.Component;
 
 import com.cloud.agent.api.to.VirtualMachineTO;
 import com.cloud.host.Host;
+import com.cloud.utils.StringUtils;
 import com.cloud.utils.exception.CloudRuntimeException;
 
 @Component
@@ -57,30 +58,35 @@ public class DataMotionServiceImpl implements DataMotionService {
             return;
         }
 
-        StrategyPriority.sortStrategies(strategies, srcData, destData);
-
-        for (DataMotionStrategy strategy : strategies) {
-            if (strategy.canHandle(srcData, destData) != Priority.CANT_HANDLE) {
-                strategy.copyAsync(srcData, destData, callback);
-                return;
-            }
+        // TODO DRY this out when the overloaded methods are DRYed out
+        DataMotionStrategy strategy = StrategyPriority.pickStrategy(strategies, srcData, destData);
+        if (strategy == null) {
+            throw new CloudRuntimeException("Can't find strategy to move data. "+
+                    "Source: "+srcData.getType().name()+" '"+srcData.getUuid()+
+                    ", Destination: "+destData.getType().name()+" '"+destData.getUuid()+"'");
         }
-        throw new CloudRuntimeException("can't find strategy to move data");
+
+        strategy.copyAsync(srcData, destData, callback);
     }
 
     @Override
     public void copyAsync(Map<VolumeInfo, DataStore> volumeMap, VirtualMachineTO vmTo, Host srcHost, Host destHost,
             AsyncCompletionCallback<CopyCommandResult> callback) {
 
-        StrategyPriority.sortStrategies(strategies, volumeMap, srcHost, destHost);
-
-        for (DataMotionStrategy strategy : strategies) {
-            if (strategy.canHandle(volumeMap, srcHost, destHost) != Priority.CANT_HANDLE) {
-                strategy.copyAsync(volumeMap, vmTo, srcHost, destHost, callback);
-                return;
+        // TODO DRY this out when the overloaded methods are DRYed out
+        DataMotionStrategy strategy = StrategyPriority.pickStrategy(strategies, volumeMap, srcHost, destHost);
+        if (strategy == null) {
+            List<String> volumeIds = new LinkedList<String>();
+            for (final VolumeInfo volumeInfo : volumeMap.keySet()) {
+                volumeIds.add(volumeInfo.getUuid());
             }
+
+            throw new CloudRuntimeException("Can't find strategy to move data. "+
+                    "Source Host: "+srcHost.getName()+", Destination Host: "+destHost.getName()+
+                    ", Volume UUIDs: "+StringUtils.join(volumeIds, ","));
         }
-        throw new CloudRuntimeException("can't find strategy to move data");
+
+        strategy.copyAsync(volumeMap, vmTo, srcHost, destHost, callback);
     }
 
     public void setStrategies(List<DataMotionStrategy> strategies) {

http://git-wip-us.apache.org/repos/asf/cloudstack/blob/0ed7ebd7/engine/storage/integration-test/test/org/apache/cloudstack/storage/test/FakePrimaryDataStoreDriver.java
----------------------------------------------------------------------
diff --git a/engine/storage/integration-test/test/org/apache/cloudstack/storage/test/FakePrimaryDataStoreDriver.java b/engine/storage/integration-test/test/org/apache/cloudstack/storage/test/FakePrimaryDataStoreDriver.java
index 810afd1..de64b8f 100644
--- a/engine/storage/integration-test/test/org/apache/cloudstack/storage/test/FakePrimaryDataStoreDriver.java
+++ b/engine/storage/integration-test/test/org/apache/cloudstack/storage/test/FakePrimaryDataStoreDriver.java
@@ -18,8 +18,8 @@
  */
 package org.apache.cloudstack.storage.test;
 
-import com.cloud.agent.api.to.DataStoreTO;
-import com.cloud.agent.api.to.DataTO;
+import java.util.UUID;
+
 import org.apache.cloudstack.engine.subsystem.api.storage.ChapInfo;
 import org.apache.cloudstack.engine.subsystem.api.storage.CopyCommandResult;
 import org.apache.cloudstack.engine.subsystem.api.storage.CreateCmdResult;
@@ -33,7 +33,8 @@ import org.apache.cloudstack.storage.command.CommandResult;
 import org.apache.cloudstack.storage.command.CreateObjectAnswer;
 import org.apache.cloudstack.storage.to.SnapshotObjectTO;
 
-import java.util.UUID;
+import com.cloud.agent.api.to.DataStoreTO;
+import com.cloud.agent.api.to.DataTO;
 
 public class FakePrimaryDataStoreDriver implements PrimaryDataStoreDriver {
     boolean snapshotResult = true;

http://git-wip-us.apache.org/repos/asf/cloudstack/blob/0ed7ebd7/engine/storage/integration-test/test/org/apache/cloudstack/storage/test/SnapshotTest.java
----------------------------------------------------------------------
diff --git a/engine/storage/integration-test/test/org/apache/cloudstack/storage/test/SnapshotTest.java b/engine/storage/integration-test/test/org/apache/cloudstack/storage/test/SnapshotTest.java
index 81f77d6..a46ebd8 100644
--- a/engine/storage/integration-test/test/org/apache/cloudstack/storage/test/SnapshotTest.java
+++ b/engine/storage/integration-test/test/org/apache/cloudstack/storage/test/SnapshotTest.java
@@ -39,13 +39,16 @@ import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory;
 import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
 import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotService;
 import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotStrategy;
+import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotStrategy.SnapshotOperation;
+import org.apache.cloudstack.engine.subsystem.api.storage.StrategyPriority;
+import org.apache.cloudstack.engine.subsystem.api.storage.StrategyPriority.Priority;
 import org.apache.cloudstack.engine.subsystem.api.storage.TemplateDataFactory;
 import org.apache.cloudstack.engine.subsystem.api.storage.TemplateInfo;
 import org.apache.cloudstack.engine.subsystem.api.storage.TemplateService;
+import org.apache.cloudstack.engine.subsystem.api.storage.TemplateService.TemplateApiResult;
 import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
 import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
 import org.apache.cloudstack.engine.subsystem.api.storage.VolumeService;
-import org.apache.cloudstack.engine.subsystem.api.storage.TemplateService.TemplateApiResult;
 import org.apache.cloudstack.engine.subsystem.api.storage.VolumeService.VolumeApiResult;
 import org.apache.cloudstack.framework.async.AsyncCallFuture;
 import org.apache.cloudstack.storage.LocalHostEndpoint;
@@ -281,7 +284,7 @@ public class SnapshotTest extends CloudStackTestNGBase {
         Mockito.when(epSelector.select(Matchers.any(DataObject.class))).thenReturn(remoteEp);
         Mockito.when(epSelector.select(Matchers.any(DataStore.class))).thenReturn(remoteEp);
         Mockito.when(hyGuruMgr.getGuruProcessedCommandTargetHost(Matchers.anyLong(), Matchers.any(Command.class)))
-                .thenReturn(this.host.getId());
+        .thenReturn(this.host.getId());
 
     }
 
@@ -367,10 +370,10 @@ public class SnapshotTest extends CloudStackTestNGBase {
         result = future.get();
         Assert.assertTrue(result.isSuccess());
         return result.getVolume();
-        
+
     }
 
-   
+
 
     private VMTemplateVO createTemplateInDb() {
         VMTemplateVO image = new VMTemplateVO();
@@ -401,13 +404,10 @@ public class SnapshotTest extends CloudStackTestNGBase {
         SnapshotInfo snapshot = this.snapshotFactory.getSnapshot(snapshotVO.getId(), vol.getDataStore());
         boolean result = false;
 
-        StrategyPriority.sortStrategies(snapshotStrategies, snapshot);
-
-        for (SnapshotStrategy strategy : this.snapshotStrategies) {
-            if (strategy.canHandle(snapshot) != Priority.CANT_HANDLE) {
-                snapshot = strategy.takeSnapshot(snapshot);
-                result = true;
-            }
+        SnapshotStrategy snapshotStrategy = StrategyPriority.pickStrategy(snapshotStrategies, snapshot, SnapshotOperation.TAKE);
+        if (snapshotStrategy != null) {
+            snapshot = snapshotStrategy.takeSnapshot(snapshot);
+            result = true;
         }
 
         AssertJUnit.assertTrue(result);
@@ -426,18 +426,15 @@ public class SnapshotTest extends CloudStackTestNGBase {
         SnapshotInfo snapshot = this.snapshotFactory.getSnapshot(snapshotVO.getId(), vol.getDataStore());
         SnapshotInfo newSnapshot = null;
 
-        StrategyPriority.sortStrategies(snapshotStrategies, newSnapshot);
-
-        for (SnapshotStrategy strategy : this.snapshotStrategies) {
-            if (strategy.canHandle(snapshot) != Priority.CANT_HANDLE) {
-                newSnapshot = strategy.takeSnapshot(snapshot);
-            }
+        SnapshotStrategy snapshotStrategy = StrategyPriority.pickStrategy(snapshotStrategies, snapshot, SnapshotOperation.TAKE);
+        if (snapshotStrategy != null) {
+            newSnapshot = snapshotStrategy.takeSnapshot(snapshot);
         }
         AssertJUnit.assertNotNull(newSnapshot);
 
         // create another snapshot
         for (SnapshotStrategy strategy : this.snapshotStrategies) {
-            if (strategy.canHandle(snapshot) != Priority.CANT_HANDLE) {
+            if (strategy.canHandle(snapshot, SnapshotOperation.DELETE) != Priority.CANT_HANDLE) {
                 strategy.deleteSnapshot(newSnapshot.getId());
             }
         }
@@ -451,13 +448,10 @@ public class SnapshotTest extends CloudStackTestNGBase {
         SnapshotInfo snapshot = this.snapshotFactory.getSnapshot(snapshotVO.getId(), vol.getDataStore());
         boolean result = false;
 
-        StrategyPriority.sortStrategies(snapshotStrategies, snapshot);
-
-        for (SnapshotStrategy strategy : this.snapshotStrategies) {
-            if (strategy.canHandle(snapshot) != Priority.CANT_HANDLE) {
-                snapshot = strategy.takeSnapshot(snapshot);
-                result = true;
-            }
+        SnapshotStrategy snapshotStrategy = StrategyPriority.pickStrategy(snapshotStrategies, snapshot, SnapshotOperation.TAKE);
+        if (snapshotStrategy != null) {
+            snapshot = snapshotStrategy.takeSnapshot(snapshot);
+            result = true;
         }
 
         AssertJUnit.assertTrue(result);
@@ -484,13 +478,11 @@ public class SnapshotTest extends CloudStackTestNGBase {
         SnapshotInfo snapshot = this.snapshotFactory.getSnapshot(snapshotVO.getId(), vol.getDataStore());
         SnapshotInfo newSnapshot = null;
 
-        StrategyPriority.sortStrategies(snapshotStrategies, newSnapshot);
-
-        for (SnapshotStrategy strategy : this.snapshotStrategies) {
-            if (strategy.canHandle(snapshot) != Priority.CANT_HANDLE) {
-                newSnapshot = strategy.takeSnapshot(snapshot);
-            }
+        SnapshotStrategy snapshotStrategy = StrategyPriority.pickStrategy(snapshotStrategies, snapshot, SnapshotOperation.TAKE);
+        if (snapshotStrategy != null) {
+            newSnapshot = snapshotStrategy.takeSnapshot(snapshot);
         }
+
         AssertJUnit.assertNotNull(newSnapshot);
 
         LocalHostEndpoint ep = new MockLocalHostEndPoint();
@@ -499,7 +491,7 @@ public class SnapshotTest extends CloudStackTestNGBase {
 
         try {
             for (SnapshotStrategy strategy : this.snapshotStrategies) {
-                if (strategy.canHandle(snapshot) != Priority.CANT_HANDLE) {
+                if (strategy.canHandle(snapshot, SnapshotOperation.DELETE) != Priority.CANT_HANDLE) {
                     boolean res = strategy.deleteSnapshot(newSnapshot.getId());
                     Assert.assertTrue(res);
                 }

http://git-wip-us.apache.org/repos/asf/cloudstack/blob/0ed7ebd7/engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotObject.java
----------------------------------------------------------------------
diff --git a/engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotObject.java b/engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotObject.java
index 3f35e1d..147f1d4 100644
--- a/engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotObject.java
+++ b/engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotObject.java
@@ -19,16 +19,18 @@
 package org.apache.cloudstack.storage.snapshot;
 
 import java.util.Date;
+import java.util.List;
 
 import javax.inject.Inject;
 
-import org.apache.log4j.Logger;
-
 import org.apache.cloudstack.engine.subsystem.api.storage.DataObjectInStore;
 import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
 import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine;
 import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory;
 import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
+import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotStrategy;
+import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotStrategy.SnapshotOperation;
+import org.apache.cloudstack.engine.subsystem.api.storage.StrategyPriority;
 import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
 import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
 import org.apache.cloudstack.storage.command.CopyCmdAnswer;
@@ -37,6 +39,7 @@ import org.apache.cloudstack.storage.datastore.ObjectInDataStoreManager;
 import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao;
 import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO;
 import org.apache.cloudstack.storage.to.SnapshotObjectTO;
+import org.apache.log4j.Logger;
 
 import com.cloud.agent.api.Answer;
 import com.cloud.agent.api.to.DataObjectType;
@@ -72,6 +75,8 @@ public class SnapshotObject implements SnapshotInfo {
     ObjectInDataStoreManager objectInStoreMgr;
     @Inject
     SnapshotDataStoreDao snapshotStoreDao;
+    @Inject
+    List<SnapshotStrategy> snapshotStrategies;
 
     public SnapshotObject() {
 
@@ -123,6 +128,16 @@ public class SnapshotObject implements SnapshotInfo {
     }
 
     @Override
+    public boolean isRevertable() {
+        SnapshotStrategy snapshotStrategy = StrategyPriority.pickStrategy(snapshotStrategies, snapshot, SnapshotOperation.REVERT);
+        if (snapshotStrategy != null) {
+            return true;
+        }
+
+        return false;
+    }
+
+    @Override
     public VolumeInfo getBaseVolume() {
         return volFactory.getVolume(snapshot.getVolumeId());
     }
@@ -268,18 +283,18 @@ public class SnapshotObject implements SnapshotInfo {
                     snapshotStore.setParentSnapshotId(0L);
                 }
                 snapshotStoreDao.update(snapshotStore.getId(), snapshotStore);
-                
+
                 // update side-effect of snapshot operation
                 if(snapshotTO.getVolume() != null && snapshotTO.getVolume().getPath() != null) {
-                	VolumeVO vol = volumeDao.findByUuid(snapshotTO.getVolume().getUuid());
-                	if(vol != null) {
-	                	s_logger.info("Update volume path change due to snapshot operation, volume " + vol.getId() + " path: "
-	                		+ vol.getPath() + "->" + snapshotTO.getVolume().getPath());
-	                	vol.setPath(snapshotTO.getVolume().getPath());
-	                	volumeDao.update(vol.getId(), vol);
-                	} else {
-                		s_logger.error("Cound't find the original volume with uuid: " + snapshotTO.getVolume().getUuid());
-                	}
+                    VolumeVO vol = volumeDao.findByUuid(snapshotTO.getVolume().getUuid());
+                    if(vol != null) {
+                        s_logger.info("Update volume path change due to snapshot operation, volume " + vol.getId() + " path: "
+                                + vol.getPath() + "->" + snapshotTO.getVolume().getPath());
+                        vol.setPath(snapshotTO.getVolume().getPath());
+                        volumeDao.update(vol.getId(), vol);
+                    } else {
+                        s_logger.error("Cound't find the original volume with uuid: " + snapshotTO.getVolume().getUuid());
+                    }
                 }
             } else {
                 throw new CloudRuntimeException("Unknown answer: " + answer.getClass());

http://git-wip-us.apache.org/repos/asf/cloudstack/blob/0ed7ebd7/engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/XenserverSnapshotStrategy.java
----------------------------------------------------------------------
diff --git a/engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/XenserverSnapshotStrategy.java b/engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/XenserverSnapshotStrategy.java
index 6a874d6..b3a64b6 100644
--- a/engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/XenserverSnapshotStrategy.java
+++ b/engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/XenserverSnapshotStrategy.java
@@ -27,6 +27,7 @@ import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
 import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotResult;
 import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotService;
 import org.apache.cloudstack.engine.subsystem.api.storage.StrategyPriority;
+import org.apache.cloudstack.engine.subsystem.api.storage.StrategyPriority.Priority;
 import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
 import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
 import org.apache.cloudstack.storage.command.CreateObjectAnswer;
@@ -310,7 +311,11 @@ public class XenserverSnapshotStrategy extends SnapshotStrategyBase {
     }
 
     @Override
-    public StrategyPriority.Priority canHandle(Snapshot snapshot) {
-        return StrategyPriority.Priority.DEFAULT;
+    public StrategyPriority.Priority canHandle(Snapshot snapshot, SnapshotOperation op) {
+        if (op == SnapshotOperation.REVERT) {
+            return Priority.CANT_HANDLE;
+        }
+
+        return Priority.DEFAULT;
     }
 }