This is an automated email from the ASF dual-hosted git repository.
weizhouapache pushed a commit to branch 4.20
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/4.20 by this push:
new a4b102fb3f9 Fix skip DRS for a VM (#12994)
a4b102fb3f9 is described below
commit a4b102fb3f99cfe45e3aab813d43e161efe59866
Author: Vishesh <[email protected]>
AuthorDate: Thu Jul 9 18:25:54 2026 +0530
Fix skip DRS for a VM (#12994)
---
.../resourcedetail/ResourceDetailsDao.java | 2 +
.../resourcedetail/ResourceDetailsDaoBase.java | 14 +++++++
.../cloudstack/cluster/ClusterDrsServiceImpl.java | 34 ++++++++--------
.../cluster/ClusterDrsServiceImplTest.java | 46 ++++++++++++----------
4 files changed, 57 insertions(+), 39 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 6b6fe200c10..e3241731f71 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<R extends
ResourceDetail> extends GenericDao
long batchExpungeForResources(List<Long> ids, Long batchSize);
String getActualValue(ResourceDetail resourceDetail);
+
+ List<R> listDetailsForResourceIdsAndKey(List<Long> 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 29d3f88fd90..74c4f7bcfe0 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;
@@ -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);
+ }
}
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 7e1011ff931..fbfed03aa1e 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.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(
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 b76ef51c523..6dc73b7a630 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.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);
