Fix wait time for remove PowerFlex MDMs, and some code improvements

Author: sureshanaparti

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ScaleIOStorageAdaptor.java

@@ -152,7 +152,7 @@ public KVMPhysicalDisk getPhysicalDisk(String volumePath, KVMStoragePool pool) {
     @Override
     public KVMStoragePool createStoragePool(String uuid, String host, int port, String path, String userInfo, Storage.StoragePoolType type, Map<String, String> details, boolean isPrimaryStorage) {
         ScaleIOStoragePool storagePool = new ScaleIOStoragePool(uuid, host, port, path, type, details, this);
-        if (details != null && details.containsKey(ScaleIOSDCManager.ConnectOnDemand.key())) {
+        if (MapUtils.isNotEmpty(details) && details.containsKey(ScaleIOSDCManager.ConnectOnDemand.key())) {
             String connectOnDemand = details.get(ScaleIOSDCManager.ConnectOnDemand.key());
             if (connectOnDemand != null && !Boolean.parseBoolean(connectOnDemand)) {
                 Ternary<Boolean, Map<String, String>, String> prepareStorageClientStatus = prepareStorageClient(uuid, details);
@@ -180,19 +180,24 @@ public KVMStoragePool createStoragePool(String uuid, String host, int port, Stri
      */
     private void validateMdmState(Map<String, String> details) {
         String configKey = ScaleIOSDCManager.ValidateMdmsOnConnect.key();
+        if (MapUtils.isEmpty(details) || !details.containsKey(configKey)) {
+            logger.debug(String.format("Skipped PowerFlex MDMs validation as property %s not sent by Management Server", configKey));
+            return;
+        }
+
         String configValue = details.get(configKey);
 
         // be as much verbose as possible, otherwise it will be difficult to troubleshoot operational issue without logs
         if (StringUtils.isEmpty(configValue)) {
-            logger.debug(String.format("Skipped ScaleIO validation as property %s not sent by Management Server", configKey));
+            logger.debug(String.format("Skipped PowerFlex MDMs validation as property %s sent by Management Server is empty", configKey));
         } else if (Boolean.valueOf(configValue).equals(Boolean.FALSE)) {
-            logger.debug(String.format("Skipped ScaleIO validation as property %s received as %s", configKey, configValue));
+            logger.debug(String.format("Skipped PowerFlex MDMs validation as property %s received as %s", configKey, configValue));
         } else {
-            Collection<String> mdmsConfig = ScaleIOUtil.getMdmsFromConfig();
-            Collection<String> mdmsCli = ScaleIOUtil.getMdmsFromCli();
-            if (!mdmsCli.equals(mdmsConfig)) {
-                String msg = String.format("MDM addresses from memory and configuration file don't match. " +
-                        "Memory values: %s, configuration file values: %s", mdmsCli, mdmsConfig);
+            Collection<String> mdmsFromConfigFile = ScaleIOUtil.getMdmsFromConfigFile();
+            Collection<String> mdmsFromCliCmd = ScaleIOUtil.getMdmsFromCliCmd();
+            if (!mdmsFromCliCmd.equals(mdmsFromConfigFile)) {
+                String msg = String.format("PowerFlex MDM addresses from CLI and Configuration File doesn't match. " +
+                        "CLI values: %s, Configuration File values: %s", mdmsFromCliCmd, mdmsFromConfigFile);
                 logger.warn(msg);
                 throw new CloudRuntimeException(msg);
             }
@@ -210,7 +215,7 @@ public boolean deleteStoragePool(String uuid) {
 
     @Override
     public boolean deleteStoragePool(String uuid, Map<String, String> details) {
-        if (details != null && details.containsKey(ScaleIOSDCManager.ConnectOnDemand.key())) {
+        if (MapUtils.isNotEmpty(details) && details.containsKey(ScaleIOSDCManager.ConnectOnDemand.key())) {
             String connectOnDemand = details.get(ScaleIOSDCManager.ConnectOnDemand.key());
             if (connectOnDemand != null && !Boolean.parseBoolean(connectOnDemand)) {
                 Pair<Boolean, String> unprepareStorageClientStatus = unprepareStorageClient(uuid, details);
@@ -296,7 +301,7 @@ public boolean connectPhysicalDisk(String volumePath, KVMStoragePool pool, Map<S
         volumePath = ScaleIOUtil.getVolumePath(volumePath);
 
         int waitTimeInSec = DEFAULT_DISK_WAIT_TIME_IN_SECS;
-        if (details != null && details.containsKey(StorageManager.STORAGE_POOL_DISK_WAIT.toString())) {
+        if (MapUtils.isNotEmpty(details) && details.containsKey(StorageManager.STORAGE_POOL_DISK_WAIT.toString())) {
             String waitTime = details.get(StorageManager.STORAGE_POOL_DISK_WAIT.toString());
             if (StringUtils.isNotEmpty(waitTime)) {
                 waitTimeInSec = Integer.valueOf(waitTime).intValue();
@@ -664,7 +669,7 @@ public Ternary<Boolean, Map<String, String>, String> prepareStorageClient(String
                     return new Ternary<>(false, null, "Failed to add MDMs");
                 } else {
                     logger.debug(String.format("MDMs %s added to storage pool %s", mdms, uuid));
-                    applyTimeout(details);
+                    applyMdmsChangeWaitTime(details);
                 }
             }
         }
@@ -688,7 +693,7 @@ public Pair<Boolean, String> unprepareStorageClient(String uuid, Map<String, Str
             return new Pair<>(true, "SDC service not enabled on host, no need to unprepare the SDC client");
         }
 
-        if (details != null && details.containsKey(ScaleIOGatewayClient.STORAGE_POOL_MDMS)) {
+        if (MapUtils.isNotEmpty(details) && details.containsKey(ScaleIOGatewayClient.STORAGE_POOL_MDMS)) {
             String mdms = details.get(ScaleIOGatewayClient.STORAGE_POOL_MDMS);
             String[] mdmAddresses = mdms.split(",");
             if (mdmAddresses.length > 0) {
@@ -704,17 +709,22 @@ public Pair<Boolean, String> unprepareStorageClient(String uuid, Map<String, Str
                         logger.debug(String.format("Configuration key %s provided as %s", configKey, configValue));
                     }
                     Boolean blockUnprepare = Boolean.valueOf(configValue);
-                    if (!ScaleIOUtil.isRemoveMdmCliSupported() && !ScaleIOUtil.getVolumeIds().isEmpty() && Boolean.TRUE.equals(blockUnprepare)) {
+                    if (!ScaleIOUtil.isRemoveMdmCliSupported() // scini restart required when --remove_mdm is not supported
+                            && !ScaleIOUtil.getVolumeIds().isEmpty()
+                            && Boolean.TRUE.equals(blockUnprepare)) {
                         return new Pair<>(false, "Failed to remove MDMs, SDC client requires service to be restarted, but there are Volumes attached to the Host");
                     }
                 }
 
+                // Immediate removal of MDMs after unmapping volume throws Error: "Volume is mappedKernel module rejects removing MDM"
+                // Wait before removing MDMs for any volumes to get unmapped.
+                applyMdmsChangeWaitTime(details);
                 ScaleIOUtil.removeMdms(mdmAddresses);
                 if (ScaleIOUtil.isMdmPresent(mdmAddresses[0])) {
                     return new Pair<>(false, "Failed to remove MDMs, unable to unprepare the SDC client");
                 } else {
                     logger.debug(String.format("MDMs %s removed from storage pool %s", mdms, uuid));
-                    applyTimeout(details);
+                    applyMdmsChangeWaitTime(details);
                 }
             }
         }
@@ -730,36 +740,41 @@ public Pair<Boolean, String> unprepareStorageClient(String uuid, Map<String, Str
     }
 
     /**
-     * Check whether details map has timeout configured and do "apply timeout" pause before returning response
+     * Check whether details map has wait time configured and do "apply wait time" pause before returning response
      * (to have ScaleIO changes applied).
      *
      * @param details see {@link PrepareStorageClientCommand#getDetails()}
      *                and {@link @UnprepareStorageClientCommand#getDetails()}, expected to contain
-     *                {@link ScaleIOSDCManager#MdmsChangeApplyTimeout#key()}
+     *                {@link ScaleIOSDCManager#MdmsChangeApplyWaitTime#key()}
      */
-    private void applyTimeout(Map<String, String> details) {
-        String configKey = ScaleIOSDCManager.MdmsChangeApplyTimeout.key();
-        String configValue = details.get(configKey);
+    private void applyMdmsChangeWaitTime(Map<String, String> details) {
+        String configKey = ScaleIOSDCManager.MdmsChangeApplyWaitTime.key();
+        if (MapUtils.isEmpty(details) || !details.containsKey(configKey)) {
+            logger.debug(String.format("Apply wait time property %s not sent by Management Server, skip", configKey));
+            return;
+        }
 
+        String configValue = details.get(configKey);
         if (StringUtils.isEmpty(configValue)) {
-            logger.debug(String.format("Apply timeout value not defined in property %s, skip", configKey));
+            logger.debug(String.format("Apply wait time value not defined in property %s, skip", configKey));
             return;
         }
         long timeoutMs;
         try {
             timeoutMs = Long.parseLong(configValue);
         } catch (NumberFormatException e) {
-            logger.warn(String.format("Invalid apply timeout value defined in property %s, skip", configKey), e);
+            logger.warn(String.format("Invalid apply wait time value defined in property %s, skip", configKey), e);
             return;
         }
         if (timeoutMs < 1) {
-            logger.warn(String.format("Apply timeout value is too small (%s ms), skipping", timeoutMs));
+            logger.warn(String.format("Apply wait time value is too small (%s ms), skipping", timeoutMs));
             return;
         }
         try {
+            logger.debug(String.format("Waiting for %d ms as defined in property %s", timeoutMs, configKey));
             Thread.sleep(timeoutMs);
         } catch (InterruptedException e) {
-            logger.warn(String.format("Apply timeout %s ms interrupted", timeoutMs), e);
+            logger.warn(String.format("Waiting for %d ms interrupted", timeoutMs), e);
         }
     }
 
@@ -774,7 +789,7 @@ private Map<String, String> getSDCDetails(Map<String, String> details) {
             return sdcDetails;
         }
 
-        int waitTimeInSecs = 5;
+        int numberOfTries = 5;
         int timeBetweenTries = 1000; // Try more frequently (every sec) and return early when SDC Id or Guid found
         do {
             String sdcId = ScaleIOUtil.getSdcId(storageSystemId);
@@ -793,8 +808,8 @@ private Map<String, String> getSDCDetails(Map<String, String> details) {
                 Thread.sleep(timeBetweenTries);
             } catch (Exception ignore) {
             }
-            waitTimeInSecs--;
-        } while (waitTimeInSecs > 0);
+            numberOfTries--;
+        } while (numberOfTries > 0);
 
         return sdcDetails;
     }

plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/lifecycle/ScaleIOPrimaryDataStoreLifeCycle.java

@@ -417,7 +417,7 @@ private void populateScaleIOConfiguration(Map<String, String> details, long data
             details = new HashMap<>();
         }
 
-        details.put(ScaleIOSDCManager.MdmsChangeApplyTimeout.key(), String.valueOf(ScaleIOSDCManager.MdmsChangeApplyTimeout.valueIn(dataCenterId)));
+        details.put(ScaleIOSDCManager.MdmsChangeApplyWaitTime.key(), String.valueOf(ScaleIOSDCManager.MdmsChangeApplyWaitTime.valueIn(dataCenterId)));
         details.put(ScaleIOSDCManager.ValidateMdmsOnConnect.key(), String.valueOf(ScaleIOSDCManager.ValidateMdmsOnConnect.valueIn(dataCenterId)));
         details.put(ScaleIOSDCManager.BlockSdcUnprepareIfRestartNeededAndVolumesAreAttached.key(), String.valueOf(ScaleIOSDCManager.BlockSdcUnprepareIfRestartNeededAndVolumesAreAttached.valueIn(dataCenterId)));
     }

plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/manager/ScaleIOSDCManager.java

@@ -36,27 +36,27 @@ public interface ScaleIOSDCManager {
      * Timeout for Host to wait after MDM changes made on Host until changes will be applied.
      * Needed to avoid cases when Storage Pool is not connected yet, but Agent already starts to use Storage Pool.
      */
-    ConfigKey<Integer> MdmsChangeApplyTimeout = new ConfigKey<>("Storage",
+    ConfigKey<Integer> MdmsChangeApplyWaitTime = new ConfigKey<>("Storage",
             Integer.class,
-            "powerflex.mdm.change.apply.timeout.ms",
+            "powerflex.mdm.change.apply.wait",
             "1000",
-            "Timeout (in ms) for Host to wait after MDM changes made on Host until changes will be applied, default value: 1000 ms",
+            "Time (in ms) to wait after MDM addition, and before & after MDM removal changes made on the Host, default value: 1000 ms",
             Boolean.TRUE,
             ConfigKey.Scope.Zone);
 
     ConfigKey<Boolean> ValidateMdmsOnConnect = new ConfigKey<>("Storage",
             Boolean.class,
             "powerflex.mdm.validate.on.connect",
             Boolean.FALSE.toString(),
-            "Flag to validate MDMs on Host, present in configuration file and in CLI, default value: false",
+            "Flag to validate PowerFlex MDMs on the host, present in Configuration File and in CLI during storage pool registration in agent, default value: false",
             Boolean.TRUE,
             ConfigKey.Scope.Zone);
 
     ConfigKey<Boolean> BlockSdcUnprepareIfRestartNeededAndVolumesAreAttached = new ConfigKey<>("Storage",
             Boolean.class,
             "powerflex.block.sdc.unprepare",
             Boolean.FALSE.toString(),
-            "Block Storage Client un-preparation if SDC service restart needed but there are Volumes attached to the Host",
+            "Block storage client un-preparation if SDC service restart needed after PowerFlex MDM removal (i.e. no support for --remove_mdm in drv_cfg cmd) when there are Volumes mapped to the Host",
             Boolean.TRUE,
             ConfigKey.Scope.Zone);
 

plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/manager/ScaleIOSDCManagerImpl.java

@@ -207,7 +207,7 @@ private String prepareSDCOnHost(Host host, DataStore dataStore, String systemId,
         Map<String, String> details = new HashMap<>();
         details.put(ScaleIOGatewayClient.STORAGE_POOL_SYSTEM_ID, systemId);
         details.put(ScaleIOGatewayClient.STORAGE_POOL_MDMS, mdms);
-        details.put(MdmsChangeApplyTimeout.key(), String.valueOf(MdmsChangeApplyTimeout.valueIn(host.getDataCenterId())));
+        details.put(MdmsChangeApplyWaitTime.key(), String.valueOf(MdmsChangeApplyWaitTime.valueIn(host.getDataCenterId())));
 
         PrepareStorageClientCommand cmd = new PrepareStorageClientCommand(((PrimaryDataStore) dataStore).getPoolType(), dataStore.getUuid(), details);
         int timeoutSeconds = 60;
@@ -325,7 +325,7 @@ private boolean unprepareSDCOnHost(Host host, DataStore dataStore, String mdms)
         logger.debug(String.format("Unpreparing SDC on the host %s (%s)", host.getId(), host.getName()));
         Map<String,String> details = new HashMap<>();
         details.put(ScaleIOGatewayClient.STORAGE_POOL_MDMS, mdms);
-        details.put(MdmsChangeApplyTimeout.key(), String.valueOf(MdmsChangeApplyTimeout.valueIn(host.getDataCenterId())));
+        details.put(MdmsChangeApplyWaitTime.key(), String.valueOf(MdmsChangeApplyWaitTime.valueIn(host.getDataCenterId())));
         UnprepareStorageClientCommand cmd = new UnprepareStorageClientCommand(((PrimaryDataStore) dataStore).getPoolType(), dataStore.getUuid(), details);
         int timeoutSeconds = 60;
         cmd.setWait(timeoutSeconds);
@@ -492,6 +492,6 @@ public String getConfigComponentName() {
 
     @Override
     public ConfigKey<?>[] getConfigKeys() {
-        return new ConfigKey[]{ConnectOnDemand, MdmsChangeApplyTimeout, ValidateMdmsOnConnect, BlockSdcUnprepareIfRestartNeededAndVolumesAreAttached};
+        return new ConfigKey[]{ConnectOnDemand, MdmsChangeApplyWaitTime, ValidateMdmsOnConnect, BlockSdcUnprepareIfRestartNeededAndVolumesAreAttached};
     }
 }

plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/provider/ScaleIOHostListener.java

@@ -253,7 +253,7 @@ private void populateScaleIOConfiguration(Map<String, String> details, long data
             details = new HashMap<>();
         }
 
-        details.put(ScaleIOSDCManager.MdmsChangeApplyTimeout.key(), String.valueOf(ScaleIOSDCManager.MdmsChangeApplyTimeout.valueIn(dataCenterId)));
+        details.put(ScaleIOSDCManager.MdmsChangeApplyWaitTime.key(), String.valueOf(ScaleIOSDCManager.MdmsChangeApplyWaitTime.valueIn(dataCenterId)));
         details.put(ScaleIOSDCManager.ValidateMdmsOnConnect.key(), String.valueOf(ScaleIOSDCManager.ValidateMdmsOnConnect.valueIn(dataCenterId)));
         details.put(ScaleIOSDCManager.BlockSdcUnprepareIfRestartNeededAndVolumesAreAttached.key(), String.valueOf(ScaleIOSDCManager.BlockSdcUnprepareIfRestartNeededAndVolumesAreAttached.valueIn(dataCenterId)));
     }

plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/util/ScaleIOUtil.java

@@ -245,7 +245,7 @@ public static boolean removeMdms(String... mdmAddresses) {
     /**
      * Returns MDM entries from {@link ScaleIOUtil#DRV_CFG_FILE}.
      */
-    public static Collection<String> getMdmsFromConfig() {
+    public static Collection<String> getMdmsFromConfigFile() {
         List<String> configFileLines;
         try {
             configFileLines = Files.readAllLines(Path.of(DRV_CFG_FILE));
@@ -284,9 +284,9 @@ public static Collection<String> getVolumeIds() {
     }
 
     /**
-     * Returns MDM entries from CLI using {@code --query_mdms}.
+     * Returns MDM entries from CLI Cmd using {@code --query_mdms}.
      */
-    public static Collection<String> getMdmsFromCli() {
+    public static Collection<String> getMdmsFromCliCmd() {
         String command = ScaleIOUtil.SDC_HOME_PATH + "/bin/" + ScaleIOUtil.QUERY_MDMS_CMD;
         Pair<String, String> result = Script.executeCommand(command);
         String stdOut = result.first();