bhouse-nexthop opened a new pull request, #14108: URL: https://github.com/apache/cloudstack/pull/14108
### Description This PR fixes DRS generating and executing migration plans that violate host anti-affinity. Four defects, one per commit: | # | Defect | |---|---| | 1 | The reserved-capacity branch in `HostAntiAffinityProcessor` was chained to the wrong condition, making it unreachable for live VMs and throwing `NullPointerException` for a group mapping pointing at a deleted VM | | 2 | The processor accepted a list of planned placements and ignored it, reading every group member's host from the database | | 3 | Same gap in `NonStrictHostAffinityProcessor`, which `NonStrictHostAntiAffinityProcessor` extends | | 4 | Nothing re-checked a plan before executing it, and non-strict groups were discarded entirely | **Why 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. `HostAffinityProcessor` already 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. `ClusterDrsServiceImpl` built 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 - `migrateVirtualMachine` does 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 `CANCELLED` rather than `FAILED`, 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 - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] New feature (non-breaking change which adds functionality) - [x] Bug fix (non-breaking change which fixes an issue) - [ ] Enhancement (improves an existing feature and functionality) - [ ] Cleanup (Code refactoring and cleanup, that may add test cases) - [ ] Build/CI - [ ] Test (unit or integration test code) ### Feature/Enhancement Scale or Bug Severity #### Bug Severity - [ ] BLOCKER - [ ] Critical - [x] Major - [ ] Minor - [ ] Trivial ### 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 both `Stopped` and `Starting` states, 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 test` on `api`, `server` and 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? - **Non-DRS callers.** Initial deployment and the find-hosts-for-migration API pass an empty placement list. Verified that path is unchanged by commit 2 and 3. Commit 1 does change it, and deliberately: a `Stopped` group 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 of `capacity.skipcounting.hours`. Calling it out explicitly since it affects more than DRS. - **Removed VMs** are no longer avoided at all, which is the other direction of the same fix - a removed VM is not running anywhere. - **Swap plans**, where two migrations exchange hosts, are the case the source-host tracking exists for. Covered by test. - **Plans that go stale**: VM stopped, VM already moved, destination filled by something else. - **NPE hunting** on null hosts, null VMs, and group mappings pointing at deleted VMs. - The affinity processors also cover dedicated resources and DPDK, so the skip message does not claim every refusal is about an affinity group. -- 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]
