bhouse-nexthop commented on code in PR #14108:
URL: https://github.com/apache/cloudstack/pull/14108#discussion_r4274389330
##########
server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java:
##########
@@ -761,6 +787,47 @@ void processPlans() {
}
}
+ /**
+ * 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.
+ *
+ * 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
+ * @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,
+ 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 (dispatchedSourceHosts.contains(destHost.getId())) {
Review Comment:
Yes, it did. The source-host check was a flat host list and never looked at
groups. So with a plan of A host1→host3 then B host2→host1, B was cancelled
even when A and B have nothing to do with each other. The existing test even
asserted that.
Fixed in a2e7a68: the flat list is gone. The check now runs the affinity
processors twice:
- once with the queued VMs placed on their destinations
- once against the database, which still has them on their sources until the
job finishes
It refuses only when either view excludes the destination. So host1 still
counts as occupied by A, but only for VMs that share a group with A. In the
unrelated case B now goes ahead. Tests cover both, plus a group member heading
onto the destination.
##########
server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java:
##########
@@ -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) {
Review Comment:
You're right, and it's a regression. Before this PR DRS ignored non-strict
priorities and moved those VMs freely. Now every host comes out dispreferred,
so they never move.
Fixed in a2e7a68: a host is now excluded only when it is dispreferred more
than the host the VM is already on. Example: one non-strict anti-affinity group
on 3 hosts, with A and D on h1, B on h2, C on h3:
- before: all three hosts are lowered, so A is frozen
- now: h2 and h3 are no worse than h1, so A can move to either of them. A
host with two group members would still be refused.
When the VM shares its host with no group member, the threshold stays at the
default, so behaviour is the same as before. Raised priorities (non-strict host
affinity) are clamped so they don't count as a penalty on the other hosts. DRS
still won't move a VM just to fix an existing violation; that's out of scope
here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
