From 259a88f07fa4734b2572546333f60a8d2f722ca4 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Thu, 18 Jun 2026 15:19:30 +0530 Subject: [PATCH 1/3] Multi-Host support for NFS3 and iSCSI --- .../driver/OntapPrimaryDatastoreDriver.java | 99 +++++++--- .../storage/feign/model/ExportRule.java | 14 +- .../storage/listener/OntapHostListener.java | 180 ++++++++++++++---- .../storage/service/StorageStrategy.java | 20 +- .../storage/service/UnifiedNASStrategy.java | 140 ++++++++++++-- .../storage/service/model/AccessGroup.java | 19 +- .../storage/service/StorageStrategyTest.java | 33 ++-- 7 files changed, 395 insertions(+), 110 deletions(-) diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java index ece29f7cd0ac..4ba52a0c2694 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java @@ -18,26 +18,13 @@ */ package org.apache.cloudstack.storage.driver; -import org.apache.cloudstack.storage.utils.OntapStorageConstants; -import com.cloud.agent.api.Answer; -import com.cloud.agent.api.to.DataObjectType; -import com.cloud.agent.api.to.DataStoreTO; -import com.cloud.agent.api.to.DataTO; -import com.cloud.exception.InvalidParameterValueException; -import com.cloud.host.Host; -import com.cloud.host.HostVO; -import com.cloud.storage.Storage; -import com.cloud.storage.StoragePool; -import com.cloud.storage.Volume; -import com.cloud.storage.VolumeDetailVO; -import com.cloud.storage.VolumeVO; -import com.cloud.storage.ScopeType; -import com.cloud.storage.dao.SnapshotDetailsDao; -import com.cloud.storage.dao.SnapshotDetailsVO; -import com.cloud.storage.dao.VolumeDao; -import com.cloud.storage.dao.VolumeDetailsDao; -import com.cloud.utils.Pair; -import com.cloud.utils.exception.CloudRuntimeException; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import javax.inject.Inject; + import org.apache.cloudstack.engine.subsystem.api.storage.ChapInfo; import org.apache.cloudstack.engine.subsystem.api.storage.CopyCommandResult; import org.apache.cloudstack.engine.subsystem.api.storage.CreateCmdResult; @@ -66,17 +53,36 @@ import org.apache.cloudstack.storage.service.model.CloudStackVolume; import org.apache.cloudstack.storage.service.model.ProtocolType; import org.apache.cloudstack.storage.to.SnapshotObjectTO; +import org.apache.cloudstack.storage.utils.OntapStorageConstants; import org.apache.cloudstack.storage.utils.OntapStorageUtils; import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.Nullable; -import javax.inject.Inject; -import java.util.ArrayList; -import java.util.HashMap; -import java.util.List; -import java.util.Map; +import com.cloud.agent.api.Answer; +import com.cloud.agent.api.to.DataObjectType; +import com.cloud.agent.api.to.DataStoreTO; +import com.cloud.agent.api.to.DataTO; +import com.cloud.exception.InvalidParameterValueException; +import com.cloud.exception.StorageConflictException; +import com.cloud.exception.StorageUnavailableException; +import com.cloud.host.Host; +import com.cloud.host.HostVO; +import com.cloud.storage.ScopeType; +import com.cloud.storage.Storage; +import com.cloud.storage.StorageManager; +import com.cloud.storage.StoragePool; +import com.cloud.storage.StoragePoolHostVO; +import com.cloud.storage.Volume; +import com.cloud.storage.VolumeDetailVO; +import com.cloud.storage.VolumeVO; +import com.cloud.storage.dao.SnapshotDetailsDao; +import com.cloud.storage.dao.SnapshotDetailsVO; +import com.cloud.storage.dao.VolumeDao; +import com.cloud.storage.dao.VolumeDetailsDao; +import com.cloud.utils.Pair; +import com.cloud.utils.exception.CloudRuntimeException; /** * Primary datastore driver for NetApp ONTAP storage systems. @@ -91,6 +97,7 @@ public class OntapPrimaryDatastoreDriver implements PrimaryDataStoreDriver { @Inject private VolumeDao volumeDao; @Inject private VolumeDetailsDao volumeDetailsDao; @Inject private SnapshotDetailsDao snapshotDetailsDao; + @Inject private StorageManager storageManager; @Override public Map getCapabilities() { @@ -392,8 +399,9 @@ public boolean grantAccess(DataObject dataObject, Host host, DataStore dataStore // Only retrieve LUN name for iSCSI volumes grantAccessIscsi(host, volumeVO, details, svmName, storagePool); } else if (ProtocolType.NFS3.name().equalsIgnoreCase(details.get(OntapStorageConstants.PROTOCOL))) { - // For NFS, no access grant needed - file is accessible via mount - logger.debug("grantAccess: NFS volume [{}], no igroup mapping required", volumeVO.getUuid()); + // For NFS, ensure export policy has host rule and host is connected to pool. + ensureNfsHostAccess(host, storagePool, details); + logger.debug("grantAccess: NFS volume [{}], ensured host access to storage pool [{}]", volumeVO.getUuid(), storagePool.getId()); return true; } volumeVO.setPoolType(storagePool.getPoolType()); @@ -410,6 +418,43 @@ public boolean grantAccess(DataObject dataObject, Host host, DataStore dataStore } } + private void ensureNfsHostAccess(Host host, StoragePoolVO storagePool, Map details) { + boolean hostConnectedToPool = false; + List connectedPools = storageManager.findStoragePoolsConnectedToHost(host.getId()); + if (connectedPools != null) { + for (StoragePoolHostVO connectedPoolRef : connectedPools) { + if (connectedPoolRef.getPoolId() == storagePool.getId()) { + hostConnectedToPool = true; + break; + } + } + } + + if (!hostConnectedToPool) { + try { + logger.info("grantAccess: Host [{}] is not connected to NFS pool [{}], reconnecting host to pool", host.getId(), storagePool.getId()); + boolean connected = storageManager.connectHostToSharedPool(host, storagePool.getId()); + if (!connected) { + throw new CloudRuntimeException("Failed to connect host " + host.getId() + " to NFS pool " + storagePool.getId()); + } + } catch (StorageUnavailableException | StorageConflictException e) { + throw new CloudRuntimeException("Unable to connect host " + host.getId() + " to NFS pool " + storagePool.getId(), e); + } + return; + } + + if (!(host instanceof HostVO)) { + throw new CloudRuntimeException("Host object is not of type HostVO for host id: " + host.getId()); + } + + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(storagePool.getId()); + accessGroup.setHostsToConnect(List.of((HostVO) host)); + + StorageStrategy strategy = OntapStorageUtils.getStrategyByStoragePoolDetails(details); + strategy.updateAccessGroup(accessGroup); + } + private void grantAccessIscsi(Host host, VolumeVO volumeVO, Map details, String svmName, StoragePoolVO storagePool) { String cloudStackVolumeName = volumeDetailsDao.findDetail(volumeVO.getId(), OntapStorageConstants.LUN_DOT_NAME).getValue(); UnifiedSANStrategy sanStrategy = (UnifiedSANStrategy) OntapStorageUtils.getStrategyByStoragePoolDetails(details); diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/ExportRule.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/ExportRule.java index 087e9aa681b4..c515fe60e9aa 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/ExportRule.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/ExportRule.java @@ -19,10 +19,13 @@ package org.apache.cloudstack.storage.feign.model; +import java.util.List; + +import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.annotation.JsonProperty; -import java.util.List; +import com.fasterxml.jackson.annotation.JsonValue; /** * ExportRule @@ -54,6 +57,7 @@ public enum ProtocolsEnum { this.value = value; } + @JsonValue public String getValue() { return value; } @@ -63,13 +67,17 @@ public String toString() { return String.valueOf(value); } + @JsonCreator public static ProtocolsEnum fromValue(String text) { + if (text == null) { + return null; + } for (ProtocolsEnum b : ProtocolsEnum.values()) { - if (String.valueOf(b.value).equals(text)) { + if (String.valueOf(b.value).equalsIgnoreCase(text) || b.name().equalsIgnoreCase(text)) { return b; } } - return null; + throw new IllegalArgumentException("Unexpected protocol value '" + text + "'"); } } diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java index ecdd3efd2c5c..ca1d3d1a01f4 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java @@ -19,30 +19,38 @@ package org.apache.cloudstack.storage.listener; +import java.util.List; +import java.util.Map; + import javax.inject.Inject; -import com.cloud.agent.api.ModifyStoragePoolCommand; +import org.apache.cloudstack.engine.subsystem.api.storage.HypervisorHostListener; +import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; +import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao; +import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; +import org.apache.cloudstack.storage.service.StorageStrategy; +import org.apache.cloudstack.storage.service.model.AccessGroup; +import org.apache.cloudstack.storage.service.model.ProtocolType; +import org.apache.cloudstack.storage.utils.OntapStorageConstants; +import org.apache.cloudstack.storage.utils.OntapStorageUtils; +import org.apache.commons.lang3.StringUtils; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +import com.cloud.agent.AgentManager; +import com.cloud.agent.api.Answer; import com.cloud.agent.api.ModifyStoragePoolAnswer; +import com.cloud.agent.api.ModifyStoragePoolCommand; import com.cloud.agent.api.StoragePoolInfo; import com.cloud.alert.AlertManager; +import com.cloud.host.Host; +import com.cloud.host.HostVO; +import com.cloud.host.dao.HostDao; import com.cloud.hypervisor.Hypervisor; +import com.cloud.storage.StoragePool; import com.cloud.storage.StoragePoolHostVO; import com.cloud.storage.dao.StoragePoolHostDao; -import org.apache.logging.log4j.Logger; -import org.apache.logging.log4j.LogManager; -import com.cloud.agent.AgentManager; -import com.cloud.agent.api.Answer; -import com.cloud.agent.api.DeleteStoragePoolCommand; -import com.cloud.host.Host; -import com.cloud.storage.StoragePool; import com.cloud.utils.exception.CloudRuntimeException; -import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; -import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; -import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao; -import org.apache.cloudstack.engine.subsystem.api.storage.HypervisorHostListener; -import com.cloud.host.dao.HostDao; - -import java.util.Map; public class OntapHostListener implements HypervisorHostListener { protected Logger logger = LogManager.getLogger(getClass()); @@ -83,6 +91,19 @@ public boolean hostConnect(long hostId, long poolId) { try { // Load storage pool details from database to pass mount options and other config to agent Map detailsMap = _storagePoolDetailsDao.listDetailsKeyPairs(poolId); + if (detailsMap == null || detailsMap.isEmpty()) { + logger.error("Failed to load storage pool details for pool id: {}", poolId); + return false; + } + + if (detailsMap.get(OntapStorageConstants.PROTOCOL) == null) { + logger.error("Storage pool details missing required protocol type for pool id: {}", poolId); + return false; + } + + // Update NFS export policy for this connected host when the pool protocol is NFS3. + updateNfsExportPolicyForConnectedHostIfNeeded(poolId, hostId, host, detailsMap); + // Create the ModifyStoragePoolCommand to send to the agent // Note: Always send command even if database entry exists, because agent may have restarted // and lost in-memory pool registration. The command handler is idempotent. @@ -151,43 +172,85 @@ public boolean hostConnect(long hostId, long poolId) { return true; } + private void updateNfsExportPolicyForConnectedHostIfNeeded(long poolId, long hostId, Host host, Map detailsMap) { + if (!ProtocolType.NFS3.name().equalsIgnoreCase(detailsMap.get(OntapStorageConstants.PROTOCOL))) { + return; + } + + if (host == null) { + throw new CloudRuntimeException("Host was not found with id: " + hostId); + } + + if (!(host instanceof HostVO)) { + throw new CloudRuntimeException("Host object is not of type HostVO for hostId: " + hostId); + } + + HostVO hostVO = (HostVO) host; + if (!isNfs3EnabledOnHost(hostVO)) { + throw new CloudRuntimeException("NFS protocol is not enabled on host with id: " + hostId); + } + + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(poolId); + accessGroup.setHostsToConnect(List.of(hostVO)); + + StorageStrategy strategy = OntapStorageUtils.getStrategyByStoragePoolDetails(detailsMap); + strategy.updateAccessGroup(accessGroup); + logger.info("Updated NFS export policy rules for host {} on storage pool {}", host.getName(), poolId); + } + + private boolean isNfs3EnabledOnHost(HostVO host) { + if (host == null) { + return false; + } + + String storageIp = host.getStorageIpAddress() != null ? host.getStorageIpAddress().trim() : ""; + if (storageIp.isEmpty() && StringUtils.isBlank(host.getPrivateIpAddress())) { + logger.warn("Host {} is not eligible for NFS3 protocol: both storage IP and private IP are empty", + host.getId()); + return false; + } + + return true; + } + @Override public boolean hostDisconnected(long hostId, long poolId) { logger.info("Disconnect from host " + hostId + " from pool " + poolId); + // Note: This is not currently being called for NetApp ONTAP storage plugin. + return false; + } + + @Override + public boolean hostAboutToBeRemoved(long hostId) { + logger.info("Host {} is about to be removed", hostId); - Host hostToremove = _hostDao.findById(hostId); - if (hostToremove == null) { - logger.error("Failed to add host by HostListener as host was not found with id : {}", hostId); + Host host = _hostDao.findById(hostId); + if (host == null) { + logger.warn("Host not found with id: {}", hostId); return false; } - StoragePool pool = _storagePoolDao.findById(poolId); - if (pool == null) { - logger.error("Failed to disconnect host - storage pool not found with id: {}", poolId); + if (!host.getHypervisorType().equals(Hypervisor.HypervisorType.KVM)) { + logger.debug("ONTAP plugin does not support {} type host, skipping cleanup", host.getHypervisorType()); return false; } - logger.info("Disconnecting host {} from ONTAP storage pool {}", hostToremove.getName(), pool.getName()); - try { - DeleteStoragePoolCommand cmd = new DeleteStoragePoolCommand(pool); - Answer answer = _agentMgr.easySend(hostId, cmd); - if (answer != null && answer.getResult()) { - logger.info("Successfully disconnected host {} from ONTAP storage pool {}", hostToremove.getName(), pool.getName()); - return true; - } else { - String errMsg = (answer != null) ? answer.getDetails() : "Unknown error"; - logger.warn("Failed to disconnect host {} from storage pool {}. Error: {}", hostToremove.getName(), pool.getName(), errMsg); - return false; + List poolHostRefs = storagePoolHostDao.listByHostId(hostId); + if (poolHostRefs == null || poolHostRefs.isEmpty()) { + logger.debug("No storage pool associations found for host {}", hostId); + return true; + } + + for (StoragePoolHostVO ref : poolHostRefs) { + StoragePoolVO pool = _storagePoolDao.findById(ref.getPoolId()); + if (pool != null) { + removeHostFromOntapPoolIfNeeded(hostId, pool, host); } - } catch (Exception e) { - logger.error("Exception while disconnecting host {} from storage pool {}", hostToremove.getName(), pool.getName(), e); - return false; } - } - @Override - public boolean hostAboutToBeRemoved(long hostId) { - return false; + logger.info("Cleaned up ONTAP export policies for host {} about to be removed", hostId); + return true; } @Override @@ -195,6 +258,45 @@ public boolean hostRemoved(long hostId, long clusterId) { return false; } + private void removeHostFromOntapPoolIfNeeded(long hostId, StoragePoolVO pool, Host host) { + try { + Map detailsMap = _storagePoolDetailsDao.listDetailsKeyPairs(pool.getId()); + if (detailsMap == null || detailsMap.isEmpty()) { + logger.debug("No pool details found for pool id: {}", pool.getId()); + return; + } + + // Skip non-NFS3 pools + if (!ProtocolType.NFS3.name().equalsIgnoreCase(detailsMap.get(OntapStorageConstants.PROTOCOL))) { + return; + } + + if (!(host instanceof HostVO)) { + logger.warn("Host object is not of type HostVO for hostId: {}", hostId); + return; + } + + logger.info("Removing export policy rule for host {} from storage pool {}", host.getName(), pool.getName()); + HostVO hostVO = (HostVO) host; + if (!isNfs3EnabledOnHost(hostVO)) { + logger.warn("Skipping NFS export policy removal for host {} on pool {} as host is not NFS-enabled", + hostId, pool.getId()); + return; + } + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(pool.getId()); + accessGroup.setHostsToConnect(List.of(hostVO)); + accessGroup.setHostRuleAction(AccessGroup.HostRuleAction.REMOVE); + + StorageStrategy strategy = OntapStorageUtils.getStrategyByStoragePoolDetails(detailsMap); + strategy.updateAccessGroup(accessGroup); + logger.info("Removed NFS export policy rules for removed host {} from storage pool {}", host.getName(), pool.getName()); + } catch (Exception e) { + logger.warn("Failed to remove NFS export policy rule for host {} from pool {}: {}", hostId, pool.getName(), e.getMessage()); + // Continue processing other pools even if one fails + } + } + @Override public boolean hostEnabled(long hostId) { return false; diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java index f4ab806d6885..d1a38e3eddf5 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java @@ -19,13 +19,17 @@ package org.apache.cloudstack.storage.service; -import com.cloud.utils.exception.CloudRuntimeException; -import feign.FeignException; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; + import org.apache.cloudstack.storage.feign.FeignClientFactory; import org.apache.cloudstack.storage.feign.client.AggregateFeignClient; import org.apache.cloudstack.storage.feign.client.JobFeignClient; -import org.apache.cloudstack.storage.feign.client.NetworkFeignClient; import org.apache.cloudstack.storage.feign.client.NASFeignClient; +import org.apache.cloudstack.storage.feign.client.NetworkFeignClient; import org.apache.cloudstack.storage.feign.client.SANFeignClient; import org.apache.cloudstack.storage.feign.client.SnapshotFeignClient; import org.apache.cloudstack.storage.feign.client.SvmFeignClient; @@ -48,11 +52,9 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import java.util.HashMap; -import java.util.ArrayList; -import java.util.List; -import java.util.Map; -import java.util.Objects; +import com.cloud.utils.exception.CloudRuntimeException; + +import feign.FeignException; /** * Storage Strategy represents the communication path for all the ONTAP storage options @@ -590,7 +592,7 @@ public abstract JobResponse revertSnapshotForCloudStackVolume(String snapshotNam * @param accessGroup the access group to update * @return the updated AccessGroup object */ - abstract AccessGroup updateAccessGroup(AccessGroup accessGroup); + public abstract AccessGroup updateAccessGroup(AccessGroup accessGroup); /** * Method encapsulates the behavior based on the opted protocol in subclasses diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java index 477e92630387..1bd82fa0eabd 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java @@ -19,19 +19,21 @@ package org.apache.cloudstack.storage.service; -import com.cloud.agent.api.Answer; -import com.cloud.host.HostVO; -import com.cloud.storage.Storage; -import com.cloud.storage.VolumeVO; -import com.cloud.storage.dao.VolumeDao; -import com.cloud.utils.exception.CloudRuntimeException; -import feign.FeignException; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; + +import javax.inject.Inject; + import org.apache.cloudstack.engine.subsystem.api.storage.DataObject; import org.apache.cloudstack.engine.subsystem.api.storage.EndPoint; import org.apache.cloudstack.engine.subsystem.api.storage.EndPointSelector; import org.apache.cloudstack.storage.command.CreateObjectCommand; import org.apache.cloudstack.storage.command.DeleteCommand; import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao; +import org.apache.cloudstack.storage.feign.model.CliSnapshotRestoreRequest; import org.apache.cloudstack.storage.feign.model.ExportPolicy; import org.apache.cloudstack.storage.feign.model.ExportRule; import org.apache.cloudstack.storage.feign.model.FileInfo; @@ -42,19 +44,22 @@ import org.apache.cloudstack.storage.feign.model.Volume; import org.apache.cloudstack.storage.feign.model.response.JobResponse; import org.apache.cloudstack.storage.feign.model.response.OntapResponse; -import org.apache.cloudstack.storage.feign.model.CliSnapshotRestoreRequest; import org.apache.cloudstack.storage.service.model.AccessGroup; import org.apache.cloudstack.storage.service.model.CloudStackVolume; -import org.apache.cloudstack.storage.volume.VolumeObject; import org.apache.cloudstack.storage.utils.OntapStorageConstants; import org.apache.cloudstack.storage.utils.OntapStorageUtils; +import org.apache.cloudstack.storage.volume.VolumeObject; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import javax.inject.Inject; -import java.util.ArrayList; -import java.util.List; -import java.util.Map; +import com.cloud.agent.api.Answer; +import com.cloud.host.HostVO; +import com.cloud.storage.Storage; +import com.cloud.storage.VolumeVO; +import com.cloud.storage.dao.VolumeDao; +import com.cloud.utils.exception.CloudRuntimeException; + +import feign.FeignException; public class UnifiedNASStrategy extends NASStrategy { private static final Logger logger = LogManager.getLogger(UnifiedNASStrategy.class); @@ -193,7 +198,114 @@ public void deleteAccessGroup(AccessGroup accessGroup) { @Override public AccessGroup updateAccessGroup(AccessGroup accessGroup) { - return null; + if (accessGroup == null) { + throw new CloudRuntimeException("Invalid accessGroup object - accessGroup is null"); + } + if (accessGroup.getStoragePoolId() == null) { + throw new CloudRuntimeException("Invalid accessGroup object - storagePoolId is null"); + } + if (accessGroup.getHostsToConnect() == null || accessGroup.getHostsToConnect().isEmpty()) { + throw new CloudRuntimeException("Invalid accessGroup object - hostsToConnect is null or empty"); + } + + Map details = storagePoolDetailsDao.listDetailsKeyPairs(accessGroup.getStoragePoolId()); + String exportPolicyId = details.get(OntapStorageConstants.EXPORT_POLICY_ID); + + // No policy exists yet (e.g. pool was created before any eligible hosts came up), create it now. + if (exportPolicyId == null || exportPolicyId.isEmpty()) { + logger.info("updateAccessGroup: No export policy found for pool {}. Creating one now.", accessGroup.getStoragePoolId()); + return createAccessGroup(accessGroup); + } + + try { + String authHeader = OntapStorageUtils.generateAuthHeader(storage.getUsername(), storage.getPassword()); + ExportPolicy existingPolicy = nasFeignClient.getExportPolicyById(authHeader, exportPolicyId); + if (existingPolicy == null) { + throw new CloudRuntimeException("Failed to fetch existing export policy with id: " + exportPolicyId); + } + + List rules = existingPolicy.getRules(); + if (rules == null || rules.isEmpty()) { + rules = new ArrayList<>(); + ExportRule newRule = new ExportRule(); + newRule.setProtocols(List.of(ExportRule.ProtocolsEnum.NFS3)); + newRule.setRoRule(List.of("sys")); + newRule.setRwRule(List.of("sys")); + newRule.setSuperuser(List.of("sys")); + newRule.setClients(new ArrayList<>()); + rules.add(newRule); + } + + ExportRule exportRule = rules.get(0); + List exportClients = exportRule.getClients(); + if (exportClients == null) { + exportClients = new ArrayList<>(); + exportRule.setClients(exportClients); + } + + Set hostMatches = new HashSet<>(); + for (HostVO host : accessGroup.getHostsToConnect()) { + String hostStorageIp = host.getStorageIpAddress(); + String ip = (hostStorageIp != null && !hostStorageIp.isEmpty()) ? hostStorageIp : host.getPrivateIpAddress(); + if (ip == null || ip.isEmpty()) { + logger.warn("updateAccessGroup: Host {} has no storage/private IP, skipping export rule update", host.getName()); + continue; + } + hostMatches.add(ip + "/32"); + } + + if (hostMatches.isEmpty()) { + accessGroup.setPolicy(existingPolicy); + return accessGroup; + } + + boolean updated = false; + if (AccessGroup.HostRuleAction.REMOVE.equals(accessGroup.getHostRuleAction())) { + updated = exportClients.removeIf(c -> c != null && c.getMatch() != null && hostMatches.contains(c.getMatch())); + if (!updated) { + logger.info("updateAccessGroup: No matching host IPs found in export policy {} for removal", existingPolicy.getName()); + } + } else { + Set existingMatches = new HashSet<>(); + for (ExportRule.ExportClient exportClient : exportClients) { + if (exportClient != null && exportClient.getMatch() != null) { + existingMatches.add(exportClient.getMatch()); + } + } + + for (String match : hostMatches) { + if (existingMatches.add(match)) { + ExportRule.ExportClient exportClient = new ExportRule.ExportClient(); + exportClient.setMatch(match); + exportClients.add(exportClient); + updated = true; + } + } + } + + if (!updated) { + if (!AccessGroup.HostRuleAction.REMOVE.equals(accessGroup.getHostRuleAction())) { + logger.info("updateAccessGroup: No new host IPs to add to export policy {}", existingPolicy.getName()); + } + } + + if (!updated) { + accessGroup.setPolicy(existingPolicy); + return accessGroup; + } + + ExportPolicy updateRequest = new ExportPolicy(); + updateRequest.setRules(rules); + nasFeignClient.updateExportPolicy(authHeader, exportPolicyId, updateRequest); + + existingPolicy.setRules(rules); + accessGroup.setPolicy(existingPolicy); + logger.info("updateAccessGroup: Successfully updated export policy {} with new host client rules", existingPolicy.getName()); + return accessGroup; + } catch (Exception e) { + logger.error("updateAccessGroup: Failed to update export policy for pool {}", accessGroup.getStoragePoolId(), e); + throw new CloudRuntimeException("Failed to update export policy for NFS host connection: " + e.getMessage(), e); + } } @Override diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/model/AccessGroup.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/model/AccessGroup.java index 9815724fc1aa..8b1aa24fe5d9 100755 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/model/AccessGroup.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/model/AccessGroup.java @@ -19,21 +19,28 @@ package org.apache.cloudstack.storage.service.model; -import com.cloud.host.HostVO; +import java.util.List; + import org.apache.cloudstack.engine.subsystem.api.storage.Scope; import org.apache.cloudstack.storage.feign.model.ExportPolicy; import org.apache.cloudstack.storage.feign.model.Igroup; -import java.util.List; +import com.cloud.host.HostVO; public class AccessGroup { + public enum HostRuleAction { + ADD, + REMOVE + } + private Igroup igroup; private ExportPolicy exportPolicy; private List hostsToConnect; private Long storagePoolId; private Scope scope; + private HostRuleAction hostRuleAction = HostRuleAction.ADD; public Igroup getIgroup() { @@ -74,4 +81,12 @@ public Scope getScope() { public void setScope(Scope scope) { this.scope = scope; } + + public HostRuleAction getHostRuleAction() { + return hostRuleAction; + } + + public void setHostRuleAction(HostRuleAction hostRuleAction) { + this.hostRuleAction = hostRuleAction; + } } diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java index df9afe2542f9..d04d14240b02 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java @@ -18,8 +18,11 @@ */ package org.apache.cloudstack.storage.service; -import com.cloud.utils.exception.CloudRuntimeException; -import feign.FeignException; +import java.lang.reflect.Field; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + import org.apache.cloudstack.storage.feign.client.AggregateFeignClient; import org.apache.cloudstack.storage.feign.client.JobFeignClient; import org.apache.cloudstack.storage.feign.client.NetworkFeignClient; @@ -39,32 +42,30 @@ import org.apache.cloudstack.storage.service.model.CloudStackVolume; import org.apache.cloudstack.storage.service.model.ProtocolType; import org.apache.cloudstack.storage.utils.OntapStorageConstants; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.Mock; -import org.mockito.junit.jupiter.MockitoExtension; -import org.mockito.junit.jupiter.MockitoSettings; -import org.mockito.quality.Strictness; - -import java.lang.reflect.Field; -import java.util.ArrayList; -import java.util.List; -import java.util.Map; - import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyMap; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; +import org.mockito.Mock; import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import org.mockito.junit.jupiter.MockitoExtension; +import org.mockito.junit.jupiter.MockitoSettings; +import org.mockito.quality.Strictness; + +import com.cloud.utils.exception.CloudRuntimeException; + +import feign.FeignException; @ExtendWith(MockitoExtension.class) @MockitoSettings(strictness = Strictness.LENIENT) @@ -158,7 +159,7 @@ public void deleteAccessGroup(AccessGroup accessGroup) { } @Override - AccessGroup updateAccessGroup(AccessGroup accessGroup) { + public AccessGroup updateAccessGroup(AccessGroup accessGroup) { return null; } From 6c24f0e7b49dac1fcaaf1c453c921aeb28e9096c Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Mon, 29 Jun 2026 22:55:39 +0530 Subject: [PATCH 2/3] Addressed comments --- .../driver/OntapPrimaryDatastoreDriver.java | 45 +----------- .../storage/service/UnifiedNASStrategy.java | 69 +++++++++++++------ 2 files changed, 51 insertions(+), 63 deletions(-) diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java index 4ba52a0c2694..f9c055cbcf40 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java @@ -65,15 +65,12 @@ import com.cloud.agent.api.to.DataStoreTO; import com.cloud.agent.api.to.DataTO; import com.cloud.exception.InvalidParameterValueException; -import com.cloud.exception.StorageConflictException; -import com.cloud.exception.StorageUnavailableException; import com.cloud.host.Host; import com.cloud.host.HostVO; import com.cloud.storage.ScopeType; import com.cloud.storage.Storage; import com.cloud.storage.StorageManager; import com.cloud.storage.StoragePool; -import com.cloud.storage.StoragePoolHostVO; import com.cloud.storage.Volume; import com.cloud.storage.VolumeDetailVO; import com.cloud.storage.VolumeVO; @@ -399,9 +396,8 @@ public boolean grantAccess(DataObject dataObject, Host host, DataStore dataStore // Only retrieve LUN name for iSCSI volumes grantAccessIscsi(host, volumeVO, details, svmName, storagePool); } else if (ProtocolType.NFS3.name().equalsIgnoreCase(details.get(OntapStorageConstants.PROTOCOL))) { - // For NFS, ensure export policy has host rule and host is connected to pool. - ensureNfsHostAccess(host, storagePool, details); - logger.debug("grantAccess: NFS volume [{}], ensured host access to storage pool [{}]", volumeVO.getUuid(), storagePool.getId()); + // For NFS, no access grant needed - file is accessible via mount + logger.debug("grantAccess: NFS volume [{}], no igroup mapping required", volumeVO.getUuid()); return true; } volumeVO.setPoolType(storagePool.getPoolType()); @@ -418,43 +414,6 @@ public boolean grantAccess(DataObject dataObject, Host host, DataStore dataStore } } - private void ensureNfsHostAccess(Host host, StoragePoolVO storagePool, Map details) { - boolean hostConnectedToPool = false; - List connectedPools = storageManager.findStoragePoolsConnectedToHost(host.getId()); - if (connectedPools != null) { - for (StoragePoolHostVO connectedPoolRef : connectedPools) { - if (connectedPoolRef.getPoolId() == storagePool.getId()) { - hostConnectedToPool = true; - break; - } - } - } - - if (!hostConnectedToPool) { - try { - logger.info("grantAccess: Host [{}] is not connected to NFS pool [{}], reconnecting host to pool", host.getId(), storagePool.getId()); - boolean connected = storageManager.connectHostToSharedPool(host, storagePool.getId()); - if (!connected) { - throw new CloudRuntimeException("Failed to connect host " + host.getId() + " to NFS pool " + storagePool.getId()); - } - } catch (StorageUnavailableException | StorageConflictException e) { - throw new CloudRuntimeException("Unable to connect host " + host.getId() + " to NFS pool " + storagePool.getId(), e); - } - return; - } - - if (!(host instanceof HostVO)) { - throw new CloudRuntimeException("Host object is not of type HostVO for host id: " + host.getId()); - } - - AccessGroup accessGroup = new AccessGroup(); - accessGroup.setStoragePoolId(storagePool.getId()); - accessGroup.setHostsToConnect(List.of((HostVO) host)); - - StorageStrategy strategy = OntapStorageUtils.getStrategyByStoragePoolDetails(details); - strategy.updateAccessGroup(accessGroup); - } - private void grantAccessIscsi(Host host, VolumeVO volumeVO, Map details, String svmName, StoragePoolVO storagePool) { String cloudStackVolumeName = volumeDetailsDao.findDetail(volumeVO.getId(), OntapStorageConstants.LUN_DOT_NAME).getValue(); UnifiedSANStrategy sanStrategy = (UnifiedSANStrategy) OntapStorageUtils.getStrategyByStoragePoolDetails(details); diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java index 1bd82fa0eabd..ebbe4cb9459b 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java @@ -201,52 +201,59 @@ public AccessGroup updateAccessGroup(AccessGroup accessGroup) { if (accessGroup == null) { throw new CloudRuntimeException("Invalid accessGroup object - accessGroup is null"); } + // Check if an AccessGroup was constructed without associating it to a storage pool. if (accessGroup.getStoragePoolId() == null) { throw new CloudRuntimeException("Invalid accessGroup object - storagePoolId is null"); } + // At least one host is required regardless of ADD or REMOVE action. + // An empty list means there is nothing to add to or remove from the export policy client list. if (accessGroup.getHostsToConnect() == null || accessGroup.getHostsToConnect().isEmpty()) { throw new CloudRuntimeException("Invalid accessGroup object - hostsToConnect is null or empty"); } Map details = storagePoolDetailsDao.listDetailsKeyPairs(accessGroup.getStoragePoolId()); + if (details == null || details.isEmpty()) { + throw new CloudRuntimeException("No storage pool details found for storagePoolId: " + accessGroup.getStoragePoolId()); + } String exportPolicyId = details.get(OntapStorageConstants.EXPORT_POLICY_ID); - - // No policy exists yet (e.g. pool was created before any eligible hosts came up), create it now. if (exportPolicyId == null || exportPolicyId.isEmpty()) { - logger.info("updateAccessGroup: No export policy found for pool {}. Creating one now.", accessGroup.getStoragePoolId()); - return createAccessGroup(accessGroup); + throw new CloudRuntimeException("No export policy found for storagePoolId: " + accessGroup.getStoragePoolId()); } + try { String authHeader = OntapStorageUtils.generateAuthHeader(storage.getUsername(), storage.getPassword()); ExportPolicy existingPolicy = nasFeignClient.getExportPolicyById(authHeader, exportPolicyId); + // Check if the export policy was deleted externally on ONTAP or the stored ID is stale. if (existingPolicy == null) { throw new CloudRuntimeException("Failed to fetch existing export policy with id: " + exportPolicyId); } List rules = existingPolicy.getRules(); + // An existing export policy should always have at least one rule. A null or empty rules list + // indicates the policy was corrupted or modified externally on ONTAP and is in an unexpected + // state. updateAccessGroup only modifies an existing policy — it does not create rules from scratch. if (rules == null || rules.isEmpty()) { - rules = new ArrayList<>(); - ExportRule newRule = new ExportRule(); - newRule.setProtocols(List.of(ExportRule.ProtocolsEnum.NFS3)); - newRule.setRoRule(List.of("sys")); - newRule.setRwRule(List.of("sys")); - newRule.setSuperuser(List.of("sys")); - newRule.setClients(new ArrayList<>()); - rules.add(newRule); + throw new CloudRuntimeException("Export policy " + existingPolicy.getName() + " has no rules. " + + "Cannot update an export policy with no existing rules."); } - ExportRule exportRule = rules.get(0); - List exportClients = exportRule.getClients(); - if (exportClients == null) { - exportClients = new ArrayList<>(); - exportRule.setClients(exportClients); + // This plugin creates a single NFS export rule per policy. More than one rule means the + // policy was changed outside the plugin and we no longer know which rule should be mutated. + if (rules.size() != 1) { + throw new CloudRuntimeException("Export policy " + existingPolicy.getName() + + " is expected to have exactly one rule but found: " + rules.size()); } + ExportRule targetRule = rules.get(0); + Set hostMatches = new HashSet<>(); for (HostVO host : accessGroup.getHostsToConnect()) { String hostStorageIp = host.getStorageIpAddress(); String ip = (hostStorageIp != null && !hostStorageIp.isEmpty()) ? hostStorageIp : host.getPrivateIpAddress(); + // Occurs when a CloudStack host has neither a storage IP nor a private IP configured + // (misconfigured or partially registered host). Skip it to avoid inserting a broken + // or empty match entry into the ONTAP export rule. if (ip == null || ip.isEmpty()) { logger.warn("updateAccessGroup: Host {} has no storage/private IP, skipping export rule update", host.getName()); continue; @@ -254,26 +261,45 @@ public AccessGroup updateAccessGroup(AccessGroup accessGroup) { hostMatches.add(ip + "/32"); } + // Occurs when every host in hostsToConnect had no valid IP (all were skipped above). + // There is nothing to add or remove, so skip the ONTAP API call and return early. if (hostMatches.isEmpty()) { accessGroup.setPolicy(existingPolicy); return accessGroup; } boolean updated = false; + // Differentiates between removing hosts (e.g., host decommissioned or removed from the cluster) + // and the default ADD path (e.g., new host being connected to the storage pool). + List exportClients = targetRule.getClients(); + // Existing rules can legitimately have no clients yet; treat that as an empty list. + if (exportClients == null) { + exportClients = new ArrayList<>(); + targetRule.setClients(exportClients); + } + if (AccessGroup.HostRuleAction.REMOVE.equals(accessGroup.getHostRuleAction())) { updated = exportClients.removeIf(c -> c != null && c.getMatch() != null && hostMatches.contains(c.getMatch())); + // None of the requested host IPs were present in the policy — log for diagnostics + // so operators can investigate whether the policy state is already correct or stale. if (!updated) { logger.info("updateAccessGroup: No matching host IPs found in export policy {} for removal", existingPolicy.getName()); } } else { Set existingMatches = new HashSet<>(); for (ExportRule.ExportClient exportClient : exportClients) { + // Skips null client entries or entries with a null match field that may have been + // inserted externally on ONTAP. Avoids polluting the dedup set with null values + // which would cause subsequent hosts to be incorrectly treated as duplicates. if (exportClient != null && exportClient.getMatch() != null) { existingMatches.add(exportClient.getMatch()); } } for (String match : hostMatches) { + // Set.add() returns false when the element was already present, acting as a dedup check. + // Prevents inserting a duplicate client match entry for a host that is already allowed + // in the export policy — ONTAP may reject or behave unpredictably with duplicate matches. if (existingMatches.add(match)) { ExportRule.ExportClient exportClient = new ExportRule.ExportClient(); exportClient.setMatch(match); @@ -283,13 +309,16 @@ public AccessGroup updateAccessGroup(AccessGroup accessGroup) { } } + // Occurs when the export policy is already in the desired state: + // ADD path — all provided host IPs were already present (all were duplicates). + // REMOVE path — none of the provided host IPs matched any existing entry. + // In both cases, skip the ONTAP PATCH call to avoid an unnecessary round-trip. if (!updated) { + // Only log the "nothing to add" message for the ADD path; the REMOVE no-op + // is already logged above in its own branch to avoid double-logging. if (!AccessGroup.HostRuleAction.REMOVE.equals(accessGroup.getHostRuleAction())) { logger.info("updateAccessGroup: No new host IPs to add to export policy {}", existingPolicy.getName()); } - } - - if (!updated) { accessGroup.setPolicy(existingPolicy); return accessGroup; } From 1f0e3c1decbe22936b97b71fbe81fee72139f298 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Tue, 30 Jun 2026 13:08:49 +0530 Subject: [PATCH 3/3] Reverted few unnecessary changes --- .../storage/driver/OntapPrimaryDatastoreDriver.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java index f9c055cbcf40..6bf682ccf90f 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java @@ -69,7 +69,6 @@ import com.cloud.host.HostVO; import com.cloud.storage.ScopeType; import com.cloud.storage.Storage; -import com.cloud.storage.StorageManager; import com.cloud.storage.StoragePool; import com.cloud.storage.Volume; import com.cloud.storage.VolumeDetailVO; @@ -94,7 +93,6 @@ public class OntapPrimaryDatastoreDriver implements PrimaryDataStoreDriver { @Inject private VolumeDao volumeDao; @Inject private VolumeDetailsDao volumeDetailsDao; @Inject private SnapshotDetailsDao snapshotDetailsDao; - @Inject private StorageManager storageManager; @Override public Map getCapabilities() { @@ -1046,4 +1044,4 @@ private void updateSnapshotDetails(long csSnapshotId, long csVolumeId, String fl } } -} +} \ No newline at end of file