domain dedication - #13654
Open
DaanHoogland wants to merge 7 commits into
Open
Conversation
…r (issue apache#5803) - Fix findAvoidSetForNonExplicitUserVM to check domain/account dedication before adding pods/clusters/hosts to the avoid list - Domain-dedicated resources are now accessible to users in the same domain - Add 4 unit tests covering same-domain, different-domain, same-account, different-account scenarios - Use mutable ArrayList in test return values to support list.clear() calls in production code
…d tempStorage.clear() - Use Filter without offset/limit to retrieve all dedicated resources (not just the first) - Eliminate tempStorage variable and clear() calls to avoid potential side effects - Code is now cleaner and consistent with correct behavior
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: dahn <daan.hoogland@gmail.com>
findAvoidSetForNonExplicitUserVM referenced _affinityGroupDao, which has no corresponding mock in DeploymentPlanningManagerImplTest, so it was always null via Mockito's @Injectmocks and every call NPE'd. Use _affinityGroupDomainMapDao.listByDomain, already injected and used by the analogous findAvoiSetForRouterVM method, to detect a domain-level ExplicitDedication affinity group. Also add a default empty stub for searchDedicatedPods, mirroring the existing cluster/host stubs, since the fix now queries pods for both the account and the domain. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
14 tasks
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13654 +/- ##
============================================
+ Coverage 17.89% 19.65% +1.75%
- Complexity 16092 19794 +3702
============================================
Files 5936 6368 +432
Lines 532734 575140 +42406
Branches 65165 70379 +5214
============================================
+ Hits 95347 113029 +17682
- Misses 426711 449818 +23107
- Partials 10676 12293 +1617
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:
|
Contributor
Author
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #5803 by adjusting deployment planning so that non-explicit User VMs don’t automatically avoid dedicated resources that should be available to them (notably, domain-dedicated pods), and adds unit tests to cover dedicated pod access scenarios.
Changes:
- Update
findAvoidSetForNonExplicitUserVMto remove (from the avoid list candidates) resources dedicated to the VM owner’s account, and conditionally those dedicated to the VM owner’s domain. - Relax/adjust the debug message to reflect “not dedicated” rather than “not explicitly dedicated”.
- Add tests covering domain-dedicated and account-dedicated pod behavior for same-domain/same-account vs different-domain/different-account users.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java | Changes avoid-list construction for non-explicit user VMs to allow certain dedicated resources. |
| server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java | Adds test coverage for domain/account-dedicated pod access expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+973
to
+977
| // If the VM owner's domain has an ExplicitDedication domain-level affinity group, | ||
| // resources dedicated to that domain are accessible to the VM owner (fixes issue #5803). | ||
| List<AffinityGroupDomainMapVO> domainGroupMappings = _affinityGroupDomainMapDao.listByDomain(vmDomainId); | ||
| boolean hasDomainExplicitDedicationGroup = domainGroupMappings != null && !domainGroupMappings.isEmpty(); | ||
|
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18684 |
Contributor
Author
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-16634)
|
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: #5803
now using both co-pilot and claude… ;)
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?