From 537dd175e7d3ab83d6698d90b5066ebb34292097 Mon Sep 17 00:00:00 2001 From: vishesh92 Date: Fri, 10 Apr 2026 12:13:03 +0530 Subject: [PATCH 1/3] Fix skip DRS for a VM --- .../cluster/ClusterDrsServiceImpl.java | 24 +++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java b/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java index 7e1011ff9314..e34cb38830a4 100644 --- a/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java +++ b/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java @@ -51,11 +51,13 @@ import com.cloud.utils.db.Transaction; import com.cloud.utils.db.TransactionCallback; import com.cloud.utils.exception.CloudRuntimeException; +import com.cloud.vm.UserVmDetailVO; import com.cloud.vm.VMInstanceVO; import com.cloud.vm.VirtualMachine; import com.cloud.vm.VirtualMachineProfile; import com.cloud.vm.VirtualMachineProfileImpl; import com.cloud.vm.VmDetailConstants; +import com.cloud.vm.dao.UserVmDetailsDao; import com.cloud.vm.dao.VMInstanceDao; import org.apache.cloudstack.api.ApiCommandResourceType; import org.apache.cloudstack.api.ApiConstants; @@ -134,6 +136,9 @@ public class ClusterDrsServiceImpl extends ManagerBase implements ClusterDrsServ @Inject ServiceOfferingDao serviceOfferingDao; + @Inject + UserVmDetailsDao userVmdetailsDao; + @Inject ManagementServer managementServer; @@ -477,10 +482,7 @@ private Pair>, Map>> get for (VirtualMachine vm : vmList) { // Skip ineligible VMs - if (vm.getType().isUsedBySystem() || - vm.getState() != VirtualMachine.State.Running || - (MapUtils.isNotEmpty(vm.getDetails()) && - "true".equalsIgnoreCase(vm.getDetails().get(VmDetailConstants.SKIP_DRS)))) { + if (shouldSkipVMForDRS(vm)) { continue; } @@ -633,12 +635,20 @@ Pair getBestMigration(Cluster cluster, ClusterDrsAlgorithm return bestMigration; } - private boolean skipDrs(VirtualMachine vm, List compatibleHosts, ServiceOffering serviceOffering) { + private boolean shouldSkipVMForDRS(VirtualMachine vm) { if (vm.getType().isUsedBySystem() || vm.getState() != VirtualMachine.State.Running) { return true; } - if (MapUtils.isNotEmpty(vm.getDetails()) && - "true".equalsIgnoreCase(vm.getDetails().get(VmDetailConstants.SKIP_DRS))) { + + UserVmDetailVO skipDrsDetail = userVmdetailsDao.findDetail(vm.getId(), VmDetailConstants.SKIP_DRS); + if (skipDrsDetail != null && skipDrsDetail.getValue().equalsIgnoreCase("true")) { + return true; + } + return false; + } + + private boolean skipDrs(VirtualMachine vm, List compatibleHosts, ServiceOffering serviceOffering) { + if (shouldSkipVMForDRS(vm)) { return true; } if (CollectionUtils.isEmpty(compatibleHosts)) { From e7e4e9d5c58ea0d1231e46d2c0f3f57b936fe4a9 Mon Sep 17 00:00:00 2001 From: Vishesh <8760112+vishesh92@users.noreply.github.com> Date: Fri, 10 Apr 2026 12:21:28 +0530 Subject: [PATCH 2/3] Update server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java Co-authored-by: Suresh Kumar Anaparti --- .../org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java b/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java index e34cb38830a4..c913e55e732b 100644 --- a/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java +++ b/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java @@ -641,7 +641,7 @@ private boolean shouldSkipVMForDRS(VirtualMachine vm) { } UserVmDetailVO skipDrsDetail = userVmdetailsDao.findDetail(vm.getId(), VmDetailConstants.SKIP_DRS); - if (skipDrsDetail != null && skipDrsDetail.getValue().equalsIgnoreCase("true")) { + if (skipDrsDetail != null && "true".equalsIgnoreCase(skipDrsDetail.getValue())) { return true; } return false; From 0a6803f170ad77adf35109d59a4633533bc9c603 Mon Sep 17 00:00:00 2001 From: vishesh92 Date: Fri, 10 Apr 2026 13:00:34 +0530 Subject: [PATCH 3/3] Address comments & fix tests --- .../resourcedetail/ResourceDetailsDao.java | 2 + .../ResourceDetailsDaoBase.java | 18 ++++++++ .../cluster/ClusterDrsServiceImpl.java | 35 +++++--------- .../cluster/ClusterDrsServiceImplTest.java | 46 ++++++++++--------- 4 files changed, 56 insertions(+), 45 deletions(-) diff --git a/engine/schema/src/main/java/org/apache/cloudstack/resourcedetail/ResourceDetailsDao.java b/engine/schema/src/main/java/org/apache/cloudstack/resourcedetail/ResourceDetailsDao.java index 6b6fe200c10d..e3241731f719 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/resourcedetail/ResourceDetailsDao.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/resourcedetail/ResourceDetailsDao.java @@ -103,4 +103,6 @@ public interface ResourceDetailsDao extends GenericDao long batchExpungeForResources(List ids, Long batchSize); String getActualValue(ResourceDetail resourceDetail); + + List listDetailsForResourceIdsAndKey(List resourceIds, String key); } diff --git a/engine/schema/src/main/java/org/apache/cloudstack/resourcedetail/ResourceDetailsDaoBase.java b/engine/schema/src/main/java/org/apache/cloudstack/resourcedetail/ResourceDetailsDaoBase.java index 29d3f88fd902..8f777d8af545 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/resourcedetail/ResourceDetailsDaoBase.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/resourcedetail/ResourceDetailsDaoBase.java @@ -16,11 +16,13 @@ // under the License. package org.apache.cloudstack.resourcedetail; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.stream.Collectors; +import com.cloud.utils.StringUtils; import org.apache.commons.collections.CollectionUtils; import com.cloud.utils.crypt.DBEncryptionUtil; @@ -246,4 +248,20 @@ public String getActualValue(ResourceDetail resourceDetail) { } return resourceDetail.getValue(); } + + @Override + public List listDetailsForResourceIdsAndKey(List resourceIds, String key) { + if (CollectionUtils.isEmpty(resourceIds) || StringUtils.isBlank(key)) { + return Collections.emptyList(); + } + SearchBuilder sb = createSearchBuilder(); + sb.and("name", sb.entity().getName(), Op.EQ); + sb.and("resourceIdIn", sb.entity().getResourceId(), Op.IN); + sb.done(); + + SearchCriteria sc = sb.create(); + sc.setParameters("name", key); + sc.setParameters("resourceIdIn", resourceIds.toArray()); + return search(sc, null); + } } diff --git a/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java b/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java index c913e55e732b..44d929848095 100644 --- a/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java +++ b/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java @@ -80,7 +80,6 @@ import org.apache.cloudstack.jobs.JobInfo; import org.apache.cloudstack.managed.context.ManagedContextTimerTask; import org.apache.commons.collections.CollectionUtils; -import org.apache.commons.collections.MapUtils; import org.apache.commons.lang3.time.DateUtils; import javax.inject.Inject; @@ -137,7 +136,7 @@ public class ClusterDrsServiceImpl extends ManagerBase implements ClusterDrsServ ServiceOfferingDao serviceOfferingDao; @Inject - UserVmDetailsDao userVmdetailsDao; + UserVmDetailsDao userVmDetailsDao; @Inject ManagementServer managementServer; @@ -480,9 +479,15 @@ private Pair>, Map>> get Map> vmToCompatibleHostsCache = new HashMap<>(); Map> vmToStorageMotionCache = new HashMap<>(); + List vmIds = vmList.stream().map(VirtualMachine::getId).collect(Collectors.toList()); + Set skipDrsVmIds = userVmDetailsDao.listDetailsForResourceIdsAndKey(vmIds, VmDetailConstants.SKIP_DRS) + .stream().filter(d -> "true".equalsIgnoreCase(d.getValue())) + .map(UserVmDetailVO::getResourceId) + .collect(Collectors.toSet()); + for (VirtualMachine vm : vmList) { // Skip ineligible VMs - if (shouldSkipVMForDRS(vm)) { + if (shouldSkipVMForDRS(vm, skipDrsVmIds)) { continue; } @@ -609,7 +614,7 @@ Pair getBestMigration(Cluster cluster, ClusterDrsAlgorithm ExcludeList excludes = vmToExcludesMap.get(vm.getId()); ServiceOffering serviceOffering = vmIdServiceOfferingMap.get(vm.getId()); - if (skipDrs(vm, compatibleHosts, serviceOffering)) { + if (CollectionUtils.isEmpty(compatibleHosts) || serviceOffering == null) { continue; } @@ -635,29 +640,11 @@ Pair getBestMigration(Cluster cluster, ClusterDrsAlgorithm return bestMigration; } - private boolean shouldSkipVMForDRS(VirtualMachine vm) { + private boolean shouldSkipVMForDRS(VirtualMachine vm, Set skipDrsVmIds) { if (vm.getType().isUsedBySystem() || vm.getState() != VirtualMachine.State.Running) { return true; } - - UserVmDetailVO skipDrsDetail = userVmdetailsDao.findDetail(vm.getId(), VmDetailConstants.SKIP_DRS); - if (skipDrsDetail != null && "true".equalsIgnoreCase(skipDrsDetail.getValue())) { - return true; - } - return false; - } - - private boolean skipDrs(VirtualMachine vm, List compatibleHosts, ServiceOffering serviceOffering) { - if (shouldSkipVMForDRS(vm)) { - return true; - } - if (CollectionUtils.isEmpty(compatibleHosts)) { - return true; - } - if (serviceOffering == null) { - return true; - } - return false; + return skipDrsVmIds.contains(vm.getId()); } private Pair> getBaseMetricsArrayAndHostIdIndexMap( diff --git a/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java b/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java index b76ef51c5234..6dc73b7a6307 100644 --- a/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java @@ -40,9 +40,11 @@ import com.cloud.utils.Ternary; import com.cloud.utils.db.GlobalLock; import com.cloud.utils.exception.CloudRuntimeException; +import com.cloud.vm.UserVmDetailVO; import com.cloud.vm.VMInstanceVO; import com.cloud.vm.VirtualMachine; import com.cloud.vm.VmDetailConstants; +import com.cloud.vm.dao.UserVmDetailsDao; import com.cloud.vm.dao.VMInstanceDao; import org.apache.cloudstack.affinity.dao.AffinityGroupVMMapDao; import org.apache.cloudstack.api.command.admin.cluster.GenerateClusterDrsPlanCmd; @@ -121,6 +123,9 @@ public class ClusterDrsServiceImplTest { @Mock private AffinityGroupVMMapDao affinityGroupVMMapDao; + @Mock + private UserVmDetailsDao userVmDetailsDao; + @Spy @InjectMocks private ClusterDrsServiceImpl clusterDrsService = new ClusterDrsServiceImpl(); @@ -294,6 +299,8 @@ public void testGetDrsPlanWithSystemVMs() throws ConfigurationException { List> result = clusterDrsService.getDrsPlan(cluster, 5); assertEquals(0, result.size()); + Mockito.verify(managementServer, Mockito.never()).listHostsForMigrationOfVM( + Mockito.eq(systemVm), Mockito.anyLong(), Mockito.anyLong(), Mockito.any(), Mockito.anyList()); } @Test @@ -334,6 +341,8 @@ public void testGetDrsPlanWithNonRunningVMs() throws ConfigurationException { List> result = clusterDrsService.getDrsPlan(cluster, 5); assertEquals(0, result.size()); + Mockito.verify(managementServer, Mockito.never()).listHostsForMigrationOfVM( + Mockito.eq(stoppedVm), Mockito.anyLong(), Mockito.anyLong(), Mockito.any(), Mockito.anyList()); } @Test @@ -350,9 +359,6 @@ public void testGetDrsPlanWithSkipDrsFlag() throws ConfigurationException { Mockito.when(skippedVm.getHostId()).thenReturn(1L); Mockito.when(skippedVm.getType()).thenReturn(VirtualMachine.Type.User); Mockito.when(skippedVm.getState()).thenReturn(VirtualMachine.State.Running); - Map details = new HashMap<>(); - details.put(VmDetailConstants.SKIP_DRS, "true"); - Mockito.when(skippedVm.getDetails()).thenReturn(details); List hostList = new ArrayList<>(); hostList.add(host1); @@ -370,6 +376,11 @@ public void testGetDrsPlanWithSkipDrsFlag() throws ConfigurationException { Mockito.when(hostJoin1.getMemReservedCapacity()).thenReturn(0L); Mockito.when(hostJoin1.getTotalMemory()).thenReturn(8192L); + // Return the SKIP_DRS detail for skippedVm so the flag is actually honoured + UserVmDetailVO skipDrsDetail = new UserVmDetailVO(1L, VmDetailConstants.SKIP_DRS, "true", true); + Mockito.when(userVmDetailsDao.listDetailsForResourceIdsAndKey(Mockito.anyList(), + Mockito.eq(VmDetailConstants.SKIP_DRS))).thenReturn(List.of(skipDrsDetail)); + Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList); Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn(true); @@ -377,6 +388,9 @@ public void testGetDrsPlanWithSkipDrsFlag() throws ConfigurationException { List> result = clusterDrsService.getDrsPlan(cluster, 5); assertEquals(0, result.size()); + // Verify the VM was skipped before any host-compatibility lookup was attempted + Mockito.verify(managementServer, Mockito.never()).listHostsForMigrationOfVM( + Mockito.eq(skippedVm), Mockito.anyLong(), Mockito.anyLong(), Mockito.any(), Mockito.anyList()); } @Test @@ -393,7 +407,6 @@ public void testGetDrsPlanWithNoCompatibleHosts() throws ConfigurationException Mockito.when(vm1.getHostId()).thenReturn(1L); Mockito.when(vm1.getType()).thenReturn(VirtualMachine.Type.User); Mockito.when(vm1.getState()).thenReturn(VirtualMachine.State.Running); - Mockito.when(vm1.getDetails()).thenReturn(Collections.emptyMap()); List hostList = new ArrayList<>(); hostList.add(host1); @@ -418,6 +431,10 @@ public void testGetDrsPlanWithNoCompatibleHosts() throws ConfigurationException Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn(true); Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(Mockito.anyLong(), Mockito.anyLong())).thenReturn(serviceOffering); Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); + // Return a Ternary with an empty suitable-hosts list to exercise the "no compatible hosts" path + Mockito.when(managementServer.listHostsForMigrationOfVM(Mockito.eq(vm1), Mockito.anyLong(), + Mockito.anyLong(), Mockito.any(), Mockito.anyList())) + .thenReturn(new Ternary<>(new Pair<>(Collections.emptyList(), 0), Collections.emptyList(), Collections.emptyMap())); List> result = clusterDrsService.getDrsPlan(cluster, 5); assertEquals(0, result.size()); @@ -438,7 +455,6 @@ public void testGetDrsPlanWithExceptionInCompatibilityCheck() throws Configurati Mockito.when(vm1.getHostId()).thenReturn(1L); Mockito.when(vm1.getType()).thenReturn(VirtualMachine.Type.User); Mockito.when(vm1.getState()).thenReturn(VirtualMachine.State.Running); - Mockito.when(vm1.getDetails()).thenReturn(Collections.emptyMap()); List hostList = new ArrayList<>(); hostList.add(host1); @@ -463,6 +479,10 @@ public void testGetDrsPlanWithExceptionInCompatibilityCheck() throws Configurati Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn(true); Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(Mockito.anyLong(), Mockito.anyLong())).thenReturn(serviceOffering); Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); + // Throw an explicit exception so the catch-and-log path is exercised intentionally + Mockito.when(managementServer.listHostsForMigrationOfVM(Mockito.eq(vm1), Mockito.anyLong(), + Mockito.anyLong(), Mockito.any(), Mockito.anyList())) + .thenThrow(new RuntimeException("Simulated host compatibility check failure")); List> result = clusterDrsService.getDrsPlan(cluster, 5); assertEquals(0, result.size()); @@ -484,7 +504,6 @@ public void testGetDrsPlanWithNoBestMigration() throws ConfigurationException { Mockito.when(vm1.getHostId()).thenReturn(1L); Mockito.when(vm1.getType()).thenReturn(VirtualMachine.Type.User); Mockito.when(vm1.getState()).thenReturn(VirtualMachine.State.Running); - Mockito.when(vm1.getDetails()).thenReturn(Collections.emptyMap()); List hostList = new ArrayList<>(); hostList.add(host1); @@ -539,14 +558,12 @@ public void testGetDrsPlanWithMultipleIterations() throws ConfigurationException Mockito.when(vm1.getHostId()).thenReturn(1L); Mockito.when(vm1.getType()).thenReturn(VirtualMachine.Type.User); Mockito.when(vm1.getState()).thenReturn(VirtualMachine.State.Running); - Mockito.when(vm1.getDetails()).thenReturn(Collections.emptyMap()); VMInstanceVO vm2 = Mockito.mock(VMInstanceVO.class); Mockito.when(vm2.getId()).thenReturn(2L); Mockito.when(vm2.getHostId()).thenReturn(1L); Mockito.when(vm2.getType()).thenReturn(VirtualMachine.Type.User); Mockito.when(vm2.getState()).thenReturn(VirtualMachine.State.Running); - Mockito.when(vm2.getDetails()).thenReturn(Collections.emptyMap()); List hostList = new ArrayList<>(); hostList.add(host1); @@ -619,7 +636,6 @@ public void testGetDrsPlanWithMigrationToOriginalHost() throws ConfigurationExce Mockito.when(vm1.getHostId()).thenReturn(1L); Mockito.when(vm1.getType()).thenReturn(VirtualMachine.Type.User); Mockito.when(vm1.getState()).thenReturn(VirtualMachine.State.Running); - Mockito.when(vm1.getDetails()).thenReturn(Collections.emptyMap()); List hostList = new ArrayList<>(); hostList.add(host1); @@ -811,15 +827,9 @@ public void testGetBestMigration() throws ConfigurationException { VMInstanceVO vm1 = Mockito.mock(VMInstanceVO.class); Mockito.when(vm1.getId()).thenReturn(1L); - Mockito.when(vm1.getType()).thenReturn(VirtualMachine.Type.User); - Mockito.when(vm1.getState()).thenReturn(VirtualMachine.State.Running); - Mockito.when(vm1.getDetails()).thenReturn(Collections.emptyMap()); VMInstanceVO vm2 = Mockito.mock(VMInstanceVO.class); Mockito.when(vm2.getId()).thenReturn(2L); - Mockito.when(vm2.getType()).thenReturn(VirtualMachine.Type.User); - Mockito.when(vm2.getState()).thenReturn(VirtualMachine.State.Running); - Mockito.when(vm2.getDetails()).thenReturn(Collections.emptyMap()); List vmList = new ArrayList<>(); vmList.add(vm1); @@ -890,15 +900,9 @@ public void testGetBestMigrationDifferentCluster() throws ConfigurationException VMInstanceVO vm1 = Mockito.mock(VMInstanceVO.class); Mockito.when(vm1.getId()).thenReturn(1L); - Mockito.when(vm1.getType()).thenReturn(VirtualMachine.Type.User); - Mockito.when(vm1.getState()).thenReturn(VirtualMachine.State.Running); - Mockito.when(vm1.getDetails()).thenReturn(Collections.emptyMap()); VMInstanceVO vm2 = Mockito.mock(VMInstanceVO.class); Mockito.when(vm2.getId()).thenReturn(2L); - Mockito.when(vm2.getType()).thenReturn(VirtualMachine.Type.User); - Mockito.when(vm2.getState()).thenReturn(VirtualMachine.State.Running); - Mockito.when(vm2.getDetails()).thenReturn(Collections.emptyMap()); List vmList = new ArrayList<>(); vmList.add(vm1);