Copilot commented on code in PR #13529:
URL: https://github.com/apache/cloudstack/pull/13529#discussion_r3529525539
##########
server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java:
##########
@@ -937,6 +942,208 @@ private void assertAvoidIsEmpty(ExcludeList avoids,
boolean isDcEmpty, boolean i
Assert.assertEquals(isHostsEmpty,
CollectionUtils.isEmpty(avoids.getHostsToAvoid()));
}
+ /**
+ * Helper: set up mocks for checkForNonDedicatedResources tests.
+ * Sets up a non-root, non-explicit user VM with the given domain and
account.
+ */
+ private VMInstanceVO setupNonExplicitUserVmMocks(long vmDomainId, long
vmAccountId, DataCenter mockDc) {
+ // Zone is not dedicated
+
Mockito.when(_dedicatedDao.findByZoneId(Mockito.anyLong())).thenReturn(null);
+ // Not root admin
+
Mockito.when(_accountMgr.isRootAdmin(Mockito.anyLong())).thenReturn(false);
+ // No explicit dedication affinity group for the VM
+ Mockito.when(_affinityGroupVMMapDao.findByVmIdType(Mockito.anyLong(),
Mockito.eq("ExplicitDedication")))
+ .thenReturn(new ArrayList<>());
+
+ VMInstanceVO mockVm = Mockito.mock(VMInstanceVO.class);
+ Mockito.when(mockVm.getType()).thenReturn(VirtualMachine.Type.User);
+ Mockito.when(mockVm.getDomainId()).thenReturn(vmDomainId);
+ Mockito.when(mockVm.getAccountId()).thenReturn(vmAccountId);
+ Mockito.when(mockVm.getId()).thenReturn(instanceId);
+
+ // No dedicated clusters or hosts in DC
+
Mockito.when(_clusterDao.listAllClusterIds(Mockito.anyLong())).thenReturn(new
ArrayList<>());
+ Mockito.when(_dedicatedDao.listAllClusters()).thenReturn(new
ArrayList<>());
+ Mockito.when(_hostDao.listAllHosts(Mockito.anyLong())).thenReturn(new
ArrayList<>());
+ Mockito.when(_dedicatedDao.listAllHosts()).thenReturn(new
ArrayList<>());
+ Mockito.when(_dedicatedDao.searchDedicatedClusters(Mockito.any(),
Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()))
+ .thenReturn(new Pair<>(new ArrayList<>(), 0));
+ Mockito.when(_dedicatedDao.searchDedicatedHosts(Mockito.any(),
Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()))
+ .thenReturn(new Pair<>(new ArrayList<>(), 0));
+
+ return mockVm;
+ }
+
+ /**
+ * Issue #5803: A regular user in the same domain as a domain-dedicated pod
+ * must be allowed to deploy on that pod (i.e., the pod must NOT appear in
the avoid list).
+ */
+ @Test
+ public void
checkForNonDedicatedResources_domainDedicatedPod_sameDomainUser_podNotInAvoidList()
{
+ long vmDomainId = 10L;
+ long vmAccountId = 200L;
+ long dedicatedPodId = 42L;
+
+ DataCenter mockDc = Mockito.mock(DataCenter.class);
+ Mockito.when(mockDc.getId()).thenReturn(dataCenterId);
+
+ VMInstanceVO mockVm = setupNonExplicitUserVmMocks(vmDomainId,
vmAccountId, mockDc);
+
+ // Pod in DC and it is dedicated
+ List<Long> podsInDc = new ArrayList<>(Arrays.asList(dedicatedPodId));
+ Mockito.when(_podDao.listAllPods(dataCenterId)).thenReturn(podsInDc);
+ Mockito.when(_dedicatedDao.listAllPods()).thenReturn(new
ArrayList<>(Arrays.asList(dedicatedPodId)));
+
+ // 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));
Review Comment:
In this test, `_dpm.findAvoidSetForNonExplicitUserVM` always calls
`_dedicatedDao.searchDedicatedPods(null, vmDomainId, vmAccountId, ...)`
(account-level) before the domain-level query. Since this test only stubs the
domain-level `searchDedicatedPods(..., accountId=null, ...)`, the account-level
call will return `null` on the mock and will NPE when `.first()` is accessed.
##########
server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java:
##########
@@ -963,8 +963,51 @@ private void findAvoiSetForRouterVM(ExcludeList avoids,
VirtualMachine vm, List<
}
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<>();
+
+ // Check if the VM owner's domain has explicit dedication affinity
group mappings.
+ // If so, resources dedicated to that domain are accessible to the VM
owner (fixes
+ // issue where users in a domain-dedicated pod could not deploy with
ImplicitDedicationPlanner).
+ List<AffinityGroupDomainMapVO> domainGroupMappings =
_affinityGroupDomainMapDao.listByDomain(vmDomainId);
+
+ // 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()) {
+ allPodsFromDedicatedID.add(vo.getPodId());
+ }
+ for (DedicatedResourceVO vo :
_dedicatedDao.searchDedicatedClusters(null, vmDomainId, vmAccountId, null,
filter).first()) {
+ allClustersFromDedicatedID.add(vo.getClusterId());
+ }
+ for (DedicatedResourceVO vo : _dedicatedDao.searchDedicatedHosts(null,
vmDomainId, vmAccountId, null, filter).first()) {
+ allHostsFromDedicatedID.add(vo.getHostId());
+ }
+
+ if (domainGroupMappings != null && !domainGroupMappings.isEmpty()) {
+ // Domain has explicit dedication affinity groups: also allow
resources dedicated to this domain
Review Comment:
The code/comment says it is checking for an "explicit dedication affinity
group" for the domain, but `AffinityGroupDomainMapDao.listByDomain(vmDomainId)`
returns mappings for any domain-level affinity group. Since this value is only
used as a boolean, consider checking for the ExplicitDedication domain-level
group directly to match the intent and reduce indirect coupling to the
domain-map table.
--
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]
