Fix skip DRS for a VM (#12994)

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

@@ -103,4 +103,6 @@ public interface ResourceDetailsDao<R extends ResourceDetail> extends GenericDao
     long batchExpungeForResources(List<Long> ids, Long batchSize);
 
     String getActualValue(ResourceDetail resourceDetail);
+
+    List<R> listDetailsForResourceIdsAndKey(List<Long> resourceIds, String key);
 }

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;
@@ -47,6 +49,7 @@ public abstract class ResourceDetailsDaoBase<R extends ResourceDetail> extends G
     public ResourceDetailsDaoBase() {
         AllFieldsSearch = createSearchBuilder();
         AllFieldsSearch.and("resourceId", AllFieldsSearch.entity().getResourceId(), SearchCriteria.Op.EQ);
+        AllFieldsSearch.and("resourceIdIn", AllFieldsSearch.entity().getResourceId(), SearchCriteria.Op.IN);
         AllFieldsSearch.and("name", AllFieldsSearch.entity().getName(), SearchCriteria.Op.EQ);
         AllFieldsSearch.and("value", AllFieldsSearch.entity().getValue(), SearchCriteria.Op.EQ);
         // FIXME SnapshotDetailsVO doesn't have a display field
@@ -246,4 +249,15 @@ public abstract class ResourceDetailsDaoBase<R extends ResourceDetail> extends G
         }
         return resourceDetail.getValue();
     }
+
+    @Override
+    public List<R> listDetailsForResourceIdsAndKey(List<Long> resourceIds, String key) {
+        if (CollectionUtils.isEmpty(resourceIds) || StringUtils.isBlank(key)) {
+            return Collections.emptyList();
+        }
+        SearchCriteria<R> sc = AllFieldsSearch.create();
+        sc.setParameters("name", key);
+        sc.setParameters("resourceIdIn", resourceIds.toArray());
+        return search(sc, null);
+    }
 }

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

@@ -51,11 +51,13 @@ import com.cloud.utils.db.GlobalLock;
 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;
@@ -78,7 +80,6 @@ import org.apache.cloudstack.framework.jobs.impl.AsyncJobVO;
 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;
@@ -134,6 +135,9 @@ public class ClusterDrsServiceImpl extends ManagerBase implements ClusterDrsServ
     @Inject
     ServiceOfferingDao serviceOfferingDao;
 
+    @Inject
+    UserVmDetailsDao userVmDetailsDao;
+
     @Inject
     ManagementServer managementServer;
 
@@ -475,12 +479,16 @@ public class ClusterDrsServiceImpl extends ManagerBase implements ClusterDrsServ
         Map<Long, List<? extends Host>> vmToCompatibleHostsCache = new HashMap<>();
         Map<Long, Map<Host, Boolean>> vmToStorageMotionCache = new HashMap<>();
 
