DRS: honour anti-affinity when planning and executing migrations - #14108
Open
bhouse-nexthop wants to merge 6 commits into
Open
DRS: honour anti-affinity when planning and executing migrations#14108bhouse-nexthop wants to merge 6 commits into
bhouse-nexthop wants to merge 6 commits into
Conversation
The reserved-capacity branch was chained to the wrong condition, so it only ran when the group VM was null or removed: - a Stopped VM still holding reserved capacity never had its last host avoided - the branch was dead for every live VM - a group mapping pointing at a deleted VM hit the branch with a null and threw NullPointerException Restructured to match NonStrictHostAffinityProcessor, which already had the intended shape: skip null/removed, then avoid the current host, else the last host while capacity is still reserved. Signed-off-by: Brad House <bhouse@nexthop.ai>
The processor already accepted a vmList of placements and ignored it, reading every group member's host from the database instead. DRS builds a multi-migration plan in memory and persists it only at the end, so while the plan is being built the database still shows the old host for every VM the plan has already moved. Anti-affinity was therefore evaluated against stale placements, and a plan could put two anti-affine VMs on the same host. - resolve each group member from vmList first, fall back to the database - mirrors what HostAffinityProcessor already does with the same argument - no change when vmList is empty, which is every non-DRS caller Signed-off-by: Brad House <bhouse@nexthop.ai>
Same gap as the strict processor: vmList was accepted and ignored, so host priorities were adjusted from database placements even when the caller supplied newer ones. NonStrictHostAntiAffinityProcessor extends this class and only overrides the priority direction, so it is fixed by the same change. Signed-off-by: Brad House <bhouse@nexthop.ai>
A DRS plan is generated once and executed later, and nothing downstream re-checks it - migrateVirtualMachine does not enforce affinity groups. By execution time the cluster may have changed, so a plan that was valid when generated can violate anti-affinity when it runs. - validate each migration against current state before queueing it - skip and mark failed instead of migrating into a violation - track destinations already queued in this run, since the jobs are asynchronous and the database does not reflect them yet Signed-off-by: Brad House <bhouse@nexthop.ai>
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>
- 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>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.22 #14108 +/- ##
============================================
+ Coverage 17.91% 17.93% +0.01%
- Complexity 16109 16132 +23
============================================
Files 5928 5928
Lines 534964 535050 +86
Branches 65463 65471 +8
============================================
+ Hits 95865 95973 +108
+ Misses 428191 428156 -35
- Partials 10908 10921 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bhouse-nexthop
requested review from
DaanHoogland,
vishesh92 and
weizhouapache
September 10, 2026 00:20
9 tasks
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new execution-time validation currently skips re-checking DPDK/dedicated-resource exclusions for VMs without affinity groups (and there is also an orphaned/misleading Javadoc block after the refactor).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR addresses DRS migration planning/execution violating host (anti-)affinity by ensuring affinity processors honor in-memory “planned placements”, by preserving non-strict affinity preferences through to DRS decision-making, and by re-validating queued migrations against current placement rules at execution time.
Changes:
- Update host anti-affinity and non-strict host affinity processors to prefer caller-supplied planned placements over stale DB placement.
- Preserve non-strict affinity preferences by converting lowered host priorities into DRS exclusions.
- Re-check each planned migration at execution time and cancel (not fail) migrations that are no longer valid.
File summaries
| File | Description |
|---|---|
| server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java | Converts non-strict priority adjustments to exclusions and adds execution-time migration re-checking. |
| server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java | Adds coverage for execution-time re-checking, queued-migration occupancy, and non-strict preference handling. |
| api/src/main/java/org/apache/cloudstack/affinity/AffinityProcessorBase.java | Adds helper to index caller-supplied planned placements by VM id. |
| plugins/affinity-group-processors/host-anti-affinity/src/main/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessor.java | Honors planned placements when building avoid sets; refactors per-VM host avoidance logic. |
| plugins/affinity-group-processors/host-anti-affinity/src/test/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessorTest.java | New unit tests covering planned placements, reserved-capacity behavior, and removed/missing members. |
| plugins/affinity-group-processors/non-strict-host-affinity/src/main/java/org/apache/cloudstack/affinity/NonStrictHostAffinityProcessor.java | Honors planned placements when adjusting host priorities for non-strict affinity. |
| plugins/affinity-group-processors/non-strict-host-affinity/src/test/java/org/apache/cloudstack/affinity/NonStrictHostAffinityProcessorTest.java | Adds a test ensuring planned placement wins over DB placement. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+824
to
+826
| if (CollectionUtils.isEmpty(affinityGroupVMMapDao.listByInstanceId(vm.getId()))) { | ||
| return false; | ||
| } |
Comment on lines
790
to
+797
| /** | ||
| * 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 | ||
| */ | ||
| /** |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes DRS generating and executing migration plans that violate host anti-affinity.
Four defects, one per commit:
HostAntiAffinityProcessorwas chained to the wrong condition, making it unreachable for live VMs and throwingNullPointerExceptionfor a group mapping pointing at a deleted VMNonStrictHostAffinityProcessor, whichNonStrictHostAntiAffinityProcessorextendsWhy the planned placements matter. DRS builds a plan of several migrations in memory and persists it only at the end. While the plan is being built, the database still shows the old host for every VM the plan has already moved, so anti-affinity was evaluated against stale placements and a plan could put two anti-affine VMs on the same host.
HostAffinityProcessoralready honours the same argument; the anti-affinity processors did not.Why non-strict groups are honoured when rebalancing. Non-strict groups express themselves by lowering a host's priority on the deployment plan rather than by excluding it.
ClusterDrsServiceImplbuilt a plan object, handed it to the processors, read only the exclude list, and discarded the plan - so the preference was written to an object nothing read.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 available to DRS. Being better balanced is not a reason to break it.
Why execution is re-checked. A plan is generated once and executed later, and nothing downstream re-checks it -
migrateVirtualMachinedoes not enforce affinity groups. By execution time the cluster may have changed. A queued migration also occupies both of its hosts until it completes, and may never complete, so a swap plan (A: host1 to host3, B: host2 to host1) could clear B for host1 while A was still on it.A migration that is no longer valid is now skipped and recorded as
CANCELLEDrather thanFAILED, with an event, since nothing went wrong - the plan went out of date.Related to #12473, which was closed on the grounds that non-strict groups permit live migration. They do, but that is about migrations an operator asks for, not about DRS choosing to undo a placement that already satisfied the group. Strict groups were also affected, which that report did not isolate.
Types of changes
Feature/Enhancement Scale or Bug Severity
Bug Severity
How Has This Been Tested?
Unit tests, run against
4.22:HostAntiAffinityProcessorTest- new, 10 cases. Covers the running host, the reserved-capacity last host in bothStoppedandStartingstates, expiry of the capacity release interval, removed and missing group members, and planned placements taking precedence over the database.NonStrictHostAffinityProcessorTest- added a case asserting a planned placement wins and the database is not consulted.ClusterDrsServiceImplTest- 33 cases. Added coverage for the pre-migration re-check, a VM that stopped between planning and execution, a destination still occupied by a queued migration, and non-strict preferences becoming exclusions.The two commit-1 cases were confirmed to fail against unpatched code (one assertion failure, one
NullPointerException).Full
mvn testonapi,serverand the four affinity processor plugins, checkstyle and license checks enabled: 0 failures.How did you try to break this feature and the system with this change?
Stoppedgroup member still holding reserved capacity now has its last host avoided, where previously the branch was unreachable. That widens the avoid set on the deployment path and restores the behaviour that predates the refactor which broke it. On a small cluster it can make a deployment fail where it previously succeeded, for the duration ofcapacity.skipcounting.hours. Calling it out explicitly since it affects more than DRS.