Fix reorder/list pools when cluster details are not set, while deploying vm / attaching volume (#8373)

This PR fixes reorder/list pools when cluster details are not set, while deploying vm / attaching volume.

Problem:
Attach volume to a VM fails, on infra with zone-wide pools & vm.allocation.algorithm=userdispersing as the cluster details are not set (passed as null) while reordering / listing pools by volumes.

Solution:
Ignore cluster details when not set, while reordering / listing pools by volumes.

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

@@ -83,8 +83,9 @@ public class VolumeDaoImpl extends GenericDaoBase<VolumeVO, Long> implements Vol
     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 = ?";
 
-    private static final String ORDER_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 = ? "
+    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 = ? ";
-            + " AND pool.pod_id = ? AND pool.cluster_id = ? " + " GROUP BY pool.id ORDER BY 2 ASC ";
+    private static final String ORDER_POOLS_NUMBER_OF_VOLUMES_FOR_ACCOUNT_PART2 = " GROUP BY pool.id ORDER BY 2 ASC ";
+
     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 ";
 
@@ -612,14 +613,27 @@ public class VolumeDaoImpl extends GenericDaoBase<VolumeVO, Long> implements Vol
     public List<Long> listPoolIdsByVolumeCount(long dcId, Long podId, Long clusterId, long accountId) {
         TransactionLegacy txn = TransactionLegacy.currentTxn();
         PreparedStatement pstmt = null;
-        List<Long> result = new ArrayList<Long>();
+        List<Long> result = new ArrayList<>();
+        StringBuilder sql = new StringBuilder(ORDER_POOLS_NUMBER_OF_VOLUMES_FOR_ACCOUNT_PART1);
         try {
-            String sql = ORDER_POOLS_NUMBER_OF_VOLUMES_FOR_ACCOUNT;
+            List<Long> resourceIdList = new ArrayList<>();
-            pstmt = txn.prepareAutoCloseStatement(sql);
+            resourceIdList.add(accountId);
-            pstmt.setLong(1, accountId);
+            resourceIdList.add(dcId);
-            pstmt.setLong(2, dcId);
+
-            pstmt.setLong(3, podId);
+            if (podId != null) {
-            pstmt.setLong(4, clusterId);
+                sql.append(" AND pool.pod_id = ?");
+                resourceIdList.add(podId);
+            }
+            if (clusterId != null) {
+                sql.append(" AND pool.cluster_id = ?");
+                resourceIdList.add(clusterId);
+            }
+            sql.append(ORDER_POOLS_NUMBER_OF_VOLUMES_FOR_ACCOUNT_PART2);
+
+            pstmt = txn.prepareAutoCloseStatement(sql.toString());
+            for (int i = 0; i < resourceIdList.size(); i++) {
+                pstmt.setLong(i + 1, resourceIdList.get(i));
+            }
 
             ResultSet rs = pstmt.executeQuery();
             while (rs.next()) {
@@ -627,9 +641,11 @@ public class VolumeDaoImpl extends GenericDaoBase<VolumeVO, Long> implements Vol
             }
             return result;
         } catch (SQLException e) {
-            throw new CloudRuntimeException("DB Exception on: " + ORDER_POOLS_NUMBER_OF_VOLUMES_FOR_ACCOUNT, e);
+            s_logger.debug("DB Exception on: " + sql.toString(), e);
+            throw new CloudRuntimeException(e);
         } catch (Throwable e) {
-            throw new CloudRuntimeException("Caught: " + ORDER_POOLS_NUMBER_OF_VOLUMES_FOR_ACCOUNT, e);
+            s_logger.debug("Caught: " + sql.toString(), e);
+            throw new CloudRuntimeException(e);
         }
     }
 

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

@@ -0,0 +1,105 @@
+// 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.
+package com.cloud.storage.dao;
+
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.ArgumentMatchers.startsWith;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.sql.SQLException;
+
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import com.cloud.utils.db.TransactionLegacy;
+
+@RunWith(MockitoJUnitRunner.class)
+public class VolumeDaoImplTest {
+    @Mock
+    private PreparedStatement preparedStatementMock;
+
+    @Mock
+    private TransactionLegacy transactionMock;
+
+    private static MockedStatic<TransactionLegacy> mockedTransactionLegacy;
+
+    private final VolumeDaoImpl volumeDao = new VolumeDaoImpl();
+
+    @BeforeClass
+    public static void init() {
+        mockedTransactionLegacy = Mockito.mockStatic(TransactionLegacy.class);
+    }
+
+    @AfterClass
+    public static void close() {
+        mockedTransactionLegacy.close();
+    }
+
+    @Test
+    public void testListPoolIdsByVolumeCount_with_cluster_details() throws SQLException {
+        final String ORDER_POOLS_NUMBER_OF_VOLUMES_FOR_ACCOUNT_QUERY_WITH_CLUSTER =
+                "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.pod_id = ? AND pool.cluster_id = ? GROUP BY pool.id ORDER BY 2 ASC ";
+        final long dcId = 1, accountId = 1;
+        final Long podId = 1L, clusterId = 1L;
+
+        when(TransactionLegacy.currentTxn()).thenReturn(transactionMock);
+        when(transactionMock.prepareAutoCloseStatement(startsWith(ORDER_POOLS_NUMBER_OF_VOLUMES_FOR_ACCOUNT_QUERY_WITH_CLUSTER))).thenReturn(preparedStatementMock);
+        ResultSet rs = Mockito.mock(ResultSet.class);
+        when(preparedStatementMock.executeQuery()).thenReturn(rs, rs);
+
+        volumeDao.listPoolIdsByVolumeCount(dcId, podId, clusterId, accountId);
+
+        verify(transactionMock, times(1)).prepareAutoCloseStatement(ORDER_POOLS_NUMBER_OF_VOLUMES_FOR_ACCOUNT_QUERY_WITH_CLUSTER);
+        verify(preparedStatementMock, times(1)).setLong(1, accountId);
+        verify(preparedStatementMock, times(1)).setLong(2, dcId);
+        verify(preparedStatementMock, times(1)).setLong(3, podId);
+        verify(preparedStatementMock, times(1)).setLong(4, clusterId);
+        verify(preparedStatementMock, times(4)).setLong(anyInt(), anyLong());
+        verify(preparedStatementMock, times(1)).executeQuery();
+    }
+
+    @Test
+    public void testListPoolIdsByVolumeCount_without_cluster_details() throws SQLException {
+        final String ORDER_POOLS_NUMBER_OF_VOLUMES_FOR_ACCOUNT_QUERY_WITHOUT_CLUSTER =
+                "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 = ?  GROUP BY pool.id ORDER BY 2 ASC ";
+        final long dcId = 1, accountId = 1;
+
+        when(TransactionLegacy.currentTxn()).thenReturn(transactionMock);
+        when(transactionMock.prepareAutoCloseStatement(startsWith(ORDER_POOLS_NUMBER_OF_VOLUMES_FOR_ACCOUNT_QUERY_WITHOUT_CLUSTER))).thenReturn(preparedStatementMock);
+        ResultSet rs = Mockito.mock(ResultSet.class);
+        when(preparedStatementMock.executeQuery()).thenReturn(rs, rs);
+
+        volumeDao.listPoolIdsByVolumeCount(dcId, null, null, accountId);
+
+        verify(transactionMock, times(1)).prepareAutoCloseStatement(ORDER_POOLS_NUMBER_OF_VOLUMES_FOR_ACCOUNT_QUERY_WITHOUT_CLUSTER);
+        verify(preparedStatementMock, times(1)).setLong(1, accountId);
+        verify(preparedStatementMock, times(1)).setLong(2, dcId);
+        verify(preparedStatementMock, times(2)).setLong(anyInt(), anyLong());
+        verify(preparedStatementMock, times(1)).executeQuery();
+    }
+}

engine/storage/src/test/java/org/apache/cloudstack/storage/allocator/AbstractStoragePoolAllocatorTest.java

@@ -17,13 +17,13 @@
 package org.apache.cloudstack.storage.allocator;
 
 
-import com.cloud.deploy.DeploymentPlan;
+import static org.mockito.Mockito.when;
-import com.cloud.deploy.DeploymentPlanner;
+
-import com.cloud.storage.Storage;
+import java.util.ArrayList;
-import com.cloud.storage.StoragePool;
+import java.util.HashSet;
-import com.cloud.user.Account;
+import java.util.List;
-import com.cloud.vm.DiskProfile;
+import java.util.Set;
-import com.cloud.vm.VirtualMachineProfile;
+
 import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
 import org.junit.After;
 import org.junit.Assert;
@@ -34,10 +34,14 @@ import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.mockito.junit.MockitoJUnitRunner;
 
-import java.util.ArrayList;
+import com.cloud.deploy.DeploymentPlan;
-import java.util.HashSet;
+import com.cloud.deploy.DeploymentPlanner;
-import java.util.List;
+import com.cloud.storage.Storage;
-import java.util.Set;
+import com.cloud.storage.StoragePool;
+import com.cloud.storage.dao.VolumeDao;
+import com.cloud.user.Account;
+import com.cloud.vm.DiskProfile;
+import com.cloud.vm.VirtualMachineProfile;
 
 @RunWith(MockitoJUnitRunner.class)
 public class AbstractStoragePoolAllocatorTest {
@@ -51,6 +55,9 @@ public class AbstractStoragePoolAllocatorTest {
     Account account;
     private List<StoragePool> pools;
 
+    @Mock
+    VolumeDao volumeDao;
+
     @Before
     public void setUp() {
         pools = new ArrayList<>();
@@ -83,6 +90,29 @@ public class AbstractStoragePoolAllocatorTest {
         Mockito.verify(allocator, Mockito.times(0)).reorderRandomPools(pools);
     }
 
+    @Test
+    public void reorderStoragePoolsBasedOnAlgorithm_userdispersing_reorder_check() {
+        allocator.allocationAlgorithm = "userdispersing";
+        allocator.volumeDao = volumeDao;
+
+        when(plan.getDataCenterId()).thenReturn(1l);
+        when(plan.getPodId()).thenReturn(1l);
+        when(plan.getClusterId()).thenReturn(1l);
+        when(account.getAccountId()).thenReturn(1l);
+        List<Long> poolIds = new ArrayList<>();
+        poolIds.add(1l);
+        poolIds.add(9l);
+        when(volumeDao.listPoolIdsByVolumeCount(1l,1l,1l,1l)).thenReturn(poolIds);
+
+        List<StoragePool> reorderedPools = allocator.reorderStoragePoolsBasedOnAlgorithm(pools, plan, account);
+        Assert.assertEquals(poolIds.size(),reorderedPools.size());
+
+        Mockito.verify(allocator, Mockito.times(0)).reorderPoolsByCapacity(plan, pools);
+        Mockito.verify(allocator, Mockito.times(1)).reorderPoolsByNumberOfVolumes(plan, pools, account);
+        Mockito.verify(allocator, Mockito.times(0)).reorderRandomPools(pools);
+        Mockito.verify(volumeDao, Mockito.times(1)).listPoolIdsByVolumeCount(1l,1l,1l,1l);
+    }
+
     @Test
     public void reorderStoragePoolsBasedOnAlgorithm_firstfitleastconsumed() {
         allocator.allocationAlgorithm = "firstfitleastconsumed";