Leave system VM and router volumes out of all-volumes resource alert rules

Author: Damans227

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

@@ -34,7 +34,10 @@ public interface VolumeDao extends GenericDao<VolumeVO, Long>, StateDao<Volume.S
 
     List<VolumeVO> findByAccount(long accountId);
 
-    List<Long> listIdsByAccountOrDomainsAndState(Long accountId, List<Long> domainIds, Volume.State state);
+    /**
+     * Lists IDs of volumes that are detached or attached to user VMs, leaving out system VM and router volumes.
+     */
+    List<Long> listUserVolumeIdsByAccountOrDomainsAndState(Long accountId, List<Long> domainIds, Volume.State state);
 
     List<VolumeVO> findIncludingRemovedByAccount(long accountId);
 

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

@@ -75,7 +75,6 @@ public class VolumeDaoImpl extends GenericDaoBase<VolumeVO, Long> implements Vol
     private final SearchBuilder<VolumeVO> storeAndInstallPathSearch;
     private final SearchBuilder<VolumeVO> volumeIdSearch;
     protected GenericSearchBuilder<VolumeVO, Long> CountByAccount;
-    protected final GenericSearchBuilder<VolumeVO, Long> IdsByAccountOrDomainsAndStateSearch;
     protected final SearchBuilder<VolumeVO> ExternalUuidSearch;
     protected GenericSearchBuilder<VolumeVO, SumCount> primaryStorageSearch;
     protected GenericSearchBuilder<VolumeVO, SumCount> primaryStorageSearch2;
@@ -100,6 +99,9 @@ public class VolumeDaoImpl extends GenericDaoBase<VolumeVO, Long> implements Vol
 
     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 ";
+    private static final String LIST_USER_VOLUME_IDS = "SELECT vol.id FROM `cloud`.`volumes` vol "
+            + "LEFT JOIN `cloud`.`vm_instance` vm ON vm.id = vol.instance_id "
+            + "WHERE vol.removed IS NULL AND (vol.instance_id IS NULL OR vm.type = 'User')";
 
     private static final String ORDER_ZONE_WIDE_POOLS_NUMBER_OF_VOLUMES_FOR_ACCOUNT = "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 = ? "
             + " AND pool.scope = 'ZONE' AND pool.status='Up' " + " GROUP BY pool.id ORDER BY 2 ASC ";
@@ -120,18 +122,44 @@ public List<VolumeVO> findByAccount(long accountId) {
     }
 
     @Override
