[CSTACKEX-339] ASUP Upstream review comments

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/asup/OntapAsupManager.java

@@ -268,14 +268,18 @@ protected void pushAsupTelemetry() {
                 logger.debug("ONTAP ASUP: another management server holds the ASUP lock; skipping this cycle.");
                 return;
             }
-            logger.debug("ONTAP ASUP: pushing telemetry for {} pool(s) [CloudStack version={}]",
-                    pools.size(), getCloudStackVersion());
-            Map<String, AsupClusterClient> clientsByStorageIp = new HashMap<>();
-            for (StoragePoolVO pool : pools) {
-                pushAsupForStoragePool(pool, clientsByStorageIp);
+            try {
+                logger.debug("ONTAP ASUP: pushing telemetry for {} pool(s) [CloudStack version={}]",
+                        pools.size(), getCloudStackVersion());
+                Map<String, AsupClusterClient> clientsByStorageIp = new HashMap<>();
+                for (StoragePoolVO pool : pools) {
+                    pushAsupForStoragePool(pool, clientsByStorageIp);
+                }
+            } finally {
+                lock.unlock();
             }
         } finally {
-            lock.unlock();
+            lock.releaseRef();
         }
     }
 
@@ -430,17 +434,20 @@ private void addStoragePoolUsage(StoragePoolVO pool, Map<String, Object> payload
             // Only count volumes that definitely have a physical object on the ONTAP FlexVolume.
             // "Allocated" volumes have a pool_id row in the CS DB but ONTAP provisioning has not
             // yet been called, so including them would inflate counts and provisioned size.
-            List<VolumeVO> cstackVolumes = volumes.stream()
-                    .filter(v -> CS_VOLUME_STATES.contains(v.getState()))
-                    .collect(java.util.stream.Collectors.toList());
-
-            long rootDiskCount = cstackVolumes.stream()
-                    .filter(v -> Volume.Type.ROOT.equals(v.getVolumeType())).count();
-            long dataDiskCount = cstackVolumes.stream()
-                    .filter(v -> Volume.Type.DATADISK.equals(v.getVolumeType())).count();
-
-            long totalLogicalSizeBytes = cstackVolumes.stream()
-                    .mapToLong(v -> v.getSize() != null ? v.getSize() : 0L).sum();
+            long rootDiskCount = 0L;
+            long dataDiskCount = 0L;
+            long totalLogicalSizeBytes = 0L;
+            for (VolumeVO volume : volumes) {
+                if (!CS_VOLUME_STATES.contains(volume.getState())) {
+                    continue;
+                }
+                if (Volume.Type.ROOT.equals(volume.getVolumeType())) {
+                    rootDiskCount++;
+                } else if (Volume.Type.DATADISK.equals(volume.getVolumeType())) {
+                    dataDiskCount++;
+                }
+                totalLogicalSizeBytes += volume.getSize() != null ? volume.getSize() : 0L;
+            }
             payload.put(OntapStorageConstants.ASUP_ROOT_DISK_COUNT, rootDiskCount);
             payload.put(OntapStorageConstants.ASUP_DATA_DISK_COUNT, dataDiskCount);
             payload.put(OntapStorageConstants.ASUP_TOTAL_LOGICAL_SIZE_BYTES, totalLogicalSizeBytes);

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

@@ -152,6 +152,8 @@ public Cluster getClusterInfo() {
      * Sets {@link Cluster#setModel(String)} and {@link Cluster#setPlatformType(String)} from one
      * {@code GET /api/cluster/nodes}. Distinct models are joined with a comma. Platform type is
      * {@code performance}, {@code capacity}, {@code fas}, or {@code composite} when personalities mix.
+     * A node that omits {@code is_all_flash_optimized} is left out of the rollup so a partial
+     * response is not reported as {@code fas}.
      */
     private void populateNodeAsupFields(Cluster cluster, String authHeader) {
         try {
@@ -170,7 +172,10 @@ private void populateNodeAsupFields(Cluster cluster, String authHeader) {
                 if (node.getModel() != null && !node.getModel().isBlank()) {
                     models.add(node.getModel().trim());
                 }
-                platformTypes.add(classifyNodePlatformType(node));
+                String platformType = classifyNodePlatformType(node);
+                if (platformType != null) {
+                    platformTypes.add(platformType);
+                }
             }
             if (!models.isEmpty()) {
                 cluster.setModel(String.join(OntapStorageConstants.COMMA, models));
@@ -210,14 +215,17 @@ public String getClusterVersion(Cluster cluster) {
     /**
      * Classifies one node: not all-flash → {@code fas}; all-flash + capacity → {@code capacity};
      * otherwise all-flash (including performance-optimized or classic AFF) → {@code performance}.
+     * Returns {@code null} when {@code is_all_flash_optimized} is absent, so unknown hardware is
+     * not reported as {@code fas}.
      */
     private static String classifyNodePlatformType(ClusterNode node) {
-        boolean allFlash = Boolean.TRUE.equals(node.getAllFlashOptimized());
-        boolean capacity = Boolean.TRUE.equals(node.getCapacityOptimized());
-        if (!allFlash) {
+        if (node.getAllFlashOptimized() == null) {
+            return null;
+        }
+        if (!node.getAllFlashOptimized()) {
             return OntapStorageConstants.ASUP_PLATFORM_TYPE_FAS;
         }
-        if (capacity) {
+        if (Boolean.TRUE.equals(node.getCapacityOptimized())) {
             return OntapStorageConstants.ASUP_PLATFORM_TYPE_CAPACITY;
         }
         return OntapStorageConstants.ASUP_PLATFORM_TYPE_PERFORMANCE;

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

@@ -368,6 +368,31 @@ public void testGetClusterInfo_mixedPersonalitiesIsComposite() {
         assertEquals(OntapStorageConstants.ASUP_PLATFORM_TYPE_COMPOSITE, result.getPlatformType());
     }
 
+    @Test
+    public void testGetClusterInfo_missingAllFlashFlagLeavesPlatformUnset() {
+        stubClusterGet();
+        when(clusterFeignClient.getClusterNodes(anyString(), anyMap()))
+                .thenReturn(new OntapResponse<>(List.of(
+                        clusterNode("AFF-A400", null, null, null))));
+
+        Cluster result = storageStrategy.getClusterInfo();
+        assertEquals("AFF-A400", result.getModel());
+        assertNull(result.getPlatformType());
+    }
+
+    @Test
+    public void testGetClusterInfo_missingFlagDoesNotChangeKnownPlatform() {
+        stubClusterGet();
+        when(clusterFeignClient.getClusterNodes(anyString(), anyMap()))
+                .thenReturn(new OntapResponse<>(List.of(
+                        clusterNode("AFF-A400", true, true, false),
+                        clusterNode("AFF-A400", null, null, null))));
+
+        Cluster result = storageStrategy.getClusterInfo();
+        assertEquals("AFF-A400", result.getModel());
+        assertEquals(OntapStorageConstants.ASUP_PLATFORM_TYPE_PERFORMANCE, result.getPlatformType());
+    }
+
     @Test
     public void testGetClusterInfo_nodesGetFailureLeavesModelUnset() {
         stubClusterGet();