Address the rest of the DRS anti-affinity review

- a skipped migration is CANCELLED, not FAILED. Nothing went wrong; the
  plan went out of date. FAILED was indistinguishable from a migration
  that genuinely broke, and left no record of why
- record an event when one is skipped, so it is visible rather than a
  silent no-op in an otherwise successful plan
- the processors also cover dedicated resources and DPDK, so the refusal
  message no longer claims every skip is about an affinity group
- hoist the third copy of getVmIdVmMap into AffinityProcessorBase

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

Author: bhouse-nexthop

api/src/main/java/org/apache/cloudstack/affinity/AffinityProcessorBase.java

@@ -25,7 +25,9 @@
 import com.cloud.vm.VirtualMachineProfile;
 
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
 public class AffinityProcessorBase extends AdapterBase implements AffinityGroupProcessor {
 
@@ -41,6 +43,23 @@ public void process(VirtualMachineProfile vm, DeploymentPlan plan, ExcludeList a
 
     }
 
+
+    /**
+     * Indexes placements supplied by the caller. Callers such as DRS build a plan of several moves
+     * in memory and persist it only at the end, so during planning the database still shows the old
+     * host for every VM the plan has already moved.
+     */
+    protected Map<Long, VirtualMachine> getVmIdVmMap(List<VirtualMachine> vmList) {
+        Map<Long, VirtualMachine> vmIdVmMap = new HashMap<>();
+        if (vmList == null) {
+            return vmIdVmMap;
+        }
+        for (VirtualMachine vm : vmList) {
+            vmIdVmMap.put(vm.getId(), vm);
+        }
+        return vmIdVmMap;
+    }
+
     @Override
     public String getType() {
         return _type;

plugins/affinity-group-processors/host-anti-affinity/src/main/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessor.java

@@ -18,7 +18,6 @@
 
 import java.util.Arrays;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.stream.Collectors;
@@ -133,14 +132,6 @@ protected void processAffinityGroup(AffinityGroupVMMapVO vmGroupMapping, Exclude
         }
     }
 
-    protected Map<Long, VirtualMachine> getVmIdVmMap(List<VirtualMachine> vmList) {
-        Map<Long, VirtualMachine> vmIdVmMap = new HashMap<>();
-        for (VirtualMachine vm : vmList) {
-            vmIdVmMap.put(vm.getId(), vm);
-        }
-        return vmIdVmMap;
-    }
-
     protected void avoidHostOfVmInAffinityGroup(ExcludeList avoid, VMInstanceVO groupVM) {
         if (groupVM.getHostId() != null) {
             avoid.addHost(groupVM.getHostId());

plugins/affinity-group-processors/non-strict-host-affinity/src/main/java/org/apache/cloudstack/affinity/NonStrictHostAffinityProcessor.java

@@ -18,7 +18,6 @@
 
 import java.util.Arrays;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
@@ -113,14 +112,6 @@ protected void processAffinityGroup(AffinityGroupVMMapVO vmGroupMapping, Deploym
         }
     }
 
-    protected Map<Long, VirtualMachine> getVmIdVmMap(List<VirtualMachine> vmList) {
-        Map<Long, VirtualMachine> vmIdVmMap = new HashMap<>();
-        for (VirtualMachine vm : vmList) {
-            vmIdVmMap.put(vm.getId(), vm);
-        }
-        return vmIdVmMap;
-    }
-
     protected void processVmInAffinityGroup(DeploymentPlan plan, VMInstanceVO groupVM) {
         if (groupVM.getHostId() != null) {
             Integer priority = adjustHostPriority(plan, groupVM.getHostId());

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

@@ -795,13 +795,16 @@ void processPlans() {
      *         the DRS plan to be executed
      */
     /**
-     * Checks a planned migration against the affinity rules as they stand now.
+     * Checks a planned migration against the placement rules as they stand now.
      *
      * A plan is generated once and executed later, so state can have moved on: VMs may have been
      * created, migrated or destroyed in between. Anti-affinity in particular is only meaningful
      * 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.
+     *
      * @param vm
      *         the VM the plan wants to move
      * @param destHost
@@ -863,10 +866,16 @@ void executeDrsPlan(ClusterDrsPlanVO plan) {
                 }
 
                 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);
+                    String reason = String.format("Skipped DRS migration of %s to %s: the destination no longer "
+                            + "satisfies the placement rules for that VM. The plan was generated against older "
+                            + "state.", vm, host);
+                    logger.warn(reason);
+                    // cancelled rather than failed: nothing went wrong, the plan went out of date
+                    migration.setStatus(JobInfo.Status.CANCELLED);
                     drsPlanMigrationDao.update(migration.getId(), migration);
+                    ActionEventUtils.onCompletedActionEvent(User.UID_SYSTEM, Account.ACCOUNT_ID_SYSTEM,
+                            EventVO.LEVEL_WARN, EventTypes.EVENT_CLUSTER_DRS, false, reason,
+                            plan.getClusterId(), ApiCommandResourceType.Cluster.toString(), plan.getEventId());
                     continue;
                 }
 

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

@@ -1110,7 +1110,7 @@ public void testExecuteDrsPlanSkipsMigrationThatViolatesAffinity() {
 
         clusterDrsService.executeDrsPlan(plan);
 
-        Mockito.verify(migration).setStatus(JobInfo.Status.FAILED);
+        Mockito.verify(migration).setStatus(JobInfo.Status.CANCELLED);
         Mockito.verify(clusterDrsService, Mockito.never())
                 .createMigrateVMAsyncJob(Mockito.any(), Mockito.any(), Mockito.anyLong());
     }