domain dedication

[DaanHoogland]

Description

This PR...

Fixes: #5803

now using both co-pilot and claude… ;)

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• 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

Feature/Enhancement Scale

• Major
• Minor

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

[codecov]

Codecov Report

❌ Patch coverage is 28.12500% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 19.65%. Comparing base (a5b6bc3) to head (3281a55).
⚠️ Report is 430 commits behind head on main.

Files with missing lines	Patch %	Lines
...om/cloud/deploy/DeploymentPlanningManagerImpl.java	28.12%	22 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #13654      +/-   ##
============================================
+ Coverage     17.89%   19.65%   +1.75%     
- Complexity    16092    19799    +3707     
============================================
  Files          5936     6368     +432     
  Lines        532734   574914   +42180     
  Branches      65165    70359    +5194     
============================================
+ Hits          95347   113006   +17659     
- Misses       426711   449631   +22920     
- Partials      10676    12277    +1601     

Flag	Coverage Δ
uitests	3.41% <ø> (-0.28%)	⬇️
unittests	20.93% <28.12%> (+1.93%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

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 findAvoidSetForNonExplicitUserVM to 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.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (6)

server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java:1

• This adds up to 4–7 DB calls on the deploy path (affinity group lookup + 3 account searches + 0/3 domain searches). Consider reducing round-trips and allocations: use Set<Long> for IDs (to avoid duplicates and speed up membership/removal), and consider a DAO method that fetches dedicated resources for (domain, optional account) in one query (or fetch all relevant dedicated resources once, then partition by pod/cluster/host). This keeps deployment planning latency more predictable at scale.

// Licensed to the Apache Software Foundation (ASF) under one

server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java:1012

• This mutates the allPodsInDc / allClustersInDc / allHostsInDc lists passed into the method. If the caller reuses these lists later (or expects them to remain unchanged), this will cause incorrect behavior. Prefer computing a separate toAvoid collection (e.g., copy the input lists first, then remove allowed IDs from the copies) before adding them to avoids.

        allPodsInDc.removeAll(allPodsFromDedicatedID);
        allClustersInDc.removeAll(allClustersFromDedicatedID);
        allHostsInDc.removeAll(allHostsFromDedicatedID);

server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java:981

• This adds up to 4–7 DB calls on the deploy path (affinity group lookup + 3 account searches + 0/3 domain searches). Consider reducing round-trips and allocations: use Set<Long> for IDs (to avoid duplicates and speed up membership/removal), and consider a DAO method that fetches dedicated resources for (domain, optional account) in one query (or fetch all relevant dedicated resources once, then partition by pod/cluster/host). This keeps deployment planning latency more predictable at scale.

        boolean hasDomainExplicitDedicationGroup =
                _affinityGroupDao.findDomainLevelGroupByType(vmDomainId, "ExplicitDedication") != null;

server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java:981

• The affinity group type is a hard-coded string. If there is an existing constant/enum for the ExplicitDedication type (used elsewhere in CloudStack), prefer referencing it to avoid typos and to keep refactors safe (the tests also repeat this string).

                _affinityGroupDao.findDomainLevelGroupByType(vmDomainId, "ExplicitDedication") != null;

server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java:1022

• These assertions are harder to read and partially duplicate null/empty handling. Consider using a single, intention-revealing assertion (e.g., assert that the collection does not contain the ID using CollectionUtils.contains/Hamcrest/AssertJ), which will also improve failure messages when the list is non-empty.

        assertTrue("Domain-dedicated pod should not be in avoid list for same-domain user",
                CollectionUtils.isEmpty(avoids.getPodsToAvoid()) || !avoids.getPodsToAvoid().contains(dedicatedPodId));

server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java:987

• The new behavior in findAvoidSetForNonExplicitUserVM applies equally to pods, clusters, and hosts, but the added tests only cover pods. Consider adding equivalent tests for domain/account-dedicated clusters and hosts to ensure the fix for #5803 is fully covered across all three resource types.

    @Test
    public void checkForNonDedicatedResources_domainDedicatedPod_sameDomainUser_podNotInAvoidList() {

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (8)

server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java:952

• The parameter mockDc is not used inside setupNonExplicitUserVmMocks(...). Dropping the unused parameter (and updating callers) will reduce noise and make the helper’s intent clearer.

    private VMInstanceVO setupNonExplicitUserVmMocks(long vmDomainId, long vmAccountId, DataCenter mockDc) {

server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java:976

• The names allPodsFromDedicatedID/allClustersFromDedicatedID/allHostsFromDedicatedID are hard to interpret (they read like IDs 'from' an ID). These appear to be the sets of dedicated resources that should be allowed for this VM (account and optionally domain). Consider renaming to something like allowedDedicatedPodIds, allowedDedicatedClusterIds, allowedDedicatedHostIds (or similar) to reflect their purpose.

        List<Long> allPodsFromDedicatedID = new ArrayList<>();
        List<Long> allClustersFromDedicatedID = new ArrayList<>();
        List<Long> allHostsFromDedicatedID = new ArrayList<>();

server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java:1012

• removeAll(...) is potentially O(n*m) here because the right-hand side is a List. Since these collections can grow with DC size, consider using a Set<Long> for the allowed IDs (and de-duplicating naturally), e.g. build HashSets for allowed pods/clusters/hosts and remove via removeAll(set) or removeIf(set::contains).

        allPodsInDc.removeAll(allPodsFromDedicatedID);
        allClustersInDc.removeAll(allClustersFromDedicatedID);
        allHostsInDc.removeAll(allHostsFromDedicatedID);

server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java:984

• The comment states 'no pagination — retrieve all', but it’s not clear that new Filter(DedicatedResourceVO.class, \"id\", true) guarantees an unlimited result size. If Filter defaults to a page size/limit, this can silently miss some allowed dedicated resources, leaving them incorrectly in the avoid list for large environments. Ensure the filter is explicitly unbounded (or iterate through pages until exhausted) so all dedicated resources are considered.

        // Sort by id ascending, no pagination — retrieve all dedicated resources for this domain/account
        Filter filter = new Filter(DedicatedResourceVO.class, "id", true);

server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java:987

• The updated production logic also applies the same 'allow account, optionally allow domain' behavior to clusters and hosts, but the added tests cover only pods. Adding analogous tests for domain/account-dedicated clusters and hosts would better protect the new behavior from regressions and ensure the fix applies consistently across all three resource types.

    @Test
    public void checkForNonDedicatedResources_domainDedicatedPod_sameDomainUser_podNotInAvoidList() {

server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java:1029

• The updated production logic also applies the same 'allow account, optionally allow domain' behavior to clusters and hosts, but the added tests cover only pods. Adding analogous tests for domain/account-dedicated clusters and hosts would better protect the new behavior from regressions and ensure the fix applies consistently across all three resource types.

    @Test
    public void checkForNonDedicatedResources_domainDedicatedPod_differentDomainUser_podInAvoidList() {

server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java:1071

• The updated production logic also applies the same 'allow account, optionally allow domain' behavior to clusters and hosts, but the added tests cover only pods. Adding analogous tests for domain/account-dedicated clusters and hosts would better protect the new behavior from regressions and ensure the fix applies consistently across all three resource types.

    @Test
    public void checkForNonDedicatedResources_accountDedicatedPod_sameAccount_podNotInAvoidList() {

server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java:1114

• The updated production logic also applies the same 'allow account, optionally allow domain' behavior to clusters and hosts, but the added tests cover only pods. Adding analogous tests for domain/account-dedicated clusters and hosts would better protect the new behavior from regressions and ensure the fix applies consistently across all three resource types.

    @Test
    public void checkForNonDedicatedResources_accountDedicatedPod_differentAccount_podInAvoidList() {

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java:981

• The affinity group type is hard-coded as the string literal "ExplicitDedication" (and repeated in tests). This creates a silent failure risk if the value changes or is misspelled in one place. Recommend replacing it with an existing constant (if available in the codebase) or introducing a dedicated constant in a shared location and reusing it here and in the tests.

        // 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).
        boolean hasDomainExplicitDedicationGroup =
                _affinityGroupDao.findDomainLevelGroupByType(vmDomainId, "ExplicitDedication") != null;

server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java:1012

• This method mutates the input lists (allPodsInDc, allClustersInDc, allHostsInDc) via removeAll. If the caller reuses these lists after this call (even just for logging/metrics/other decisions), this introduces unintended side effects and can lead to incorrect behavior elsewhere. Prefer computing separate toAvoid lists (e.g., copy inputs first) and leaving the caller-provided collections unchanged.

        allPodsInDc.removeAll(allowedDedicatedPodIds);
        allClustersInDc.removeAll(allowedDedicatedClusterIds);
        allHostsInDc.removeAll(allowedDedicatedHostIds);

server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java:1008

• This introduces up to 6 DAO searches per invocation (pods/clusters/hosts for account, plus pods/clusters/hosts for domain). Since deployment planning can be a hot path, consider reducing calls by only querying for resource types that actually exist in this DC (e.g., skip searchDedicatedPods if allPodsInDc.isEmpty(), etc.), and/or using Set<Long> for allowed IDs to avoid duplicate accumulation and make membership/removal more efficient.

        // Sort by id ascending, no pagination — retrieve all dedicated resources for this domain/account
        Filter filter = new Filter(DedicatedResourceVO.class, "id", true);

        // Always allow resources dedicated to the VM owner's account.
        for (DedicatedResourceVO vo : _dedicatedDao.searchDedicatedPods(null, vmDomainId, vmAccountId, null, filter).first()) {
            allowedDedicatedPodIds.add(vo.getPodId());
        }
        for (DedicatedResourceVO vo : _dedicatedDao.searchDedicatedClusters(null, vmDomainId, vmAccountId, null, filter).first()) {
            allowedDedicatedClusterIds.add(vo.getClusterId());
        }
        for (DedicatedResourceVO vo : _dedicatedDao.searchDedicatedHosts(null, vmDomainId, vmAccountId, null, filter).first()) {
            allowedDedicatedHostIds.add(vo.getHostId());
        }

        if (hasDomainExplicitDedicationGroup) {
            // Domain has explicit dedication affinity groups: also allow resources dedicated to this domain
            for (DedicatedResourceVO vo : _dedicatedDao.searchDedicatedPods(null, vmDomainId, null, null, filter).first()) {
                allowedDedicatedPodIds.add(vo.getPodId());
            }
            for (DedicatedResourceVO vo : _dedicatedDao.searchDedicatedClusters(null, vmDomainId, null, null, filter).first()) {
                allowedDedicatedClusterIds.add(vo.getClusterId());
            }
            for (DedicatedResourceVO vo : _dedicatedDao.searchDedicatedHosts(null, vmDomainId, null, null, filter).first()) {
                allowedDedicatedHostIds.add(vo.getHostId());
            }
        }

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@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.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18717

[DaanHoogland]

@blueorangutan test

[blueorangutan]

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests