From 8668b044f0e629c897702bae3962a36e3ea8ada7 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Thu, 18 Jun 2026 15:19:30 +0530 Subject: [PATCH 01/10] Multi-Host support for NFS3 and iSCSI --- .../driver/OntapPrimaryDatastoreDriver.java | 103 +++++++--- .../storage/feign/model/ExportRule.java | 14 +- .../storage/listener/OntapHostListener.java | 180 ++++++++++++++---- .../storage/service/StorageStrategy.java | 18 +- .../storage/service/UnifiedNASStrategy.java | 140 ++++++++++++-- .../storage/service/model/AccessGroup.java | 19 +- .../storage/service/StorageStrategyTest.java | 33 ++-- 7 files changed, 396 insertions(+), 111 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 5e7a80b1af7c..b9135a787d61 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,28 +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.SnapshotVO; -import com.cloud.storage.dao.SnapshotDao; -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; @@ -68,16 +53,37 @@ 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.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.SnapshotVO; +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.SnapshotDao; +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. @@ -93,6 +99,7 @@ public class OntapPrimaryDatastoreDriver implements PrimaryDataStoreDriver { @Inject private VolumeDetailsDao volumeDetailsDao; @Inject private SnapshotDetailsDao snapshotDetailsDao; @Inject private SnapshotDao snapshotDao; + @Inject private StorageManager storageManager; @Override public Map getCapabilities() { @@ -430,8 +437,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()); @@ -448,6 +456,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 d349305e5e43..69c17f1289cd 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,16 @@ package org.apache.cloudstack.storage.service; -import com.cloud.utils.exception.CloudRuntimeException; -import feign.FeignException; +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,10 +51,9 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import java.util.HashMap; -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 @@ -615,7 +617,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 198957ca5db8..1e6d12c8d288 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); @@ -191,7 +196,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 6f09c3649757..6091c45ffdad 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; @@ -40,33 +43,31 @@ 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.never; 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) @@ -165,7 +166,7 @@ public void deleteAccessGroup(AccessGroup accessGroup) { } @Override - AccessGroup updateAccessGroup(AccessGroup accessGroup) { + public AccessGroup updateAccessGroup(AccessGroup accessGroup) { return null; } From 891d1ea5c39ca2e5279fc2d50c766c5f0f489ad6 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Mon, 29 Jun 2026 22:55:39 +0530 Subject: [PATCH 02/10] 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 b9135a787d61..d022e0af68cb 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 @@ -64,8 +64,6 @@ 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; @@ -73,7 +71,6 @@ 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; @@ -437,9 +434,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()); @@ -456,43 +452,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 1e6d12c8d288..f48aa6def468 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 @@ -199,52 +199,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; @@ -252,26 +259,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); @@ -281,13 +307,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 d7651f37c950768b5cb211d3cd53d2800f64771e Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Wed, 8 Jul 2026 11:02:52 +0530 Subject: [PATCH 03/10] Addressed comments --- .../cloudstack/storage/listener/OntapHostListener.java | 5 ----- 1 file changed, 5 deletions(-) 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 ca1d3d1a01f4..336a5145637d 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 @@ -231,11 +231,6 @@ public boolean hostAboutToBeRemoved(long hostId) { return false; } - if (!host.getHypervisorType().equals(Hypervisor.HypervisorType.KVM)) { - logger.debug("ONTAP plugin does not support {} type host, skipping cleanup", host.getHypervisorType()); - return false; - } - List poolHostRefs = storagePoolHostDao.listByHostId(hostId); if (poolHostRefs == null || poolHostRefs.isEmpty()) { logger.debug("No storage pool associations found for host {}", hostId); From 47868b90779b4f76732d64fb10e06fe9323ffaa2 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Fri, 10 Jul 2026 12:58:37 +0530 Subject: [PATCH 04/10] Revert unwanted changes in file --- .../driver/OntapPrimaryDatastoreDriver.java | 45 ++++++++++++++++++- 1 file changed, 43 insertions(+), 2 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 d022e0af68cb..b9135a787d61 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 @@ -64,6 +64,8 @@ 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; @@ -71,6 +73,7 @@ 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; @@ -434,8 +437,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()); @@ -452,6 +456,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); From 30412d4b2bfe07b3dc3078211005f834c56d69c7 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Fri, 10 Jul 2026 13:01:02 +0530 Subject: [PATCH 05/10] Revert unwanted changes in file --- .../driver/OntapPrimaryDatastoreDriver.java | 105 +++++------------- 1 file changed, 30 insertions(+), 75 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 b9135a787d61..4d7d3df8f145 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,13 +18,28 @@ */ package org.apache.cloudstack.storage.driver; -import java.util.ArrayList; -import java.util.HashMap; -import java.util.List; -import java.util.Map; - -import javax.inject.Inject; - +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.SnapshotVO; +import com.cloud.storage.dao.SnapshotDao; +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 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; @@ -53,37 +68,16 @@ 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.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.Nullable; -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.SnapshotVO; -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.SnapshotDao; -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 javax.inject.Inject; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; /** * Primary datastore driver for NetApp ONTAP storage systems. @@ -99,7 +93,6 @@ public class OntapPrimaryDatastoreDriver implements PrimaryDataStoreDriver { @Inject private VolumeDetailsDao volumeDetailsDao; @Inject private SnapshotDetailsDao snapshotDetailsDao; @Inject private SnapshotDao snapshotDao; - @Inject private StorageManager storageManager; @Override public Map getCapabilities() { @@ -437,9 +430,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()); @@ -456,43 +448,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); @@ -1107,4 +1062,4 @@ private void updateSnapshotDetails(long csSnapshotId, long csVolumeId, String fl } } -} +} \ No newline at end of file From 26892e9a833b2a48ec44913ed989fe907a481bb1 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Mon, 13 Jul 2026 15:35:30 +0530 Subject: [PATCH 06/10] Fixed review comments --- .../driver/OntapPrimaryDatastoreDriver.java | 2 +- .../storage/feign/model/ExportRule.java | 2 +- .../storage/listener/OntapHostListener.java | 25 +- .../storage/service/UnifiedNASStrategy.java | 9 +- .../service/UnifiedNASStrategyTest.java | 365 +++++++++++++++++- 5 files changed, 378 insertions(+), 25 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 4d7d3df8f145..5e7a80b1af7c 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 @@ -1062,4 +1062,4 @@ private void updateSnapshotDetails(long csSnapshotId, long csVolumeId, String fl } } -} \ No newline at end of file +} 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 c515fe60e9aa..adece0cc6923 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 @@ -77,7 +77,7 @@ public static ProtocolsEnum fromValue(String text) { return b; } } - throw new IllegalArgumentException("Unexpected protocol value '" + text + "'"); + return null; } } 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 336a5145637d..bce96845f27c 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 @@ -181,25 +181,20 @@ private void updateNfsExportPolicyForConnectedHostIfNeeded(long poolId, long hos 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)) { + if (!isNfs3EnabledOnHost(host)) { 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)); + accessGroup.setHostsToConnect(List.of((HostVO) host)); 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) { + private boolean isNfs3EnabledOnHost(Host host) { if (host == null) { return false; } @@ -227,8 +222,8 @@ public boolean hostAboutToBeRemoved(long hostId) { Host host = _hostDao.findById(hostId); if (host == null) { - logger.warn("Host not found with id: {}", hostId); - return false; + logger.warn("Host not found with id: {}, considering it as no-op", hostId); + return true; } List poolHostRefs = storagePoolHostDao.listByHostId(hostId); @@ -266,21 +261,15 @@ private void removeHostFromOntapPoolIfNeeded(long hostId, StoragePoolVO pool, Ho 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)) { + if (!isNfs3EnabledOnHost(host)) { 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.setHostsToConnect(List.of((HostVO) host)); accessGroup.setHostRuleAction(AccessGroup.HostRuleAction.REMOVE); StorageStrategy strategy = OntapStorageUtils.getStrategyByStoragePoolDetails(detailsMap); 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 f48aa6def468..a39a0fa01d5f 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 @@ -247,8 +247,9 @@ public AccessGroup updateAccessGroup(AccessGroup accessGroup) { Set hostMatches = new HashSet<>(); for (HostVO host : accessGroup.getHostsToConnect()) { - String hostStorageIp = host.getStorageIpAddress(); - String ip = (hostStorageIp != null && !hostStorageIp.isEmpty()) ? hostStorageIp : host.getPrivateIpAddress(); + String hostStorageIp = host.getStorageIpAddress() != null ? host.getStorageIpAddress().trim() : null; + String ip = (hostStorageIp != null && !hostStorageIp.isEmpty()) ? hostStorageIp + : (host.getPrivateIpAddress() != null ? host.getPrivateIpAddress().trim() : null); // 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. @@ -448,10 +449,10 @@ private ExportPolicy createExportPolicyRequest(AccessGroup accessGroup,String sv List exportClients = new ArrayList<>(); List hosts = accessGroup.getHostsToConnect(); for (HostVO host : hosts) { - String hostStorageIp = host.getStorageIpAddress(); + String hostStorageIp = host.getStorageIpAddress() != null ? host.getStorageIpAddress().trim() : null; String ip = (hostStorageIp != null && !hostStorageIp.isEmpty()) ? hostStorageIp - : host.getPrivateIpAddress(); + : (host.getPrivateIpAddress() != null ? host.getPrivateIpAddress().trim() : null); String ipToUse = ip + "/32"; ExportRule.ExportClient exportClient = new ExportRule.ExportClient(); exportClient.setMatch(ipToUse); diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedNASStrategyTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedNASStrategyTest.java index c4d5ddf6878c..fd4d10882949 100755 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedNASStrategyTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedNASStrategyTest.java @@ -37,6 +37,7 @@ import org.apache.cloudstack.storage.feign.client.NetworkFeignClient; import org.apache.cloudstack.storage.feign.client.SANFeignClient; import org.apache.cloudstack.storage.feign.model.ExportPolicy; +import org.apache.cloudstack.storage.feign.model.ExportRule; import org.apache.cloudstack.storage.feign.model.Job; import org.apache.cloudstack.storage.feign.model.OntapStorage; import org.apache.cloudstack.storage.feign.model.response.JobResponse; @@ -63,6 +64,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyLong; @@ -72,6 +74,7 @@ import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -582,4 +585,364 @@ public void testDeleteCloudStackVolume_AnswerNull() throws Exception { strategy.deleteCloudStackVolume(cloudStackVolume); }); } -} + + // ------------------------------------------------------------------------- + // updateAccessGroup tests + // ------------------------------------------------------------------------- + + private Map detailsWithExportPolicyId() { + Map details = new HashMap<>(); + details.put(OntapStorageConstants.EXPORT_POLICY_ID, "policy-42"); + return details; + } + + private ExportPolicy existingPolicyWithClients(String... matchIps) { + ExportRule rule = new ExportRule(); + List clients = new ArrayList<>(); + for (String ip : matchIps) { + ExportRule.ExportClient client = new ExportRule.ExportClient(); + client.setMatch(ip); + clients.add(client); + } + rule.setClients(clients); + ExportPolicy policy = new ExportPolicy(); + policy.setName("test-policy"); + policy.setRules(new ArrayList<>(List.of(rule))); + return policy; + } + + // updateAccessGroup - null accessGroup + @Test + public void testUpdateAccessGroup_NullAccessGroup() { + assertThrows(CloudRuntimeException.class, () -> strategy.updateAccessGroup(null)); + } + + // updateAccessGroup - null storagePoolId + @Test + public void testUpdateAccessGroup_NullStoragePoolId() { + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setHostsToConnect(List.of(mock(HostVO.class))); + // storagePoolId is null by default + assertThrows(CloudRuntimeException.class, () -> strategy.updateAccessGroup(accessGroup)); + } + + // updateAccessGroup - null hostsToConnect + @Test + public void testUpdateAccessGroup_NullHostsToConnect() { + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + // hostsToConnect is null by default + assertThrows(CloudRuntimeException.class, () -> strategy.updateAccessGroup(accessGroup)); + } + + // updateAccessGroup - empty hostsToConnect + @Test + public void testUpdateAccessGroup_EmptyHostsToConnect() { + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(new ArrayList<>()); + assertThrows(CloudRuntimeException.class, () -> strategy.updateAccessGroup(accessGroup)); + } + + // updateAccessGroup - storagePoolDetailsDao returns null + @Test + public void testUpdateAccessGroup_NoStoragePoolDetails() { + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(mock(HostVO.class))); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(null); + assertThrows(CloudRuntimeException.class, () -> strategy.updateAccessGroup(accessGroup)); + } + + // updateAccessGroup - details missing EXPORT_POLICY_ID key + @Test + public void testUpdateAccessGroup_MissingExportPolicyId() { + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(mock(HostVO.class))); + Map details = new HashMap<>(); + details.put("someOtherKey", "someValue"); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(details); + assertThrows(CloudRuntimeException.class, () -> strategy.updateAccessGroup(accessGroup)); + } + + // updateAccessGroup - getExportPolicyById returns null + @Test + public void testUpdateAccessGroup_ExportPolicyNotFound() { + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(mock(HostVO.class))); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(null); + assertThrows(CloudRuntimeException.class, () -> strategy.updateAccessGroup(accessGroup)); + } + + // updateAccessGroup - existing policy has null rules + @Test + public void testUpdateAccessGroup_NullRules() { + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(mock(HostVO.class))); + ExportPolicy policy = new ExportPolicy(); + policy.setName("test-policy"); + policy.setRules(null); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(policy); + assertThrows(CloudRuntimeException.class, () -> strategy.updateAccessGroup(accessGroup)); + } + + // updateAccessGroup - existing policy has empty rules list + @Test + public void testUpdateAccessGroup_EmptyRules() { + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(mock(HostVO.class))); + ExportPolicy policy = new ExportPolicy(); + policy.setName("test-policy"); + policy.setRules(new ArrayList<>()); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(policy); + assertThrows(CloudRuntimeException.class, () -> strategy.updateAccessGroup(accessGroup)); + } + + // updateAccessGroup - existing policy has multiple rules + @Test + public void testUpdateAccessGroup_MultipleRules() { + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(mock(HostVO.class))); + ExportPolicy policy = new ExportPolicy(); + policy.setName("test-policy"); + policy.setRules(List.of(new ExportRule(), new ExportRule())); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(policy); + assertThrows(CloudRuntimeException.class, () -> strategy.updateAccessGroup(accessGroup)); + } + + // updateAccessGroup - all hosts have no IP: returns early without ONTAP patch + @Test + public void testUpdateAccessGroup_AllHostsHaveNoIp_ReturnsEarly() { + HostVO host = mock(HostVO.class); + when(host.getStorageIpAddress()).thenReturn(null); + when(host.getPrivateIpAddress()).thenReturn(null); + + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(host)); + + ExportPolicy existingPolicy = existingPolicyWithClients("10.0.0.1/32"); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(existingPolicy); + + AccessGroup result = strategy.updateAccessGroup(accessGroup); + + assertNotNull(result); + assertSame(existingPolicy, result.getPolicy()); + verify(nasFeignClient, never()).updateExportPolicy(anyString(), anyString(), any()); + } + + // updateAccessGroup - ADD: new host IP added to policy + @Test + public void testUpdateAccessGroup_Add_NewHost_Success() { + HostVO host = mock(HostVO.class); + when(host.getStorageIpAddress()).thenReturn("10.0.0.2"); + + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(host)); + // default action is ADD + + ExportPolicy existingPolicy = existingPolicyWithClients("10.0.0.1/32"); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(existingPolicy); + + AccessGroup result = strategy.updateAccessGroup(accessGroup); + + assertNotNull(result); + assertSame(existingPolicy, result.getPolicy()); + // Existing client + new client = 2 + assertEquals(2, existingPolicy.getRules().get(0).getClients().size()); + verify(nasFeignClient).updateExportPolicy(anyString(), eq("policy-42"), any(ExportPolicy.class)); + } + + // updateAccessGroup - ADD: host uses private IP when storage IP is absent + @Test + public void testUpdateAccessGroup_Add_UsesPrivateIpWhenStorageIpAbsent() { + HostVO host = mock(HostVO.class); + when(host.getStorageIpAddress()).thenReturn(null); + when(host.getPrivateIpAddress()).thenReturn("192.168.1.50"); + + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(host)); + + ExportPolicy existingPolicy = existingPolicyWithClients(); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(existingPolicy); + + AccessGroup result = strategy.updateAccessGroup(accessGroup); + + assertNotNull(result); + List clients = existingPolicy.getRules().get(0).getClients(); + assertEquals(1, clients.size()); + assertEquals("192.168.1.50/32", clients.get(0).getMatch()); + verify(nasFeignClient).updateExportPolicy(anyString(), eq("policy-42"), any(ExportPolicy.class)); + } + + // updateAccessGroup - ADD: host IP already present in policy (no-op) + @Test + public void testUpdateAccessGroup_Add_DuplicateHost_NoUpdate() { + HostVO host = mock(HostVO.class); + when(host.getStorageIpAddress()).thenReturn("10.0.0.1"); + + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(host)); + + ExportPolicy existingPolicy = existingPolicyWithClients("10.0.0.1/32"); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(existingPolicy); + + AccessGroup result = strategy.updateAccessGroup(accessGroup); + + assertNotNull(result); + assertSame(existingPolicy, result.getPolicy()); + // Client count must remain 1 (no duplicate inserted) + assertEquals(1, existingPolicy.getRules().get(0).getClients().size()); + verify(nasFeignClient, never()).updateExportPolicy(anyString(), anyString(), any()); + } + + // updateAccessGroup - ADD: existing rule has null clients list + @Test + public void testUpdateAccessGroup_Add_NullClientsInRule() { + HostVO host = mock(HostVO.class); + when(host.getStorageIpAddress()).thenReturn("10.0.0.5"); + + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(host)); + + ExportRule rule = new ExportRule(); + rule.setClients(null); // null clients list + ExportPolicy policy = new ExportPolicy(); + policy.setName("test-policy"); + policy.setRules(new ArrayList<>(List.of(rule))); + + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(policy); + + AccessGroup result = strategy.updateAccessGroup(accessGroup); + + assertNotNull(result); + assertEquals(1, rule.getClients().size()); + assertEquals("10.0.0.5/32", rule.getClients().get(0).getMatch()); + verify(nasFeignClient).updateExportPolicy(anyString(), eq("policy-42"), any(ExportPolicy.class)); + } + + // updateAccessGroup - REMOVE: matching host IP removed from policy + @Test + public void testUpdateAccessGroup_Remove_MatchingHost_Success() { + HostVO host = mock(HostVO.class); + when(host.getStorageIpAddress()).thenReturn("10.0.0.1"); + + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(host)); + accessGroup.setHostRuleAction(AccessGroup.HostRuleAction.REMOVE); + + ExportPolicy existingPolicy = existingPolicyWithClients("10.0.0.1/32", "10.0.0.2/32"); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(existingPolicy); + + AccessGroup result = strategy.updateAccessGroup(accessGroup); + + assertNotNull(result); + assertSame(existingPolicy, result.getPolicy()); + // Only 10.0.0.2/32 should remain + List clients = existingPolicy.getRules().get(0).getClients(); + assertEquals(1, clients.size()); + assertEquals("10.0.0.2/32", clients.get(0).getMatch()); + verify(nasFeignClient).updateExportPolicy(anyString(), eq("policy-42"), any(ExportPolicy.class)); + } + + // updateAccessGroup - REMOVE: IP not in policy (no-op) + @Test + public void testUpdateAccessGroup_Remove_IpNotPresent_NoUpdate() { + HostVO host = mock(HostVO.class); + when(host.getStorageIpAddress()).thenReturn("10.0.0.99"); + + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(host)); + accessGroup.setHostRuleAction(AccessGroup.HostRuleAction.REMOVE); + + ExportPolicy existingPolicy = existingPolicyWithClients("10.0.0.1/32"); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(existingPolicy); + + AccessGroup result = strategy.updateAccessGroup(accessGroup); + + assertNotNull(result); + assertSame(existingPolicy, result.getPolicy()); + assertEquals(1, existingPolicy.getRules().get(0).getClients().size()); + verify(nasFeignClient, never()).updateExportPolicy(anyString(), anyString(), any()); + } + + // updateAccessGroup - FeignException from ONTAP wrapped in CloudRuntimeException + @Test + public void testUpdateAccessGroup_FeignExceptionWrapped() { + HostVO host = mock(HostVO.class); + when(host.getStorageIpAddress()).thenReturn("10.0.0.1"); + + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(host)); + + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))) + .thenThrow(new RuntimeException("ONTAP unreachable")); + + assertThrows(CloudRuntimeException.class, () -> strategy.updateAccessGroup(accessGroup)); + } + // updateAccessGroup - whitespace in storage IP is trimmed before building match + @Test + public void testUpdateAccessGroup_TrimsWhitespaceFromStorageIp() { + HostVO host = mock(HostVO.class); + when(host.getStorageIpAddress()).thenReturn(" 10.0.0.2 "); + + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(host)); + + ExportPolicy existingPolicy = existingPolicyWithClients(); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(existingPolicy); + + strategy.updateAccessGroup(accessGroup); + + List clients = existingPolicy.getRules().get(0).getClients(); + assertEquals(1, clients.size()); + assertEquals("10.0.0.2/32", clients.get(0).getMatch()); + } + + // updateAccessGroup - whitespace in private IP is trimmed when storage IP absent + @Test + public void testUpdateAccessGroup_TrimsWhitespaceFromPrivateIp() { + HostVO host = mock(HostVO.class); + when(host.getStorageIpAddress()).thenReturn(null); + when(host.getPrivateIpAddress()).thenReturn(" 192.168.1.10 "); + + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(host)); + + ExportPolicy existingPolicy = existingPolicyWithClients(); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(existingPolicy); + + strategy.updateAccessGroup(accessGroup); + + List clients = existingPolicy.getRules().get(0).getClients(); + assertEquals(1, clients.size()); + assertEquals("192.168.1.10/32", clients.get(0).getMatch()); + }} From d1ed46a7bf663931953fd65b3d03ab282f97b37d Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Mon, 13 Jul 2026 15:35:56 +0530 Subject: [PATCH 07/10] Fixed review comments --- .../apache/cloudstack/storage/service/StorageStrategyTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 6091c45ffdad..a563dbca33de 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 @@ -166,7 +166,7 @@ public void deleteAccessGroup(AccessGroup accessGroup) { } @Override - public AccessGroup updateAccessGroup(AccessGroup accessGroup) { + public AccessGroup updateAccessGroup(AccessGroup accessGroup) { return null; } From 8df4bc98a288dd0cf3bea5548358a6919e7133ed Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Mon, 13 Jul 2026 15:38:46 +0530 Subject: [PATCH 08/10] Fixed more review comments --- .../org/apache/cloudstack/storage/feign/model/ExportRule.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 adece0cc6923..15374811bf74 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 @@ -73,7 +73,7 @@ public static ProtocolsEnum fromValue(String text) { return null; } for (ProtocolsEnum b : ProtocolsEnum.values()) { - if (String.valueOf(b.value).equalsIgnoreCase(text) || b.name().equalsIgnoreCase(text)) { + if (String.valueOf(b.value).equalsIgnoreCase(text)) { return b; } } From 76bdf24769a3a2c7ce2cd0f34d2fcd91bbf39b94 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Mon, 13 Jul 2026 22:27:49 +0530 Subject: [PATCH 09/10] Fixed few more review comments --- .../storage/listener/OntapHostListener.java | 56 +++++++++---------- .../storage/service/UnifiedNASStrategy.java | 12 +--- 2 files changed, 29 insertions(+), 39 deletions(-) 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 bce96845f27c..993e2d182804 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 @@ -71,33 +71,33 @@ public class OntapHostListener implements HypervisorHostListener { @Override public boolean hostConnect(long hostId, long poolId) { - logger.info("Connect to host " + hostId + " from pool " + poolId); + logger.info("hostConnect: Connecting host {} to pool {}", hostId, poolId); Host host = _hostDao.findById(hostId); if (host == null) { - logger.error("host was not found with id : {}", hostId); + logger.error("hostConnect: Host was not found with id: {}", hostId); return false; } if (!host.getHypervisorType().equals(Hypervisor.HypervisorType.KVM)) { - logger.error("ONTAP plugin does not support {} type host currently ", host.getHypervisorType()); + logger.error("hostConnect: ONTAP plugin does not support {} type host currently", host.getHypervisorType()); return false; } StoragePool pool = _storagePoolDao.findById(poolId); if (pool == null) { - logger.error("Failed to connect host - storage pool not found with id: {}", poolId); + logger.error("hostConnect: Failed to connect host - storage pool not found with id: {}", poolId); return false; } - logger.info("Connecting host {} to ONTAP storage pool {}", host.getName(), pool.getName()); + logger.info("hostConnect: Connecting host {} to ONTAP storage pool {}", host.getName(), pool.getName()); 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); + logger.error("hostConnect: 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); + logger.error("hostConnect: Storage pool details missing required protocol type for pool id: {}", poolId); return false; } @@ -139,7 +139,7 @@ public boolean hostConnect(long hostId, long poolId) { } String localPath = poolInfo.getLocalPath(); - logger.info("Storage pool {} successfully mounted at: {}", pool.getName(), localPath); + logger.info("hostConnect: Storage pool {} successfully mounted at: {}", pool.getName(), localPath); // Update or create the storage_pool_host_ref entry with the correct local_path StoragePoolHostVO storagePoolHost = storagePoolHostDao.findByPoolHost(poolId, hostId); @@ -147,11 +147,11 @@ public boolean hostConnect(long hostId, long poolId) { if (storagePoolHost == null) { storagePoolHost = new StoragePoolHostVO(poolId, hostId, localPath); storagePoolHostDao.persist(storagePoolHost); - logger.info("Created storage_pool_host_ref entry for pool {} and host {}", pool.getName(), host.getName()); + logger.info("hostConnect: Created storage_pool_host_ref entry for pool {} and host {}", pool.getName(), host.getName()); } else { storagePoolHost.setLocalPath(localPath); storagePoolHostDao.update(storagePoolHost.getId(), storagePoolHost); - logger.info("Updated storage_pool_host_ref entry with local_path: {}", localPath); + logger.info("hostConnect: Updated storage_pool_host_ref entry with local_path: {}", localPath); } // Update pool capacity/usage information @@ -160,11 +160,11 @@ public boolean hostConnect(long hostId, long poolId) { poolVO.setCapacityBytes(poolInfo.getCapacityBytes()); poolVO.setUsedBytes(poolInfo.getCapacityBytes() - poolInfo.getAvailableBytes()); _storagePoolDao.update(poolVO.getId(), poolVO); - logger.info("Updated storage pool capacity: {} GB, used: {} GB", poolInfo.getCapacityBytes() / (1024 * 1024 * 1024), (poolInfo.getCapacityBytes() - poolInfo.getAvailableBytes()) / (1024 * 1024 * 1024)); + logger.info("hostConnect: Updated storage pool capacity: {} GB, used: {} GB", poolInfo.getCapacityBytes() / (1024 * 1024 * 1024), (poolInfo.getCapacityBytes() - poolInfo.getAvailableBytes()) / (1024 * 1024 * 1024)); } } catch (Exception e) { - logger.error("Exception while connecting host {} to storage pool {}", host.getName(), pool.getName(), e); + logger.error("hostConnect: Exception while connecting host {} to storage pool {}", host.getName(), pool.getName(), e); // CRITICAL: Don't throw exception - it crashes the agent and causes restart loops // Return false to indicate failure without crashing return false; @@ -191,7 +191,7 @@ private void updateNfsExportPolicyForConnectedHostIfNeeded(long poolId, long hos StorageStrategy strategy = OntapStorageUtils.getStrategyByStoragePoolDetails(detailsMap); strategy.updateAccessGroup(accessGroup); - logger.info("Updated NFS export policy rules for host {} on storage pool {}", host.getName(), poolId); + logger.info("hostConnect: updateNfsExportPolicyForConnectedHostIfNeeded: Updated NFS export policy rules for host {} on storage pool {}", host.getName(), poolId); } private boolean isNfs3EnabledOnHost(Host host) { @@ -201,7 +201,7 @@ private boolean isNfs3EnabledOnHost(Host host) { 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", + logger.warn("isNfs3EnabledOnHost: Host {} is not eligible for NFS3 protocol: both storage IP and private IP are empty", host.getId()); return false; } @@ -211,35 +211,35 @@ private boolean isNfs3EnabledOnHost(Host host) { @Override public boolean hostDisconnected(long hostId, long poolId) { - logger.info("Disconnect from host " + hostId + " from pool " + poolId); + logger.info("hostDisconnected: Disconnecting host {} from pool {}", hostId, 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); + logger.info("hostAboutToBeRemoved: Host {} is about to be removed", hostId); Host host = _hostDao.findById(hostId); if (host == null) { - logger.warn("Host not found with id: {}, considering it as no-op", hostId); + logger.warn("hostAboutToBeRemoved: Host not found with id: {}, considering it as no-op", hostId); return true; } List poolHostRefs = storagePoolHostDao.listByHostId(hostId); if (poolHostRefs == null || poolHostRefs.isEmpty()) { - logger.debug("No storage pool associations found for host {}", hostId); + logger.debug("hostAboutToBeRemoved: 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); + removeHostFromOntapPoolIfNeeded(pool, host); } } - logger.info("Cleaned up ONTAP export policies for host {} about to be removed", hostId); + logger.info("hostAboutToBeRemoved: Cleaned up ONTAP export policies for host {} about to be removed", hostId); return true; } @@ -248,23 +248,23 @@ public boolean hostRemoved(long hostId, long clusterId) { return false; } - private void removeHostFromOntapPoolIfNeeded(long hostId, StoragePoolVO pool, Host host) { + private void removeHostFromOntapPoolIfNeeded(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()); + logger.debug("hostAboutToBeRemoved: removeHostFromOntapPoolIfNeeded: No pool details found for pool id: {}", pool.getId()); return; } - // Skip non-NFS3 pools + // Skip non-NFS3 pools; Currently, for iSCSI type, iGroup rules are being handled as part of revokeAccess in OntapPrimaryDataStoreDriver, so no need to handle here. if (!ProtocolType.NFS3.name().equalsIgnoreCase(detailsMap.get(OntapStorageConstants.PROTOCOL))) { return; } - logger.info("Removing export policy rule for host {} from storage pool {}", host.getName(), pool.getName()); + logger.info("hostAboutToBeRemoved: removeHostFromOntapPoolIfNeeded: Removing export policy rule for host {} from storage pool {}", host.getName(), pool.getName()); if (!isNfs3EnabledOnHost(host)) { - logger.warn("Skipping NFS export policy removal for host {} on pool {} as host is not NFS-enabled", - hostId, pool.getId()); + logger.warn("hostAboutToBeRemoved: removeHostFromOntapPoolIfNeeded: Skipping NFS export policy removal for host {} on pool {} as host is not NFS-enabled", + host.getId(), pool.getId()); return; } AccessGroup accessGroup = new AccessGroup(); @@ -274,9 +274,9 @@ private void removeHostFromOntapPoolIfNeeded(long hostId, StoragePoolVO pool, Ho 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()); + logger.info("hostAboutToBeRemoved: removeHostFromOntapPoolIfNeeded: 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()); + logger.warn("hostAboutToBeRemoved: removeHostFromOntapPoolIfNeeded: Failed to remove NFS export policy rule for host {} from pool {}: {}", host.getId(), pool.getName(), e.getMessage()); // Continue processing other pools even if one fails } } 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 a39a0fa01d5f..0a257a29527b 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 @@ -228,19 +228,9 @@ public AccessGroup updateAccessGroup(AccessGroup accessGroup) { } 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()) { - throw new CloudRuntimeException("Export policy " + existingPolicy.getName() + " has no rules. " + - "Cannot update an export policy with no existing rules."); - } - - // 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()); + " has no rules — unexpected state, the plugin always creates a rule at pool registration"); } ExportRule targetRule = rules.get(0); From 6af0e372607dff7c8776abbcae19205826beb9f3 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Tue, 14 Jul 2026 08:29:40 +0530 Subject: [PATCH 10/10] Removed a UT which no longer is necessary --- .../storage/service/UnifiedNASStrategyTest.java | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedNASStrategyTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedNASStrategyTest.java index fd4d10882949..b04e9c0b1b26 100755 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedNASStrategyTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedNASStrategyTest.java @@ -705,20 +705,6 @@ public void testUpdateAccessGroup_EmptyRules() { assertThrows(CloudRuntimeException.class, () -> strategy.updateAccessGroup(accessGroup)); } - // updateAccessGroup - existing policy has multiple rules - @Test - public void testUpdateAccessGroup_MultipleRules() { - AccessGroup accessGroup = new AccessGroup(); - accessGroup.setStoragePoolId(1L); - accessGroup.setHostsToConnect(List.of(mock(HostVO.class))); - ExportPolicy policy = new ExportPolicy(); - policy.setName("test-policy"); - policy.setRules(List.of(new ExportRule(), new ExportRule())); - when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); - when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(policy); - assertThrows(CloudRuntimeException.class, () -> strategy.updateAccessGroup(accessGroup)); - } - // updateAccessGroup - all hosts have no IP: returns early without ONTAP patch @Test public void testUpdateAccessGroup_AllHostsHaveNoIp_ReturnsEarly() {