CSTACKEX-46: Fixed code as per comments received

Author: Locharla, Sandeep

plugins/storage/volume/ontap/README.md

@@ -0,0 +1,123 @@
+<!--
+  Licensed to the Apache Software Foundation (ASF) under one
+  or more contributor license agreements.  See the NOTICE file
+  distributed with this work for additional information
+  regarding copyright ownership.  The ASF licenses this file
+  to you under the Apache License, Version 2.0 (the
+  "License"); you may not use this file except in compliance
+  with the License.  You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing,
+  software distributed under the License is distributed on an
+  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+  KIND, either express or implied.  See the License for the
+  specific language governing permissions and limitations
+  under the License.
+-->
+
+# Apache CloudStack - NetApp ONTAP Storage Plugin
+
+## Overview
+
+The NetApp ONTAP Storage Plugin provides integration between Apache CloudStack and NetApp ONTAP storage systems. This plugin enables CloudStack to provision and manage primary storage on ONTAP clusters, supporting both NAS (NFS) and SAN (iSCSI) protocols.
+
+## Features
+
+- **Primary Storage Support**: Provision and manage primary storage pools on NetApp ONTAP
+- **Multiple Protocols**: Support for NFS 3.0 and iSCSI protocols
+- **Unified Storage**: Integration with traditional ONTAP unified storage architecture
+- **KVM Hypervisor Support**: Supports KVM hypervisor environments
+- **Managed Storage**: Operates as managed storage with full lifecycle management
+- **Flexible Scoping**: Support for Zone-wide and Cluster-scoped storage pools
+
+## Architecture
+
+### Component Structure
+
+| Package | Description                                           |
+|---------|-------------------------------------------------------|
+| `driver` | Primary datastore driver implementation               |
+| `feign` | REST API clients and data models for ONTAP operations |
+| `lifecycle` | Storage pool lifecycle management                     |
+| `listener` | Host connection event handlers                        |
+| `provider` | Main provider and strategy factory                    |
+| `service` | ONTAP Storage strategy implementations (NAS/SAN)      |
+| `utils` | Constants and helper utilities                        |
+
+## Requirements
+
+### ONTAP Requirements
+
+- NetApp ONTAP 9.15.1 or higher
+- Storage Virtual Machine (SVM) configured with appropriate protocols enabled
+- Management LIF accessible from CloudStack management server
+- Data LIF(s) accessible from hypervisor hosts and are of IPv4 type
+- Aggregates assigned to the SVM with sufficient capacity
+
+### CloudStack Requirements
+
+- Apache CloudStack current version or higher
+- KVM hypervisor hosts
+- For iSCSI: Hosts must have iSCSI initiator configured with valid IQN
+- For NFS: Hosts must have NFS client packages installed
+
+### Minimum Volume Size
+
+ONTAP requires a minimum volume size of **1.56 GB** (1,677,721,600 bytes). The plugin will automatically adjust requested sizes below this threshold.
+
+## Configuration
+
+### Storage Pool Creation Parameters
+
+When creating an ONTAP primary storage pool, provide the following details in the URL field (semicolon-separated key=value pairs):
+
+| Parameter | Required | Description |
+|-----------|----------|-------------|
+| `username` | Yes | ONTAP cluster admin username |
+| `password` | Yes | ONTAP cluster admin password |
+| `svmName` | Yes | Storage Virtual Machine name |
+| `protocol` | Yes | Storage protocol (`NFS3` or `ISCSI`) |
+| `managementLIF` | Yes | ONTAP cluster management LIF IP address |
+
+### Example URL Format
+
+```
+username=admin;password=secretpass;svmName=svm1;protocol=ISCSI;managementLIF=192.168.1.100
+```
+
+## Port Configuration
+
+| Protocol | Default Port |
+|----------|--------------|
+| NFS | 2049 |
+| iSCSI | 3260 |
+| ONTAP Management API | 443 (HTTPS) |
+
+## Limitations
+
+- Supports only **KVM** hypervisor
+- Supports only **Unified ONTAP** storage (disaggregated not supported)
+- Supports only **NFS3** and **iSCSI** protocols
+- IPv6 type and FQDN LIFs are not supported
+
+## Troubleshooting
+
+### Common Issues
+
+1. **Connection Failures**
+    - Verify management LIF is reachable from CloudStack management server
+    - Check firewall rules for port 443
+
+2. **Protocol Errors**
+    - Ensure the protocol (NFS/iSCSI) is enabled on the SVM
+    - Verify Data LIFs are configured for the protocol
+
+3. **Capacity Errors**
+    - Check aggregate space availability
+    - Ensure requested volume size meets minimum requirements (1.56 GB)
+
+4. **Host Connection Issues**
+    - For iSCSI: Verify host IQN is properly configured in host's storage URL
+    - For NFS: Ensure NFS client is installed and running

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java

