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

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

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



##########
File path: api/src/main/java/org/apache/cloudstack/api/response/UserVmResponse.java
##########
@@ -310,6 +310,10 @@
     @Param(description = "Guest vm Boot Type")
     private String bootType;
 
+    @SerializedName(ApiConstants.POOL_TYPE)
+    @Param(description = "the pool type of the virtual machine")

Review comment:
       ```suggestion
       @Param(description = "the pool type of the virtual machine", since="4.16)
   ```

##########
File path: api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateDiskOfferingCmd.java
##########
@@ -152,6 +156,9 @@
             since = "4.14")
     private String cacheMode;
 
+    @Parameter(name = ApiConstants.DETAILS, type = CommandType.MAP, description = "details to specify disk offering parameters")

Review comment:
       will be good to add a since tag here
   ```suggestion
       @Parameter(name = ApiConstants.DETAILS, type = CommandType.MAP, description = "details to specify disk offering parameters", since="4.16)
   ```

##########
File path: engine/api/src/main/java/com/cloud/vm/VirtualMachineManager.java
##########
@@ -59,7 +59,13 @@
             "The default label name for the config drive", false);
 
     ConfigKey<Boolean> VmConfigDriveOnPrimaryPool = new ConfigKey<>("Advanced", Boolean.class, "vm.configdrive.primarypool.enabled", "false",
-            "If config drive need to be created and hosted on primary storage pool. Currently only supported for KVM.", true);
+            "If config drive need to be created and hosted on primary storage pool. Currently only supported for KVM.", true, ConfigKey.Scope.Zone);

Review comment:
       @sureshanaparti this config is changed from global scope to zone. Will that affect the existing setup in any way?

##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
##########
@@ -1465,11 +1543,17 @@ public void prepareForMigration(VirtualMachineProfile vm, DeployDestination dest
                     long hostId = vm.getVirtualMachine().getHostId();
                     Host host = _hostDao.findById(hostId);
 
-                    volService.grantAccess(volFactory.getVolume(newVol.getId()), host, destPool);
+                    try {
+                        volService.grantAccess(volFactory.getVolume(newVol.getId()), host, destPool);
+                    } catch (Exception e) {
+                        throw new StorageAccessException("Unable to grant access to volume: " + newVol.getId() + " on host: " + host.getId());
+                    }

Review comment:
       Curious, why we need to catch all _Exception_ here? And we are not even logging it then

##########
File path: engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java
##########
@@ -2301,7 +2325,9 @@ private void handleCreateTemplateFromManagedVolume(VolumeInfo volumeInfo, Templa
         CopyCmdAnswer copyCmdAnswer = null;
 
         try {
-            if (!ImageFormat.QCOW2.equals(volumeInfo.getFormat())) {
+            StoragePoolVO storagePoolVO = _storagePoolDao.findById(volumeInfo.getPoolId());
+
+            if (!ImageFormat.QCOW2.equals(volumeInfo.getFormat()) && !(ImageFormat.RAW.equals(volumeInfo.getFormat()) && StoragePoolType.PowerFlex == storagePoolVO.getPoolType())) {

Review comment:
       We listed couple of more image formats with powerflex support

##########
File path: engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java
##########
@@ -574,6 +574,14 @@ private void handleVolumeMigrationFromManagedStorageToNonManagedStorage(VolumeIn
         }
     }
 
+    private void verifyFormatWithPoolType(ImageFormat imageFormat, StoragePoolType poolType) {
+        if (imageFormat != ImageFormat.VHD && imageFormat != ImageFormat.OVA && imageFormat != ImageFormat.QCOW2 &&
+                !(imageFormat == ImageFormat.RAW && StoragePoolType.PowerFlex == poolType)) {
+            throw new CloudRuntimeException("Only the following image types are currently supported: " +
+                    ImageFormat.VHD.toString() + ", " + ImageFormat.OVA.toString() + ", " + ImageFormat.QCOW2.toString() + ", and " + ImageFormat.RAW.toString() + "(for PowerFlex)");

Review comment:
       Is this message in-line with the fact that we support PowerFlex only for KVM right now?

##########
File path: ui/scripts/instances.js
##########
@@ -4189,6 +4189,10 @@
                 allowedActions.push("storageSnapshot");
             }
 
+            if (jsonObj.hypervisor == 'KVM' && jsonObj.pooltype == 'PowerFlex') {
+                allowedActions.push("snapshot");
+            }
+

Review comment:
       @sureshanaparti do we need a Primate change for this or is it already handled there?




----------------------------------------------------------------
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