[CSTACKEX-339] ASUP Upstream review comments

engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java

@@ -183,8 +183,8 @@ public interface VolumeDao extends GenericDao<VolumeVO, Long>, StateDao<Volume.S
     boolean existsWithKmsKey(long kmsKeyId);
 
     /**
-     * Returns true if any VM with a non-destroyed ROOT volume on {@code poolId} also has a
-     * non-destroyed DATADISK on a different primary storage pool. Existence check only
+     * Returns true if any VM with a Ready ROOT volume on poolId also has an attached
+     * Ready DATADISK on a different primary storage pool. Existence check only
      * ({@code LIMIT 1}); does not load volumes into memory.
      */
     boolean hasMultiPrimaryStoragePoolVm(long poolId);

engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java

@@ -96,14 +96,14 @@ public class VolumeDaoImpl extends GenericDaoBase<VolumeVO, Long> implements Vol
     protected static final String SELECT_HYPERTYPE_FROM_CLUSTER_VOLUME = "SELECT c.hypervisor_type from volumes v, storage_pool s, cluster c where v.pool_id = s.id and s.cluster_id = c.id and v.id = ?";
     protected static final String SELECT_HYPERTYPE_FROM_ZONE_VOLUME = "SELECT s.hypervisor from volumes v, storage_pool s where v.pool_id = s.id and v.id = ?";
     protected static final String SELECT_POOLSCOPE = "SELECT s.scope from storage_pool s, volumes v where s.id = v.pool_id and v.id = ?";
-    // Looks for a VM whose root disk is on this pool and whose data disk is on another pool.
+    // Looks for a VM whose Ready root disk is on this pool and whose attached Ready data disk is on another pool.
     // LIMIT 1 stops after the first match so we do not load all volumes.
     private static final String HAS_MULTI_PRIMARY_STORAGE_POOL_VM =
             "SELECT 1 FROM volumes root INNER JOIN volumes data ON data.instance_id = root.instance_id "
                     + "WHERE root.pool_id = ? AND root.volume_type = 'ROOT' AND root.instance_id IS NOT NULL "
-                    + "AND root.removed IS NULL AND root.state NOT IN ('Destroy', 'Expunged') "
+                    + "AND root.removed IS NULL AND root.state = 'Ready' "
                     + "AND data.volume_type = 'DATADISK' AND data.pool_id IS NOT NULL AND data.pool_id <> ? "
-                    + "AND data.removed IS NULL AND data.state NOT IN ('Destroy', 'Expunged') LIMIT 1";
+                    + "AND data.removed IS NULL AND data.state = 'Ready' LIMIT 1";
 
     private static final String ORDER_POOLS_NUMBER_OF_VOLUMES_FOR_ACCOUNT_PART1 = "SELECT pool.id, SUM(IF(vol.state='Ready' AND vol.account_id = ?, 1, 0)) FROM `cloud`.`storage_pool` pool LEFT JOIN `cloud`.`volumes` vol ON pool.id = vol.pool_id WHERE pool.data_center_id = ? ";
     private static final String ORDER_POOLS_NUMBER_OF_VOLUMES_FOR_ACCOUNT_PART2 = " GROUP BY pool.id ORDER BY 2 ASC ";

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);
@@ -450,9 +457,9 @@ private void addStoragePoolUsage(StoragePoolVO pool, Map<String, Object> payload
     }
 
     /**
-     * Adds {@code hasMultiPrimaryStoragePoolVm}: true when at least one VM with ROOT on this
-     * pool also has an attached DATADISK on a different primary storage pool. Uses a single
-     * {@code LIMIT 1} existence query.
+     * Adds {@code hasMultiPrimaryStoragePoolVm}: true when at least one VM with a Ready ROOT
+     * on this pool also has an attached Ready DATADISK on a different primary storage pool.
+     * Uses a single {@code LIMIT 1} existence query.
      */
     private void hasMultiPrimaryStoragePoolVm(StoragePoolVO pool, Map<String, Object> payload) {
         try {

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();