@@ -69,16 +69,7 @@
 
 /**
  * Primary datastore driver for NetApp ONTAP storage systems.
- * This driver handles volume lifecycle operations (create, delete, grant/revoke access)
- * for both iSCSI (LUN-based) and NFS protocols against ONTAP storage backends.
- *
- * For iSCSI protocol:
- * - Creates LUNs on ONTAP and maps them to initiator groups (igroups)
- * - Manages LUN mappings for host access control
- *
- * For NFS protocol:
- * - Delegates file operations to KVM host/libvirt
- * - ONTAP volume/export management handled at storage pool creation time
+ * Handles volume lifecycle operations for iSCSI and NFS protocols.
  */
 public class OntapPrimaryDatastoreDriver implements PrimaryDataStoreDriver {
 
@@ -111,26 +102,16 @@ public DataStoreTO getStoreTO(DataStore store) {
     }
 
     /**
-     * Asynchronously creates a volume on the ONTAP storage system.
-     *
-     * For iSCSI protocol:
-     * - Creates a LUN on ONTAP via the SAN strategy
-     * - Stores LUN UUID and name in volume_details table for later reference
-     * - Creates a LUN mapping to the appropriate igroup (based on cluster/zone scope)
-     * - Sets the iSCSI path on the volume for host attachment
-     *
-     * For NFS protocol:
-     * - Associates the volume with the storage pool (actual file creation handled by hypervisor)
-     *
-     * @param dataStore The target data store (storage pool)
-     * @param dataObject The volume to create
-     * @param callback Callback to notify completion
+     * Creates a volume on the ONTAP storage system.
      */
     @Override
     public void createAsync(DataStore dataStore, DataObject dataObject, AsyncCompletionCallback<CreateCmdResult> callback) {
         CreateCmdResult createCmdResult = null;
         String errMsg;
 
+        if (dataObject == null) {
+            throw new InvalidParameterValueException("createAsync: dataObject should not be null");
+        }
         if (dataStore == null) {
             throw new InvalidParameterValueException("createAsync: dataStore should not be null");
         }
@@ -150,6 +131,7 @@ public void createAsync(DataStore dataStore, DataObject dataObject, AsyncComplet
                 s_logger.error("createAsync: Storage Pool not found for id: " + dataStore.getId());
                 throw new CloudRuntimeException("createAsync: Storage Pool not found for id: " + dataStore.getId());
             }
+            String storagePoolUuid = dataStore.getUuid();
 
             Map<String, String> details = storagePoolDetailsDao.listDetailsKeyPairs(dataStore.getId());
 
@@ -186,7 +168,7 @@ public void createAsync(DataStore dataStore, DataObject dataObject, AsyncComplet
 
                         // Create LUN-to-igroup mapping and retrieve the assigned LUN ID
                         UnifiedSANStrategy sanStrategy = (UnifiedSANStrategy) Utility.getStrategyByStoragePoolDetails(details);
-                        String accessGroupName = Utility.getIgroupName(svmName, storagePool.getScope(), scopeId);
+                        String accessGroupName = Utility.getIgroupName(svmName, storagePoolUuid);
                         String lunNumber = sanStrategy.ensureLunMapped(svmName, lunName, accessGroupName);
 
                         // Construct iSCSI path: /<iqn>/<lun_id> format for KVM/libvirt attachment
@@ -223,12 +205,7 @@ public void createAsync(DataStore dataStore, DataObject dataObject, AsyncComplet
     }
 
     /**
-     * Creates a CloudStack volume on the ONTAP backend using the appropriate storage strategy.
-     *
-     * @param dataStore The target data store
-     * @param dataObject The volume to create
-     * @param details Storage pool configuration details
-     * @return CloudStackVolume containing the created backend object (LUN for iSCSI)
+     * Creates a volume on the ONTAP backend.
      */
     private CloudStackVolume createCloudStackVolume(DataStore dataStore, DataObject dataObject, Map<String, String> details) {
         StoragePoolVO storagePool = storagePoolDao.findById(dataStore.getId());
@@ -249,18 +226,7 @@ private CloudStackVolume createCloudStackVolume(DataStore dataStore, DataObject
     }
 
     /**
-     * Asynchronously deletes a volume from the ONTAP storage system.
-     *
-     * For iSCSI protocol:
-     * - Retrieves LUN details from volume_details table
-     * - Deletes the LUN from ONTAP (LUN mappings are automatically removed)
-     *
-     * For NFS protocol:
-     * - No ONTAP operation needed; file deletion handled by KVM host/libvirt
-     *
-     * @param store The data store containing the volume
-     * @param data The volume to delete
-     * @param callback Callback to notify completion
+     * Deletes a volume from the ONTAP storage system.
      */
     @Override
     public void deleteAsync(DataStore store, DataObject data, AsyncCompletionCallback<CommandResult> callback) {
@@ -344,20 +310,7 @@ public ChapInfo getChapInfo(DataObject dataObject) {
     }
 
     /**
-     * Grants a host access to a volume on the ONTAP storage system.
-     *
-     * For iSCSI protocol:
-     * - Validates that the host's iSCSI initiator (IQN) is present in the target igroup
-     * - Ensures the LUN is mapped to the igroup (creates mapping if not exists)
-     * - Updates the volume's iSCSI path with the assigned LUN ID
-     *
-     * For NFS protocol:
-     * - No explicit grant needed; NFS exports are configured at storage pool level
-     *
-     * @param dataObject The volume to grant access to
-     * @param host The host requesting access
-     * @param dataStore The data store containing the volume
-     * @return true if access was granted successfully
+     * Grants a host access to a volume.
      */
     @Override
     public boolean grantAccess(DataObject dataObject, Host host, DataStore dataStore) {
@@ -377,6 +330,7 @@ public boolean grantAccess(DataObject dataObject, Host host, DataStore dataStore
                 s_logger.error("grantAccess: Storage Pool not found for id: " + dataStore.getId());
                 throw new CloudRuntimeException("grantAccess: Storage Pool not found for id: " + dataStore.getId());
             }
+            String storagePoolUuid = dataStore.getUuid();
 
             // ONTAP managed storage only supports cluster and zone scoped pools
             if (storagePool.getScope() != ScopeType.CLUSTER && storagePool.getScope() != ScopeType.ZONE) {
@@ -398,7 +352,7 @@ public boolean grantAccess(DataObject dataObject, Host host, DataStore dataStore
 
                 if (ProtocolType.ISCSI.name().equalsIgnoreCase(details.get(Constants.PROTOCOL))) {
                     UnifiedSANStrategy sanStrategy = (UnifiedSANStrategy) Utility.getStrategyByStoragePoolDetails(details);
-                    String accessGroupName = Utility.getIgroupName(svmName, storagePool.getScope(), scopeId);
+                    String accessGroupName = Utility.getIgroupName(svmName, storagePoolUuid);
 
                     // Verify host initiator is registered in the igroup before allowing access
                     if (!sanStrategy.validateInitiatorInAccessGroup(host.getStorageUrl(), svmName, accessGroupName)) {
@@ -432,18 +386,7 @@ public boolean grantAccess(DataObject dataObject, Host host, DataStore dataStore
     }
 
     /**
-     * Revokes a host's access to a volume on the ONTAP storage system.
-     *
-     * For iSCSI protocol:
-     * - Validates the volume is not attached to an active VM
-     * - Removes the LUN mapping from the igroup
-     *
-     * For NFS protocol:
-     * - No explicit revoke needed; NFS exports remain at storage pool level
-     *
-     * @param dataObject The volume to revoke access from
-     * @param host The host losing access
-     * @param dataStore The data store containing the volume
+     * Revokes a host's access to a volume.
      */
     @Override
     public void revokeAccess(DataObject dataObject, Host host, DataStore dataStore) {
@@ -467,7 +410,7 @@ public void revokeAccess(DataObject dataObject, Host host, DataStore dataStore)
                             VirtualMachine.State.Destroyed,
                             VirtualMachine.State.Expunging,
                             VirtualMachine.State.Error).contains(vm.getState())) {
-                        s_logger.debug("revokeAccess: Volume [{}] is still attached to VM [{}] in state [{}], skipping revokeAccess",
+                        s_logger.warn("revokeAccess: Volume [{}] is still attached to VM [{}] in state [{}], skipping revokeAccess",
                                 dataObject.getId(), vm.getInstanceName(), vm.getState());
                         return;
                     }
@@ -503,23 +446,19 @@ public void revokeAccess(DataObject dataObject, Host host, DataStore dataStore)
     }
 
     /**
-     * Revokes volume access by removing the LUN mapping from the igroup.
-     * This method handles the iSCSI-specific logic for access revocation.
-     *
-     * @param storagePool The storage pool containing the volume
-     * @param volumeVO The volume to revoke access from
-     * @param host The host losing access
+     * Revokes volume access for the specified host.
      */
     private void revokeAccessForVolume(StoragePoolVO storagePool, VolumeVO volumeVO, Host host) {
         s_logger.info("revokeAccessForVolume: Revoking access to volume [{}] for host [{}]", volumeVO.getName(), host.getName());
 
         Map<String, String> details = storagePoolDetailsDao.listDetailsKeyPairs(storagePool.getId());
         StorageStrategy storageStrategy = Utility.getStrategyByStoragePoolDetails(details);
         String svmName = details.get(Constants.SVM_NAME);
+        String storagePoolUuid = storagePool.getUuid();
         long scopeId = (storagePool.getScope() == ScopeType.CLUSTER) ? host.getClusterId() : host.getDataCenterId();
 
         if (ProtocolType.ISCSI.name().equalsIgnoreCase(details.get(Constants.PROTOCOL))) {
-            String accessGroupName = Utility.getIgroupName(svmName, storagePool.getScope(), scopeId);
+            String accessGroupName = Utility.getIgroupName(svmName, storagePoolUuid);
 
             // Retrieve LUN name from volume details; if missing, volume may not have been fully created
             String lunName = volumeDetailsDao.findDetail(volumeVO.getId(), Constants.LUN_DOT_NAME) != null ?
@@ -563,12 +502,7 @@ private void revokeAccessForVolume(StoragePoolVO storagePool, VolumeVO volumeVO,
     }
 
     /**
-     * Retrieves a CloudStack volume (LUN) from ONTAP by name.
-     *
-     * @param storageStrategy The storage strategy to use for the lookup
-     * @param svmName The SVM name containing the LUN
-     * @param cloudStackVolumeName The LUN name to look up
-     * @return CloudStackVolume if found, null otherwise
+     * Retrieves a volume from ONTAP by name.
      */
     private CloudStackVolume getCloudStackVolumeByName(StorageStrategy storageStrategy, String svmName, String cloudStackVolumeName) {
         Map<String, String> getCloudStackVolumeMap = new HashMap<>();
@@ -584,12 +518,7 @@ private CloudStackVolume getCloudStackVolumeByName(StorageStrategy storageStrate
     }
 
     /**
-     * Retrieves an access group (igroup) from ONTAP by name.
-     *
-     * @param storageStrategy The storage strategy to use for the lookup
-     * @param svmName The SVM name containing the igroup
-     * @param accessGroupName The igroup name to look up
-     * @return AccessGroup if found, null otherwise
+     * Retrieves an access group from ONTAP by name.
      */
     private AccessGroup getAccessGroupByName(StorageStrategy storageStrategy, String svmName, String accessGroupName) {
         Map<String, String> getAccessGroupMap = new HashMap<>();

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java

@@ -152,16 +152,21 @@ public DataStore initialize(Map<String, Object> dsInfos) {
             throw new CloudRuntimeException("ONTAP primary storage must be managed");
         }
 
-        // Required ONTAP detail keys
         Set<String> requiredKeys = Set.of(
                 Constants.USERNAME,
                 Constants.PASSWORD,
                 Constants.SVM_NAME,
                 Constants.PROTOCOL,
-                Constants.MANAGEMENT_LIF,
+                Constants.MANAGEMENT_LIF
+        );
+
+        Set<String> optionalKeys = Set.of(
                 Constants.IS_DISAGGREGATED
         );
 
+        Set<String> allowedKeys = new java.util.HashSet<>(requiredKeys);
+        allowedKeys.addAll(optionalKeys);
+
         // Parse key=value pairs from URL into details (skip empty segments)
         if (url != null && !url.isEmpty()) {
             for (String segment : url.split(Constants.SEMICOLON)) {
@@ -249,13 +254,13 @@ public DataStore initialize(Map<String, Object> dsInfos) {
             case NFS3:
                 parameters.setType(Storage.StoragePoolType.NetworkFilesystem);
                 path = Constants.SLASH + storagePoolName;
-                port = 2049;
+                port = Constants.NFS3_PORT;
                 s_logger.info("Setting NFS path for storage pool: " + path + ", port: " + port);
                 break;
             case ISCSI:
                 parameters.setType(Storage.StoragePoolType.Iscsi);
                 path = storageStrategy.getStoragePath();
-                port = 3260;
+                port = Constants.ISCSI_PORT;
                 s_logger.info("Setting iSCSI path for storage pool: " + path + ", port: " + port);
                 break;
             default: