Changing locationType to PRIMARY and SECONDARY

Mike Tutkowski · syed · commit 67ca1bb31571 · 2016-08-31T16:57:23.000-04:00
diff --git a/api/src/com/cloud/storage/Snapshot.java b/api/src/com/cloud/storage/Snapshot.java
@@ -67,7 +67,7 @@ enum Event {
     }
 
     enum LocationType {
-        Standard, Archive
+        PRIMARY, SECONDARY
     }
 
     public static final long MANUAL_POLICY_ID = 0L;
diff --git a/api/src/org/apache/cloudstack/api/command/user/snapshot/CreateSnapshotCmd.java b/api/src/org/apache/cloudstack/api/command/user/snapshot/CreateSnapshotCmd.java
@@ -24,6 +24,7 @@
 import com.cloud.storage.Snapshot;
 import com.cloud.storage.Volume;
 import com.cloud.user.Account;
+import com.cloud.utils.exception.CloudRuntimeException;
 import org.apache.cloudstack.api.APICommand;
 import org.apache.cloudstack.api.ApiCommandJobType;
 import org.apache.cloudstack.api.ApiConstants;
@@ -72,8 +73,8 @@ public class CreateSnapshotCmd extends BaseAsyncCreateCmd {
     private Boolean quiescevm;
 
     @Parameter(name = ApiConstants.LOCATION_TYPE, type = CommandType.SHORT, required = false, description = "Currently applicable only for managed storage. " +
-            "Valid location types: 1 = 'standard'; 2 = 'archive'. Default = 1.")
-    private Short locationType;
+            "Valid location types: 'primary', 'secondary'. Default = 'primary'.")
+    private String locationType;
 
     @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, description = "the name of the snapshot")
     private String snapshotName;
