From 1faba7dc6e2555c3de952373c6c20e09fa22a34f Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 9 Sep 2026 21:18:15 +0000 Subject: [PATCH 1/7] Fix unreachable and NPE-prone branch in host anti-affinity The reserved-capacity branch was chained to the wrong condition, so it only ran when the group VM was null or removed: - a Stopped VM still holding reserved capacity never had its last host avoided - the branch was dead for every live VM - a group mapping pointing at a deleted VM hit the branch with a null and threw NullPointerException Restructured to match NonStrictHostAffinityProcessor, which already had the intended shape: skip null/removed, then avoid the current host, else the last host while capacity is still reserved. Signed-off-by: Brad House --- .../affinity/HostAntiAffinityProcessor.java | 34 ++-- .../HostAntiAffinityProcessorTest.java | 167 ++++++++++++++++++ 2 files changed, 185 insertions(+), 16 deletions(-) create mode 100644 plugins/affinity-group-processors/host-anti-affinity/src/test/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessorTest.java diff --git a/plugins/affinity-group-processors/host-anti-affinity/src/main/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessor.java b/plugins/affinity-group-processors/host-anti-affinity/src/main/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessor.java index bd29a48f2588..da4994129115 100644 --- a/plugins/affinity-group-processors/host-anti-affinity/src/main/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessor.java +++ b/plugins/affinity-group-processors/host-anti-affinity/src/main/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessor.java @@ -59,7 +59,7 @@ public class HostAntiAffinityProcessor extends AffinityProcessorBase implements protected AffinityGroupDao _affinityGroupDao; @Inject protected AffinityGroupVMMapDao _affinityGroupVMMapDao; - private int _vmCapacityReleaseInterval; + protected int _vmCapacityReleaseInterval; @Inject protected ConfigurationDao _configDao; @@ -102,22 +102,24 @@ protected void processAffinityGroup(AffinityGroupVMMapVO vmGroupMapping, Exclude for (Long groupVMId : groupVMIds) { VMInstanceVO groupVM = _vmInstanceDao.findById(groupVMId); - if (groupVM != null && !groupVM.isRemoved()) { - if (groupVM.getHostId() != null) { - avoid.addHost(groupVM.getHostId()); - if (logger.isDebugEnabled()) { - logger.debug("Added host {} to avoid set, since VM {} is present on the host", groupVM.getHostId(), groupVM); - } - } - } else if (Arrays.asList(VirtualMachine.State.Starting, VirtualMachine.State.Stopped).contains(groupVM.getState()) && groupVM.getLastHostId() != null) { - long secondsSinceLastUpdate = (DateUtil.currentGMTTime().getTime() - groupVM.getUpdateTime().getTime()) / 1000; - if (secondsSinceLastUpdate < _vmCapacityReleaseInterval) { - avoid.addHost(groupVM.getLastHostId()); - if (logger.isDebugEnabled()) { - logger.debug("Added host {} to avoid set, since VM {} is present on the host, in Stopped state but has reserved capacity", groupVM.getLastHostId(), groupVM); - } - } + if (groupVM == null || groupVM.isRemoved()) { + continue; } + avoidHostOfVmInAffinityGroup(avoid, groupVM); + } + } + } + + protected void avoidHostOfVmInAffinityGroup(ExcludeList avoid, VMInstanceVO groupVM) { + if (groupVM.getHostId() != null) { + avoid.addHost(groupVM.getHostId()); + logger.debug("Added host {} to avoid set, since VM {} is present on the host", groupVM.getHostId(), groupVM); + } else if (Arrays.asList(VirtualMachine.State.Starting, VirtualMachine.State.Stopped).contains(groupVM.getState()) && groupVM.getLastHostId() != null) { + long secondsSinceLastUpdate = (DateUtil.currentGMTTime().getTime() - groupVM.getUpdateTime().getTime()) / 1000; + if (secondsSinceLastUpdate < _vmCapacityReleaseInterval) { + avoid.addHost(groupVM.getLastHostId()); + logger.debug("Added host {} to avoid set, since VM {} is in {} state on the host but still has reserved capacity", + groupVM.getLastHostId(), groupVM, groupVM.getState()); } } } diff --git a/plugins/affinity-group-processors/host-anti-affinity/src/test/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessorTest.java b/plugins/affinity-group-processors/host-anti-affinity/src/test/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessorTest.java new file mode 100644 index 000000000000..e806e98a255d --- /dev/null +++ b/plugins/affinity-group-processors/host-anti-affinity/src/test/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessorTest.java @@ -0,0 +1,167 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +package org.apache.cloudstack.affinity; + +import com.cloud.deploy.DeploymentPlanner.ExcludeList; +import com.cloud.utils.DateUtil; +import com.cloud.vm.VMInstanceVO; +import com.cloud.vm.VirtualMachine; +import com.cloud.vm.dao.VMInstanceDao; +import org.apache.cloudstack.affinity.dao.AffinityGroupDao; +import org.apache.cloudstack.affinity.dao.AffinityGroupVMMapDao; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; +import org.mockito.Spy; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Date; +import java.util.List; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.when; + +@RunWith(JUnit4.class) +public class HostAntiAffinityProcessorTest { + + private static final long AFFINITY_GROUP_ID = 2L; + private static final long VM_ID = 3L; + private static final long GROUP_VM_ID = 1L; + private static final long HOST_ID = 10L; + private static final long LAST_HOST_ID = 11L; + private static final int CAPACITY_RELEASE_INTERVAL = 3600; + + @Mock + AffinityGroupDao _affinityGroupDao; + + @Mock + AffinityGroupVMMapDao _affinityGroupVMMapDao; + + @Mock + VMInstanceDao _vmInstanceDao; + + @Spy + @InjectMocks + HostAntiAffinityProcessor processor = new HostAntiAffinityProcessor(); + + @Mock + VirtualMachine vm; + + @Mock + VMInstanceVO groupVM; + + @Mock + AffinityGroupVO affinityGroupVO; + + @Mock + AffinityGroupVMMapVO vmGroupMapping; + + private ExcludeList avoid; + + @Before + public void setUp() { + MockitoAnnotations.openMocks(this); + processor._vmCapacityReleaseInterval = CAPACITY_RELEASE_INTERVAL; + avoid = new ExcludeList(); + + when(vm.getId()).thenReturn(VM_ID); + when(vmGroupMapping.getAffinityGroupId()).thenReturn(AFFINITY_GROUP_ID); + when(_affinityGroupDao.findById(AFFINITY_GROUP_ID)).thenReturn(affinityGroupVO); + when(affinityGroupVO.getId()).thenReturn(AFFINITY_GROUP_ID); + when(_affinityGroupVMMapDao.listVmIdsByAffinityGroup(AFFINITY_GROUP_ID)) + .thenReturn(new ArrayList<>(Arrays.asList(GROUP_VM_ID, VM_ID))); + } + + private boolean avoids(long hostId) { + return avoid.getHostsToAvoid() != null && avoid.getHostsToAvoid().contains(hostId); + } + + @Test + public void testRunningGroupVmHostIsAvoided() { + when(_vmInstanceDao.findById(GROUP_VM_ID)).thenReturn(groupVM); + when(groupVM.isRemoved()).thenReturn(false); + when(groupVM.getHostId()).thenReturn(HOST_ID); + + processor.processAffinityGroup(vmGroupMapping, avoid, vm); + + assertTrue(avoids(HOST_ID)); + } + + @Test + public void testStoppedGroupVmWithReservedCapacityLastHostIsAvoided() { + when(_vmInstanceDao.findById(GROUP_VM_ID)).thenReturn(groupVM); + when(groupVM.isRemoved()).thenReturn(false); + when(groupVM.getHostId()).thenReturn(null); + when(groupVM.getState()).thenReturn(VirtualMachine.State.Stopped); + when(groupVM.getLastHostId()).thenReturn(LAST_HOST_ID); + when(groupVM.getUpdateTime()).thenReturn(DateUtil.currentGMTTime()); + + processor.processAffinityGroup(vmGroupMapping, avoid, vm); + + assertTrue(avoids(LAST_HOST_ID)); + } + + @Test + public void testStoppedGroupVmPastReleaseIntervalIsNotAvoided() { + Date wellPast = new Date(DateUtil.currentGMTTime().getTime() - (CAPACITY_RELEASE_INTERVAL + 60) * 1000L); + when(_vmInstanceDao.findById(GROUP_VM_ID)).thenReturn(groupVM); + when(groupVM.isRemoved()).thenReturn(false); + when(groupVM.getHostId()).thenReturn(null); + when(groupVM.getState()).thenReturn(VirtualMachine.State.Stopped); + when(groupVM.getLastHostId()).thenReturn(LAST_HOST_ID); + when(groupVM.getUpdateTime()).thenReturn(wellPast); + + processor.processAffinityGroup(vmGroupMapping, avoid, vm); + + assertFalse(avoids(LAST_HOST_ID)); + } + + @Test + public void testMissingGroupVmIsSkipped() { + when(_vmInstanceDao.findById(GROUP_VM_ID)).thenReturn(null); + + processor.processAffinityGroup(vmGroupMapping, avoid, vm); + + assertTrue(avoid.getHostsToAvoid() == null || avoid.getHostsToAvoid().isEmpty()); + } + + @Test + public void testRemovedGroupVmIsSkipped() { + when(_vmInstanceDao.findById(GROUP_VM_ID)).thenReturn(groupVM); + when(groupVM.isRemoved()).thenReturn(true); + + processor.processAffinityGroup(vmGroupMapping, avoid, vm); + + assertFalse(avoids(HOST_ID)); + } + + @Test + public void testVmIsNotAvoidedAgainstItself() { + List ids = new ArrayList<>(Arrays.asList(VM_ID)); + when(_affinityGroupVMMapDao.listVmIdsByAffinityGroup(AFFINITY_GROUP_ID)).thenReturn(ids); + + processor.processAffinityGroup(vmGroupMapping, avoid, vm); + + assertTrue(avoid.getHostsToAvoid() == null || avoid.getHostsToAvoid().isEmpty()); + } +} From 494ead4f7bc9f879b81c029f763434bba052b016 Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 9 Sep 2026 21:19:07 +0000 Subject: [PATCH 2/7] Honor planned placements in host anti-affinity The processor already accepted a vmList of placements and ignored it, reading every group member's host from the database instead. DRS builds a multi-migration plan in memory and persists it only at the end, so while the plan is being built the database still shows the old host for every VM the plan has already moved. Anti-affinity was therefore evaluated against stale placements, and a plan could put two anti-affine VMs on the same host. - resolve each group member from vmList first, fall back to the database - mirrors what HostAffinityProcessor already does with the same argument - no change when vmList is empty, which is every non-DRS caller Signed-off-by: Brad House --- .../affinity/HostAntiAffinityProcessor.java | 33 +++++++++++++- .../HostAntiAffinityProcessorTest.java | 45 +++++++++++++++++++ 2 files changed, 77 insertions(+), 1 deletion(-) diff --git a/plugins/affinity-group-processors/host-anti-affinity/src/main/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessor.java b/plugins/affinity-group-processors/host-anti-affinity/src/main/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessor.java index da4994129115..9c69fbd7053c 100644 --- a/plugins/affinity-group-processors/host-anti-affinity/src/main/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessor.java +++ b/plugins/affinity-group-processors/host-anti-affinity/src/main/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessor.java @@ -17,6 +17,8 @@ package org.apache.cloudstack.affinity; import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.stream.Collectors; @@ -82,7 +84,7 @@ public void doInTransactionWithoutResult(TransactionStatus status) { _affinityGroupDao.listByIds(affinityGroupIds, true); } for (AffinityGroupVMMapVO vmGroupMapping : vmGroupMappings) { - processAffinityGroup(vmGroupMapping, avoid, vm); + processAffinityGroup(vmGroupMapping, avoid, vm, vmList); } } }); @@ -90,6 +92,18 @@ public void doInTransactionWithoutResult(TransactionStatus status) { } protected void processAffinityGroup(AffinityGroupVMMapVO vmGroupMapping, ExcludeList avoid, VirtualMachine vm) { + processAffinityGroup(vmGroupMapping, avoid, vm, Collections.emptyList()); + } + + /** + * Applies anti-affinity for one group. + * + * @param vmList + * placements to honour in preference to what the database says. DRS builds a plan of + * several migrations in memory and only persists it later, so during plan generation + * the database still shows the old host for every VM the plan has already moved. + */ + protected void processAffinityGroup(AffinityGroupVMMapVO vmGroupMapping, ExcludeList avoid, VirtualMachine vm, List vmList) { if (vmGroupMapping != null) { AffinityGroupVO group = _affinityGroupDao.findById(vmGroupMapping.getAffinityGroupId()); @@ -100,7 +114,16 @@ protected void processAffinityGroup(AffinityGroupVMMapVO vmGroupMapping, Exclude List groupVMIds = _affinityGroupVMMapDao.listVmIdsByAffinityGroup(group.getId()); groupVMIds.remove(vm.getId()); + Map plannedVms = getVmIdVmMap(vmList); + for (Long groupVMId : groupVMIds) { + VirtualMachine plannedVm = plannedVms.get(groupVMId); + if (plannedVm != null && plannedVm.getHostId() != null) { + avoid.addHost(plannedVm.getHostId()); + logger.debug("Added host {} to avoid set, since VM {} is placed on the host by the plan being built", + plannedVm.getHostId(), plannedVm); + continue; + } VMInstanceVO groupVM = _vmInstanceDao.findById(groupVMId); if (groupVM == null || groupVM.isRemoved()) { continue; @@ -110,6 +133,14 @@ protected void processAffinityGroup(AffinityGroupVMMapVO vmGroupMapping, Exclude } } + protected Map getVmIdVmMap(List vmList) { + Map vmIdVmMap = new HashMap<>(); + for (VirtualMachine vm : vmList) { + vmIdVmMap.put(vm.getId(), vm); + } + return vmIdVmMap; + } + protected void avoidHostOfVmInAffinityGroup(ExcludeList avoid, VMInstanceVO groupVM) { if (groupVM.getHostId() != null) { avoid.addHost(groupVM.getHostId()); diff --git a/plugins/affinity-group-processors/host-anti-affinity/src/test/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessorTest.java b/plugins/affinity-group-processors/host-anti-affinity/src/test/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessorTest.java index e806e98a255d..ae3297039289 100644 --- a/plugins/affinity-group-processors/host-anti-affinity/src/test/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessorTest.java +++ b/plugins/affinity-group-processors/host-anti-affinity/src/test/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessorTest.java @@ -34,6 +34,7 @@ import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.Date; import java.util.List; @@ -49,6 +50,7 @@ public class HostAntiAffinityProcessorTest { private static final long GROUP_VM_ID = 1L; private static final long HOST_ID = 10L; private static final long LAST_HOST_ID = 11L; + private static final long PLANNED_HOST_ID = 12L; private static final int CAPACITY_RELEASE_INTERVAL = 3600; @Mock @@ -155,6 +157,49 @@ public void testRemovedGroupVmIsSkipped() { assertFalse(avoids(HOST_ID)); } + @Test + public void testPlannedHostWinsOverStaleDatabaseHost() { + VMInstanceVO plannedVm = org.mockito.Mockito.mock(VMInstanceVO.class); + when(plannedVm.getId()).thenReturn(GROUP_VM_ID); + when(plannedVm.getHostId()).thenReturn(PLANNED_HOST_ID); + + // the database still shows the pre-migration host + when(_vmInstanceDao.findById(GROUP_VM_ID)).thenReturn(groupVM); + when(groupVM.isRemoved()).thenReturn(false); + when(groupVM.getHostId()).thenReturn(HOST_ID); + + processor.processAffinityGroup(vmGroupMapping, avoid, vm, Arrays.asList(plannedVm)); + + assertTrue(avoids(PLANNED_HOST_ID)); + assertFalse(avoids(HOST_ID)); + } + + @Test + public void testPlannedVmWithoutHostFallsBackToDatabase() { + VMInstanceVO plannedVm = org.mockito.Mockito.mock(VMInstanceVO.class); + when(plannedVm.getId()).thenReturn(GROUP_VM_ID); + when(plannedVm.getHostId()).thenReturn(null); + + when(_vmInstanceDao.findById(GROUP_VM_ID)).thenReturn(groupVM); + when(groupVM.isRemoved()).thenReturn(false); + when(groupVM.getHostId()).thenReturn(HOST_ID); + + processor.processAffinityGroup(vmGroupMapping, avoid, vm, Arrays.asList(plannedVm)); + + assertTrue(avoids(HOST_ID)); + } + + @Test + public void testEmptyVmListBehavesAsBefore() { + when(_vmInstanceDao.findById(GROUP_VM_ID)).thenReturn(groupVM); + when(groupVM.isRemoved()).thenReturn(false); + when(groupVM.getHostId()).thenReturn(HOST_ID); + + processor.processAffinityGroup(vmGroupMapping, avoid, vm, Collections.emptyList()); + + assertTrue(avoids(HOST_ID)); + } + @Test public void testVmIsNotAvoidedAgainstItself() { List ids = new ArrayList<>(Arrays.asList(VM_ID)); From 65d4fce6761cc9e6747a9ed3e06d85ac90fb12fa Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 9 Sep 2026 21:20:32 +0000 Subject: [PATCH 3/7] Honor planned placements in non-strict host affinity Same gap as the strict processor: vmList was accepted and ignored, so host priorities were adjusted from database placements even when the caller supplied newer ones. NonStrictHostAntiAffinityProcessor extends this class and only overrides the priority direction, so it is fixed by the same change. Signed-off-by: Brad House --- .../NonStrictHostAffinityProcessor.java | 32 +++++++++++++++++- .../NonStrictHostAffinityProcessorTest.java | 33 +++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/plugins/affinity-group-processors/non-strict-host-affinity/src/main/java/org/apache/cloudstack/affinity/NonStrictHostAffinityProcessor.java b/plugins/affinity-group-processors/non-strict-host-affinity/src/main/java/org/apache/cloudstack/affinity/NonStrictHostAffinityProcessor.java index 49e3f60ed5d0..8cb92ce6fa18 100644 --- a/plugins/affinity-group-processors/non-strict-host-affinity/src/main/java/org/apache/cloudstack/affinity/NonStrictHostAffinityProcessor.java +++ b/plugins/affinity-group-processors/non-strict-host-affinity/src/main/java/org/apache/cloudstack/affinity/NonStrictHostAffinityProcessor.java @@ -17,6 +17,8 @@ package org.apache.cloudstack.affinity; import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; @@ -67,13 +69,24 @@ public void process(VirtualMachineProfile vmProfile, DeploymentPlan plan, Exclud for (AffinityGroupVMMapVO vmGroupMapping : vmGroupMappings) { if (vmGroupMapping != null) { - processAffinityGroup(vmGroupMapping, plan, vm); + processAffinityGroup(vmGroupMapping, plan, vm, vmList); } } } protected void processAffinityGroup(AffinityGroupVMMapVO vmGroupMapping, DeploymentPlan plan, VirtualMachine vm) { + processAffinityGroup(vmGroupMapping, plan, vm, Collections.emptyList()); + } + + /** + * Adjusts host priorities for one group. + * + * @param vmList + * placements to honour in preference to what the database says, for callers such as DRS + * that build a plan in memory before persisting it. + */ + protected void processAffinityGroup(AffinityGroupVMMapVO vmGroupMapping, DeploymentPlan plan, VirtualMachine vm, List vmList) { AffinityGroupVO group = affinityGroupDao.findById(vmGroupMapping.getAffinityGroupId()); if (logger.isDebugEnabled()) { @@ -83,7 +96,16 @@ protected void processAffinityGroup(AffinityGroupVMMapVO vmGroupMapping, Deploym List groupVMIds = affinityGroupVMMapDao.listVmIdsByAffinityGroup(group.getId()); groupVMIds.remove(vm.getId()); + Map plannedVms = getVmIdVmMap(vmList); + for (Long groupVMId : groupVMIds) { + VirtualMachine plannedVm = plannedVms.get(groupVMId); + if (plannedVm != null && plannedVm.getHostId() != null) { + Integer priority = adjustHostPriority(plan, plannedVm.getHostId()); + logger.debug("Updated host {} priority to {}, since VM {} is placed on the host by the plan being built", + plannedVm.getHostId(), priority, plannedVm); + continue; + } VMInstanceVO groupVM = vmInstanceDao.findById(groupVMId); if (groupVM != null && !groupVM.isRemoved()) { processVmInAffinityGroup(plan, groupVM); @@ -91,6 +113,14 @@ protected void processAffinityGroup(AffinityGroupVMMapVO vmGroupMapping, Deploym } } + protected Map getVmIdVmMap(List vmList) { + Map vmIdVmMap = new HashMap<>(); + for (VirtualMachine vm : vmList) { + vmIdVmMap.put(vm.getId(), vm); + } + return vmIdVmMap; + } + protected void processVmInAffinityGroup(DeploymentPlan plan, VMInstanceVO groupVM) { if (groupVM.getHostId() != null) { Integer priority = adjustHostPriority(plan, groupVM.getHostId()); diff --git a/plugins/affinity-group-processors/non-strict-host-affinity/src/test/java/org/apache/cloudstack/affinity/NonStrictHostAffinityProcessorTest.java b/plugins/affinity-group-processors/non-strict-host-affinity/src/test/java/org/apache/cloudstack/affinity/NonStrictHostAffinityProcessorTest.java index cb91870cbd39..6e14b26cb690 100644 --- a/plugins/affinity-group-processors/non-strict-host-affinity/src/test/java/org/apache/cloudstack/affinity/NonStrictHostAffinityProcessorTest.java +++ b/plugins/affinity-group-processors/non-strict-host-affinity/src/test/java/org/apache/cloudstack/affinity/NonStrictHostAffinityProcessorTest.java @@ -170,4 +170,37 @@ public void testProcessWithNotRunningVM() { Assert.assertNotNull(plan.getHostPriorities().get(host2Id)); Assert.assertEquals(Integer.valueOf(1), plan.getHostPriorities().get(host2Id)); } + + @Test + public void testPlannedHostWinsOverStaleDatabaseHost() { + VirtualMachine vm = Mockito.mock(VirtualMachine.class); + when(vm.getId()).thenReturn(vmId); + VirtualMachineProfile vmProfile = Mockito.mock(VirtualMachineProfile.class); + when(vmProfile.getVirtualMachine()).thenReturn(vm); + + List vmGroupMappings = new ArrayList<>(); + vmGroupMappings.add(new AffinityGroupVMMapVO(affinityGroupId, vmId)); + when(_affinityGroupVMMapDao.findByVmIdType(eq(vmId), nullable(String.class))).thenReturn(vmGroupMappings); + + DataCenterDeployment plan = new DataCenterDeployment(zoneId); + ExcludeList avoid = new ExcludeList(); + + AffinityGroupVO affinityGroupVO = Mockito.mock(AffinityGroupVO.class); + when(affinityGroupDao.findById(affinityGroupId)).thenReturn(affinityGroupVO); + when(affinityGroupVO.getId()).thenReturn(affinityGroupId); + when(_affinityGroupVMMapDao.listVmIdsByAffinityGroup(affinityGroupId)) + .thenReturn(new ArrayList<>(Arrays.asList(vmId, vm2Id))); + + // the plan being built has already moved vm2 to host3; the database is not consulted + VMInstanceVO planned = Mockito.mock(VMInstanceVO.class); + when(planned.getId()).thenReturn(vm2Id); + when(planned.getHostId()).thenReturn(host3Id); + + processor.process(vmProfile, plan, avoid, Arrays.asList(planned)); + + Assert.assertEquals(1, plan.getHostPriorities().size()); + Assert.assertNotNull(plan.getHostPriorities().get(host3Id)); + Assert.assertNull(plan.getHostPriorities().get(host2Id)); + Mockito.verify(vmInstanceDao, Mockito.never()).findById(vm2Id); + } } From 3459b700204dc222fd753fe9802c462735531f95 Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 9 Sep 2026 21:23:18 +0000 Subject: [PATCH 4/7] Re-check affinity before executing each DRS migration A DRS plan is generated once and executed later, and nothing downstream re-checks it - migrateVirtualMachine does not enforce affinity groups. By execution time the cluster may have changed, so a plan that was valid when generated can violate anti-affinity when it runs. - validate each migration against current state before queueing it - skip and mark failed instead of migrating into a violation - track destinations already queued in this run, since the jobs are asynchronous and the database does not reflect them yet Signed-off-by: Brad House --- .../cluster/ClusterDrsServiceImpl.java | 40 ++++++++- .../cluster/ClusterDrsServiceImplTest.java | 86 +++++++++++++++++++ 2 files changed, 125 insertions(+), 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 62075aae596e..5f91666b115b 100644 --- a/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java +++ b/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java @@ -768,6 +768,29 @@ void processPlans() { * @param plan * the DRS plan to be executed */ + /** + * Checks a planned migration against the affinity rules as they stand now. + * + * A plan is generated once and executed later, so state can have moved on: VMs may have been + * created, migrated or destroyed in between. Anti-affinity in particular is only meaningful + * against current placements, and nothing downstream re-checks it - migrateVirtualMachine does + * not enforce affinity groups. + * + * @param dispatched + * migrations already queued by this run, which the database does not reflect yet + */ + protected boolean destinationViolatesAffinity(VirtualMachine vm, Host destHost, List dispatched) { + if (CollectionUtils.isEmpty(affinityGroupVMMapDao.listByInstanceId(vm.getId()))) { + return false; + } + DataCenterDeployment plan = new DataCenterDeployment(destHost.getDataCenterId(), destHost.getPodId(), + destHost.getClusterId(), null, null, null); + VirtualMachineProfile vmProfile = new VirtualMachineProfileImpl(vm, null, + serviceOfferingDao.findByIdIncludingRemoved(vm.getId(), vm.getServiceOfferingId()), null, null); + ExcludeList excludes = managementServer.applyAffinityConstraints(vm, vmProfile, plan, dispatched); + return excludes.shouldAvoid(destHost); + } + void executeDrsPlan(ClusterDrsPlanVO plan) { List planMigrations = drsPlanMigrationDao.listPlanMigrationsToExecute(plan.getId()); if (planMigrations == null || planMigrations.isEmpty()) { @@ -783,21 +806,36 @@ void executeDrsPlan(ClusterDrsPlanVO plan) { plan.setStatus(ClusterDrsPlan.Status.IN_PROGRESS); drsPlanDao.update(plan.getId(), plan); + List dispatched = new ArrayList<>(); + for (ClusterDrsPlanMigrationVO migration : planMigrations) { try { - VirtualMachine vm = vmInstanceDao.findById(migration.getVmId()); + VMInstanceVO vm = vmInstanceDao.findById(migration.getVmId()); Host host = hostDao.findById(migration.getDestHostId()); if (vm == null || host == null) { throw new CloudRuntimeException(String.format("vm %s or host %s is not found", migration.getVmId(), migration.getDestHostId())); } + if (destinationViolatesAffinity(vm, host, dispatched)) { + logger.warn("Skipping DRS migration of vm {} to host {}: it no longer satisfies the affinity " + + "rules for that VM. The plan was generated against older state.", vm, host); + migration.setStatus(JobInfo.Status.FAILED); + drsPlanMigrationDao.update(migration.getId(), migration); + continue; + } + logger.debug("Executing DRS plan {} for vm {} to host {}", plan, vm, host); long jobId = createMigrateVMAsyncJob(vm, host, plan.getEventId()); AsyncJobVO job = asyncJobManager.getAsyncJob(jobId); migration.setJobId(jobId); migration.setStatus(job.getStatus()); drsPlanMigrationDao.update(migration.getId(), migration); + + // the migration job has only been queued, so the database still shows the old host. + // record where it is headed so later migrations in this plan see it. + vm.setHostId(host.getId()); + dispatched.add(vm); } catch (Exception e) { logger.warn("Unable to execute DRS plan {} due to {}", plan, e.getMessage()); migration.setStatus(JobInfo.Status.FAILED); 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 6390b29097b5..a6b461724c45 100644 --- a/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java @@ -29,12 +29,14 @@ import com.cloud.exception.InvalidParameterValueException; import com.cloud.host.Host; import com.cloud.host.HostVO; +import com.cloud.deploy.DeploymentPlanner.ExcludeList; import com.cloud.host.dao.HostDao; import com.cloud.offering.ServiceOffering; import com.cloud.org.Cluster; import com.cloud.org.Grouping; import com.cloud.server.ManagementServer; import com.cloud.service.ServiceOfferingVO; +import org.apache.cloudstack.affinity.AffinityGroupVMMapVO; import com.cloud.service.dao.ServiceOfferingDao; import com.cloud.utils.Pair; import com.cloud.utils.Ternary; @@ -70,6 +72,7 @@ import java.lang.reflect.Field; import java.util.ArrayList; import java.util.Collections; +import org.apache.cloudstack.jobs.JobInfo; import java.util.Date; import java.util.HashMap; import java.util.List; @@ -77,6 +80,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertNull; @RunWith(MockitoJUnitRunner.class) @@ -951,4 +955,86 @@ public void testProcessPlans() { Mockito.verify(clusterDrsService, Mockito.times(2)).executeDrsPlan(Mockito.any(ClusterDrsPlanVO.class)); } + + @Test + public void testDestinationViolatesAffinityWhenVmHasNoAffinityGroups() { + VMInstanceVO vm = Mockito.mock(VMInstanceVO.class); + Mockito.when(vm.getId()).thenReturn(1L); + Mockito.when(affinityGroupVMMapDao.listByInstanceId(1L)).thenReturn(Collections.emptyList()); + + HostVO destHost = Mockito.mock(HostVO.class); + + assertFalse(clusterDrsService.destinationViolatesAffinity(vm, destHost, Collections.emptyList())); + Mockito.verify(managementServer, Mockito.never()) + .applyAffinityConstraints(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()); + } + + @Test + public void testDestinationViolatesAffinityWhenDestHostIsExcluded() { + VMInstanceVO vm = Mockito.mock(VMInstanceVO.class); + Mockito.when(vm.getId()).thenReturn(1L); + Mockito.when(vm.getServiceOfferingId()).thenReturn(5L); + Mockito.when(affinityGroupVMMapDao.listByInstanceId(1L)) + .thenReturn(Collections.singletonList(Mockito.mock(AffinityGroupVMMapVO.class))); + Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(1L, 5L)) + .thenReturn(Mockito.mock(ServiceOfferingVO.class)); + + HostVO destHost = Mockito.mock(HostVO.class); + Mockito.when(destHost.getId()).thenReturn(20L); + + ExcludeList excludes = new ExcludeList(); + excludes.addHost(20L); + Mockito.when(managementServer.applyAffinityConstraints(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any())) + .thenReturn(excludes); + + assertTrue(clusterDrsService.destinationViolatesAffinity(vm, destHost, Collections.emptyList())); + } + + @Test + public void testDestinationViolatesAffinityWhenDestHostIsAllowed() { + VMInstanceVO vm = Mockito.mock(VMInstanceVO.class); + Mockito.when(vm.getId()).thenReturn(1L); + Mockito.when(vm.getServiceOfferingId()).thenReturn(5L); + Mockito.when(affinityGroupVMMapDao.listByInstanceId(1L)) + .thenReturn(Collections.singletonList(Mockito.mock(AffinityGroupVMMapVO.class))); + Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(1L, 5L)) + .thenReturn(Mockito.mock(ServiceOfferingVO.class)); + + HostVO destHost = Mockito.mock(HostVO.class); + Mockito.when(destHost.getId()).thenReturn(20L); + + ExcludeList excludes = new ExcludeList(); + excludes.addHost(21L); + Mockito.when(managementServer.applyAffinityConstraints(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any())) + .thenReturn(excludes); + + assertFalse(clusterDrsService.destinationViolatesAffinity(vm, destHost, Collections.emptyList())); + } + + @Test + public void testExecuteDrsPlanSkipsMigrationThatViolatesAffinity() { + ClusterDrsPlanVO plan = Mockito.mock(ClusterDrsPlanVO.class); + Mockito.when(plan.getId()).thenReturn(1L); + + ClusterDrsPlanMigrationVO migration = Mockito.mock(ClusterDrsPlanMigrationVO.class); + Mockito.when(migration.getId()).thenReturn(7L); + Mockito.when(migration.getVmId()).thenReturn(1L); + Mockito.when(migration.getDestHostId()).thenReturn(20L); + Mockito.when(drsPlanMigrationDao.listPlanMigrationsToExecute(1L)) + .thenReturn(Collections.singletonList(migration)); + + VMInstanceVO vm = Mockito.mock(VMInstanceVO.class); + HostVO destHost = Mockito.mock(HostVO.class); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(hostDao.findById(20L)).thenReturn(destHost); + + Mockito.doReturn(true).when(clusterDrsService) + .destinationViolatesAffinity(Mockito.any(), Mockito.any(), Mockito.any()); + + clusterDrsService.executeDrsPlan(plan); + + Mockito.verify(migration).setStatus(JobInfo.Status.FAILED); + Mockito.verify(clusterDrsService, Mockito.never()) + .createMigrateVMAsyncJob(Mockito.any(), Mockito.any(), Mockito.anyLong()); + } } From b7ed75614fd05c9d536afcf0f1ec5330955b160a Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 9 Sep 2026 22:14:34 +0000 Subject: [PATCH 5/7] Make the DRS affinity check correct Three defects found reviewing the previous commits, all in how DRS decides whether a planned migration is still allowed. Non-strict anti-affinity was discarded entirely. Non-strict groups express themselves by lowering a host's priority on the deployment plan rather than by excluding it, and DRS built a plan, handed it to the processors, read only the exclude list and threw the plan away. Non-strict means the rule may be broken when there is nowhere else to put a VM. That cannot arise while rebalancing: the VM already runs somewhere that satisfies the group and leaving it there is always an option. Being better balanced is not a reason to break it. A host was treated as free the moment a migration away from it was queued. The jobs are asynchronous and can fail, so a queued migration occupies both ends until it completes. In a swap - A from host1 to host3, B from host2 to host1 - B was cleared for host1 while A was still on it. - track the hosts queued migrations have not actually left - refuse a destination that is one of them A VM that stopped between planning and execution threw NPE inside the affinity check, which was then logged without a stack trace. - treat a VM that is no longer running as an out of date plan - log the exception rather than its message Signed-off-by: Brad House --- .../HostAntiAffinityProcessorTest.java | 21 +++ .../cluster/ClusterDrsServiceImpl.java | 58 +++++- .../cluster/ClusterDrsServiceImplTest.java | 166 ++++++++++++++---- 3 files changed, 205 insertions(+), 40 deletions(-) diff --git a/plugins/affinity-group-processors/host-anti-affinity/src/test/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessorTest.java b/plugins/affinity-group-processors/host-anti-affinity/src/test/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessorTest.java index ae3297039289..3ef05ce07bfb 100644 --- a/plugins/affinity-group-processors/host-anti-affinity/src/test/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessorTest.java +++ b/plugins/affinity-group-processors/host-anti-affinity/src/test/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessorTest.java @@ -40,6 +40,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.when; @RunWith(JUnit4.class) @@ -151,10 +152,30 @@ public void testMissingGroupVmIsSkipped() { public void testRemovedGroupVmIsSkipped() { when(_vmInstanceDao.findById(GROUP_VM_ID)).thenReturn(groupVM); when(groupVM.isRemoved()).thenReturn(true); + // a removed VM is not running anywhere, so neither of its hosts should be avoided + lenient().when(groupVM.getHostId()).thenReturn(HOST_ID); + lenient().when(groupVM.getState()).thenReturn(VirtualMachine.State.Stopped); + lenient().when(groupVM.getLastHostId()).thenReturn(LAST_HOST_ID); + lenient().when(groupVM.getUpdateTime()).thenReturn(DateUtil.currentGMTTime()); processor.processAffinityGroup(vmGroupMapping, avoid, vm); assertFalse(avoids(HOST_ID)); + assertFalse(avoids(LAST_HOST_ID)); + } + + @Test + public void testStartingGroupVmWithReservedCapacityLastHostIsAvoided() { + when(_vmInstanceDao.findById(GROUP_VM_ID)).thenReturn(groupVM); + when(groupVM.isRemoved()).thenReturn(false); + when(groupVM.getHostId()).thenReturn(null); + when(groupVM.getState()).thenReturn(VirtualMachine.State.Starting); + when(groupVM.getLastHostId()).thenReturn(LAST_HOST_ID); + when(groupVM.getUpdateTime()).thenReturn(DateUtil.currentGMTTime()); + + processor.processAffinityGroup(vmGroupMapping, avoid, vm); + + assertTrue(avoids(LAST_HOST_ID)); } @Test 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 5f91666b115b..cbe2cfcd160a 100644 --- a/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java +++ b/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java @@ -25,6 +25,7 @@ import com.cloud.dc.ClusterVO; import com.cloud.dc.dao.ClusterDao; import com.cloud.deploy.DataCenterDeployment; +import com.cloud.deploy.DeploymentPlan; import com.cloud.deploy.DeploymentPlanner.ExcludeList; import com.cloud.domain.Domain; import com.cloud.event.ActionEventUtils; @@ -80,6 +81,7 @@ 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; @@ -432,6 +434,29 @@ private List> getMigrationPlans( return migrationPlan; } + /** + * Turns the non-strict affinity preferences recorded on the plan into exclusions. + * + * Non-strict groups express themselves by lowering a host's priority rather than by excluding + * it, and DRS only reads the exclude list - so the preference was being discarded. Rebalancing + * is never a reason to break it: the VM is already running somewhere that satisfies the group, + * and leaving it there is always available to DRS. "Non-strict" means the rule may be broken + * when there is nowhere else to put a VM, which cannot arise while merely rebalancing. + */ + protected void excludeHostsDispreferredByAffinity(DeploymentPlan plan, ExcludeList excludes) { + Map priorities = plan.getHostPriorities(); + if (MapUtils.isEmpty(priorities)) { + return; + } + for (Map.Entry entry : priorities.entrySet()) { + if (entry.getValue() != null && entry.getValue() < DeploymentPlan.DEFAULT_HOST_PRIORITY) { + excludes.addHost(entry.getKey()); + logger.debug("Host {} is dispreferred by a non-strict affinity group, so DRS will not migrate onto it", + entry.getKey()); + } + } + } + private Map getVmToExcludesMap(List vmList, Map hostMap, Set vmsWithAffinityGroups, Map> vmToCompatibleHostsCache, Map vmIdServiceOfferingMap) { @@ -452,6 +477,7 @@ private Map getVmToExcludesMap(List vmList, M excludes = managementServer.applyAffinityConstraints( vm, vmProfile, plan, vmList); + excludeHostsDispreferredByAffinity(plan, excludes); } else { // VM has no affinity groups - create minimal ExcludeList (just source host) excludes = new ExcludeList(); @@ -776,18 +802,35 @@ void processPlans() { * against current placements, and nothing downstream re-checks it - migrateVirtualMachine does * not enforce affinity groups. * + * @param vm + * the VM the plan wants to move + * @param destHost + * where the plan wants to move it * @param dispatched * migrations already queued by this run, which the database does not reflect yet + * @param dispatchedSourceHosts + * hosts those queued migrations have not actually left yet + * @return true when the migration should not go ahead */ - protected boolean destinationViolatesAffinity(VirtualMachine vm, Host destHost, List dispatched) { + protected boolean destinationViolatesAffinity(VirtualMachine vm, Host destHost, List dispatched, + List dispatchedSourceHosts) { + if (vm.getHostId() == null) { + logger.debug("VM {} is no longer running, so its planned migration is out of date", vm); + return true; + } if (CollectionUtils.isEmpty(affinityGroupVMMapDao.listByInstanceId(vm.getId()))) { return false; } + if (dispatchedSourceHosts.contains(destHost.getId())) { + logger.debug("Host {} is still occupied by a VM whose migration away from it is only queued", destHost); + return true; + } DataCenterDeployment plan = new DataCenterDeployment(destHost.getDataCenterId(), destHost.getPodId(), destHost.getClusterId(), null, null, null); VirtualMachineProfile vmProfile = new VirtualMachineProfileImpl(vm, null, serviceOfferingDao.findByIdIncludingRemoved(vm.getId(), vm.getServiceOfferingId()), null, null); ExcludeList excludes = managementServer.applyAffinityConstraints(vm, vmProfile, plan, dispatched); + excludeHostsDispreferredByAffinity(plan, excludes); return excludes.shouldAvoid(destHost); } @@ -806,7 +849,9 @@ void executeDrsPlan(ClusterDrsPlanVO plan) { plan.setStatus(ClusterDrsPlan.Status.IN_PROGRESS); drsPlanDao.update(plan.getId(), plan); + // a queued migration occupies both ends until it completes, and it may not complete at all List dispatched = new ArrayList<>(); + List dispatchedSourceHosts = new ArrayList<>(); for (ClusterDrsPlanMigrationVO migration : planMigrations) { try { @@ -817,7 +862,7 @@ void executeDrsPlan(ClusterDrsPlanVO plan) { migration.getDestHostId())); } - if (destinationViolatesAffinity(vm, host, dispatched)) { + if (destinationViolatesAffinity(vm, host, dispatched, dispatchedSourceHosts)) { logger.warn("Skipping DRS migration of vm {} to host {}: it no longer satisfies the affinity " + "rules for that VM. The plan was generated against older state.", vm, host); migration.setStatus(JobInfo.Status.FAILED); @@ -833,11 +878,16 @@ void executeDrsPlan(ClusterDrsPlanVO plan) { drsPlanMigrationDao.update(migration.getId(), migration); // the migration job has only been queued, so the database still shows the old host. - // record where it is headed so later migrations in this plan see it. + // record both ends: the VM is headed for the destination but has not left the + // source, and if the job fails it never will. + Long sourceHostId = vm.getHostId(); + if (sourceHostId != null) { + dispatchedSourceHosts.add(sourceHostId); + } vm.setHostId(host.getId()); dispatched.add(vm); } catch (Exception e) { - logger.warn("Unable to execute DRS plan {} due to {}", plan, e.getMessage()); + logger.warn("Unable to execute DRS plan {}", plan, e); migration.setStatus(JobInfo.Status.FAILED); drsPlanMigrationDao.update(migration.getId(), migration); } 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 a6b461724c45..4557df32d87e 100644 --- a/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java @@ -30,6 +30,8 @@ import com.cloud.host.Host; import com.cloud.host.HostVO; import com.cloud.deploy.DeploymentPlanner.ExcludeList; +import com.cloud.deploy.DeploymentPlan; +import com.cloud.deploy.DataCenterDeployment; import com.cloud.host.dao.HostDao; import com.cloud.offering.ServiceOffering; import com.cloud.org.Cluster; @@ -63,6 +65,7 @@ import org.mockito.Mock; import org.mockito.MockedStatic; import org.mockito.Mockito; +import org.mockito.ArgumentCaptor; import org.mockito.MockitoAnnotations; import org.mockito.Spy; import org.mockito.junit.MockitoJUnitRunner; @@ -72,7 +75,10 @@ import java.lang.reflect.Field; import java.util.ArrayList; import java.util.Collections; +import java.util.Arrays; import org.apache.cloudstack.jobs.JobInfo; +import org.apache.cloudstack.framework.jobs.AsyncJobManager; +import org.apache.cloudstack.framework.jobs.impl.AsyncJobVO; import java.util.Date; import java.util.HashMap; import java.util.List; @@ -127,6 +133,9 @@ public class ClusterDrsServiceImplTest { @Mock private AffinityGroupVMMapDao affinityGroupVMMapDao; + @Mock + private AsyncJobManager asyncJobManager; + @Mock private VMInstanceDetailsDao vmInstanceDetailsDao; @@ -956,59 +965,127 @@ public void testProcessPlans() { Mockito.verify(clusterDrsService, Mockito.times(2)).executeDrsPlan(Mockito.any(ClusterDrsPlanVO.class)); } + private VMInstanceVO vmWithAffinityGroup(long id, Long hostId) { + VMInstanceVO vm = Mockito.mock(VMInstanceVO.class); + Mockito.lenient().when(vm.getId()).thenReturn(id); + Mockito.lenient().when(vm.getHostId()).thenReturn(hostId); + Mockito.lenient().when(vm.getServiceOfferingId()).thenReturn(5L); + Mockito.lenient().when(affinityGroupVMMapDao.listByInstanceId(id)) + .thenReturn(Collections.singletonList(Mockito.mock(AffinityGroupVMMapVO.class))); + Mockito.lenient().when(serviceOfferingDao.findByIdIncludingRemoved(id, 5L)) + .thenReturn(Mockito.mock(ServiceOfferingVO.class)); + return vm; + } + + private HostVO host(long id) { + HostVO host = Mockito.mock(HostVO.class); + Mockito.lenient().when(host.getId()).thenReturn(id); + return host; + } + + private void affinityExcludes(Long... hostIds) { + ExcludeList excludes = new ExcludeList(); + for (Long hostId : hostIds) { + excludes.addHost(hostId); + } + Mockito.when(managementServer.applyAffinityConstraints(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any())) + .thenReturn(excludes); + } + @Test - public void testDestinationViolatesAffinityWhenVmHasNoAffinityGroups() { + public void testDestinationAllowedWhenVmHasNoAffinityGroups() { VMInstanceVO vm = Mockito.mock(VMInstanceVO.class); Mockito.when(vm.getId()).thenReturn(1L); + Mockito.when(vm.getHostId()).thenReturn(10L); Mockito.when(affinityGroupVMMapDao.listByInstanceId(1L)).thenReturn(Collections.emptyList()); - HostVO destHost = Mockito.mock(HostVO.class); - - assertFalse(clusterDrsService.destinationViolatesAffinity(vm, destHost, Collections.emptyList())); + assertFalse(clusterDrsService.destinationViolatesAffinity(vm, host(20L), + Collections.emptyList(), Collections.emptyList())); Mockito.verify(managementServer, Mockito.never()) .applyAffinityConstraints(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()); } @Test - public void testDestinationViolatesAffinityWhenDestHostIsExcluded() { - VMInstanceVO vm = Mockito.mock(VMInstanceVO.class); - Mockito.when(vm.getId()).thenReturn(1L); - Mockito.when(vm.getServiceOfferingId()).thenReturn(5L); - Mockito.when(affinityGroupVMMapDao.listByInstanceId(1L)) - .thenReturn(Collections.singletonList(Mockito.mock(AffinityGroupVMMapVO.class))); - Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(1L, 5L)) - .thenReturn(Mockito.mock(ServiceOfferingVO.class)); + public void testDestinationRefusedWhenAffinityExcludesIt() { + VMInstanceVO vm = vmWithAffinityGroup(1L, 10L); + affinityExcludes(20L); - HostVO destHost = Mockito.mock(HostVO.class); - Mockito.when(destHost.getId()).thenReturn(20L); + assertTrue(clusterDrsService.destinationViolatesAffinity(vm, host(20L), + Collections.emptyList(), Collections.emptyList())); + } - ExcludeList excludes = new ExcludeList(); - excludes.addHost(20L); - Mockito.when(managementServer.applyAffinityConstraints(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any())) - .thenReturn(excludes); + @Test + public void testDestinationAllowedWhenAffinityExcludesSomewhereElse() { + VMInstanceVO vm = vmWithAffinityGroup(1L, 10L); + affinityExcludes(21L); - assertTrue(clusterDrsService.destinationViolatesAffinity(vm, destHost, Collections.emptyList())); + assertFalse(clusterDrsService.destinationViolatesAffinity(vm, host(20L), + Collections.emptyList(), Collections.emptyList())); } @Test - public void testDestinationViolatesAffinityWhenDestHostIsAllowed() { + public void testDestinationRefusedWhenVmIsNoLongerRunning() { VMInstanceVO vm = Mockito.mock(VMInstanceVO.class); - Mockito.when(vm.getId()).thenReturn(1L); - Mockito.when(vm.getServiceOfferingId()).thenReturn(5L); - Mockito.when(affinityGroupVMMapDao.listByInstanceId(1L)) - .thenReturn(Collections.singletonList(Mockito.mock(AffinityGroupVMMapVO.class))); - Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(1L, 5L)) - .thenReturn(Mockito.mock(ServiceOfferingVO.class)); + Mockito.when(vm.getHostId()).thenReturn(null); - HostVO destHost = Mockito.mock(HostVO.class); - Mockito.when(destHost.getId()).thenReturn(20L); + assertTrue("a plan for a VM that has since stopped is out of date", + clusterDrsService.destinationViolatesAffinity(vm, host(20L), + Collections.emptyList(), Collections.emptyList())); + } - ExcludeList excludes = new ExcludeList(); - excludes.addHost(21L); - Mockito.when(managementServer.applyAffinityConstraints(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any())) - .thenReturn(excludes); + @Test + public void testDestinationRefusedWhileItIsStillOccupiedByAQueuedMigration() { + // the swap case: A is queued to leave host1, so host1 is not free for B yet - and if A's + // migration fails, A never leaves at all + VMInstanceVO b = vmWithAffinityGroup(2L, 11L); + + assertTrue("a host is not free until the VM leaving it has actually gone", + clusterDrsService.destinationViolatesAffinity(b, host(10L), + Collections.emptyList(), Collections.singletonList(10L))); + Mockito.verify(managementServer, Mockito.never()) + .applyAffinityConstraints(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()); + } + + @Test + public void testExecuteDrsPlanKeepsSourceHostsOccupiedAcrossMigrations() { + // plan: A host10 -> host12, B host11 -> host10. B must not be sent to host10. + ClusterDrsPlanVO plan = Mockito.mock(ClusterDrsPlanVO.class); + Mockito.when(plan.getId()).thenReturn(1L); + + ClusterDrsPlanMigrationVO first = Mockito.mock(ClusterDrsPlanMigrationVO.class); + Mockito.when(first.getVmId()).thenReturn(1L); + Mockito.when(first.getDestHostId()).thenReturn(12L); + ClusterDrsPlanMigrationVO second = Mockito.mock(ClusterDrsPlanMigrationVO.class); + Mockito.when(second.getId()).thenReturn(8L); + Mockito.when(second.getVmId()).thenReturn(2L); + Mockito.when(second.getDestHostId()).thenReturn(10L); + Mockito.when(drsPlanMigrationDao.listPlanMigrationsToExecute(1L)) + .thenReturn(Arrays.asList(first, second)); + + VMInstanceVO a = Mockito.mock(VMInstanceVO.class); + Mockito.when(a.getHostId()).thenReturn(10L); + VMInstanceVO b = Mockito.mock(VMInstanceVO.class); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(a); + Mockito.when(vmInstanceDao.findById(2L)).thenReturn(b); + HostVO host12 = host(12L); + HostVO host10 = host(10L); + Mockito.when(hostDao.findById(12L)).thenReturn(host12); + Mockito.when(hostDao.findById(10L)).thenReturn(host10); + + Mockito.doReturn(false).when(clusterDrsService).destinationViolatesAffinity( + Mockito.eq(a), Mockito.any(), Mockito.any(), Mockito.any()); + Mockito.doReturn(1L).when(clusterDrsService) + .createMigrateVMAsyncJob(Mockito.any(), Mockito.any(), Mockito.anyLong()); + Mockito.when(asyncJobManager.getAsyncJob(1L)).thenReturn(Mockito.mock(AsyncJobVO.class)); + + clusterDrsService.executeDrsPlan(plan); - assertFalse(clusterDrsService.destinationViolatesAffinity(vm, destHost, Collections.emptyList())); + // A's source host must have been carried into the check for B + ArgumentCaptor sources = ArgumentCaptor.forClass(List.class); + Mockito.verify(clusterDrsService).destinationViolatesAffinity( + Mockito.eq(b), Mockito.any(), Mockito.any(), sources.capture()); + assertTrue("the host A is leaving must still count as occupied", + sources.getValue().contains(10L)); } @Test @@ -1024,12 +1101,12 @@ public void testExecuteDrsPlanSkipsMigrationThatViolatesAffinity() { .thenReturn(Collections.singletonList(migration)); VMInstanceVO vm = Mockito.mock(VMInstanceVO.class); - HostVO destHost = Mockito.mock(HostVO.class); + HostVO host20 = host(20L); Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); - Mockito.when(hostDao.findById(20L)).thenReturn(destHost); + Mockito.when(hostDao.findById(20L)).thenReturn(host20); Mockito.doReturn(true).when(clusterDrsService) - .destinationViolatesAffinity(Mockito.any(), Mockito.any(), Mockito.any()); + .destinationViolatesAffinity(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()); clusterDrsService.executeDrsPlan(plan); @@ -1037,4 +1114,21 @@ public void testExecuteDrsPlanSkipsMigrationThatViolatesAffinity() { Mockito.verify(clusterDrsService, Mockito.never()) .createMigrateVMAsyncJob(Mockito.any(), Mockito.any(), Mockito.anyLong()); } + + @Test + public void testNonStrictAntiAffinityIsHonoured() { + // a non-strict group lowers a host's priority rather than excluding it. rebalancing is + // never a reason to break it, since not migrating is always available. + DataCenterDeployment plan = new DataCenterDeployment(1L, 1L, 1L, null, null, null); + plan.adjustHostPriority(30L, DeploymentPlan.HostPriorityAdjustment.LOWER); + plan.adjustHostPriority(31L, DeploymentPlan.HostPriorityAdjustment.HIGHER); + + ExcludeList excludes = new ExcludeList(); + clusterDrsService.excludeHostsDispreferredByAffinity(plan, excludes); + + assertTrue("a dispreferred host must not be a DRS destination", + excludes.getHostsToAvoid().contains(30L)); + assertFalse("a preferred host must stay available", + excludes.getHostsToAvoid().contains(31L)); + } } From 192bab536d32a079101ddc29ad44279d0eb8b4eb Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 9 Sep 2026 22:36:04 +0000 Subject: [PATCH 6/7] Address the rest of the DRS anti-affinity review - a skipped migration is CANCELLED, not FAILED. Nothing went wrong; the plan went out of date. FAILED was indistinguishable from a migration that genuinely broke, and left no record of why - record an event when one is skipped, so it is visible rather than a silent no-op in an otherwise successful plan - the processors also cover dedicated resources and DPDK, so the refusal message no longer claims every skip is about an affinity group - hoist the third copy of getVmIdVmMap into AffinityProcessorBase Signed-off-by: Brad House --- .../affinity/AffinityProcessorBase.java | 19 +++++++++++++++++++ .../affinity/HostAntiAffinityProcessor.java | 9 --------- .../NonStrictHostAffinityProcessor.java | 9 --------- .../cluster/ClusterDrsServiceImpl.java | 17 +++++++++++++---- .../cluster/ClusterDrsServiceImplTest.java | 2 +- 5 files changed, 33 insertions(+), 23 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/affinity/AffinityProcessorBase.java b/api/src/main/java/org/apache/cloudstack/affinity/AffinityProcessorBase.java index 9995d8039e1f..550e61a68655 100644 --- a/api/src/main/java/org/apache/cloudstack/affinity/AffinityProcessorBase.java +++ b/api/src/main/java/org/apache/cloudstack/affinity/AffinityProcessorBase.java @@ -25,7 +25,9 @@ import com.cloud.vm.VirtualMachineProfile; import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; public class AffinityProcessorBase extends AdapterBase implements AffinityGroupProcessor { @@ -41,6 +43,23 @@ public void process(VirtualMachineProfile vm, DeploymentPlan plan, ExcludeList a } + + /** + * Indexes placements supplied by the caller. Callers such as DRS build a plan of several moves + * in memory and persist it only at the end, so during planning the database still shows the old + * host for every VM the plan has already moved. + */ + protected Map getVmIdVmMap(List vmList) { + Map vmIdVmMap = new HashMap<>(); + if (vmList == null) { + return vmIdVmMap; + } + for (VirtualMachine vm : vmList) { + vmIdVmMap.put(vm.getId(), vm); + } + return vmIdVmMap; + } + @Override public String getType() { return _type; diff --git a/plugins/affinity-group-processors/host-anti-affinity/src/main/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessor.java b/plugins/affinity-group-processors/host-anti-affinity/src/main/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessor.java index 9c69fbd7053c..b71142d0a9e5 100644 --- a/plugins/affinity-group-processors/host-anti-affinity/src/main/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessor.java +++ b/plugins/affinity-group-processors/host-anti-affinity/src/main/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessor.java @@ -18,7 +18,6 @@ import java.util.Arrays; import java.util.Collections; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.stream.Collectors; @@ -133,14 +132,6 @@ protected void processAffinityGroup(AffinityGroupVMMapVO vmGroupMapping, Exclude } } - protected Map getVmIdVmMap(List vmList) { - Map vmIdVmMap = new HashMap<>(); - for (VirtualMachine vm : vmList) { - vmIdVmMap.put(vm.getId(), vm); - } - return vmIdVmMap; - } - protected void avoidHostOfVmInAffinityGroup(ExcludeList avoid, VMInstanceVO groupVM) { if (groupVM.getHostId() != null) { avoid.addHost(groupVM.getHostId()); diff --git a/plugins/affinity-group-processors/non-strict-host-affinity/src/main/java/org/apache/cloudstack/affinity/NonStrictHostAffinityProcessor.java b/plugins/affinity-group-processors/non-strict-host-affinity/src/main/java/org/apache/cloudstack/affinity/NonStrictHostAffinityProcessor.java index 8cb92ce6fa18..c79efca5e7de 100644 --- a/plugins/affinity-group-processors/non-strict-host-affinity/src/main/java/org/apache/cloudstack/affinity/NonStrictHostAffinityProcessor.java +++ b/plugins/affinity-group-processors/non-strict-host-affinity/src/main/java/org/apache/cloudstack/affinity/NonStrictHostAffinityProcessor.java @@ -18,7 +18,6 @@ import java.util.Arrays; import java.util.Collections; -import java.util.HashMap; import java.util.List; import java.util.Map; @@ -113,14 +112,6 @@ protected void processAffinityGroup(AffinityGroupVMMapVO vmGroupMapping, Deploym } } - protected Map getVmIdVmMap(List vmList) { - Map vmIdVmMap = new HashMap<>(); - for (VirtualMachine vm : vmList) { - vmIdVmMap.put(vm.getId(), vm); - } - return vmIdVmMap; - } - protected void processVmInAffinityGroup(DeploymentPlan plan, VMInstanceVO groupVM) { if (groupVM.getHostId() != null) { Integer priority = adjustHostPriority(plan, groupVM.getHostId()); 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 cbe2cfcd160a..e495e048a815 100644 --- a/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java +++ b/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java @@ -795,13 +795,16 @@ void processPlans() { * the DRS plan to be executed */ /** - * Checks a planned migration against the affinity rules as they stand now. + * Checks a planned migration against the placement rules as they stand now. * * A plan is generated once and executed later, so state can have moved on: VMs may have been * created, migrated or destroyed in between. Anti-affinity in particular is only meaningful * against current placements, and nothing downstream re-checks it - migrateVirtualMachine does * not enforce affinity groups. * + * The processors also cover dedicated resources and DPDK, so a refusal is not necessarily about + * an affinity group. + * * @param vm * the VM the plan wants to move * @param destHost @@ -863,10 +866,16 @@ void executeDrsPlan(ClusterDrsPlanVO plan) { } if (destinationViolatesAffinity(vm, host, dispatched, dispatchedSourceHosts)) { - logger.warn("Skipping DRS migration of vm {} to host {}: it no longer satisfies the affinity " + - "rules for that VM. The plan was generated against older state.", vm, host); - migration.setStatus(JobInfo.Status.FAILED); + String reason = String.format("Skipped DRS migration of %s to %s: the destination no longer " + + "satisfies the placement rules for that VM. The plan was generated against older " + + "state.", vm, host); + logger.warn(reason); + // cancelled rather than failed: nothing went wrong, the plan went out of date + migration.setStatus(JobInfo.Status.CANCELLED); drsPlanMigrationDao.update(migration.getId(), migration); + ActionEventUtils.onCompletedActionEvent(User.UID_SYSTEM, Account.ACCOUNT_ID_SYSTEM, + EventVO.LEVEL_WARN, EventTypes.EVENT_CLUSTER_DRS, false, reason, + plan.getClusterId(), ApiCommandResourceType.Cluster.toString(), plan.getEventId()); continue; } 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 4557df32d87e..6c43a4ae7951 100644 --- a/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java @@ -1110,7 +1110,7 @@ public void testExecuteDrsPlanSkipsMigrationThatViolatesAffinity() { clusterDrsService.executeDrsPlan(plan); - Mockito.verify(migration).setStatus(JobInfo.Status.FAILED); + Mockito.verify(migration).setStatus(JobInfo.Status.CANCELLED); Mockito.verify(clusterDrsService, Mockito.never()) .createMigrateVMAsyncJob(Mockito.any(), Mockito.any(), Mockito.anyLong()); } From 9c867f4ccac63ca52868a1bb9e7086f19c37e39e Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 10 Sep 2026 12:30:52 +0000 Subject: [PATCH 7/7] Re-check every migration, not only VMs in an affinity group Review feedback on the PR. applyAffinityConstraints also applies DPDK and dedicated-resource exclusions, and neither depends on affinity group membership. Returning early for VMs with no groups therefore skipped those checks, which the method's own documentation said it covered. The cost is one call per planned migration, bounded by drs.max.migrations, at execution time rather than in any hot loop. Also move executeDrsPlan's javadoc back onto executeDrsPlan; it was left stranded above the method inserted before it. Signed-off-by: Brad House --- .../cluster/ClusterDrsServiceImpl.java | 22 +++++++++---------- .../cluster/ClusterDrsServiceImplTest.java | 11 +++++++--- 2 files changed, 18 insertions(+), 15 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 e495e048a815..ac7f621625f1 100644 --- a/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java +++ b/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java @@ -787,13 +787,6 @@ void processPlans() { } } - /** - * Executes the DRS plan by migrating virtual machines to their destination hosts. - * If there are no migrations to be executed, the plan is marked as completed. - * - * @param plan - * the DRS plan to be executed - */ /** * Checks a planned migration against the placement rules as they stand now. * @@ -802,8 +795,9 @@ void processPlans() { * against current placements, and nothing downstream re-checks it - migrateVirtualMachine does * not enforce affinity groups. * - * The processors also cover dedicated resources and DPDK, so a refusal is not necessarily about - * an affinity group. + * Checked for every VM, not only those in an affinity group: applyAffinityConstraints also + * applies DPDK and dedicated-resource exclusions, which apply regardless of group membership. + * A refusal here is therefore not necessarily about an affinity group. * * @param vm * the VM the plan wants to move @@ -821,9 +815,6 @@ protected boolean destinationViolatesAffinity(VirtualMachine vm, Host destHost, logger.debug("VM {} is no longer running, so its planned migration is out of date", vm); return true; } - if (CollectionUtils.isEmpty(affinityGroupVMMapDao.listByInstanceId(vm.getId()))) { - return false; - } if (dispatchedSourceHosts.contains(destHost.getId())) { logger.debug("Host {} is still occupied by a VM whose migration away from it is only queued", destHost); return true; @@ -837,6 +828,13 @@ protected boolean destinationViolatesAffinity(VirtualMachine vm, Host destHost, return excludes.shouldAvoid(destHost); } + /** + * Executes the DRS plan by migrating virtual machines to their destination hosts. + * If there are no migrations to be executed, the plan is marked as completed. + * + * @param plan + * the DRS plan to be executed + */ void executeDrsPlan(ClusterDrsPlanVO plan) { List planMigrations = drsPlanMigrationDao.listPlanMigrationsToExecute(plan.getId()); if (planMigrations == null || planMigrations.isEmpty()) { 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 6c43a4ae7951..c5d0c47809bd 100644 --- a/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java @@ -993,15 +993,20 @@ private void affinityExcludes(Long... hostIds) { } @Test - public void testDestinationAllowedWhenVmHasNoAffinityGroups() { + public void testAVmWithNoAffinityGroupsIsStillRechecked() { + // applyAffinityConstraints also applies DPDK and dedicated-resource exclusions, which do + // not depend on affinity group membership, so every VM has to go through it VMInstanceVO vm = Mockito.mock(VMInstanceVO.class); Mockito.when(vm.getId()).thenReturn(1L); Mockito.when(vm.getHostId()).thenReturn(10L); - Mockito.when(affinityGroupVMMapDao.listByInstanceId(1L)).thenReturn(Collections.emptyList()); + Mockito.when(vm.getServiceOfferingId()).thenReturn(5L); + Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(1L, 5L)) + .thenReturn(Mockito.mock(ServiceOfferingVO.class)); + affinityExcludes(21L); assertFalse(clusterDrsService.destinationViolatesAffinity(vm, host(20L), Collections.emptyList(), Collections.emptyList())); - Mockito.verify(managementServer, Mockito.never()) + Mockito.verify(managementServer) .applyAffinityConstraints(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()); }