Re-check every migration, not only VMs in an affinity group

Review feedback on the PR.

applyAffinityConstraints also applies DPDK and dedicated-resource
exclusions, and neither depends on affinity group membership. Returning
early for VMs with no groups therefore skipped those checks, which the
method's own documentation said it covered.

The cost is one call per planned migration, bounded by
drs.max.migrations, at execution time rather than in any hot loop.

Also move executeDrsPlan's javadoc back onto executeDrsPlan; it was left
stranded above the method inserted before it.

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

Author: bhouse-nexthop

server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java

@@ -787,13 +787,6 @@ void processPlans() {
         }
     }
 
-    /**
-     * Executes the DRS plan by migrating virtual machines to their destination hosts.
-     * If there are no migrations to be executed, the plan is marked as completed.
-     *
-     * @param plan
-     *         the DRS plan to be executed
-     */
     /**
      * Checks a planned migration against the placement rules as they stand now.
      *
@@ -802,8 +795,9 @@ void processPlans() {
      * against current placements, and nothing downstream re-checks it - migrateVirtualMachine does
      * not enforce affinity groups.
      *
-     * The processors also cover dedicated resources and DPDK, so a refusal is not necessarily about
-     * an affinity group.
+     * Checked for every VM, not only those in an affinity group: applyAffinityConstraints also
+     * applies DPDK and dedicated-resource exclusions, which apply regardless of group membership.
+     * A refusal here is therefore not necessarily about an affinity group.
      *
      * @param vm
      *         the VM the plan wants to move
@@ -821,9 +815,6 @@ protected boolean destinationViolatesAffinity(VirtualMachine vm, Host destHost,
             logger.debug("VM {} is no longer running, so its planned migration is out of date", vm);
             return true;
         }
-        if (CollectionUtils.isEmpty(affinityGroupVMMapDao.listByInstanceId(vm.getId()))) {
-            return false;
-        }
         if (dispatchedSourceHosts.contains(destHost.getId())) {
             logger.debug("Host {} is still occupied by a VM whose migration away from it is only queued", destHost);
             return true;
@@ -837,6 +828,13 @@ protected boolean destinationViolatesAffinity(VirtualMachine vm, Host destHost,
         return excludes.shouldAvoid(destHost);
     }
 
+    /**
+     * Executes the DRS plan by migrating virtual machines to their destination hosts.
+     * If there are no migrations to be executed, the plan is marked as completed.
+     *
+     * @param plan
+     *         the DRS plan to be executed
+     */
     void executeDrsPlan(ClusterDrsPlanVO plan) {
         List<ClusterDrsPlanMigrationVO> planMigrations = drsPlanMigrationDao.listPlanMigrationsToExecute(plan.getId());
         if (planMigrations == null || planMigrations.isEmpty()) {

server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java

@@ -993,15 +993,20 @@ private void affinityExcludes(Long... hostIds) {
     }
 
     @Test
-    public void testDestinationAllowedWhenVmHasNoAffinityGroups() {
+    public void testAVmWithNoAffinityGroupsIsStillRechecked() {
+        // applyAffinityConstraints also applies DPDK and dedicated-resource exclusions, which do
+        // not depend on affinity group membership, so every VM has to go through it
         VMInstanceVO vm = Mockito.mock(VMInstanceVO.class);
         Mockito.when(vm.getId()).thenReturn(1L);
         Mockito.when(vm.getHostId()).thenReturn(10L);
-        Mockito.when(affinityGroupVMMapDao.listByInstanceId(1L)).thenReturn(Collections.emptyList());
+        Mockito.when(vm.getServiceOfferingId()).thenReturn(5L);
+        Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(1L, 5L))
+                .thenReturn(Mockito.mock(ServiceOfferingVO.class));
+        affinityExcludes(21L);
 
         assertFalse(clusterDrsService.destinationViolatesAffinity(vm, host(20L),
                 Collections.emptyList(), Collections.emptyList()));
-        Mockito.verify(managementServer, Mockito.never())
+        Mockito.verify(managementServer)
                 .applyAffinityConstraints(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any());
     }