Rank each candidate host once, however many tags it carries

listAllUpAndEnabledNonHAHosts joins host_tags to filter out hosts whose
tags are rules, and the join carries no DISTINCT, so a host comes back
once per non-rule tag it holds. That path is taken whenever a VM has no
host tag to filter the candidate list by, which in practice means system
VMs: a virtual router deploying onto a twelve host cluster where three
hosts carry a second tag is handed fifteen candidates.

An allocator that walks the list and takes the first host that fits is
indifferent to a repeat, which is why this has gone unnoticed. The
weighted ranking is not. Scores are keyed by host, so a repeat changes no
score, but the selection spread shuffles the leading few entries and the
caller takes whichever ends up first - so a host holding two of the three
slots draws two thirds of the deployments rather than one third.

Collapse the repeats in the DAO, where the join creates them and every
caller benefits, and again in the scorer, so ranking cannot be skewed by
whatever a future caller hands it.

Signed-off-by: Brad House <bhouse@nexthop.ai>

Author: bhouse-nexthop

engine/schema/src/main/java/com/cloud/host/dao/HostDaoImpl.java

@@ -24,6 +24,7 @@
 import java.util.Date;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -935,7 +936,14 @@ public List<HostVO> listAllUpAndEnabledNonHAHosts(Type type, Long clusterId, Lon
         sc.setParameters("status", Status.Up);
         sc.setParameters("resourceState", ResourceState.Enabled);
 
-        return listBy(sc);
+        // The tag join carries no DISTINCT, so a host comes back once per non-rule tag it holds.
+        // Callers read this as a set of candidate hosts, so collapse the repeats here rather than
+        // leave each of them to cope with the same host arriving more than once.
+        Map<Long, HostVO> distinctHosts = new LinkedHashMap<>();
+        for (HostVO host : listBy(sc)) {
+            distinctHosts.putIfAbsent(host.getId(), host);
+        }
+        return new ArrayList<>(distinctHosts.values());
     }
 
     @Override

engine/schema/src/test/java/com/cloud/host/dao/HostDaoImplTest.java

@@ -39,6 +39,7 @@
 
 import com.cloud.cpu.CPU;
 import com.cloud.host.Host;
+import com.cloud.host.HostTagVO;
 import com.cloud.host.HostVO;
 import com.cloud.host.Status;
 import com.cloud.hypervisor.Hypervisor;
