Copilot/fix issue 5803 domain dedication - #13529
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
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #5803 by adjusting how DeploymentPlanningManagerImpl builds the avoid list for non-explicit user VMs when dedicated pods/clusters/hosts exist, and adds unit tests to validate domain/account dedication behavior.
Changes:
- Update
findAvoidSetForNonExplicitUserVMto account for domain-dedicated resources (in addition to account-dedicated ones) when building the avoid set. - Add new unit tests covering domain-dedicated and account-dedicated pod accessibility scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java | Updates avoid-list construction for non-explicit user VMs to better handle dedicated resource access. |
| server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java | Adds tests for dedicated-pod access rules across same/different domain and same/different account cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #13529 +/- ##
=============================================
- Coverage 17.89% 0 -17.90%
=============================================
Files 5936 0 -5936
Lines 532734 0 -532734
Branches 65165 0 -65165
=============================================
- Hits 95347 0 -95347
+ Misses 426711 0 -426711
+ Partials 10676 0 -10676
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:
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
| Mockito.when(_dedicatedDao.searchDedicatedPods(Mockito.isNull(), Mockito.eq(vmDomainId), | ||
| Mockito.isNull(), Mockito.isNull(), Mockito.any(com.cloud.utils.db.Filter.class))) | ||
| .thenReturn(new Pair<>(new ArrayList<>(Arrays.asList(dedicatedPod)), 1)); |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: dahn <daan.hoogland@gmail.com>
| @Inject | ||
| HostDao _hostDao; | ||
|
|
||
| @Inject | ||
| AffinityGroupDomainMapDao _affinityGroupDomainMapDao; | ||
|
|
| // Domain has affinity group mappings (pod dedicated to this domain). | ||
| // The content of the list entries does not matter; only emptiness is checked. | ||
| AffinityGroupDomainMapVO domainMap = Mockito.mock(AffinityGroupDomainMapVO.class); | ||
| Mockito.when(_affinityGroupDomainMapDao.listByDomain(vmDomainId)) | ||
| .thenReturn(Arrays.asList(domainMap)); | ||
|
|
||
| // The pod is found when searching by domain (accountId = null) | ||
| DedicatedResourceVO dedicatedPod = Mockito.mock(DedicatedResourceVO.class); | ||
| Mockito.when(dedicatedPod.getPodId()).thenReturn(dedicatedPodId); | ||
| Mockito.when(_dedicatedDao.searchDedicatedPods(Mockito.isNull(), Mockito.eq(vmDomainId), | ||
| Mockito.isNull(), Mockito.isNull(), Mockito.any(com.cloud.utils.db.Filter.class))) | ||
| .thenReturn(new Pair<>(new ArrayList<>(Arrays.asList(dedicatedPod)), 1)); | ||
|
|
| private void findAvoidSetForNonExplicitUserVM(ExcludeList avoids, VirtualMachine vm, List<Long> allPodsInDc, List<Long> allClustersInDc, List<Long> allHostsInDc) { | ||
| long vmAccountId = vm.getAccountId(); | ||
| long vmDomainId = vm.getDomainId(); | ||
|
|
||
| List<Long> allPodsFromDedicatedID = new ArrayList<>(); | ||
| List<Long> allClustersFromDedicatedID = new ArrayList<>(); | ||
| List<Long> allHostsFromDedicatedID = new ArrayList<>(); | ||
|
|
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 18478 |
|
superceded by #13654 |
…eturn Restore the type-specific AffinityGroupDao.findDomainLevelGroupByType check (as Copilot's review on PR apache#13529 requested), instead of the looser AffinityGroupDomainMapDao.listByDomain used in the previous fix commit, which matches any domain-level affinity group rather than specifically ExplicitDedication. Add the missing @mock for AffinityGroupDao and update the new tests to stub the type-specific call, so this no longer NPEs the way the last "apply suggestions" commit did. Also add an early return in findAvoidSetForNonExplicitUserVM when nothing in the DC is dedicated, per another unaddressed review comment, to avoid unnecessary DAO calls on the common case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Description
This PR...
Fixes: #5803
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?