From ebb6c0271ef3744b7febf5b1bcdd7a425b8f765e Mon Sep 17 00:00:00 2001 From: mprokopchuk Date: Tue, 8 Apr 2025 16:58:37 -0700 Subject: [PATCH 01/14] Cumulative enhancements fix for ScaleIO: MDM add/remove, Host prepare/unprepare, validate Storage Pool can be created in Agent. - Implemented validation to fail Host disconnect from Storage Pool if there are Volumes attached and SDC client MDM removal requires scini service to be restarted - Implemented Storage Pool validation by checking whether MDM addresses from configuration file and from memory (using CLI) matches, otherwise file ModifyStoragePool command. - Introduced configuration key to apply timeout after making MDM changes for ScaleIO: powerflex.mdm.change.apply.timeout.ms (default 1000ms) - Implemented logic to apply timeout after making MDM changes for ScaleIO in prepare and unprepare logic - Added detection of MDM removal support via CLI - If MDM removal support via CLI supported then use CLI, fall back to edit drv_cfg.txt and restart scini instead --- ...ibvirtModifyStoragePoolCommandWrapper.java | 16 +- .../kvm/storage/ScaleIOStorageAdaptor.java | 98 +++++- .../storage/ScaleIOStorageAdaptorTest.java | 7 +- .../kvm/storage/ScaleIOStoragePoolTest.java | 6 +- .../client/ScaleIOGatewayClient.java | 3 + .../ScaleIOPrimaryDataStoreLifeCycle.java | 4 + .../datastore/manager/ScaleIOSDCManager.java | 18 + .../manager/ScaleIOSDCManagerImpl.java | 7 +- .../provider/ScaleIOHostListener.java | 4 + .../storage/datastore/util/ScaleIOUtil.java | 313 +++++++++++++++--- .../java/com/cloud/utils/script/Script.java | 19 ++ 11 files changed, 436 insertions(+), 59 deletions(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtModifyStoragePoolCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtModifyStoragePoolCommandWrapper.java index 06883c708d96..990cefda8f33 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtModifyStoragePoolCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtModifyStoragePoolCommandWrapper.java @@ -31,6 +31,7 @@ import com.cloud.resource.CommandWrapper; import com.cloud.resource.ResourceWrapper; import com.cloud.storage.template.TemplateProp; +import com.cloud.utils.exception.CloudRuntimeException; @ResourceWrapper(handles = ModifyStoragePoolCommand.class) public final class LibvirtModifyStoragePoolCommandWrapper extends CommandWrapper { @@ -49,11 +50,16 @@ public Answer execute(final ModifyStoragePoolCommand command, final LibvirtCompu return answer; } - final KVMStoragePool storagepool = - storagePoolMgr.createStoragePool(command.getPool().getUuid(), command.getPool().getHost(), command.getPool().getPort(), command.getPool().getPath(), command.getPool() - .getUserInfo(), command.getPool().getType(), command.getDetails()); - if (storagepool == null) { - return new Answer(command, false, " Failed to create storage pool"); + final KVMStoragePool storagepool; + try { + storagepool = + storagePoolMgr.createStoragePool(command.getPool().getUuid(), command.getPool().getHost(), command.getPool().getPort(), command.getPool().getPath(), command.getPool() + .getUserInfo(), command.getPool().getType(), command.getDetails()); + if (storagepool == null) { + return new Answer(command, false, " Failed to create storage pool"); + } + } catch (CloudRuntimeException e) { + return new Answer(command, false, String.format("Failed to create storage pool: %s", e.getLocalizedMessage())); } final Map tInfo = new HashMap(); diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ScaleIOStorageAdaptor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ScaleIOStorageAdaptor.java index 29c152f934f6..f4d769a183a0 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ScaleIOStorageAdaptor.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ScaleIOStorageAdaptor.java @@ -22,11 +22,13 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.UUID; +import com.cloud.agent.api.PrepareStorageClientCommand; import org.apache.cloudstack.storage.datastore.client.ScaleIOGatewayClient; import org.apache.cloudstack.storage.datastore.manager.ScaleIOSDCManager; import org.apache.cloudstack.storage.datastore.util.ScaleIOUtil; @@ -158,10 +160,45 @@ public KVMStoragePool createStoragePool(String uuid, String host, int port, Stri } } } + + validateMdmState(details); + MapStorageUuidToStoragePool.put(uuid, storagePool); return storagePool; } + /** + * Validate Storage Pool state to ensure it healthy and can operate requests. + * There is observed situation where ScaleIO configuration file has different values than ScaleIO CLI. + * Validation compares values from both drv_cfg.txt and drv_cfg CLI and throws exception if there is mismatch. + * + * @param details see {@link PrepareStorageClientCommand#getDetails()} + * and {@link @UnprepareStorageClientCommand#getDetails()}, expected to contain + * {@link ScaleIOSDCManager#ValidateMdmsOnConnect#key()} + * @throws CloudRuntimeException in case if Storage Pool is not operate-able + */ + private void validateMdmState(Map details) { + + String configKey = ScaleIOSDCManager.ValidateMdmsOnConnect.key(); + 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)); + } else if (Boolean.valueOf(configValue).equals(Boolean.FALSE)) { + LOGGER.debug(String.format("Skipped ScaleIO validation as property %s received as %s", configKey, configValue)); + } else { + Collection mdmsConfig = ScaleIOUtil.getMdmsFromConfig(); + Collection 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); + LOGGER.warn(msg); + throw new CloudRuntimeException(msg); + } + } + } + @Override public boolean deleteStoragePool(String uuid) { ScaleIOStoragePool storagePool = (ScaleIOStoragePool) MapStorageUuidToStoragePool.get(uuid); @@ -617,13 +654,16 @@ public Ternary, String> prepareStorageClient(String String mdms = details.get(ScaleIOGatewayClient.STORAGE_POOL_MDMS); String[] mdmAddresses = mdms.split(","); if (mdmAddresses.length > 0) { - if (ScaleIOUtil.mdmAdded(mdmAddresses[0])) { + if (ScaleIOUtil.isMdmPresent(mdmAddresses[0])) { return new Ternary<>(true, getSDCDetails(details), "MDM added, no need to prepare the SDC client"); } - ScaleIOUtil.addMdms(Arrays.asList(mdmAddresses)); - if (!ScaleIOUtil.mdmAdded(mdmAddresses[0])) { + ScaleIOUtil.addMdms(mdmAddresses); + if (!ScaleIOUtil.isMdmPresent(mdmAddresses[0])) { 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); } } } @@ -646,20 +686,66 @@ public Pair unprepareStorageClient(String uuid, Map 0) { - if (!ScaleIOUtil.mdmAdded(mdmAddresses[0])) { + if (!ScaleIOUtil.isMdmPresent(mdmAddresses[0])) { return new Pair<>(true, "MDM not added, no need to unprepare the SDC client"); + } else if (!ScaleIOUtil.isRemoveMdmCliSupported() && !ScaleIOUtil.getVolumeIds().isEmpty()) { + return new Pair<>(false, "Failed to remove MDMs, SDC client requires service to be restarted, but there are Volumes attached to the Host"); } - ScaleIOUtil.removeMdms(Arrays.asList(mdmAddresses)); - if (ScaleIOUtil.mdmAdded(mdmAddresses[0])) { + 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); } } } + /* + * TODO: + * 1. Verify on-demand is true + * 2. If on-demand is true check whether other MDM addresses are still present + * 3. If there are no MDM addresses, then stop SDC service. + */ + return new Pair<>(true, "Unprepared SDC client successfully"); } + /** + * Check whether details map has timeout configured and do "apply timeout" 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()} + */ + private void applyTimeout(Map details) { + String configKey = ScaleIOSDCManager.MdmsChangeApplyTimeout.key(); + String configValue = details.get(configKey); + + if (StringUtils.isEmpty(configValue)) { + LOGGER.debug(String.format("Apply timeout 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); + return; + } + if (timeoutMs < 1) { + LOGGER.warn(String.format("Apply timeout value is too small (%s ms), skipping", timeoutMs)); + return; + } + try { + Thread.sleep(timeoutMs); + } catch (InterruptedException e) { + LOGGER.warn(String.format("Apply timeout %s ms interrupted", timeoutMs), e); + } + } + private Map getSDCDetails(Map details) { Map sdcDetails = new HashMap(); if (details == null || !details.containsKey(ScaleIOGatewayClient.STORAGE_POOL_SYSTEM_ID)) { diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ScaleIOStorageAdaptorTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ScaleIOStorageAdaptorTest.java index 421fee096341..6bcd43797be5 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ScaleIOStorageAdaptorTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ScaleIOStorageAdaptorTest.java @@ -180,7 +180,7 @@ public void testUnprepareStorageClient_MDMNotAdded() { details.put(ScaleIOGatewayClient.STORAGE_POOL_MDMS, "1.1.1.1,2.2.2.2"); when(Script.runSimpleBashScriptForExitValue(Mockito.eq("systemctl status scini"))).thenReturn(3); when(Script.runSimpleBashScriptForExitValue(Mockito.eq("systemctl is-enabled scini"))).thenReturn(0); - when(Script.runSimpleBashScript(Mockito.eq("/opt/emc/scaleio/sdc/bin/drv_cfg --query_mdms|grep 1.1.1.1"))).thenReturn("MDM-ID 71fd458f0775010f SDC ID 4421a91a00000000 INSTALLATION ID 204930df2cbcaf8e IPs [0]-3.3.3.3 [1]-4.4.4.4"); + when(Script.runSimpleBashScript(Mockito.eq("/opt/emc/scaleio/sdc/bin/drv_cfg --query_mdms --file /etc/emc/scaleio/drv_cfg.txt|grep 1.1.1.1"))).thenReturn("MDM-ID 71fd458f0775010f SDC ID 4421a91a00000000 INSTALLATION ID 204930df2cbcaf8e IPs [0]-3.3.3.3 [1]-4.4.4.4"); Pair result = scaleIOStorageAdaptor.unprepareStorageClient(poolUuid, details); @@ -195,7 +195,10 @@ public void testUnprepareStorageClient_RemoveMDMFailed() { when(Script.runSimpleBashScriptForExitValue(Mockito.eq("systemctl status scini"))).thenReturn(3); when(Script.runSimpleBashScriptForExitValue(Mockito.eq("systemctl is-enabled scini"))).thenReturn(0); when(Script.runSimpleBashScriptForExitValue(Mockito.eq("systemctl restart scini"))).thenReturn(0); - when(Script.runSimpleBashScript(Mockito.eq("/opt/emc/scaleio/sdc/bin/drv_cfg --query_mdms|grep 1.1.1.1"))).thenReturn("MDM-ID 71fd458f0775010f SDC ID 4421a91a00000000 INSTALLATION ID 204930df2cbcaf8e IPs [0]-1.1.1.1 [1]-2.2.2.2"); + when(Script.runSimpleBashScript(Mockito.eq("/opt/emc/scaleio/sdc/bin/drv_cfg --query_mdms --file /etc/emc/scaleio/drv_cfg.txt|grep 1.1.1.1"))).thenReturn("MDM-ID 71fd458f0775010f SDC ID 4421a91a00000000 INSTALLATION ID 204930df2cbcaf8e IPs [0]-1.1.1.1 [1]-2.2.2.2"); + when(Script.executeCommand(Mockito.eq("/opt/emc/scaleio/sdc/bin/drv_cfg"))).thenReturn(new Pair<>(null, null)); + when(Script.executeCommand(Mockito.eq("/opt/emc/scaleio/sdc/bin/drv_cfg --query_vols --file /etc/emc/scaleio/drv_cfg.txt"))).thenReturn(new Pair<>("", null)); + Pair result = scaleIOStorageAdaptor.unprepareStorageClient(poolUuid, details); diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ScaleIOStoragePoolTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ScaleIOStoragePoolTest.java index ba6f9ce4b0a8..7bf3e95314ed 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ScaleIOStoragePoolTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ScaleIOStoragePoolTest.java @@ -96,7 +96,7 @@ public void testSdcIdAttribute() { try (MockedStatic