Skip to content

Commit 0a6803f

Browse files
vishesh92vishesh92
committed
Address comments & fix tests
1 parent e7e4e9d commit 0a6803f

File tree

4 files changed

+56
-45
lines changed

4 files changed

+56
-45
lines changed

‎engine/schema/src/main/java/org/apache/cloudstack/resourcedetail/ResourceDetailsDao.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,4 +103,6 @@ public interface ResourceDetailsDao<R extends ResourceDetail> extends GenericDao
103103
long batchExpungeForResources(List<Long> ids, Long batchSize);
104104

105105
String getActualValue(ResourceDetail resourceDetail);
106+
107+
List<R> listDetailsForResourceIdsAndKey(List<Long> resourceIds, String key);
106108
}

‎engine/schema/src/main/java/org/apache/cloudstack/resourcedetail/ResourceDetailsDaoBase.java‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,13 @@
1616
// under the License.
1717
package org.apache.cloudstack.resourcedetail;
1818

19+
import java.util.Collections;
1920
import java.util.HashMap;
2021
import java.util.List;
2122
import java.util.Map;
2223
import java.util.stream.Collectors;
2324

25+
import com.cloud.utils.StringUtils;
2426
import org.apache.commons.collections.CollectionUtils;
2527

2628
import com.cloud.utils.crypt.DBEncryptionUtil;
@@ -246,4 +248,20 @@ public String getActualValue(ResourceDetail resourceDetail) {
246248
}
247249
return resourceDetail.getValue();
248250
}
251+
252+
@Override
253+
public List<R> listDetailsForResourceIdsAndKey(List<Long> resourceIds, String key) {
254+
if (CollectionUtils.isEmpty(resourceIds) || StringUtils.isBlank(key)) {
255+
return Collections.emptyList();
256+
}
257+
SearchBuilder<R> sb = createSearchBuilder();
258+
sb.and("name", sb.entity().getName(), Op.EQ);
259+
sb.and("resourceIdIn", sb.entity().getResourceId(), Op.IN);
260+
sb.done();
261+
262+
SearchCriteria<R> sc = sb.create();
263+
sc.setParameters("name", key);
264+
sc.setParameters("resourceIdIn", resourceIds.toArray());
265+
return search(sc, null);
266+
}
249267
}

‎server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java‎

Lines changed: 11 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,6 @@
8080
import org.apache.cloudstack.jobs.JobInfo;
8181
import org.apache.cloudstack.managed.context.ManagedContextTimerTask;
8282
import org.apache.commons.collections.CollectionUtils;
83-
import org.apache.commons.collections.MapUtils;
8483
import org.apache.commons.lang3.time.DateUtils;
8584

8685
import javax.inject.Inject;
@@ -137,7 +136,7 @@ public class ClusterDrsServiceImpl extends ManagerBase implements ClusterDrsServ
137136
ServiceOfferingDao serviceOfferingDao;
138137

139138
@Inject
140-
UserVmDetailsDao userVmdetailsDao;
139+
UserVmDetailsDao userVmDetailsDao;
141140

142141
@Inject
143142
ManagementServer managementServer;
@@ -480,9 +479,15 @@ private Pair<Map<Long, List<? extends Host>>, Map<Long, Map<Host, Boolean>>> get
480479
Map<Long, List<? extends Host>> vmToCompatibleHostsCache = new HashMap<>();
481480
Map<Long, Map<Host, Boolean>> vmToStorageMotionCache = new HashMap<>();
482481

