Make the DRS affinity check correct

Three defects found reviewing the previous commits, all in how DRS
decides whether a planned migration is still allowed.

Non-strict anti-affinity was discarded entirely. Non-strict groups
express themselves by lowering a host's priority on the deployment plan
rather than by excluding it, and DRS built a plan, handed it to the
processors, read only the exclude list and threw the plan away.

  Non-strict means the rule may be broken when there is nowhere else to
  put a VM. That cannot arise while rebalancing: the VM already runs
  somewhere that satisfies the group and leaving it there is always an
  option. Being better balanced is not a reason to break it.

A host was treated as free the moment a migration away from it was
queued. The jobs are asynchronous and can fail, so a queued migration
occupies both ends until it completes. In a swap - A from host1 to
host3, B from host2 to host1 - B was cleared for host1 while A was
still on it.

  - track the hosts queued migrations have not actually left
  - refuse a destination that is one of them

A VM that stopped between planning and execution threw NPE inside the
affinity check, which was then logged without a stack trace.

  - treat a VM that is no longer running as an out of date plan
  - log the exception rather than its message

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

Author: bhouse-nexthop

plugins/affinity-group-processors/host-anti-affinity/src/test/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessorTest.java

@@ -40,6 +40,7 @@
 
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.lenient;
 import static org.mockito.Mockito.when;
 
 @RunWith(JUnit4.class)
