load balance individual hosts when clusters are to big to distribute

engine/orchestration/src/main/java/com/cloud/cluster/agentlb/ClusterBasedAgentLoadBalancerPlanner.java

@@ -88,37 +88,54 @@ public List<HostVO> getHostsToRebalance(ManagementServerHostVO ms, int avLoad) {
         hostToClusterMap = sortByClusterSize(hostToClusterMap);
 
         int hostsToGive = allHosts.size() - avLoad;
-        int hostsLeftToGive = hostsToGive;
-        int hostsLeft = directHosts.size();
-        List<HostVO> hostsToReturn = new ArrayList<HostVO>();
 
         logger.debug("Management server {} can give away {} as it currently owns {} and the " +
                 "average agent load in the system is {}; finalyzing list of hosts to give away...",
                 ms, hostsToGive, allHosts.size(), avLoad);
+        List<HostVO> hostsToReturn = selectHostsToGiveAway(hostToClusterMap, hostsToGive, directHosts.size());
+
+        logger.debug("Management server {} is ready to give away {} hosts", ms, hostsToReturn.size());
+        return hostsToReturn;
+    }
+
+    /**
+     * Picks hosts to hand over from a management server that is over its average agent load.
+     * Whole clusters are preferred (so a hypervisor cluster stays on a single management server
+     * whenever possible), but when no combination of whole clusters can satisfy {@code hostsToGive}
+     * (e.g. a single cluster holds all the hosts), a partial subset of the last cluster considered
+     * is taken instead of giving away nothing, so rebalancing still makes progress.
+     */
+    protected List<HostVO> selectHostsToGiveAway(Map<Long, List<HostVO>> hostToClusterMap, int hostsToGive, int totalDirectHosts) {
+        int hostsLeftToGive = hostsToGive;
+        int hostsLeft = totalDirectHosts;
+        List<HostVO> hostsToReturn = new ArrayList<HostVO>();
+
         for (Long cluster : hostToClusterMap.keySet()) {
             List<HostVO> hostsInCluster = hostToClusterMap.get(cluster);
             hostsLeft = hostsLeft - hostsInCluster.size();
-            if (hostsToReturn.size() < hostsToGive) {
-                logger.debug("Trying cluster id=" + cluster);
-
-                if (hostsInCluster.size() > hostsLeftToGive) {
-                    logger.debug("Skipping cluster id=" + cluster + " as it has more hosts than we need: " + hostsInCluster.size() + " vs " + hostsLeftToGive);
-                    if (hostsLeft >= hostsLeftToGive) {
-                        continue;
-                    } else {
-                        break;
-                    }
-                } else {
-                    logger.debug("Taking all " + hostsInCluster.size() + " hosts: " + hostsInCluster + " from cluster id=" + cluster);
-                    hostsToReturn.addAll(hostsInCluster);
-                    hostsLeftToGive = hostsLeftToGive - hostsInCluster.size();
-                }
+            if (hostsToReturn.size() >= hostsToGive) {
+                break;
+            }
+
+            logger.debug("Trying cluster id=" + cluster);
+
+            if (hostsInCluster.size() <= hostsLeftToGive) {
+                logger.debug("Taking all " + hostsInCluster.size() + " hosts: " + hostsInCluster + " from cluster id=" + cluster);
+                hostsToReturn.addAll(hostsInCluster);
+                hostsLeftToGive = hostsLeftToGive - hostsInCluster.size();
+            } else if (hostsLeft >= hostsLeftToGive) {
+                logger.debug("Skipping cluster id=" + cluster + " as it has more hosts than we need: " + hostsInCluster.size() + " vs " + hostsLeftToGive
+                        + ", and remaining clusters can still satisfy the quota");
+                continue;
             } else {
+                logger.debug("No combination of whole clusters can satisfy the quota; taking a partial subset of " + hostsLeftToGive
+                        + " hosts from cluster id=" + cluster + " instead of giving away nothing");
+                hostsToReturn.addAll(hostsInCluster.subList(0, hostsLeftToGive));
+                hostsLeftToGive = 0;
                 break;
             }
         }
 
-        logger.debug("Management server {} is ready to give away {} hosts", ms, hostsToReturn.size());
         return hostsToReturn;
     }
 

engine/orchestration/src/test/java/com/cloud/cluster/agentlb/ClusterBasedAgentLoadBalancerPlannerTest.java

@@ -0,0 +1,98 @@
+// 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.cluster.agentlb;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
+import java.util.List;
+
+import org.junit.Test;
+
+import com.cloud.host.HostVO;
+
+public class ClusterBasedAgentLoadBalancerPlannerTest {
+
+    private final ClusterBasedAgentLoadBalancerPlanner planner = new ClusterBasedAgentLoadBalancerPlanner();
+
+    private HostVO host(long clusterId) {
+        HostVO host = new HostVO("guid-" + clusterId + "-" + System.nanoTime());
+        host.setClusterId(clusterId);
+        return host;
+    }
+
+    private LinkedHashMap<Long, List<HostVO>> clusterOf(int... clusterSizes) {
+        LinkedHashMap<Long, List<HostVO>> map = new LinkedHashMap<>();
+        long clusterId = 1;
+        for (int size : clusterSizes) {
+            List<HostVO> hosts = new ArrayList<>();
+            for (int i = 0; i < size; i++) {
+                hosts.add(host(clusterId));
+            }
+            map.put(clusterId, hosts);
+            clusterId++;
+        }
+        return map;
+    }
+
+    @Test
+    public void singleOversizedClusterGivesAwayPartialHosts() {
+        LinkedHashMap<Long, List<HostVO>> hostToClusterMap = clusterOf(2);
+        int totalDirectHosts = 2;
+        int avLoad = 1;
+        int hostsToGive = totalDirectHosts - avLoad;
+
+        List<HostVO> hostsToReturn = planner.selectHostsToGiveAway(hostToClusterMap, hostsToGive, totalDirectHosts);
+
+        assertEquals("a lone oversized cluster must still give away hosts to satisfy the quota", 1, hostsToReturn.size());
+    }
+
+    @Test
+    public void wholeClusterIsPreferredWhenItExactlyFitsTheQuota() {
+        LinkedHashMap<Long, List<HostVO>> hostToClusterMap = clusterOf(2, 3);
+        int totalDirectHosts = 5;
+        int hostsToGive = 2;
+
+        List<HostVO> hostsToReturn = planner.selectHostsToGiveAway(hostToClusterMap, hostsToGive, totalDirectHosts);
+
+        assertEquals(2, hostsToReturn.size());
+        assertTrue("should take the whole 2-host cluster rather than split the larger one", hostToClusterMap.get(1L).containsAll(hostsToReturn));
+    }
+
+    @Test
+    public void smallerClusterIsPreferredOverSplittingWhenBothCanSatisfyQuota() {
+        LinkedHashMap<Long, List<HostVO>> hostToClusterMap = clusterOf(5, 2);
+        int totalDirectHosts = 7;
+        int hostsToGive = 2;
+
+        List<HostVO> hostsToReturn = planner.selectHostsToGiveAway(hostToClusterMap, hostsToGive, totalDirectHosts);
+
+        assertEquals(2, hostsToReturn.size());
+        assertTrue("should skip the oversized cluster and take the smaller cluster whole", hostToClusterMap.get(2L).containsAll(hostsToReturn));
+    }
+
+    @Test
+    public void noHostsGivenAwayWhenAlreadyUnderThreshold() {
+        LinkedHashMap<Long, List<HostVO>> hostToClusterMap = clusterOf(1);
+
+        List<HostVO> hostsToReturn = planner.selectHostsToGiveAway(hostToClusterMap, 0, 1);
+
+        assertTrue(hostsToReturn.isEmpty());
+    }
+}