@@ -60,6 +61,32 @@ public class HostDaoImplTest {
     @Mock
     private SearchCriteria<HostVO> mockSearchCriteria;
 
+    @Test
+    public void testListAllUpAndEnabledNonHAHostsCollapsesRepeatedHosts() {
+        // the host_tags join carries no DISTINCT, so a host comes back once per non-rule tag it
+        // holds. Callers read the result as a set of candidate hosts.
+        SearchBuilder<HostTagVO> tagSearchBuilder = mock(SearchBuilder.class);
+        when(tagSearchBuilder.entity()).thenReturn(mock(HostTagVO.class));
+        HostTagsDao hostTagsDao = mock(HostTagsDao.class);
+        when(hostTagsDao.createSearchBuilder()).thenReturn(tagSearchBuilder);
+        hostDao._hostTagsDao = hostTagsDao;
+
+        when(mockSearchBuilder.entity()).thenReturn(mock(HostVO.class));
+        when(mockSearchBuilder.create()).thenReturn(mockSearchCriteria);
+        doReturn(mockSearchBuilder).when(hostDao).createSearchBuilder();
+
+        HostVO twoTags = mock(HostVO.class);
+        when(twoTags.getId()).thenReturn(1L);
+        HostVO oneTag = mock(HostVO.class);
+        when(oneTag.getId()).thenReturn(2L);
+        doReturn(Arrays.asList(twoTags, twoTags, oneTag)).when(hostDao).listBy(any(SearchCriteria.class));
+
+        List<HostVO> hosts = hostDao.listAllUpAndEnabledNonHAHosts(Host.Type.Routing, 3L, 2L, 1L, null);
+
+        assertEquals(2, hosts.size());
+        assertEquals(Arrays.asList(1L, 2L), hosts.stream().map(HostVO::getId).collect(Collectors.toList()));
+    }
+
     @Test
     public void testCountUpAndEnabledHostsInZone() {
         long testZoneId = 100L;

server/src/main/java/com/cloud/agent/manager/allocator/impl/WeightedHostScorer.java

@@ -21,6 +21,7 @@
 import java.util.Comparator;
 import java.util.Date;
 import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Random;
@@ -133,16 +134,17 @@ public class WeightedHostScorer extends AdapterBase implements Configurable {
      * order at the end of the list rather than being dropped.
      */
     public List<Host> rank(long zoneId, Long podId, Long clusterId, List<? extends Host> hosts) {
-        if (hosts == null || hosts.size() <= 1) {
-            return hosts == null ? new ArrayList<>() : new ArrayList<>(hosts);
+        List<Host> candidates = distinctHosts(hosts);
+        if (candidates.size() <= 1) {
+            return candidates;
         }
 
-        Map<Long, Double> scores = score(zoneId, podId, clusterId, hosts);
+        Map<Long, Double> scores = score(zoneId, podId, clusterId, candidates);
 
         List<Host> unscored = new ArrayList<>();
         List<Host> measured = new ArrayList<>();
         List<Host> unmeasured = new ArrayList<>();
-        for (Host host : hosts) {
+        for (Host host : candidates) {
             if (!scores.containsKey(host.getId())) {
                 unscored.add(host);
             } else if (hostLoadTracker.getLoad(host.getId()).isUsable()) {
@@ -188,6 +190,24 @@ public List<Host> rank(long zoneId, Long podId, Long clusterId, List<? extends H
         return result;
     }
 
+    /**
+     * The candidate list can hold the same host twice. listAllUpAndEnabledNonHAHosts joins host_tags
+     * without collapsing the rows, so a host carrying two tags arrives once per tag, and that path is
+     * taken whenever a VM has no host tag to filter on - system VMs, most commonly. Ranking is a set
+     * operation: a repeat is harmless to the score, which is keyed by host, but it costs a slot in
+     * the selection spread and would bias the shuffle towards whichever host happens to be repeated.
+     */
+    protected List<Host> distinctHosts(List<? extends Host> hosts) {
+        if (hosts == null) {
+            return new ArrayList<>();
+        }
+        Map<Long, Host> distinct = new LinkedHashMap<>();
+        for (Host host : hosts) {
+            distinct.putIfAbsent(host.getId(), host);
+        }
+        return new ArrayList<>(distinct.values());
+    }
+
     protected Map<Long, Double> score(long zoneId, Long podId, Long clusterId, List<? extends Host> hosts) {
         List<CapacityVO> capacities = capacityDao.listHostCapacityByCapacityTypes(zoneId, clusterId,
                 List.of(Capacity.CAPACITY_TYPE_CPU, Capacity.CAPACITY_TYPE_MEMORY));

server/src/test/java/com/cloud/agent/manager/allocator/impl/WeightedHostScorerRankTest.java

@@ -133,6 +133,22 @@ public void testAllocationIsMeasuredAgainstTheOvercommittedTotal() {
                 scores.get(heavy.getId()) > scores.get(light.getId()));
     }
 
+    @Test
+    public void testARepeatedHostIsRankedOnce() {
+        // listAllUpAndEnabledNonHAHosts returns a host once per tag it carries, so the same host
+        // reaches the allocator more than once whenever a VM has no tag to filter the list by.
+        // A repeat would take an extra slot in the selection spread and bias the shuffle to it.
+        Host light = host(1L, "light", 0.10, 0.10, new HostLoad(0.10, 0.10, 10), 10);
+        Host middle = host(2L, "middle", 0.40, 0.40, new HostLoad(0.40, 0.40, 10), 40);
+        Host heavy = host(3L, "heavy", 0.70, 0.70, new HostLoad(0.70, 0.60, 10), 70);
+
+        List<String> ranked = rankedNames(Arrays.asList(middle, light, middle, heavy, light));
+
+        assertEquals("each host must be offered exactly once", 3, ranked.size());
+        assertEquals("no host may appear twice", new HashSet<>(ranked).size(), ranked.size());
+        assertTrue("no host may be dropped", ranked.containsAll(Arrays.asList("light", "middle", "heavy")));
+    }
+
     @Test
     public void testBusyHostRanksBehindQuietOneAtEqualAllocation() {
         Host quiet = host(1L, "quiet", 0.30, 0.30, new HostLoad(0.05, 0.05, 10), 30);