@@ -151,10 +152,30 @@ public void testMissingGroupVmIsSkipped() {
     public void testRemovedGroupVmIsSkipped() {
         when(_vmInstanceDao.findById(GROUP_VM_ID)).thenReturn(groupVM);
         when(groupVM.isRemoved()).thenReturn(true);
+        // a removed VM is not running anywhere, so neither of its hosts should be avoided
+        lenient().when(groupVM.getHostId()).thenReturn(HOST_ID);
+        lenient().when(groupVM.getState()).thenReturn(VirtualMachine.State.Stopped);
+        lenient().when(groupVM.getLastHostId()).thenReturn(LAST_HOST_ID);
+        lenient().when(groupVM.getUpdateTime()).thenReturn(DateUtil.currentGMTTime());
 
         processor.processAffinityGroup(vmGroupMapping, avoid, vm);
 
         assertFalse(avoids(HOST_ID));
+        assertFalse(avoids(LAST_HOST_ID));
+    }
+
+    @Test
+    public void testStartingGroupVmWithReservedCapacityLastHostIsAvoided() {
+        when(_vmInstanceDao.findById(GROUP_VM_ID)).thenReturn(groupVM);
+        when(groupVM.isRemoved()).thenReturn(false);
+        when(groupVM.getHostId()).thenReturn(null);
+        when(groupVM.getState()).thenReturn(VirtualMachine.State.Starting);
+        when(groupVM.getLastHostId()).thenReturn(LAST_HOST_ID);
+        when(groupVM.getUpdateTime()).thenReturn(DateUtil.currentGMTTime());
+
+        processor.processAffinityGroup(vmGroupMapping, avoid, vm);
+
+        assertTrue(avoids(LAST_HOST_ID));
     }
 
     @Test

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

@@ -25,6 +25,7 @@
 import com.cloud.dc.ClusterVO;
 import com.cloud.dc.dao.ClusterDao;
 import com.cloud.deploy.DataCenterDeployment;
+import com.cloud.deploy.DeploymentPlan;
 import com.cloud.deploy.DeploymentPlanner.ExcludeList;
 import com.cloud.domain.Domain;
 import com.cloud.event.ActionEventUtils;
@@ -80,6 +81,7 @@
 import org.apache.cloudstack.jobs.JobInfo;
 import org.apache.cloudstack.managed.context.ManagedContextTimerTask;
 import org.apache.commons.collections.CollectionUtils;
+import org.apache.commons.collections.MapUtils;
 import org.apache.commons.lang3.time.DateUtils;
 
 import javax.inject.Inject;
@@ -432,6 +434,29 @@ private List<Ternary<VirtualMachine, Host, Host>> getMigrationPlans(
         return migrationPlan;
     }
 
+    /**
+     * Turns the non-strict affinity preferences recorded on the plan into exclusions.
+     *
+     * Non-strict groups express themselves by lowering a host's priority rather than by excluding
+     * it, and DRS only reads the exclude list - so the preference was being discarded. Rebalancing
+     * is never a reason to break it: the VM is already running somewhere that satisfies the group,
+     * and leaving it there is always available to DRS. "Non-strict" means the rule may be broken
+     * when there is nowhere else to put a VM, which cannot arise while merely rebalancing.
+     */
+    protected void excludeHostsDispreferredByAffinity(DeploymentPlan plan, ExcludeList excludes) {
+        Map<Long, Integer> priorities = plan.getHostPriorities();
+        if (MapUtils.isEmpty(priorities)) {
+            return;
+        }
+        for (Map.Entry<Long, Integer> entry : priorities.entrySet()) {
+            if (entry.getValue() != null && entry.getValue() < DeploymentPlan.DEFAULT_HOST_PRIORITY) {
+                excludes.addHost(entry.getKey());
+                logger.debug("Host {} is dispreferred by a non-strict affinity group, so DRS will not migrate onto it",
+                        entry.getKey());
+            }
+        }
+    }
+
     private Map<Long, ExcludeList> getVmToExcludesMap(List<VirtualMachine> vmList, Map<Long, Host> hostMap,
             Set<Long> vmsWithAffinityGroups, Map<Long, List<? extends Host>> vmToCompatibleHostsCache,
             Map<Long, ServiceOffering> vmIdServiceOfferingMap) {
@@ -452,6 +477,7 @@ private Map<Long, ExcludeList> getVmToExcludesMap(List<VirtualMachine> vmList, M
 
                         excludes = managementServer.applyAffinityConstraints(
                                 vm, vmProfile, plan, vmList);
+                        excludeHostsDispreferredByAffinity(plan, excludes);
                     } else {
                         // VM has no affinity groups - create minimal ExcludeList (just source host)
                         excludes = new ExcludeList();
@@ -776,18 +802,35 @@ void processPlans() {
      * against current placements, and nothing downstream re-checks it - migrateVirtualMachine does
      * not enforce affinity groups.
      *
+     * @param vm
+     *         the VM the plan wants to move
+     * @param destHost
+     *         where the plan wants to move it
      * @param dispatched
      *         migrations already queued by this run, which the database does not reflect yet
+     * @param dispatchedSourceHosts
+     *         hosts those queued migrations have not actually left yet
+     * @return true when the migration should not go ahead
      */
-    protected boolean destinationViolatesAffinity(VirtualMachine vm, Host destHost, List<VirtualMachine> dispatched) {
+    protected boolean destinationViolatesAffinity(VirtualMachine vm, Host destHost, List<VirtualMachine> dispatched,
+            List<Long> dispatchedSourceHosts) {
+        if (vm.getHostId() == null) {
+            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;
+        }
         DataCenterDeployment plan = new DataCenterDeployment(destHost.getDataCenterId(), destHost.getPodId(),
                 destHost.getClusterId(), null, null, null);
         VirtualMachineProfile vmProfile = new VirtualMachineProfileImpl(vm, null,
                 serviceOfferingDao.findByIdIncludingRemoved(vm.getId(), vm.getServiceOfferingId()), null, null);
         ExcludeList excludes = managementServer.applyAffinityConstraints(vm, vmProfile, plan, dispatched);
+        excludeHostsDispreferredByAffinity(plan, excludes);
         return excludes.shouldAvoid(destHost);
     }
 
@@ -806,7 +849,9 @@ void executeDrsPlan(ClusterDrsPlanVO plan) {
         plan.setStatus(ClusterDrsPlan.Status.IN_PROGRESS);
         drsPlanDao.update(plan.getId(), plan);
 
+        // a queued migration occupies both ends until it completes, and it may not complete at all
         List<VirtualMachine> dispatched = new ArrayList<>();
+        List<Long> dispatchedSourceHosts = new ArrayList<>();
 
         for (ClusterDrsPlanMigrationVO migration : planMigrations) {
             try {
@@ -817,7 +862,7 @@ void executeDrsPlan(ClusterDrsPlanVO plan) {
                             migration.getDestHostId()));
                 }
 
-                if (destinationViolatesAffinity(vm, host, dispatched)) {
+                if (destinationViolatesAffinity(vm, host, dispatched, dispatchedSourceHosts)) {
                     logger.warn("Skipping DRS migration of vm {} to host {}: it no longer satisfies the affinity " +
                             "rules for that VM. The plan was generated against older state.", vm, host);
                     migration.setStatus(JobInfo.Status.FAILED);
@@ -833,11 +878,16 @@ void executeDrsPlan(ClusterDrsPlanVO plan) {
                 drsPlanMigrationDao.update(migration.getId(), migration);
 
                 // the migration job has only been queued, so the database still shows the old host.
-                // record where it is headed so later migrations in this plan see it.
+                // record both ends: the VM is headed for the destination but has not left the
+                // source, and if the job fails it never will.
+                Long sourceHostId = vm.getHostId();
+                if (sourceHostId != null) {
+                    dispatchedSourceHosts.add(sourceHostId);
+                }
                 vm.setHostId(host.getId());
                 dispatched.add(vm);
             } catch (Exception e) {
-                logger.warn("Unable to execute DRS plan {} due to {}", plan, e.getMessage());
+                logger.warn("Unable to execute DRS plan {}", plan, e);
                 migration.setStatus(JobInfo.Status.FAILED);
                 drsPlanMigrationDao.update(migration.getId(), migration);
             }