+        List<Long> vmIds = vmList.stream().map(VirtualMachine::getId).collect(Collectors.toList());
+        Set<Long> 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 (vm.getType().isUsedBySystem() ||
-                vm.getState() != VirtualMachine.State.Running ||
-                (MapUtils.isNotEmpty(vm.getDetails()) &&
-                 "true".equalsIgnoreCase(vm.getDetails().get(VmDetailConstants.SKIP_DRS)))) {
+            if (shouldSkipVMForDRS(vm, skipDrsVmIds)) {
+                logger.debug("Skipping VM {} for DRS as it is ineligible.", vm);
                 continue;
             }
 
@@ -607,7 +615,7 @@ public class ClusterDrsServiceImpl extends ManagerBase implements ClusterDrsServ
             ExcludeList excludes = vmToExcludesMap.get(vm.getId());
 
             ServiceOffering serviceOffering = vmIdServiceOfferingMap.get(vm.getId());
-            if (skipDrs(vm, compatibleHosts, serviceOffering)) {
+            if (CollectionUtils.isEmpty(compatibleHosts) || serviceOffering == null) {
                 continue;
             }
 
@@ -633,21 +641,11 @@ public class ClusterDrsServiceImpl extends ManagerBase implements ClusterDrsServ
         return bestMigration;
     }
 
-    private boolean skipDrs(VirtualMachine vm, List<? extends Host> compatibleHosts, ServiceOffering serviceOffering) {
+    private boolean shouldSkipVMForDRS(VirtualMachine vm, Set<Long> skipDrsVmIds) {
         if (vm.getType().isUsedBySystem() || vm.getState() != VirtualMachine.State.Running) {
             return true;
         }
-        if (MapUtils.isNotEmpty(vm.getDetails()) &&
-            "true".equalsIgnoreCase(vm.getDetails().get(VmDetailConstants.SKIP_DRS))) {
-            return true;
-        }
-        if (CollectionUtils.isEmpty(compatibleHosts)) {
-            return true;
-        }
-        if (serviceOffering == null) {
-            return true;
-        }
-        return false;
+        return skipDrsVmIds.contains(vm.getId());
     }
 
     private Pair<double[], Map<Long, Integer>> getBaseMetricsArrayAndHostIdIndexMap(

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

@@ -40,9 +40,11 @@ import com.cloud.utils.Pair;
 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 class ClusterDrsServiceImplTest {
 
         List<Ternary<VirtualMachine, Host, Host>> 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 class ClusterDrsServiceImplTest {
 
         List<Ternary<VirtualMachine, Host, Host>> 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 class ClusterDrsServiceImplTest {
         Mockito.when(skippedVm.getHostId()).thenReturn(1L);
         Mockito.when(skippedVm.getType()).thenReturn(VirtualMachine.Type.User);
         Mockito.when(skippedVm.getState()).thenReturn(VirtualMachine.State.Running);
-        Map<String, String> details = new HashMap<>();
-        details.put(VmDetailConstants.SKIP_DRS, "true");
-        Mockito.when(skippedVm.getDetails()).thenReturn(details);
 
         List<HostVO> hostList = new ArrayList<>();
         hostList.add(host1);
@@ -370,6 +376,11 @@ public class ClusterDrsServiceImplTest {
         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 class ClusterDrsServiceImplTest {
 
         List<Ternary<VirtualMachine, Host, Host>> 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 class ClusterDrsServiceImplTest {
         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<HostVO> hostList = new ArrayList<>();
         hostList.add(host1);
@@ -418,6 +431,10 @@ public class ClusterDrsServiceImplTest {
         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<Ternary<VirtualMachine, Host, Host>> result = clusterDrsService.getDrsPlan(cluster, 5);
         assertEquals(0, result.size());
@@ -438,7 +455,6 @@ public class ClusterDrsServiceImplTest {
         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<HostVO> hostList = new ArrayList<>();
         hostList.add(host1);
@@ -463,6 +479,10 @@ public class ClusterDrsServiceImplTest {
         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<Ternary<VirtualMachine, Host, Host>> result = clusterDrsService.getDrsPlan(cluster, 5);
         assertEquals(0, result.size());
@@ -484,7 +504,6 @@ public class ClusterDrsServiceImplTest {
         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<HostVO> hostList = new ArrayList<>();
         hostList.add(host1);
@@ -539,14 +558,12 @@ public class ClusterDrsServiceImplTest {
         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<HostVO> hostList = new ArrayList<>();
         hostList.add(host1);
@@ -619,7 +636,6 @@ public class ClusterDrsServiceImplTest {
         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<HostVO> hostList = new ArrayList<>();
         hostList.add(host1);
@@ -811,15 +827,9 @@ public class ClusterDrsServiceImplTest {
 
         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<VirtualMachine> vmList = new ArrayList<>();
         vmList.add(vm1);
@@ -890,15 +900,9 @@ public class ClusterDrsServiceImplTest {
 
         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<VirtualMachine> vmList = new ArrayList<>();
         vmList.add(vm1);