[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6307: fix pseudo random behaviour in pool selection

[GitBox <gi...@apache.org>]

DaanHoogland commented on code in PR #6307:
URL: https://github.com/apache/cloudstack/pull/6307#discussion_r857398503


##########
engine/storage/src/main/java/org/apache/cloudstack/storage/allocator/AbstractStoragePoolAllocator.java:
##########
@@ -169,25 +175,57 @@ protected List<StoragePool> reorderPoolsByNumberOfVolumes(DeploymentPlan plan, L
 
     @Override
     public List<StoragePool> reorderPools(List<StoragePool> pools, VirtualMachineProfile vmProfile, DeploymentPlan plan, DiskProfile dskCh) {
+        if (s_logger.isTraceEnabled()) {
+            s_logger.trace("reordering pools");
+        }
         if (pools == null) {
             return null;
         }
+        if (s_logger.isTraceEnabled()) {
+            s_logger.trace(String.format("reordering %d pools", pools.size()));
+        }
         Account account = null;
         if (vmProfile.getVirtualMachine() != null) {
             account = vmProfile.getOwner();
         }
 
         if (allocationAlgorithm.equals("random") || allocationAlgorithm.equals("userconcentratedpod_random") || (account == null)) {
-            // Shuffle this so that we don't check the pools in the same order.
-            Collections.shuffle(pools);
+            if (s_logger.isTraceEnabled()) {
+                s_logger.trace(String.format("Shuffle this so that we don't check the pools in the same order. Algorithm == '%s' (or no account?)",allocationAlgorithm));
+                StringBuilder pooltable = new StringBuilder();
+                pooltable.append("pools to choose from: ");
+                int i = 1;
+                for (StoragePool pool : pools) {
+                    pooltable.append("\nno ").append(i).append(": ").append(pool.getName()).append("/").append(pool.getUuid());
+                }
+                s_logger.trace(pooltable.toString());
+            }
+            Collections.shuffle(pools, secureRandom);
+            if (s_logger.isTraceEnabled()) {
+                StringBuilder pooltable = new StringBuilder();
+                pooltable.append("shuffled list of pools to choose from: ");
+                int i = 1;
+                for (StoragePool pool : pools) {
+                    pooltable.append("\nno ").append(i).append(": ").append(pool.getName()).append("/").append(pool.getUuid());
+                }
+                s_logger.trace(pooltable.toString());
+            }
         } else if (allocationAlgorithm.equals("userdispersing")) {
+            if (s_logger.isTraceEnabled()) {
+                s_logger.trace(String.format("reordering: Algorithm == '%s'",allocationAlgorithm));
+            }
             pools = reorderPoolsByNumberOfVolumes(plan, pools, account);
         } else if(allocationAlgorithm.equals("firstfitleastconsumed")){
+            if (s_logger.isTraceEnabled()) {
+                s_logger.trace(String.format("reordering: Algorithm == '%s'",allocationAlgorithm));
+            }
             pools = reorderPoolsByCapacity(plan, pools);

Review Comment:
   I don´t see how, @GutoVeronezi . let me remove the unnecessary `String.format()`. Is this what you mean?
   ```suggestion
           } else if (allocationAlgorithm.equals("userdispersing")) {
               if (s_logger.isTraceEnabled()) {
                   s_logger.trace("reordering: Algorithm == ´userdispersing´");
               }
               pools = reorderPoolsByNumberOfVolumes(plan, pools, account);
           } else if(allocationAlgorithm.equals("firstfitleastconsumed")){
               if (s_logger.isTraceEnabled()) {
                   s_logger.trace("reordering: Algorithm == 'firstfitleastconsumed'"));
               }
               pools = reorderPoolsByCapacity(plan, pools);
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org