-    public List<Long> listIdsByAccountOrDomainsAndState(Long accountId, List<Long> domainIds, Volume.State state) {
-        SearchCriteria<Long> sc = IdsByAccountOrDomainsAndStateSearch.create();
+    public List<Long> listUserVolumeIdsByAccountOrDomainsAndState(Long accountId, List<Long> domainIds, Volume.State state) {
+        if (domainIds != null && domainIds.isEmpty()) {
+            return new ArrayList<>();
+        }
+        StringBuilder sql = new StringBuilder(LIST_USER_VOLUME_IDS);
         if (accountId != null) {
-            sc.setParameters("accountId", accountId);
+            sql.append(" AND vol.account_id = ?");
         }
         if (domainIds != null) {
-            sc.setParameters("domainIds", domainIds.toArray());
+            sql.append(" AND vol.domain_id IN (").append(String.join(",", Collections.nCopies(domainIds.size(), "?"))).append(")");
         }
         if (state != null) {
-            sc.setParameters("state", state);
+            sql.append(" AND vol.state = ?");
         }
-        return customSearch(sc, null);
+        List<Long> ids = new ArrayList<>();
+        TransactionLegacy txn = TransactionLegacy.currentTxn();
+        try (PreparedStatement pstmt = txn.prepareAutoCloseStatement(sql.toString())) {
+            int i = 1;
+            if (accountId != null) {
+                pstmt.setLong(i++, accountId);
+            }
+            if (domainIds != null) {
+                for (Long domainId : domainIds) {
+                    pstmt.setLong(i++, domainId);
+                }
+            }
+            if (state != null) {
+                pstmt.setString(i, state.name());
+            }
+            try (ResultSet rs = pstmt.executeQuery()) {
+                while (rs.next()) {
+                    ids.add(rs.getLong(1));
+                }
+            }
+        } catch (SQLException e) {
+            throw new CloudRuntimeException("Unable to list user volume IDs", e);
+        }
+        return ids;
     }
 
     @Override
@@ -436,13 +464,6 @@ public VolumeDaoImpl() {
         AllFieldsSearch.and("kmsWrappedKeyId", AllFieldsSearch.entity().getKmsWrappedKeyId(), Op.EQ);
         AllFieldsSearch.done();
 
-        IdsByAccountOrDomainsAndStateSearch = createSearchBuilder(Long.class);
-        IdsByAccountOrDomainsAndStateSearch.selectFields(IdsByAccountOrDomainsAndStateSearch.entity().getId());
-        IdsByAccountOrDomainsAndStateSearch.and("accountId", IdsByAccountOrDomainsAndStateSearch.entity().getAccountId(), Op.EQ);
-        IdsByAccountOrDomainsAndStateSearch.and("domainIds", IdsByAccountOrDomainsAndStateSearch.entity().getDomainId(), Op.IN);
-        IdsByAccountOrDomainsAndStateSearch.and("state", IdsByAccountOrDomainsAndStateSearch.entity().getState(), Op.EQ);
-        IdsByAccountOrDomainsAndStateSearch.done();
-
         RootDiskStateSearch = createSearchBuilder();
         RootDiskStateSearch.and("state", RootDiskStateSearch.entity().getState(), Op.IN);
         RootDiskStateSearch.and("vType", RootDiskStateSearch.entity().getVolumeType(), Op.EQ);

engine/schema/src/test/java/com/cloud/storage/dao/VolumeDaoImplTest.java

@@ -114,6 +114,32 @@ public void testListPoolIdsByVolumeCount_without_cluster_details() throws SQLExc
         verify(preparedStatementMock, times(1)).executeQuery();
     }
 
+    @Test
+    public void listUserVolumeIdsByAccountOrDomainsAndStateSkipsSystemVmVolumes() throws SQLException {
+        final String expectedSql = "SELECT vol.id FROM `cloud`.`volumes` vol "
+                + "LEFT JOIN `cloud`.`vm_instance` vm ON vm.id = vol.instance_id "
+                + "WHERE vol.removed IS NULL AND (vol.instance_id IS NULL OR vm.type = 'User')"
+                + " AND vol.domain_id IN (?,?) AND vol.state = ?";
+        when(TransactionLegacy.currentTxn()).thenReturn(transactionMock);
+        when(transactionMock.prepareAutoCloseStatement(expectedSql)).thenReturn(preparedStatementMock);
+        ResultSet rs = Mockito.mock(ResultSet.class);
+        when(rs.next()).thenReturn(true, false);
+        when(rs.getLong(1)).thenReturn(7L);
+        when(preparedStatementMock.executeQuery()).thenReturn(rs);
+
+        List<Long> ids = volumeDao.listUserVolumeIdsByAccountOrDomainsAndState(null, List.of(1L, 2L), Volume.State.Ready);
+
+        Assert.assertEquals(List.of(7L), ids);
+        verify(preparedStatementMock).setLong(1, 1L);
+        verify(preparedStatementMock).setLong(2, 2L);
+        verify(preparedStatementMock).setString(3, "Ready");
+    }
+
+    @Test
+    public void listUserVolumeIdsByAccountOrDomainsAndStateWithNoDomains() {
+        Assert.assertTrue(volumeDao.listUserVolumeIdsByAccountOrDomainsAndState(null, List.of(), Volume.State.Ready).isEmpty());
+    }
+
     @Test
     public void findByInstanceAndNotState_queriesWithInstanceIdAndExcludedStates() {
         SearchBuilder<VolumeVO> sb = Mockito.mock(SearchBuilder.class);

plugins/resource-alerts/src/main/java/org/apache/cloudstack/resourcealert/ResourceAlertManagerImpl.java

@@ -396,7 +396,7 @@ private List<Long> getResourceIds(ResourceAlertRuleVO rule) {
             }
             case Volume: {
                 Pair<Long, List<Long>> scope = getGenericRuleScope(rule);
-                return scope == null ? Collections.emptyList() : volumeDao.listIdsByAccountOrDomainsAndState(
+                return scope == null ? Collections.emptyList() : volumeDao.listUserVolumeIdsByAccountOrDomainsAndState(
                         scope.first(), scope.second(), Volume.State.Ready);
             }
             case Host:

plugins/resource-alerts/src/test/java/org/apache/cloudstack/resourcealert/ResourceAlertManagerImplTest.java

@@ -875,7 +875,7 @@ public void testGenericVolumeRuleForRootAdminListsReadyVolumesCloudWide() {
 
         manager.evaluateRules();
 
-        verify(volumeDao).listIdsByAccountOrDomainsAndState(null, null, Volume.State.Ready);
+        verify(volumeDao).listUserVolumeIdsByAccountOrDomainsAndState(null, null, Volume.State.Ready);
     }
 
     @Test