feature/CSTACKEX-122: Comments resolution

Srivastava, Piyush · Srivastava, Piyush · commit 121bbfe25cf1 · 2026-03-11T11:55:11.000+05:30
diff --git a/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java b/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java
@@ -222,6 +222,8 @@ public class VolumeServiceImpl implements VolumeService {
     @Inject
     protected DiskOfferingDao diskOfferingDao;
 
+    private static final String ONTAP_PROVIDER_NAME = "NetApp ONTAP";
+
     public VolumeServiceImpl() {
     }
 
@@ -731,16 +733,12 @@ protected Void managedCopyBaseImageCallback(AsyncCallbackDispatcher<VolumeServic
             TemplateObjectTO templateObjectTo = (TemplateObjectTO)answer.getNewData();
 
             // For NFS managed storage, preserve the volume UUID path to avoid file collision
-            // For iSCSI, update path as before (iSCSI uses _iScsiName field for actual LUN access)
             PrimaryDataStore primaryDataStore = (PrimaryDataStore) volumeInfo.getDataStore();
-            if (primaryDataStore != null && primaryDataStore.getPoolType() == StoragePoolType.NetworkFilesystem) {
-                if (logger.isDebugEnabled()) {
-                    logger.debug("NFS managed storage - preserving volume path: " + volume.getPath() + " (not overwriting with template path: " + templateObjectTo.getPath() + ")");
-                }
-            } else {
+            if (primaryDataStore.isManaged() && ONTAP_PROVIDER_NAME.equals(primaryDataStore.getStorageProviderName()) && primaryDataStore.getPoolType() == StoragePoolType.NetworkFilesystem) {
+                logger.debug("NFS managed storage - preserving volume path: " + volume.getPath() + " (not overwriting with template path: " + templateObjectTo.getPath() + ")");
+            }else {
                 volume.setPath(templateObjectTo.getPath());
             }
-
             if (templateObjectTo.getFormat() != null) {
                 volume.setFormat(templateObjectTo.getFormat());
             }
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
@@ -163,16 +163,8 @@ public void createAsync(DataStore dataStore, DataObject dataObject, AsyncComplet
 
                         s_logger.info("createAsync: Created LUN [{}] for volume [{}]. LUN mapping will occur during grantAccess() to per-host igroup.",
                                 lunName, volumeVO.getId());
-
-                        // Path will be set during grantAccess when LUN is mapped and we get the LUN ID
-                        // Return LUN name as identifier for CloudStack tracking
-                        volumeVO.set_iScsiName(null);
-                        volumeVO.setPath(null);
                         createCmdResult = new CreateCmdResult(lunName, new Answer(null, true, null));
                     } else if (ProtocolType.NFS3.name().equalsIgnoreCase(details.get(Constants.PROTOCOL))) {
-                        // For NFS, set path to volume UUID to ensure uniqueness
-                        // This prevents multiple VMs from using the same template file path
-                        volumeVO.setPath(volInfo.getUuid());
                         createCmdResult = new CreateCmdResult(volInfo.getUuid(), new Answer(null, true, null));
                         s_logger.info("createAsync: Managed NFS volume [{}] with path [{}] associated with pool {}",
                                 volumeVO.getId(), volInfo.getUuid(), storagePool.getId());
@@ -331,7 +323,7 @@ public boolean grantAccess(DataObject dataObject, Host host, DataStore dataStore
                     Igroup igroup = new Igroup();
                     AccessGroup accessGroup = sanStrategy.getAccessGroup(getAccessGroupMap);
                     if(accessGroup == null || accessGroup.getIgroup() == null) {
-                        s_logger.info("grantAccess: Igroup does not exist for the host: Need to create Igroup " + host.getName());
+                        s_logger.info("grantAccess: Igroup {} does not exist for the host {} : Need to create Igroup for the host ", accessGroupName, host.getName());
                         // create the igroup for the host and perform lun-mapping
                         accessGroup = new AccessGroup();
                         List<HostVO> hosts = new ArrayList<>();
@@ -342,14 +334,14 @@ public boolean grantAccess(DataObject dataObject, Host host, DataStore dataStore
                     }else{
                         s_logger.info("grantAccess: Igroup {} already exist for the host {}: ", accessGroup.getIgroup().getName() ,host.getName());
                         igroup = accessGroup.getIgroup();
-                        // TODO
-                        // Verify host initiator is registered in the igroup before allowing access
-//                        if (sanStrategy.validateInitiatorInAccessGroup(host.getStorageUrl(), svmName, accessGroup.getIgroup())) {
-//                            // add host initiator to the igroup ? or fail here ?
-//                        }
-                        // Use the existing igroup and perform lun-mapping
+                        /* TODO Below cases will be covered later, for now they will be a pre-requisite on customer side
+                          1. Igroup exist with the same name but host initiator has been rempved
+                          2.  Igroup exist with the same name but host initiator has been changed may be due to new NIC or new adapter
+                          In both cases we need to verify current host initiator is registered in the igroup before allowing access
+                          Incase it is not , add it and proceed for lun-mapping
+                         */
                     }
-
+                    s_logger.info("grantAccess: Igroup {}  is present now with initiators {} ", accessGroup.getIgroup().getName(), accessGroup.getIgroup().getInitiators());
                     // Create or retrieve existing LUN mapping
                     String lunNumber = sanStrategy.ensureLunMapped(svmName, cloudStackVolumeName, accessGroupName);
 
diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java
@@ -282,11 +282,6 @@ public boolean attachCluster(DataStore dataStore, ClusterScope scope) {
         PrimaryDataStoreInfo primaryStore = (PrimaryDataStoreInfo)dataStore;
         List<HostVO> hostsToConnect = _resourceMgr.getEligibleUpAndEnabledHostsInClusterForStorageConnection(primaryStore);
         logger.debug("attachCluster: Eligible Up and Enabled hosts: {} in cluster {}", hostsToConnect, primaryStore.getClusterId());
-        if(hostsToConnect.isEmpty()) {
-            s_logger.info("attachCluster: No hosts found for primary storage");
-            throw new CloudRuntimeException("attachCluster: No hosts found for primary storage");
-        }
-
         Map<String, String> details = storagePoolDetailsDao.listDetailsKeyPairs(primaryStore.getId());
         StorageStrategy strategy = Utility.getStrategyByStoragePoolDetails(details);
 
@@ -300,12 +295,12 @@ public boolean attachCluster(DataStore dataStore, ClusterScope scope) {
         logger.debug("attachCluster: Attaching the pool to each of the host in the cluster: {}", primaryStore.getClusterId());
         // We need to create export policy at pool level and igroup at host level(in grantAccess)
         if (ProtocolType.NFS3.name().equalsIgnoreCase(details.get(Constants.PROTOCOL))) {
+            // If there are no eligible host, export policy or igroup will not be created and will be taken as part of HostListener
             if (!hostsIdentifier.isEmpty()) {
                 try {
                     AccessGroup accessGroupRequest = new AccessGroup();
                     accessGroupRequest.setHostsToConnect(hostsToConnect);
                     accessGroupRequest.setScope(scope);
-                    primaryStore.setDetails(details);// setting details as it does not come from cloudstack
                     accessGroupRequest.setStoragePoolId(storagePool.getId());
                     strategy.createAccessGroup(accessGroupRequest);
                 } catch (Exception e) {
@@ -352,11 +347,6 @@ public boolean attachZone(DataStore dataStore, ZoneScope scope, Hypervisor.Hyper
         PrimaryDataStoreInfo primaryStore = (PrimaryDataStoreInfo)dataStore;
         List<HostVO> hostsToConnect = _resourceMgr.getEligibleUpAndEnabledHostsInZoneForStorageConnection(dataStore, scope.getScopeId(), Hypervisor.HypervisorType.KVM);
         logger.debug(String.format("In createPool. Attaching the pool to each of the hosts in %s.", hostsToConnect));
-        if(hostsToConnect.isEmpty()) {
-            s_logger.info("attachCluster: No hosts found for primary storage");
-            throw new CloudRuntimeException("attachCluster: No hosts found for primary storage");
-        }
-
         Map<String, String> details = storagePoolDetailsDao.listDetailsKeyPairs(primaryStore.getId());
         StorageStrategy strategy = Utility.getStrategyByStoragePoolDetails(details);
 
@@ -370,12 +360,12 @@ public boolean attachZone(DataStore dataStore, ZoneScope scope, Hypervisor.Hyper
 
         // We need to create export policy at pool level and igroup at host level
         if (ProtocolType.NFS3.name().equalsIgnoreCase(details.get(Constants.PROTOCOL))) {
+            // If there are no eligible host, export policy or igroup will not be created and will be taken as part of HostListener
             if (!hostsIdentifier.isEmpty()) {
                 try {
                     AccessGroup accessGroupRequest = new AccessGroup();
                     accessGroupRequest.setHostsToConnect(hostsToConnect);
                     accessGroupRequest.setScope(scope);
-                    primaryStore.setDetails(details); // setting details as it does not come from cloudstack
                     accessGroupRequest.setStoragePoolId(storagePool.getId());
                     strategy.createAccessGroup(accessGroupRequest);
                 } catch (Exception e) {
diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/SANStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/SANStrategy.java
@@ -48,7 +48,7 @@ public SANStrategy(OntapStorage ontapStorage) {
      *
      * @param hostInitiator the host initiator IQN
      * @param svmName the SVM name
-     * @param accessGroupName the igroup name
+     * @param igroup the igroup
      * @return true if the initiator is found in the igroup, false otherwise
      */
     public boolean validateInitiatorInAccessGroup(String hostInitiator, String svmName, Igroup igroup) {
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
@@ -267,7 +267,7 @@ public Volume createStorageVolume(String volumeName, Long size) {
 
             if (ontapVolume == null || ontapVolume.getRecords() == null || ontapVolume.getRecords().isEmpty()) {
                 s_logger.error("OntapResponse is null for volume: " + volumeName);
-                throw new CloudRuntimeException("Failed to fetch volume " + volumeName + ": Response is null");
+                throw new CloudRuntimeException("Failed to fetch volume " + volumeName + ": no records found");
             }
             Volume volume = ontapVolume.getRecords().get(0);
             s_logger.info("Volume retrieved successfully: " + volumeName + ", UUID: " + volume.getUuid());
@@ -278,11 +278,26 @@ public Volume createStorageVolume(String volumeName, Long size) {
         }
     }
 
+     /**
+     * Updates ONTAP Flex-Volume
+     * Eligible only for Unified ONTAP storage
+     * throw exception in case of disaggregated ONTAP storage
+     *
+     * @param volume the volume to update
+     * @return the updated Volume object
+     */
     public Volume updateStorageVolume(Volume volume) {
         //TODO
         return null;
     }
 
+    /**
+     * Delete ONTAP Flex-Volume
+     * Eligible only for Unified ONTAP storage
+     * throw exception in case of disaggregated ONTAP storage
+     *
+     * @param volume the volume to delete
+     */
     public void deleteStorageVolume(Volume volume) {
         s_logger.info("Deleting ONTAP volume by name: " + volume.getName() + " and uuid: " + volume.getUuid());
         // Calling the VolumeFeignClient to delete the volume
@@ -302,7 +317,14 @@ public void deleteStorageVolume(Volume volume) {
         }
         s_logger.info("ONTAP volume deletion process completed for volume: " + volume.getName());
     }
-
+    /**
+     * Gets ONTAP Flex-Volume
+     * Eligible only for Unified ONTAP storage
+     * throw exception in case of disaggregated ONTAP storage
+     *
+     * @param volume the volume to retrieve
+     * @return the retrieved Volume object
+     */
     public Volume getStorageVolume(Volume volume) {
         //TODO
         return null;
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
@@ -31,8 +31,6 @@
 import org.apache.cloudstack.engine.subsystem.api.storage.EndPointSelector;
 import org.apache.cloudstack.storage.command.CreateObjectCommand;
 import org.apache.cloudstack.storage.command.DeleteCommand;
-import com.cloud.agent.api.to.DataTO;
-import org.apache.cloudstack.storage.to.VolumeObjectTO;
 import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao;
 import org.apache.cloudstack.storage.feign.FeignClientFactory;
 import org.apache.cloudstack.storage.feign.client.JobFeignClient;
@@ -89,10 +87,10 @@ public void setOntapStorage(OntapStorage ontapStorage) {
     public CloudStackVolume createCloudStackVolume(CloudStackVolume cloudstackVolume) {
         s_logger.info("createCloudStackVolume: Create cloudstack volume " + cloudstackVolume);
         try {
-            // Step 1: set cloudstack volume metadata and get the volumeUuid
+            // Step 1: set cloudstack volume metadata
             String volumeUuid = updateCloudStackVolumeMetadata(cloudstackVolume.getDatastoreId(), cloudstackVolume.getVolumeInfo());
-            // Step 2: Send command to KVM host with explicit volumeUuid to avoid stale cached path
-            Answer answer = createVolumeOnKVMHost(cloudstackVolume.getVolumeInfo(), volumeUuid);
+            // Step 2: Send command to KVM host to create qcow2 file using qemu-img
+            Answer answer = createVolumeOnKVMHost(cloudstackVolume.getVolumeInfo());
             if (answer == null || !answer.getResult()) {
                 String errMsg = answer != null ? answer.getDetails() : "Failed to create qcow2 on KVM host";
                 s_logger.error("createCloudStackVolume: " + errMsg);
@@ -482,7 +480,7 @@ private String updateCloudStackVolumeMetadata(String dataStoreId, DataObject vol
            String volumeUuid = volumeInfo.getUuid();
            volume.setPoolType(Storage.StoragePoolType.NetworkFilesystem);
            volume.setPoolId(Long.parseLong(dataStoreId));
-           volume.setPath(volumeUuid);  // Filename for qcow2 file - unique per volume
+           volume.setPath(volumeUuid);  // Filename for qcow2 file
            volumeDao.update(volume.getId(), volume);
            s_logger.info("Updated volume path to {} for volume ID {}", volumeUuid, volumeId);
            return volumeUuid;
@@ -492,19 +490,12 @@ private String updateCloudStackVolumeMetadata(String dataStoreId, DataObject vol
        }
     }
 
-    private Answer createVolumeOnKVMHost(DataObject volumeInfo, String correctPath) {
-        s_logger.info("createVolumeOnKVMHost called with volumeInfo: {}, correctPath: {}", volumeInfo, correctPath);
+    private Answer createVolumeOnKVMHost(DataObject volumeInfo) {
+        s_logger.info("createVolumeOnKVMHost called with volumeInfo: {} ", volumeInfo);
 
         try {
-            // Get the base TO and override the path with correct value to avoid stale cached path
-            DataTO dataTO = volumeInfo.getTO();
-            if (dataTO instanceof VolumeObjectTO) {
-                VolumeObjectTO volumeTO = (VolumeObjectTO) dataTO;
-                s_logger.info("createVolumeOnKVMHost: Original TO path: {}, overriding with correct path: {}", volumeTO.getPath(), correctPath);
-                volumeTO.setPath(correctPath);  // Override stale path with correct UUID
-            }
             s_logger.info("createVolumeOnKVMHost: Sending CreateObjectCommand to KVM agent for volume: {}", volumeInfo.getUuid());
-            CreateObjectCommand cmd = new CreateObjectCommand(dataTO);
+            CreateObjectCommand cmd = new CreateObjectCommand(volumeInfo.getTO());
             EndPoint ep = epSelector.select(volumeInfo);
             if (ep == null) {
                 String errMsg = "No remote endpoint to send CreateObjectCommand, check if host is up";
diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java
@@ -230,7 +230,6 @@ public AccessGroup createAccessGroup(AccessGroup accessGroup) {
             ProtocolType protocol = ProtocolType.valueOf(dataStoreDetails.get(Constants.PROTOCOL));
 
             // Check if all hosts support the protocol
-            List<String> hostIdentifiers = new ArrayList<>();
             if (!validateProtocolSupport(accessGroup.getHostsToConnect(), protocol)) {
                 String errMsg = "createAccessGroup: Not all hosts " + " support the protocol: " + protocol.name();
                 throw new CloudRuntimeException(errMsg);
@@ -370,7 +369,6 @@ private boolean validateProtocolSupport(List<HostVO> hosts, ProtocolType protoco
             if (host == null || host.getStorageUrl() == null || host.getStorageUrl().trim().isEmpty() || !host.getStorageUrl().startsWith(protocolPrefix)) {
                 return false;
             }
-//            hostIdentifiers.add(host.getStorageUrl());
         }
         s_logger.info("validateProtocolSupportAndFetchHostsIdentifier: All hosts support the protocol: " + protocolType.name());
         return true;
diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/Utility.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/Utility.java
@@ -146,7 +146,7 @@ public static String getIgroupName(String svmName, String hostName) {
     }
 
     public static String generateExportPolicyName(String svmName, String volumeName){
-        return Constants.EXPORT + Constants.HYPHEN + svmName + Constants.HYPHEN + volumeName;
+        return Constants.CS + Constants.HYPHEN + svmName + Constants.HYPHEN + volumeName;
     }
 
     public static String getLunName(String volName, String lunName) {

Original file line number	Diff line number	Diff line change
`@@ -48,7 +48,7 @@ public SANStrategy(OntapStorage ontapStorage) {`
`48`	`48`	`*`
`49`	`49`	`* @param hostInitiator the host initiator IQN`
`50`	`50`	`* @param svmName the SVM name`
`51`		`- * @param accessGroupName the igroup name`
	`51`	`+ * @param igroup the igroup`
`52`	`52`	`* @return true if the initiator is found in the igroup, false otherwise`
`53`	`53`	`*/`
`54`	`54`	`public boolean validateInitiatorInAccessGroup(String hostInitiator, String svmName, Igroup igroup) {`
Original file line number	Diff line number	Diff line change
`@@ -230,7 +230,6 @@ public AccessGroup createAccessGroup(AccessGroup accessGroup) {`
`230`	`230`	`ProtocolType protocol = ProtocolType.valueOf(dataStoreDetails.get(Constants.PROTOCOL));`
`231`	`231`
`232`	`232`	`// Check if all hosts support the protocol`
`233`		`- List<String> hostIdentifiers = new ArrayList<>();`
`234`	`233`	`if (!validateProtocolSupport(accessGroup.getHostsToConnect(), protocol)) {`
`235`	`234`	`String errMsg = "createAccessGroup: Not all hosts " + " support the protocol: " + protocol.name();`
`236`	`235`	`throw new CloudRuntimeException(errMsg);`
`@@ -370,7 +369,6 @@ private boolean validateProtocolSupport(List<HostVO> hosts, ProtocolType protoco`
`370`	`369`	`if (host == null \|\| host.getStorageUrl() == null \|\| host.getStorageUrl().trim().isEmpty() \|\| !host.getStorageUrl().startsWith(protocolPrefix)) {`
`371`	`370`	`return false;`
`372`	`371`	`}`
`373`		`-// hostIdentifiers.add(host.getStorageUrl());`
`374`	`372`	`}`
`375`	`373`	`s_logger.info("validateProtocolSupportAndFetchHostsIdentifier: All hosts support the protocol: " + protocolType.name());`
`376`	`374`	`return true;`
Original file line number	Diff line number	Diff line change
`@@ -146,7 +146,7 @@ public static String getIgroupName(String svmName, String hostName) {`
`146`	`146`	`}`
`147`	`147`
`148`	`148`	`public static String generateExportPolicyName(String svmName, String volumeName){`
`149`		`- return Constants.EXPORT + Constants.HYPHEN + svmName + Constants.HYPHEN + volumeName;`
	`149`	`+ return Constants.CS + Constants.HYPHEN + svmName + Constants.HYPHEN + volumeName;`
`150`	`150`	`}`
`151`	`151`
`152`	`152`	`public static String getLunName(String volName, String lunName) {`