Copilot commented on code in PR #13529:
URL: https://github.com/apache/cloudstack/pull/13529#discussion_r3529650742
##########
server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java:
##########
@@ -937,6 +945,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:
This test stubs `AffinityGroupDomainMapDao.listByDomain(...)`, but the code
under test checks `AffinityGroupDao.findDomainLevelGroupByType(vmDomainId,
"ExplicitDedication")`. Also, `findAvoidSetForNonExplicitUserVM` first calls
`searchDedicatedPods(..., vmAccountId, ...)`; since that call is not stubbed
here, Mockito returns null and the test will NPE on `.first()`. Update the
stubs to match the actual calls (account-level first, then domain-level when
the explicit dedication group exists).
##########
server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java:
##########
@@ -227,6 +229,12 @@ public class DeploymentPlanningManagerImplTest {
@Inject
HostPodDao _podDao;
+ @Inject
+ HostDao _hostDao;
+
+ @Inject
+ AffinityGroupDomainMapDao _affinityGroupDomainMapDao;
+
Review Comment:
New tests need to stub the production check
`AffinityGroupDao.findDomainLevelGroupByType(vmDomainId,
"ExplicitDedication")`, but the test class does not inject an
`AffinityGroupDao` instance (only `AffinityGroupDomainMapDao` was added).
Without injecting the DAO, the domain-dedication branch in
`findAvoidSetForNonExplicitUserVM` cannot be configured and the tests either
fail or don't test the intended behavior.
##########
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<>();
+
Review Comment:
`findAvoidSetForNonExplicitUserVM` is called on the deploy path; when there
are no dedicated pods/clusters/hosts in the DC, the method still performs
multiple DAO searches. Add an early-return (or per-resource-type guards) to
avoid unnecessary DB calls when `allPodsInDc`/`allClustersInDc`/`allHostsInDc`
are all empty.
--
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]