482+
List<Long> vmIds = vmList.stream().map(VirtualMachine::getId).collect(Collectors.toList());
483+
Set<Long> skipDrsVmIds = userVmDetailsDao.listDetailsForResourceIdsAndKey(vmIds, VmDetailConstants.SKIP_DRS)
484+
.stream().filter(d -> "true".equalsIgnoreCase(d.getValue()))
485+
.map(UserVmDetailVO::getResourceId)
486+
.collect(Collectors.toSet());
487+
483488
for (VirtualMachine vm : vmList) {
484489
// Skip ineligible VMs
485-
if (shouldSkipVMForDRS(vm)) {
490+
if (shouldSkipVMForDRS(vm, skipDrsVmIds)) {
486491
continue;
487492
}
488493

@@ -609,7 +614,7 @@ Pair<VirtualMachine, Host> getBestMigration(Cluster cluster, ClusterDrsAlgorithm
609614
ExcludeList excludes = vmToExcludesMap.get(vm.getId());
610615

611616
ServiceOffering serviceOffering = vmIdServiceOfferingMap.get(vm.getId());
612-
if (skipDrs(vm, compatibleHosts, serviceOffering)) {
617+
if (CollectionUtils.isEmpty(compatibleHosts) || serviceOffering == null) {
613618
continue;
614619
}
615620

@@ -635,29 +640,11 @@ Pair<VirtualMachine, Host> getBestMigration(Cluster cluster, ClusterDrsAlgorithm
635640
return bestMigration;
636641
}
637642

638-
private boolean shouldSkipVMForDRS(VirtualMachine vm) {
643+
private boolean shouldSkipVMForDRS(VirtualMachine vm, Set<Long> skipDrsVmIds) {
639644
if (vm.getType().isUsedBySystem() || vm.getState() != VirtualMachine.State.Running) {
640645
return true;
641646
}
642-
643-
UserVmDetailVO skipDrsDetail = userVmdetailsDao.findDetail(vm.getId(), VmDetailConstants.SKIP_DRS);
644-
if (skipDrsDetail != null && "true".equalsIgnoreCase(skipDrsDetail.getValue())) {
645-
return true;
646-
}
647-
return false;
648-
}
649-
650-
private boolean skipDrs(VirtualMachine vm, List<? extends Host> compatibleHosts, ServiceOffering serviceOffering) {
651-
if (shouldSkipVMForDRS(vm)) {
652-
return true;
653-
}
654-
if (CollectionUtils.isEmpty(compatibleHosts)) {
655-
return true;
656-
}
657-
if (serviceOffering == null) {
658-
return true;
659-
}
660-
return false;
647+
return skipDrsVmIds.contains(vm.getId());
661648
}
662649

663650
private Pair<double[], Map<Long, Integer>> getBaseMetricsArrayAndHostIdIndexMap(

‎server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java‎

Lines changed: 25 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -40,9 +40,11 @@
4040
import com.cloud.utils.Ternary;
4141
import com.cloud.utils.db.GlobalLock;
4242
import com.cloud.utils.exception.CloudRuntimeException;
43+
import com.cloud.vm.UserVmDetailVO;
4344
import com.cloud.vm.VMInstanceVO;
4445
import com.cloud.vm.VirtualMachine;
4546
import com.cloud.vm.VmDetailConstants;
47+
import com.cloud.vm.dao.UserVmDetailsDao;
4648
import com.cloud.vm.dao.VMInstanceDao;
4749
import org.apache.cloudstack.affinity.dao.AffinityGroupVMMapDao;
4850
import org.apache.cloudstack.api.command.admin.cluster.GenerateClusterDrsPlanCmd;
@@ -121,6 +123,9 @@ public class ClusterDrsServiceImplTest {
121123
@Mock
122124
private AffinityGroupVMMapDao affinityGroupVMMapDao;
123125

126+
@Mock
127+
private UserVmDetailsDao userVmDetailsDao;
128+
124129
@Spy
125130
@InjectMocks
126131
private ClusterDrsServiceImpl clusterDrsService = new ClusterDrsServiceImpl();
@@ -294,6 +299,8 @@ public void testGetDrsPlanWithSystemVMs() throws ConfigurationException {
294299

295300
List<Ternary<VirtualMachine, Host, Host>> result = clusterDrsService.getDrsPlan(cluster, 5);
296301
assertEquals(0, result.size());
302+
Mockito.verify(managementServer, Mockito.never()).listHostsForMigrationOfVM(
303+
Mockito.eq(systemVm), Mockito.anyLong(), Mockito.anyLong(), Mockito.any(), Mockito.anyList());
297304
}
298305

299306
@Test
@@ -334,6 +341,8 @@ public void testGetDrsPlanWithNonRunningVMs() throws ConfigurationException {
334341

335342
List<Ternary<VirtualMachine, Host, Host>> result = clusterDrsService.getDrsPlan(cluster, 5);
336343
assertEquals(0, result.size());
344+
Mockito.verify(managementServer, Mockito.never()).listHostsForMigrationOfVM(
345+
Mockito.eq(stoppedVm), Mockito.anyLong(), Mockito.anyLong(), Mockito.any(), Mockito.anyList());
337346
}
338347

339348
@Test
@@ -350,9 +359,6 @@ public void testGetDrsPlanWithSkipDrsFlag() throws ConfigurationException {
350359
Mockito.when(skippedVm.getHostId()).thenReturn(1L);
351360
Mockito.when(skippedVm.getType()).thenReturn(VirtualMachine.Type.User);
352361
Mockito.when(skippedVm.getState()).thenReturn(VirtualMachine.State.Running);
353-
Map<String, String> details = new HashMap<>();
354-
details.put(VmDetailConstants.SKIP_DRS, "true");
355-
Mockito.when(skippedVm.getDetails()).thenReturn(details);
356362

357363
List<HostVO> hostList = new ArrayList<>();
358364
hostList.add(host1);
@@ -370,13 +376,21 @@ public void testGetDrsPlanWithSkipDrsFlag() throws ConfigurationException {
370376
Mockito.when(hostJoin1.getMemReservedCapacity()).thenReturn(0L);
371377
Mockito.when(hostJoin1.getTotalMemory()).thenReturn(8192L);
372378

379+
// Return the SKIP_DRS detail for skippedVm so the flag is actually honoured
380+
UserVmDetailVO skipDrsDetail = new UserVmDetailVO(1L, VmDetailConstants.SKIP_DRS, "true", true);
381+
Mockito.when(userVmDetailsDao.listDetailsForResourceIdsAndKey(Mockito.anyList(),
382+
Mockito.eq(VmDetailConstants.SKIP_DRS))).thenReturn(List.of(skipDrsDetail));
383+
373384
Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList);
374385
Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList);
375386
Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn(true);
376387
Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1));
377388

378389
List<Ternary<VirtualMachine, Host, Host>> result = clusterDrsService.getDrsPlan(cluster, 5);
379390
assertEquals(0, result.size());
391+
// Verify the VM was skipped before any host-compatibility lookup was attempted
392+
Mockito.verify(managementServer, Mockito.never()).listHostsForMigrationOfVM(
393+
Mockito.eq(skippedVm), Mockito.anyLong(), Mockito.anyLong(), Mockito.any(), Mockito.anyList());
380394
}
381395

382396
@Test
@@ -393,7 +407,6 @@ public void testGetDrsPlanWithNoCompatibleHosts() throws ConfigurationException
393407
Mockito.when(vm1.getHostId()).thenReturn(1L);
394408
Mockito.when(vm1.getType()).thenReturn(VirtualMachine.Type.User);
395409
Mockito.when(vm1.getState()).thenReturn(VirtualMachine.State.Running);
396-
Mockito.when(vm1.getDetails()).thenReturn(Collections.emptyMap());
397410

398411
List<HostVO> hostList = new ArrayList<>();
399412
hostList.add(host1);
@@ -418,6 +431,10 @@ public void testGetDrsPlanWithNoCompatibleHosts() throws ConfigurationException
418431
Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn(true);
419432
Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(Mockito.anyLong(), Mockito.anyLong())).thenReturn(serviceOffering);
420433
Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1));
434+
// Return a Ternary with an empty suitable-hosts list to exercise the "no compatible hosts" path
435+
Mockito.when(managementServer.listHostsForMigrationOfVM(Mockito.eq(vm1), Mockito.anyLong(),
436+
Mockito.anyLong(), Mockito.any(), Mockito.anyList()))
437+
.thenReturn(new Ternary<>(new Pair<>(Collections.emptyList(), 0), Collections.emptyList(), Collections.emptyMap()));
421438

422439
List<Ternary<VirtualMachine, Host, Host>> result = clusterDrsService.getDrsPlan(cluster, 5);
423440
assertEquals(0, result.size());
@@ -438,7 +455,6 @@ public void testGetDrsPlanWithExceptionInCompatibilityCheck() throws Configurati
438455
Mockito.when(vm1.getHostId()).thenReturn(1L);
439456
Mockito.when(vm1.getType()).thenReturn(VirtualMachine.Type.User);
440457
Mockito.when(vm1.getState()).thenReturn(VirtualMachine.State.Running);
441-
Mockito.when(vm1.getDetails()).thenReturn(Collections.emptyMap());
442458

443459
List<HostVO> hostList = new ArrayList<>();
444460
hostList.add(host1);
@@ -463,6 +479,10 @@ public void testGetDrsPlanWithExceptionInCompatibilityCheck() throws Configurati
463479
Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn(true);
464480
Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(Mockito.anyLong(), Mockito.anyLong())).thenReturn(serviceOffering);
465481
Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1));
482+
// Throw an explicit exception so the catch-and-log path is exercised intentionally
483+
Mockito.when(managementServer.listHostsForMigrationOfVM(Mockito.eq(vm1), Mockito.anyLong(),
484+
Mockito.anyLong(), Mockito.any(), Mockito.anyList()))
485+
.thenThrow(new RuntimeException("Simulated host compatibility check failure"));
466486

