[GitHub] [cloudstack] slavkap commented on a change in pull request #4304: Storage plugin for Dell EMC PowerFlex/ScaleIO (formerly VxFlexOS)

[GitBox <gi...@apache.org>]

slavkap commented on a change in pull request #4304:
URL: https://github.com/apache/cloudstack/pull/4304#discussion_r575648139



##########
File path: server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java
##########
@@ -358,9 +363,33 @@ public VMSnapshot allocVMSnapshot(Long vmId, String vsDisplayName, String vsDesc
             throw new InvalidParameterValueException("Can not snapshot memory when VM is not in Running state");
         }
 
+        List<VolumeVO> rootVolumes = _volumeDao.findReadyRootVolumesByInstance(userVmVo.getId());
+        if (rootVolumes == null || rootVolumes.isEmpty()) {
+            throw new CloudRuntimeException("Unable to find root volume for the user vm:" + userVmVo.getUuid());
+        }
+
+        VolumeVO rootVolume = rootVolumes.get(0);
+        StoragePoolVO rootVolumePool = _storagePoolDao.findById(rootVolume.getPoolId());
+        if (rootVolumePool == null) {
+            throw new CloudRuntimeException("Unable to find root volume storage pool for the user vm:" + userVmVo.getUuid());
+        }
+
         // for KVM, only allow snapshot with memory when VM is in running state
-        if (userVmVo.getHypervisorType() == HypervisorType.KVM && userVmVo.getState() == State.Running && !snapshotMemory) {
-            throw new InvalidParameterValueException("KVM VM does not allow to take a disk-only snapshot when VM is in running state");
+        if (userVmVo.getHypervisorType() == HypervisorType.KVM) {
+            if (rootVolumePool.getPoolType() != Storage.StoragePoolType.PowerFlex) {

Review comment:
       Hi @sureshanaparti, may I suggest here to get the VMSnapshotStrategy instead of checking which is the storage pool type? If it's an instance of DefaultVMSnapshotStrategy to get into the statement. In this case, the other storage plugins could implement their own VM snapshot strategy, without checking here which is the storage pool type. I saw that Solidfire's API has a group snapshot (maybe and others have it). We at StorPool also have this functionality (but for now we have a workaround for this). I guess someday it will be easier for the plugins to adopt this functionality




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org