Addressed review comments

Author: sandeeplocharla

engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java

@@ -766,8 +766,8 @@ protected Void managedCopyBaseImageCallback(AsyncCallbackDispatcher<VolumeServic
 
             if (templateObjectTo.getFormat() != null) {
                 PrimaryDataStore primaryDataStore = context.getPrimaryDataStore();
-                boolean isOntap = primaryDataStore != null && DataStoreProvider.ONTAP_PLUGIN_NAME.equals(primaryDataStore.getStorageProviderName());
-                if (!isOntap || volume.getFormat() == null) {
+                boolean isOntapDataStore = primaryDataStore != null && DataStoreProvider.ONTAP_PLUGIN_NAME.equals(primaryDataStore.getStorageProviderName());
+                if (!isOntapDataStore || volume.getFormat() == null) {
                     volume.setFormat(templateObjectTo.getFormat());
                 }
             }

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java

@@ -33,6 +33,7 @@
 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.collections.MapUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
@@ -91,7 +92,7 @@ public boolean hostConnect(long hostId, long poolId)  {
         try {
             // Load storage pool details from database to pass mount options and other config to agent
             Map<String, String> detailsMap = _storagePoolDetailsDao.listDetailsKeyPairs(poolId);
-            if (detailsMap == null || detailsMap.isEmpty()) {
+            if (MapUtils.isEmpty(detailsMap)) {
                 logger.error("hostConnect: Failed to load storage pool details for pool id: {}", poolId);
                 return false;
             }
@@ -247,7 +248,7 @@ public boolean hostRemoved(long hostId, long clusterId) {
     private void removeHostFromOntapPoolIfNeeded(StoragePoolVO pool, Host host) {
         try {
             Map<String, String> detailsMap = _storagePoolDetailsDao.listDetailsKeyPairs(pool.getId());
-            if (detailsMap == null || detailsMap.isEmpty()) {
+            if (MapUtils.isEmpty(detailsMap)) {
                 logger.debug("hostAboutToBeRemoved: removeHostFromOntapPoolIfNeeded: No pool details found for pool id: {}", pool.getId());
                 return;
             }

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java

@@ -19,6 +19,7 @@
 
 package org.apache.cloudstack.storage.service;
 
+import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -190,6 +191,7 @@ private void validateAndSelectAggregatesForVolumeCreation(String authHeader, Str
             logger.error("No aggregates are assigned to SVM " + svmName);
             throw new CloudRuntimeException("No aggregates are assigned to SVM " + svmName);
         }
+        List<Aggregate> eligibleAggregates = new ArrayList<>();
         for (Aggregate aggr : aggrs) {
             logger.debug("Found aggregate: " + aggr.getName() + " with UUID: " + aggr.getUuid());
             Aggregate aggrResp = aggregateFeignClient.getAggregateByUUID(authHeader, aggr.getUuid(),
@@ -209,12 +211,13 @@ private void validateAndSelectAggregatesForVolumeCreation(String authHeader, Str
                 continue;
             }
             logger.info("Selected aggregate: " + aggr.getName() + " for volume operations.");
-            this.aggregates = List.of(aggr);
+            eligibleAggregates.add(aggrResp);
         }
-        if (this.aggregates == null || this.aggregates.isEmpty()) {
+        if (eligibleAggregates.isEmpty()) {
             logger.error("No suitable aggregates found on SVM " + svmName + " for volume creation.");
             throw new CloudRuntimeException("No suitable aggregates found on SVM " + svmName + " for volume creation.");
         }
+        this.aggregates = eligibleAggregates;
     }
 
     // Common methods like create/delete etc., should be here
@@ -528,7 +531,10 @@ public Pair<String, String> getNetworkInterface() {
             IpInterface fallbackInterface = null;
 
             for (IpInterface iface : response.getRecords()) {
-                if (!Boolean.TRUE.equals(iface.getEnabled()) || !OntapStorageConstants.LIF_STATE_UP.equals(iface.getState())) {
+                if (iface == null || !Boolean.TRUE.equals(iface.getEnabled()) || !OntapStorageConstants.LIF_STATE_UP.equals(iface.getState())) {
+                    continue;
+                }
+                if (iface.getIp() == null || iface.getIp().getAddress() == null) {
                     continue;
                 }
                 if (!isIPv4Address(iface.getIp().getAddress())) {

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java

@@ -49,6 +49,8 @@
 import org.apache.cloudstack.storage.utils.OntapStorageConstants;
 import org.apache.cloudstack.storage.utils.OntapStorageUtils;
 import org.apache.cloudstack.storage.volume.VolumeObject;
+import org.apache.commons.collections.MapUtils;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
@@ -213,11 +215,11 @@ public AccessGroup updateAccessGroup(AccessGroup accessGroup) {
         }
 
         Map<String, String> details = storagePoolDetailsDao.listDetailsKeyPairs(accessGroup.getStoragePoolId());
-        if (details == null || details.isEmpty()) {
+        if (MapUtils.isEmpty(details)) {
             throw new CloudRuntimeException("No storage pool details found for storagePoolId: " + accessGroup.getStoragePoolId());
         }
         String exportPolicyId = details.get(OntapStorageConstants.EXPORT_POLICY_ID);
-        if (exportPolicyId == null || exportPolicyId.isEmpty()) {
+        if (StringUtils.isBlank(exportPolicyId)) {
             throw new CloudRuntimeException("No export policy found for storagePoolId: " + accessGroup.getStoragePoolId());
         }
 
@@ -446,6 +448,10 @@ private ExportPolicy createExportPolicyRequest(AccessGroup accessGroup,String sv
             String ip = (hostStorageIp != null && !hostStorageIp.isEmpty())
                     ? hostStorageIp
                     : (host.getPrivateIpAddress() != null ? host.getPrivateIpAddress().trim() : null);
+            if (StringUtils.isBlank(ip)) {
+                logger.warn("Skipping host {} while creating export policy because it has no storage or private IP address", host.getId());
+                continue;
+            }
             String ipToUse = ip + "/32";
             ExportRule.ExportClient exportClient = new ExportRule.ExportClient();
             exportClient.setMatch(ipToUse);

plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedNASStrategyTest.java

@@ -953,4 +953,5 @@ public void testUpdateAccessGroup_TrimsWhitespaceFromPrivateIp() {
         List<ExportRule.ExportClient> clients = existingPolicy.getRules().get(0).getClients();
         assertEquals(1, clients.size());
         assertEquals("192.168.1.10/32", clients.get(0).getMatch());
-    }}
+    }
+}

server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java

@@ -1638,7 +1638,6 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) throws ResourceAllocationExc
         boolean isKvmAndFileBasedStorage = isHypervisorKvmAndFileBasedStorage(volume, storagePool);
         boolean backupSnapToSecondary = isBackupSnapshotToSecondaryForZone(volume.getDataCenterId());
 
-
         StoragePoolType poolType = volume.getStoragePoolType();
 
         updateSnapshotPayload(volume.getPoolId(), payload, isKvmAndFileBasedStorage, poolType, clusterId);
@@ -1647,7 +1646,7 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) throws ResourceAllocationExc
         // They must not use secondary archive bookkeeping (postSnapshotDirectlyToSecondary) or a physical
         // secondary copy — delete is handled via StorageSystemSnapshotStrategy → driver deleteAsync.
         boolean archiveSnapshotToSecondary = backupSnapToSecondary
-                && !isManagedPrimaryLocationSnapshot(storagePool, payload);
+                && !isOntapManagedPrimaryLocationSnapshot(storagePool, payload);
 
         if (isKvmAndFileBasedStorage && archiveSnapshotToSecondary) {
             DataStore imageStore = snapshotSrv.findSnapshotImageStore(snapshot);
@@ -1681,7 +1680,7 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) throws ResourceAllocationExc
                     postSnapshotDirectlyToSecondary(snapshot, snapshotOnPrimary, snapshotId);
                 }
             } else {
-                if (backupSnapToSecondary && isManagedPrimaryLocationSnapshot(storagePool, payload)) {
+                if (backupSnapToSecondary && isOntapManagedPrimaryLocationSnapshot(storagePool, payload)) {
                     logger.info("takeSnapshot: snapshot [{}] on NetApp ONTAP managed primary pool [{}] with locationType=PRIMARY — "
                             + "keeping snapshot on primary/array storage only; not archiving to secondary "
                             + "(backup.snapshot.after.take is ignored for this snapshot class)",
@@ -1787,7 +1786,7 @@ private void postSnapshotDirectlyToSecondary(SnapshotInfo snapshot, SnapshotInfo
      *
      * <p>Other managed storage providers are not affected by this check.</p>
      */
-    private boolean isManagedPrimaryLocationSnapshot(StoragePool storagePool, CreateSnapshotPayload payload) {
+    private boolean isOntapManagedPrimaryLocationSnapshot(StoragePool storagePool, CreateSnapshotPayload payload) {
         return storagePool != null && storagePool.isManaged()
                 && DataStoreProvider.ONTAP_PLUGIN_NAME.equals(storagePool.getStorageProviderName())
                 && Snapshot.LocationType.PRIMARY.equals(payload.getLocationType());