467487
List<Ternary<VirtualMachine, Host, Host>> result = clusterDrsService.getDrsPlan(cluster, 5);
468488
assertEquals(0, result.size());
@@ -484,7 +504,6 @@ public void testGetDrsPlanWithNoBestMigration() throws ConfigurationException {
484504
Mockito.when(vm1.getHostId()).thenReturn(1L);
485505
Mockito.when(vm1.getType()).thenReturn(VirtualMachine.Type.User);
486506
Mockito.when(vm1.getState()).thenReturn(VirtualMachine.State.Running);
487-
Mockito.when(vm1.getDetails()).thenReturn(Collections.emptyMap());
488507

489508
List<HostVO> hostList = new ArrayList<>();
490509
hostList.add(host1);
@@ -539,14 +558,12 @@ public void testGetDrsPlanWithMultipleIterations() throws ConfigurationException
539558
Mockito.when(vm1.getHostId()).thenReturn(1L);
540559
Mockito.when(vm1.getType()).thenReturn(VirtualMachine.Type.User);
541560
Mockito.when(vm1.getState()).thenReturn(VirtualMachine.State.Running);
542-
Mockito.when(vm1.getDetails()).thenReturn(Collections.emptyMap());
543561

544562
VMInstanceVO vm2 = Mockito.mock(VMInstanceVO.class);
545563
Mockito.when(vm2.getId()).thenReturn(2L);
546564
Mockito.when(vm2.getHostId()).thenReturn(1L);
547565
Mockito.when(vm2.getType()).thenReturn(VirtualMachine.Type.User);
548566
Mockito.when(vm2.getState()).thenReturn(VirtualMachine.State.Running);
549-
Mockito.when(vm2.getDetails()).thenReturn(Collections.emptyMap());
550567

551568
List<HostVO> hostList = new ArrayList<>();
552569
hostList.add(host1);
@@ -619,7 +636,6 @@ public void testGetDrsPlanWithMigrationToOriginalHost() throws ConfigurationExce
619636
Mockito.when(vm1.getHostId()).thenReturn(1L);
620637
Mockito.when(vm1.getType()).thenReturn(VirtualMachine.Type.User);
621638
Mockito.when(vm1.getState()).thenReturn(VirtualMachine.State.Running);
622-
Mockito.when(vm1.getDetails()).thenReturn(Collections.emptyMap());
623639

624640
List<HostVO> hostList = new ArrayList<>();
625641
hostList.add(host1);
@@ -811,15 +827,9 @@ public void testGetBestMigration() throws ConfigurationException {
811827

812828
VMInstanceVO vm1 = Mockito.mock(VMInstanceVO.class);
813829
Mockito.when(vm1.getId()).thenReturn(1L);
814-
Mockito.when(vm1.getType()).thenReturn(VirtualMachine.Type.User);
815-
Mockito.when(vm1.getState()).thenReturn(VirtualMachine.State.Running);
816-
Mockito.when(vm1.getDetails()).thenReturn(Collections.emptyMap());
817830

818831
VMInstanceVO vm2 = Mockito.mock(VMInstanceVO.class);
819832
Mockito.when(vm2.getId()).thenReturn(2L);
820-
Mockito.when(vm2.getType()).thenReturn(VirtualMachine.Type.User);
821-
Mockito.when(vm2.getState()).thenReturn(VirtualMachine.State.Running);
822-
Mockito.when(vm2.getDetails()).thenReturn(Collections.emptyMap());
823833

824834
List<VirtualMachine> vmList = new ArrayList<>();
825835
vmList.add(vm1);
@@ -890,15 +900,9 @@ public void testGetBestMigrationDifferentCluster() throws ConfigurationException
890900

891901
VMInstanceVO vm1 = Mockito.mock(VMInstanceVO.class);
892902
Mockito.when(vm1.getId()).thenReturn(1L);
893-
Mockito.when(vm1.getType()).thenReturn(VirtualMachine.Type.User);
894-
Mockito.when(vm1.getState()).thenReturn(VirtualMachine.State.Running);
895-
Mockito.when(vm1.getDetails()).thenReturn(Collections.emptyMap());
896903

897904
VMInstanceVO vm2 = Mockito.mock(VMInstanceVO.class);
898905
Mockito.when(vm2.getId()).thenReturn(2L);
899-
Mockito.when(vm2.getType()).thenReturn(VirtualMachine.Type.User);
900-
Mockito.when(vm2.getState()).thenReturn(VirtualMachine.State.Running);
901-
Mockito.when(vm2.getDetails()).thenReturn(Collections.emptyMap());
902906

903907
List<VirtualMachine> vmList = new ArrayList<>();
904908
vmList.add(vm1);

0 commit comments

Comments
 (0)