@@ -214,17 +215,20 @@ public void execute() {
     }
 
     private Snapshot.LocationType getLocationType() {
-        if (Snapshot.LocationType.values() == null || Snapshot.LocationType.values().length == 0) {
+
+        if (Snapshot.LocationType.values() == null || Snapshot.LocationType.values().length == 0 || locationType == null) {
             return null;
         }
 
-        Snapshot.LocationType locationTypeToReturn = Snapshot.LocationType.values()[0];
-
-        if (locationType != null && locationType >= 1 && locationType <= Snapshot.LocationType.values().length) {
-            locationTypeToReturn = Snapshot.LocationType.values()[locationType-1];
+        try {
+            String lType = locationType.trim().toUpperCase();
+            return Snapshot.LocationType.valueOf(lType);
+        } catch (IllegalArgumentException e) {
+            String errMesg = "Invalid locationType " + locationType + "Specified for volume " + getVolumeId()
+                        + " Valid values are: primary,secondary ";
+            s_logger.warn(errMesg);
+            throw  new CloudRuntimeException(errMesg);
         }
-
-        return locationTypeToReturn;
     }
 
     @Override
diff --git a/api/test/org/apache/cloudstack/api/command/test/CreateSnapshotCmdTest.java b/api/test/org/apache/cloudstack/api/command/test/CreateSnapshotCmdTest.java
@@ -37,7 +37,7 @@
 import static org.mockito.Matchers.anyBoolean;
 import static org.mockito.Matchers.anyLong;
 import static org.mockito.Matchers.anyString;
-import static org.mockito.Matchers.eq;
+import static org.mockito.Matchers.isNull;
 
 public class CreateSnapshotCmdTest extends TestCase {
 
@@ -83,7 +83,7 @@ public void testCreateSuccess() {
         try {
 
             Mockito.when(volumeApiService.takeSnapshot(anyLong(), anyLong(), anyLong(),
-                    any(Account.class), anyBoolean(), eq(Snapshot.LocationType.Standard))).thenReturn(snapshot);
+                    any(Account.class), anyBoolean(), isNull(Snapshot.LocationType.class))).thenReturn(snapshot);
 
         } catch (Exception e) {
             Assert.fail("Received exception when success expected " + e.getMessage());
@@ -113,7 +113,7 @@ public void testCreateFailure() {
 
         try {
                 Mockito.when(volumeApiService.takeSnapshot(anyLong(), anyLong(), anyLong(),
-                        any(Account.class), anyBoolean(), eq(Snapshot.LocationType.Standard))).thenReturn(null);
+                        any(Account.class), anyBoolean(), isNull(Snapshot.LocationType.class))).thenReturn(null);
         } catch (Exception e) {
             Assert.fail("Received exception when success expected " + e.getMessage());
         }
diff --git a/engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java b/engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java
@@ -349,7 +349,7 @@ private Scope getZoneScope(Scope scope) {
     /**
      * This function is responsible for copying a volume from the managed store to a secondary store. This is used in two cases
      * 1) When creating a template from a snapshot
-     * 2) When createSnapshot is called with location=Archive
+     * 2) When createSnapshot is called with location=SECONDARY
      *
      * @param snapshotInfo Source snapshot
      * @param destData destination (can be template or snapshot)
diff --git a/engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/StorageSystemSnapshotStrategy.java b/engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/StorageSystemSnapshotStrategy.java
@@ -91,7 +91,7 @@ public class StorageSystemSnapshotStrategy extends SnapshotStrategyBase {
     @Override
     public SnapshotInfo backupSnapshot(SnapshotInfo snapshotInfo) {
 
-        if (!snapshotInfo.getLocationType().equals(Snapshot.LocationType.Archive)) {
+        if (!snapshotInfo.getLocationType().equals(Snapshot.LocationType.SECONDARY)) {
 
             markAsBackedUp((SnapshotObject)snapshotInfo);
             return snapshotInfo;
@@ -102,7 +102,7 @@ public SnapshotInfo backupSnapshot(SnapshotInfo snapshotInfo) {
         // If archive flag is passed, we will copy this snapshot to secondary
         // storage and delete it from the primary storage.
 
-        Preconditions.checkState(Snapshot.LocationType.Archive.equals(snapshotInfo.getLocationType()));
+        Preconditions.checkState(Snapshot.LocationType.SECONDARY.equals(snapshotInfo.getLocationType()));
 
         HostVO host = getHost(snapshotInfo.getVolumeId());
         boolean canStorageSystemCreateVolumeFromSnapshot = canStorageSystemCreateVolumeFromSnapshot(snapshotInfo.getBaseVolume().getPoolId());
@@ -262,7 +262,7 @@ public SnapshotInfo takeSnapshot(SnapshotInfo snapshotInfo) {
                 volumeInfo.stateTransit(Volume.Event.OperationSucceeded);
 
                 Preconditions.checkState(snapshotOnPrimary != null);
-                if (Snapshot.LocationType.Archive.equals(snapshotInfo.getLocationType())) {
+                if (Snapshot.LocationType.SECONDARY.equals(snapshotInfo.getLocationType())) {
                     // cleanup the snapshot on primary
                     try {
                         snapshotSvr.deleteSnapshot(snapshotOnPrimary);
@@ -588,7 +588,7 @@ public StrategyPriority canHandle(Snapshot snapshot, SnapshotOperation op) {
         Snapshot.LocationType locationType = snapshot.getLocationType();
 
         //If the snapshot exisits on Secondary Storage, we can't delete it.
-        if (SnapshotOperation.DELETE.equals(op) && Snapshot.LocationType.Archive.equals(locationType)) {
+        if (SnapshotOperation.DELETE.equals(op) && Snapshot.LocationType.SECONDARY.equals(locationType)) {
             return StrategyPriority.CANT_HANDLE;
         }
 
diff --git a/server/src/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/com/cloud/storage/VolumeApiServiceImpl.java
@@ -2054,6 +2054,11 @@ public Snapshot takeSnapshot(Long volumeId, Long policyId, Long snapshotId, Acco
             throw new InvalidParameterValueException("VolumeId: " + volumeId + " is not in " + Volume.State.Ready + " state but " + volume.getState() + ". Cannot take snapshot.");
         }
 
+        StoragePoolVO storagePoolVO = _storagePoolDao.findById(volume.getPoolId());
+        if (storagePoolVO.isManaged() && locationType == null) {
+            locationType = Snapshot.LocationType.PRIMARY;
+        }
+
         VMInstanceVO vm = null;
         if (volume.getInstanceId() != null)
             vm = _vmInstanceDao.findById(volume.getInstanceId());
@@ -2174,8 +2179,12 @@ public Snapshot allocSnapshot(Long volumeId, Long policyId, String snapshotName,
         }
 
         StoragePoolVO storagePoolVO = _storagePoolDao.findById(volume.getPoolId());
-        if (!storagePoolVO.isManaged() && Snapshot.LocationType.Archive.equals(locationType)) {
-            throw new InvalidParameterValueException("VolumeId: " + volumeId + " Archive is supported only for managed storage");
+        if (!storagePoolVO.isManaged() && locationType != null) {
+            throw new InvalidParameterValueException("VolumeId: " + volumeId + " LocationType is supported only for managed storage");
+        }
+
+        if (storagePoolVO.isManaged() && locationType == null) {
+            locationType = Snapshot.LocationType.PRIMARY;
         }
 
         StoragePool storagePool = (StoragePool)volume.getDataStore();
diff --git a/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java b/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
@@ -1043,7 +1043,7 @@ private void updateSnapshotPayload(long storagePoolId, CreateSnapshotPayload pay
             Snapshot.LocationType locationType = payload.getLocationType();
 
             if (locationType == null) {
-                payload.setLocationType(Snapshot.LocationType.Standard);
+                payload.setLocationType(Snapshot.LocationType.PRIMARY);
             }
         }
         else {
diff --git a/server/test/com/cloud/storage/VolumeApiServiceImplTest.java b/server/test/com/cloud/storage/VolumeApiServiceImplTest.java
@@ -422,13 +422,13 @@ public void testUpdateMissingRootDiskControllerWithValidChainInfo() {
 
     @Test
     /**
-     * Setting locationType=Archive for a non-managed storage should give an error
+     * Setting locationType for a non-managed storage should give an error
      */
     public void testAllocSnapshotNonManagedStorageArchive() {
         try {
-            _svc.allocSnapshot(6L, 1L, "test", Snapshot.LocationType.Archive);
+            _svc.allocSnapshot(6L, 1L, "test", Snapshot.LocationType.SECONDARY);
         } catch (InvalidParameterValueException e) {
-            Assert.assertEquals(e.getMessage(), "VolumeId: 6 Archive is supported only for managed storage");
+            Assert.assertEquals(e.getMessage(), "VolumeId: 6 LocationType is supported only for managed storage");
             return;
         } catch (ResourceAllocationException e) {
             Assert.fail("Unexpected excepiton " + e.getMessage());
diff --git a/server/test/com/cloud/storage/snapshot/SnapshotManagerTest.java b/server/test/com/cloud/storage/snapshot/SnapshotManagerTest.java
@@ -181,7 +181,7 @@ public void tearDown() {
     public void testAllocSnapshotF1() throws ResourceAllocationException {
         when(_vmDao.findById(anyLong())).thenReturn(vmMock);
         when(vmMock.getState()).thenReturn(State.Destroyed);
-        _snapshotMgr.allocSnapshot(TEST_VOLUME_ID, Snapshot.MANUAL_POLICY_ID, null, Snapshot.LocationType.Standard);
+        _snapshotMgr.allocSnapshot(TEST_VOLUME_ID, Snapshot.MANUAL_POLICY_ID, null, null);
     }
 
     // active snapshots
@@ -196,7 +196,7 @@ public void testAllocSnapshotF2() throws ResourceAllocationException {
         List<SnapshotVO> mockList = mock(List.class);
         when(mockList.size()).thenReturn(1);
         when(_snapshotDao.listByInstanceId(TEST_VM_ID, Snapshot.State.Creating, Snapshot.State.CreatedOnPrimary, Snapshot.State.BackingUp)).thenReturn(mockList);
-        _snapshotMgr.allocSnapshot(TEST_VOLUME_ID, Snapshot.MANUAL_POLICY_ID, null, Snapshot.LocationType.Standard);
+        _snapshotMgr.allocSnapshot(TEST_VOLUME_ID, Snapshot.MANUAL_POLICY_ID, null, null);
     }
 
     // active vm snapshots
@@ -214,7 +214,7 @@ public void testAllocSnapshotF3() throws ResourceAllocationException {
         List<VMSnapshotVO> mockList2 = mock(List.class);
         when(mockList2.size()).thenReturn(1);
         when(_vmSnapshotDao.listByInstanceId(TEST_VM_ID, VMSnapshot.State.Creating, VMSnapshot.State.Reverting, VMSnapshot.State.Expunging)).thenReturn(mockList2);
-        _snapshotMgr.allocSnapshot(TEST_VOLUME_ID, Snapshot.MANUAL_POLICY_ID, null, Snapshot.LocationType.Standard);
+        _snapshotMgr.allocSnapshot(TEST_VOLUME_ID, Snapshot.MANUAL_POLICY_ID, null, null);
     }
 
     // successful test
@@ -233,7 +233,7 @@ public void testAllocSnapshotF4() throws ResourceAllocationException {
         when(mockList2.size()).thenReturn(0);
         when(_vmSnapshotDao.listByInstanceId(TEST_VM_ID, VMSnapshot.State.Creating, VMSnapshot.State.Reverting, VMSnapshot.State.Expunging)).thenReturn(mockList2);
         when(_snapshotDao.persist(any(SnapshotVO.class))).thenReturn(snapshotMock);
-        _snapshotMgr.allocSnapshot(TEST_VOLUME_ID, Snapshot.MANUAL_POLICY_ID, null, Snapshot.LocationType.Standard);
+        _snapshotMgr.allocSnapshot(TEST_VOLUME_ID, Snapshot.MANUAL_POLICY_ID, null, null);
     }
 
     @Test
diff --git a/test/integration/plugins/solidfire/TestSnapshots.py b/test/integration/plugins/solidfire/TestSnapshots.py
@@ -1633,7 +1633,7 @@ def _create_and_test_archive_snapshot(self, volume_id_for_snapshot, sf_volume):
         vol_snap = Snapshot.create(
             self.apiClient,
             volume_id=volume_id_for_snapshot,
-            locationtype=2
+            locationtype="secondary"
         )
 
         self._wait_for_snapshot_state(vol_snap.id, Snapshot.BACKED_UP)
diff --git a/test/integration/plugins/solidfire/util/sf_util.py b/test/integration/plugins/solidfire/util/sf_util.py
@@ -1,3 +1,19 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
 
 def check_list(in_list, expected_size_of_list, obj_assert, err_msg):
     obj_assert.assertEqual(

Original file line number	Diff line number	Diff line change
`@@ -67,7 +67,7 @@ enum Event {`
`67`	`67`	`}`
`68`	`68`
`69`	`69`	`enum LocationType {`
`70`		`- Standard, Archive`
	`70`	`+ PRIMARY, SECONDARY`
`71`	`71`	`}`
`72`	`72`
`73`	`73`	`public static final long MANUAL_POLICY_ID = 0L;`
Original file line number	Diff line number	Diff line change
`@@ -349,7 +349,7 @@ private Scope getZoneScope(Scope scope) {`
`349`	`349`	`/**`
`350`	`350`	`* This function is responsible for copying a volume from the managed store to a secondary store. This is used in two cases`
`351`	`351`	`* 1) When creating a template from a snapshot`
`352`		`- * 2) When createSnapshot is called with location=Archive`
	`352`	`+ * 2) When createSnapshot is called with location=SECONDARY`
`353`	`353`	`*`
`354`	`354`	`* @param snapshotInfo Source snapshot`
`355`	`355`	`* @param destData destination (can be template or snapshot)`
Original file line number	Diff line number	Diff line change
`@@ -1043,7 +1043,7 @@ private void updateSnapshotPayload(long storagePoolId, CreateSnapshotPayload pay`
`1043`	`1043`	`Snapshot.LocationType locationType = payload.getLocationType();`
`1044`	`1044`
`1045`	`1045`	`if (locationType == null) {`
`1046`		`- payload.setLocationType(Snapshot.LocationType.Standard);`
	`1046`	`+ payload.setLocationType(Snapshot.LocationType.PRIMARY);`
`1047`	`1047`	`}`
`1048`	`1048`	`}`
`1049`	`1049`	`else {`
Original file line number	Diff line number	Diff line change
`@@ -1633,7 +1633,7 @@ def _create_and_test_archive_snapshot(self, volume_id_for_snapshot, sf_volume):`
`1633`	`1633`	`vol_snap = Snapshot.create(`
`1634`	`1634`	`self.apiClient,`
`1635`	`1635`	`volume_id=volume_id_for_snapshot,`
`1636`		`- locationtype=2`
	`1636`	`+ locationtype="secondary"`
`1637`	`1637`	`)`
`1638`	`1638`
`1639`	`1639`	`self._wait_for_snapshot_state(vol_snap.id, Snapshot.BACKED_UP)`