From 6c30d483b17223aa260efec0c1549671b3e078f4 Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 9 Sep 2026 21:18:15 +0000 Subject: [PATCH 01/15] 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 a91884137a95ac0233ad9192f80ae4d4961fc493 Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 9 Sep 2026 21:19:07 +0000 Subject: [PATCH 02/15] 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 fd7919dce9047f859c25717b4f9c8aabcad2ca0a Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 9 Sep 2026 21:20:32 +0000 Subject: [PATCH 03/15] 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 8440ba554428adaeb279feb4b06b99ee1529e03b Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 9 Sep 2026 21:23:18 +0000 Subject: [PATCH 04/15] 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 ca745de9d552908045101756dc07a0ef45dc18c9 Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 9 Sep 2026 22:14:34 +0000 Subject: [PATCH 05/15] 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 907410c3858efb6451bf2f8f32837f40ccc1a6ab Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 9 Sep 2026 22:36:04 +0000 Subject: [PATCH 06/15] 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 96ca35f264ca..b330b6aa321c 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 { @@ -44,6 +46,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 4c065722840a74708e9984c538836472a28134bc Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 10 Sep 2026 12:30:52 +0000 Subject: [PATCH 07/15] 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()); } From ab6d5ed54949e3256223eb7955168a99a82c4de2 Mon Sep 17 00:00:00 2001 From: Brad House Date: Tue, 15 Sep 2026 15:41:41 +0000 Subject: [PATCH 08/15] Drop the duplicate getVmIdVmMap from HostAffinityProcessor Now that the helper is on AffinityProcessorBase, HostAffinityProcessor's own copy only shadows it. Removing it also fixes a latent difference: the copy dereferences vmList unguarded, so a null list throws, while the base class returns an empty map. Callers that pass no planned placements reach it through getHostIdSet(vmIds), which supplies an empty list, so nothing hits that today - but the two behaving differently is exactly the kind of thing that stops being harmless later. Signed-off-by: Brad House --- .../cloudstack/affinity/HostAffinityProcessor.java | 9 --------- 1 file changed, 9 deletions(-) diff --git a/plugins/affinity-group-processors/host-affinity/src/main/java/org/apache/cloudstack/affinity/HostAffinityProcessor.java b/plugins/affinity-group-processors/host-affinity/src/main/java/org/apache/cloudstack/affinity/HostAffinityProcessor.java index b97b8e224ad7..3f2dc0bf75b6 100644 --- a/plugins/affinity-group-processors/host-affinity/src/main/java/org/apache/cloudstack/affinity/HostAffinityProcessor.java +++ b/plugins/affinity-group-processors/host-affinity/src/main/java/org/apache/cloudstack/affinity/HostAffinityProcessor.java @@ -17,7 +17,6 @@ package org.apache.cloudstack.affinity; import java.util.Collections; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -121,14 +120,6 @@ protected Set getHostIdSet(List vmIds, List vmList) return hostIds; } - protected Map getVmIdVmMap(List vmList) { - Map vmIdVmMap = new HashMap<>(); - for (VirtualMachine vm : vmList) { - vmIdVmMap.put(vm.getId(), vm); - } - return vmIdVmMap; - } - /** * Get preferred host ids list from the affinity group VMs */ From bbaac19a3d96d98477add08546a2c6ae46ae1752 Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 14 Oct 2026 17:59:08 +0000 Subject: [PATCH 09/15] Only let group members constrain DRS moves and non-strict groups Review feedback on the PR. A queued migration's source host was tracked as a flat list of host ids, and any later move onto one of them was cancelled outright, without consulting the affinity groups. A plan that moves one VM off a host and an unrelated VM onto it lost the second move. The source end is now covered by running the affinity processors a second time against the database, which still shows queued VMs on the host they are leaving until the job completes. The destination end is covered as before by the queued placements. A host stays occupied by a queued VM only for VMs that share a group with it. Non-strict groups excluded every host whose priority they had lowered. When a group has more VMs than hosts, every host is lowered, so DRS never moved those VMs, where before this PR it moved them freely. A host is now excluded only when it is dispreferred more than the VM's current host, so a move never adds to the group members the VM shares a host with but is not ruled out when every host is equally bad. Raised priorities from non-strict host affinity are not treated as a penalty on the other hosts. Signed-off-by: Brad House --- .../cluster/ClusterDrsServiceImpl.java | 65 +++++---- .../cluster/ClusterDrsServiceImplTest.java | 138 +++++++++++++++--- 2 files changed, 155 insertions(+), 48 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 ac7f621625f1..a6239e299cc3 100644 --- a/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java +++ b/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java @@ -439,20 +439,31 @@ private List> getMigrationPlans( * * 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. + * is never a reason to make it worse: leaving the VM where it is is always available to DRS. + * + * A host is excluded only when it is dispreferred more than the VM's current host, so a move + * never adds to the group members the VM shares a host with. Comparing against the source + * rather than against an untouched host matters when a group has more VMs than there are + * hosts: then every host is dispreferred, and excluding all of them would stop DRS moving + * those VMs at all, which it did before non-strict groups were honoured. + * + * @param sourceHostId + * the host the VM is on now, in the placements the priorities were computed from */ - protected void excludeHostsDispreferredByAffinity(DeploymentPlan plan, ExcludeList excludes) { + protected void excludeHostsDispreferredByAffinity(DeploymentPlan plan, ExcludeList excludes, Long sourceHostId) { Map priorities = plan.getHostPriorities(); if (MapUtils.isEmpty(priorities)) { return; } + // a raised priority (non-strict host affinity) is not a penalty, so never compare above the default + Integer sourcePriority = priorities.get(sourceHostId); + int threshold = sourcePriority == null ? DeploymentPlan.DEFAULT_HOST_PRIORITY + : Math.min(sourcePriority, DeploymentPlan.DEFAULT_HOST_PRIORITY); for (Map.Entry entry : priorities.entrySet()) { - if (entry.getValue() != null && entry.getValue() < DeploymentPlan.DEFAULT_HOST_PRIORITY) { + if (entry.getValue() != null && entry.getValue() < threshold) { excludes.addHost(entry.getKey()); - logger.debug("Host {} is dispreferred by a non-strict affinity group, so DRS will not migrate onto it", - entry.getKey()); + logger.debug("Host {} is dispreferred by a non-strict affinity group more than the current host {}, " + + "so DRS will not migrate onto it", entry.getKey(), sourceHostId); } } } @@ -477,7 +488,7 @@ private Map getVmToExcludesMap(List vmList, M excludes = managementServer.applyAffinityConstraints( vm, vmProfile, plan, vmList); - excludeHostsDispreferredByAffinity(plan, excludes); + excludeHostsDispreferredByAffinity(plan, excludes, vm.getHostId()); } else { // VM has no affinity groups - create minimal ExcludeList (just source host) excludes = new ExcludeList(); @@ -799,32 +810,41 @@ void processPlans() { * applies DPDK and dedicated-resource exclusions, which apply regardless of group membership. * A refusal here is therefore not necessarily about an affinity group. * + * A queued migration occupies both of its hosts until it completes, and may never complete, so + * the check is made twice: against where queued VMs are headed, and against the database, which + * still shows them on the host they are leaving. Both go through the affinity processors, so + * only VMs that actually share a group with this one constrain it. + * * @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 + * migrations already queued by this run, placed on their destination hosts * @return true when the migration should not go ahead */ - protected boolean destinationViolatesAffinity(VirtualMachine vm, Host destHost, List dispatched, - List dispatchedSourceHosts) { + protected boolean destinationViolatesAffinity(VirtualMachine vm, Host destHost, List dispatched) { if (vm.getHostId() == null) { logger.debug("VM {} is no longer running, so its planned migration is out of date", vm); return true; } - if (dispatchedSourceHosts.contains(destHost.getId())) { + if (destinationExcluded(vm, destHost, dispatched)) { + return true; + } + if (!dispatched.isEmpty() && destinationExcluded(vm, destHost, Collections.emptyList())) { logger.debug("Host {} is still occupied by a VM whose migration away from it is only queued", destHost); return true; } + return false; + } + + private boolean destinationExcluded(VirtualMachine vm, Host destHost, List placements) { 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); + ExcludeList excludes = managementServer.applyAffinityConstraints(vm, vmProfile, plan, placements); + excludeHostsDispreferredByAffinity(plan, excludes, vm.getHostId()); return excludes.shouldAvoid(destHost); } @@ -852,7 +872,6 @@ void executeDrsPlan(ClusterDrsPlanVO 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 { @@ -863,7 +882,7 @@ void executeDrsPlan(ClusterDrsPlanVO plan) { migration.getDestHostId())); } - if (destinationViolatesAffinity(vm, host, dispatched, dispatchedSourceHosts)) { + if (destinationViolatesAffinity(vm, host, dispatched)) { 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); @@ -884,13 +903,9 @@ void executeDrsPlan(ClusterDrsPlanVO plan) { 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 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); - } + // the migration job has only been queued, so the database still shows the old host, + // which covers the source end; record the destination end here. if the job fails the + // VM never leaves the source, which the database check keeps honouring. vm.setHostId(host.getId()); dispatched.add(vm); } catch (Exception e) { 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 c5d0c47809bd..685ce8eb33b5 100644 --- a/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java @@ -1005,7 +1005,7 @@ public void testAVmWithNoAffinityGroupsIsStillRechecked() { affinityExcludes(21L); assertFalse(clusterDrsService.destinationViolatesAffinity(vm, host(20L), - Collections.emptyList(), Collections.emptyList())); + Collections.emptyList())); Mockito.verify(managementServer) .applyAffinityConstraints(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()); } @@ -1016,7 +1016,7 @@ public void testDestinationRefusedWhenAffinityExcludesIt() { affinityExcludes(20L); assertTrue(clusterDrsService.destinationViolatesAffinity(vm, host(20L), - Collections.emptyList(), Collections.emptyList())); + Collections.emptyList())); } @Test @@ -1025,7 +1025,7 @@ public void testDestinationAllowedWhenAffinityExcludesSomewhereElse() { affinityExcludes(21L); assertFalse(clusterDrsService.destinationViolatesAffinity(vm, host(20L), - Collections.emptyList(), Collections.emptyList())); + Collections.emptyList())); } @Test @@ -1035,25 +1035,66 @@ public void testDestinationRefusedWhenVmIsNoLongerRunning() { assertTrue("a plan for a VM that has since stopped is out of date", clusterDrsService.destinationViolatesAffinity(vm, host(20L), - Collections.emptyList(), Collections.emptyList())); + Collections.emptyList())); + } + + private ExcludeList excludeList(Long... hostIds) { + ExcludeList excludes = new ExcludeList(); + for (Long hostId : hostIds) { + excludes.addHost(hostId); + } + return excludes; + } + + /** + * Stubs applyAffinityConstraints separately for the two views the execution-time check takes: + * queued VMs on their destinations, and the database, which still has them on their sources. + */ + private void affinityExcludesByView(ExcludeList withQueued, ExcludeList fromDatabase) { + Mockito.when(managementServer.applyAffinityConstraints(Mockito.any(), Mockito.any(), Mockito.any(), + Mockito.argThat(list -> list != null && !list.isEmpty()))).thenReturn(withQueued); + Mockito.when(managementServer.applyAffinityConstraints(Mockito.any(), Mockito.any(), Mockito.any(), + Mockito.argThat(list -> list == null || list.isEmpty()))).thenReturn(fromDatabase); } @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 + public void testDestinationRefusedWhileAGroupMemberIsStillLeavingIt() { + // the swap case: A is queued to leave host10, and A shares a group with B. host10 is not free + // for B until A has actually gone - and if A's migration fails, A never leaves at all + VMInstanceVO a = vmWithAffinityGroup(1L, 12L); VMInstanceVO b = vmWithAffinityGroup(2L, 11L); + affinityExcludesByView(excludeList(11L, 12L), excludeList(11L, 10L)); - 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()); + assertTrue("a host is not free until the group member leaving it has actually gone", + clusterDrsService.destinationViolatesAffinity(b, host(10L), Collections.singletonList(a))); + } + + @Test + public void testDestinationAllowedWhileAnUnrelatedVmIsStillLeavingIt() { + // A is queued to leave host10, but shares no group with B, so the affinity processors avoid + // host10 in neither view and B's move onto it must go ahead + VMInstanceVO a = vmWithAffinityGroup(1L, 12L); + VMInstanceVO b = vmWithAffinityGroup(2L, 11L); + affinityExcludesByView(excludeList(11L), excludeList(11L)); + + assertFalse("a VM leaving a host only constrains VMs it shares a group with", + clusterDrsService.destinationViolatesAffinity(b, host(10L), Collections.singletonList(a))); + Mockito.verify(managementServer, Mockito.times(2)) + .applyAffinityConstraints(Mockito.eq(b), Mockito.any(), Mockito.any(), Mockito.any()); } @Test - public void testExecuteDrsPlanKeepsSourceHostsOccupiedAcrossMigrations() { - // plan: A host10 -> host12, B host11 -> host10. B must not be sent to host10. + public void testDestinationRefusedWhenAGroupMemberIsHeadingThere() { + VMInstanceVO a = vmWithAffinityGroup(1L, 10L); + VMInstanceVO b = vmWithAffinityGroup(2L, 11L); + affinityExcludesByView(excludeList(11L, 10L), excludeList(11L, 12L)); + + assertTrue(clusterDrsService.destinationViolatesAffinity(b, host(10L), Collections.singletonList(a))); + } + + @Test + public void testExecuteDrsPlanPassesQueuedMigrationsToLaterChecks() { + // plan: A host10 -> host12, B host11 -> host10. B's check must know A is queued. ClusterDrsPlanVO plan = Mockito.mock(ClusterDrsPlanVO.class); Mockito.when(plan.getId()).thenReturn(1L); @@ -1068,7 +1109,6 @@ public void testExecuteDrsPlanKeepsSourceHostsOccupiedAcrossMigrations() { .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); @@ -1078,19 +1118,20 @@ public void testExecuteDrsPlanKeepsSourceHostsOccupiedAcrossMigrations() { Mockito.when(hostDao.findById(10L)).thenReturn(host10); Mockito.doReturn(false).when(clusterDrsService).destinationViolatesAffinity( - Mockito.eq(a), Mockito.any(), Mockito.any(), Mockito.any()); + 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); - // A's source host must have been carried into the check for B - ArgumentCaptor sources = ArgumentCaptor.forClass(List.class); + ArgumentCaptor queued = 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)); + Mockito.eq(b), Mockito.any(), queued.capture()); + assertTrue("A's queued migration must be carried into the check for B", queued.getValue().contains(a)); + Mockito.verify(a).setHostId(12L); + Mockito.verify(clusterDrsService, Mockito.times(2)) + .createMigrateVMAsyncJob(Mockito.any(), Mockito.any(), Mockito.anyLong()); } @Test @@ -1111,7 +1152,7 @@ public void testExecuteDrsPlanSkipsMigrationThatViolatesAffinity() { Mockito.when(hostDao.findById(20L)).thenReturn(host20); Mockito.doReturn(true).when(clusterDrsService) - .destinationViolatesAffinity(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()); + .destinationViolatesAffinity(Mockito.any(), Mockito.any(), Mockito.any()); clusterDrsService.executeDrsPlan(plan); @@ -1129,11 +1170,62 @@ public void testNonStrictAntiAffinityIsHonoured() { plan.adjustHostPriority(31L, DeploymentPlan.HostPriorityAdjustment.HIGHER); ExcludeList excludes = new ExcludeList(); - clusterDrsService.excludeHostsDispreferredByAffinity(plan, excludes); + clusterDrsService.excludeHostsDispreferredByAffinity(plan, excludes, 10L); 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)); } + + private static boolean avoids(ExcludeList excludes, long hostId) { + return excludes.getHostsToAvoid() != null && excludes.getHostsToAvoid().contains(hostId); + } + + @Test + public void testNonStrictGroupWithMoreVmsThanHostsDoesNotFreezeItsVms() { + // 4 VMs in a non-strict anti-affinity group on 3 hosts: A and D on host10, B on host11, C on + // host12. for A every host holds a group member, so every host is dispreferred. moving A + // to host11 or host12 is no worse than staying, so DRS must still be allowed to do it. + DataCenterDeployment plan = new DataCenterDeployment(1L, 1L, 1L, null, null, null); + plan.adjustHostPriority(10L, DeploymentPlan.HostPriorityAdjustment.LOWER); + plan.adjustHostPriority(11L, DeploymentPlan.HostPriorityAdjustment.LOWER); + plan.adjustHostPriority(12L, DeploymentPlan.HostPriorityAdjustment.LOWER); + + ExcludeList excludes = new ExcludeList(); + clusterDrsService.excludeHostsDispreferredByAffinity(plan, excludes, 10L); + + assertFalse(avoids(excludes, 11L)); + assertFalse(avoids(excludes, 12L)); + } + + @Test + public void testNonStrictGroupStillRefusesAHostWorseThanTheCurrentOne() { + // A shares host10 with one group member; host11 has two. moving there makes it worse. + DataCenterDeployment plan = new DataCenterDeployment(1L, 1L, 1L, null, null, null); + plan.adjustHostPriority(10L, DeploymentPlan.HostPriorityAdjustment.LOWER); + plan.adjustHostPriority(11L, DeploymentPlan.HostPriorityAdjustment.LOWER); + plan.adjustHostPriority(11L, DeploymentPlan.HostPriorityAdjustment.LOWER); + plan.adjustHostPriority(12L, DeploymentPlan.HostPriorityAdjustment.LOWER); + + ExcludeList excludes = new ExcludeList(); + clusterDrsService.excludeHostsDispreferredByAffinity(plan, excludes, 10L); + + assertTrue(avoids(excludes, 11L)); + assertFalse(avoids(excludes, 12L)); + } + + @Test + public void testNonStrictHostAffinityOnTheCurrentHostDoesNotFreezeTheVm() { + // a raised priority is a preference, not a penalty: it must not make untouched hosts look + // dispreferred by comparison + DataCenterDeployment plan = new DataCenterDeployment(1L, 1L, 1L, null, null, null); + plan.adjustHostPriority(10L, DeploymentPlan.HostPriorityAdjustment.HIGHER); + plan.adjustHostPriority(11L, DeploymentPlan.HostPriorityAdjustment.DEFAULT); + + ExcludeList excludes = new ExcludeList(); + clusterDrsService.excludeHostsDispreferredByAffinity(plan, excludes, 10L); + + assertFalse(avoids(excludes, 11L)); + } } From cd3146e2a440284e01d57868cd5098032e1c8d7d Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 9 Sep 2026 22:52:29 +0000 Subject: [PATCH 10/15] Stop DRS planning from redoing the same work Plan generation took minutes on a busy cluster. Two causes, both about repeating work rather than about the algorithm. Working out where a VM could go runs the host allocators and inspects every volume, and it was done once per VM. VMs that would get the same answer - same offering, template, current host, storage and affinity groups - now share one pass. A cluster of similar VMs goes from one pass per VM to one per handful. Affinity was re-evaluated for every VM in the cluster on every iteration, up to drs.max.migrations times. Only VMs sharing a group with the one just moved can have changed, so only those are re-evaluated after the first pass. Also: a VM whose grouping key cannot be worked out is now considered on its own rather than silently dropped from the plan. Signed-off-by: Brad House --- .../cluster/ClusterDrsServiceImpl.java | 98 ++++++++++++-- .../cluster/ClusterDrsEquivalenceTest.java | 121 ++++++++++++++++++ .../cluster/ClusterDrsServiceImplTest.java | 4 + 3 files changed, 214 insertions(+), 9 deletions(-) create mode 100644 server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsEquivalenceTest.java 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 a6239e299cc3..614f56f7c070 100644 --- a/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java +++ b/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java @@ -44,6 +44,8 @@ import com.cloud.user.User; import com.cloud.utils.DateUtil; import com.cloud.utils.Pair; +import com.cloud.storage.VolumeVO; +import com.cloud.storage.dao.VolumeDao; import com.cloud.utils.Ternary; import com.cloud.utils.component.ComponentContext; import com.cloud.utils.component.ManagerBase; @@ -94,6 +96,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.Timer; import java.util.TimerTask; @@ -146,6 +149,9 @@ public class ClusterDrsServiceImpl extends ManagerBase implements ClusterDrsServ @Inject AffinityGroupVMMapDao affinityGroupVMMapDao; + @Inject + VolumeDao volumeDao; + List drsAlgorithms = new ArrayList<>(); Map drsAlgorithmMap = new HashMap<>(); @@ -393,13 +399,22 @@ private List> getMigrationPlans( ClusterDrsAlgorithm algorithm = getDrsAlgorithm(ClusterDrsAlgorithm.valueIn(cluster.getId())); int iteration = 0; List> migrationPlan = new ArrayList<>(); + Map vmToExcludesMap = null; + Set staleAffinityVmIds = new HashSet<>(); while (iteration < maxIterations && algorithm.needsDrs(cluster, new ArrayList<>(hostCpuMap.values()), new ArrayList<>(hostMemoryMap.values()))) { logger.debug("Starting DRS iteration {} for cluster {}", iteration + 1, cluster); - // Re-evaluate affinity constraints with current (simulated) VM placements - Map vmToExcludesMap = getVmToExcludesMap(vmList, hostMap, vmsWithAffinityGroups, - vmToCompatibleHostsCache, vmIdServiceOfferingMap); + // Affinity only changes for VMs that share a group with the one just moved, so after + // the first pass only those are re-evaluated rather than every VM in the cluster + if (vmToExcludesMap == null) { + vmToExcludesMap = getVmToExcludesMap(vmList, hostMap, vmsWithAffinityGroups, + vmToCompatibleHostsCache, vmIdServiceOfferingMap, null); + } else if (!staleAffinityVmIds.isEmpty()) { + vmToExcludesMap.putAll(getVmToExcludesMap(vmList, hostMap, vmsWithAffinityGroups, + vmToCompatibleHostsCache, vmIdServiceOfferingMap, staleAffinityVmIds)); + staleAffinityVmIds = new HashSet<>(); + } logger.debug("Completed affinity evaluation for DRS iteration {} for cluster {}", iteration + 1, cluster); @@ -429,6 +444,7 @@ private List> getMigrationPlans( hostMemoryMap.get(vm.getHostId()).first(hostMemoryMap.get(vm.getHostId()).first() - vmMemory); hostMemoryMap.get(destHost.getId()).first(hostMemoryMap.get(destHost.getId()).first() + vmMemory); vm.setHostId(destHost.getId()); + staleAffinityVmIds = affinityPeersOf(vm, vmsWithAffinityGroups); iteration++; } return migrationPlan; @@ -468,11 +484,34 @@ protected void excludeHostsDispreferredByAffinity(DeploymentPlan plan, ExcludeLi } } + /** + * The VMs whose affinity picture changes when the given VM moves: the VM itself, and everything + * sharing an affinity group with it. + */ + protected Set affinityPeersOf(VirtualMachine vm, Set vmsWithAffinityGroups) { + Set peers = new HashSet<>(); + peers.add(vm.getId()); + if (!vmsWithAffinityGroups.contains(vm.getId())) { + return peers; + } + for (AffinityGroupVMMapVO mapping : affinityGroupVMMapDao.listByInstanceId(vm.getId())) { + peers.addAll(affinityGroupVMMapDao.listVmIdsByAffinityGroup(mapping.getAffinityGroupId())); + } + return peers; + } + + /** + * @param onlyVmIds + * when set, re-evaluates just these VMs rather than the whole cluster + */ private Map getVmToExcludesMap(List vmList, Map hostMap, Set vmsWithAffinityGroups, Map> vmToCompatibleHostsCache, - Map vmIdServiceOfferingMap) { + Map vmIdServiceOfferingMap, Set onlyVmIds) { Map vmToExcludesMap = new HashMap<>(); for (VirtualMachine vm : vmList) { + if (onlyVmIds != null && !onlyVmIds.contains(vm.getId())) { + continue; + } if (vmToCompatibleHostsCache.containsKey(vm.getId())) { Host srcHost = hostMap.get(vm.getHostId()); if (srcHost != null) { @@ -522,20 +561,39 @@ private Pair>, Map>> get .map(VMInstanceDetailVO::getResourceId) .collect(Collectors.toSet()); + // Working out where a VM could go is the expensive part of planning: it runs the host + // allocators and inspects every volume. VMs that would get the same answer are grouped and + // the work is done once for the group. In a cluster of similar VMs this is the difference + // between one pass per VM and one pass per handful. + Map, Integer>, List, Map>> byEquivalence = + new HashMap<>(); + int computed = 0; + for (VirtualMachine vm : vmList) { - // Skip ineligible VMs if (shouldSkipVMForDRS(vm, skipDrsVmIds)) { logger.debug("Skipping VM {} for DRS as it is ineligible.", vm); continue; } + // a VM whose key cannot be worked out is treated as one of a kind rather than dropped + String key; + try { + key = migrationEquivalenceKey(vm); + } catch (Exception e) { + logger.debug("Could not group VM {} with others, considering it on its own", vm, e); + key = "vm-" + vm.getId(); + } + try { - // Use listHostsForMigrationOfVM to get suitable hosts (validated by getCapableSuitableHosts) - // This ensures the same validation as the "find host for migration" command Ternary, Integer>, List, Map> hostsForMigration = - managementServer.listHostsForMigrationOfVM(vm, 0L, 500L, null, vmList); + byEquivalence.get(key); + if (hostsForMigration == null) { + hostsForMigration = managementServer.listHostsForMigrationOfVM(vm, 0L, 500L, null, vmList); + byEquivalence.put(key, hostsForMigration); + computed++; + } - List suitableHosts = hostsForMigration.second(); // Get suitable hosts (validated by HostAllocator) + List suitableHosts = hostsForMigration.second(); Map requiresStorageMotion = hostsForMigration.third(); if (suitableHosts != null && !suitableHosts.isEmpty()) { @@ -546,9 +604,31 @@ private Pair>, Map>> get logger.debug("Could not get suitable hosts for VM {}: {}", vm, e.getMessage()); } } + logger.debug("Worked out candidate hosts for {} VMs in {} passes", vmToCompatibleHostsCache.size(), computed); return new Pair<>(vmToCompatibleHostsCache, vmToStorageMotionCache); } + /** + * Identifies VMs that would get the same answer from listHostsForMigrationOfVM. + * + * The answer depends on what the VM asks for (its offering and template), where it is now, the + * volumes that would have to follow it, and the affinity groups it belongs to. Two VMs matching + * on all of those are interchangeable for the purpose of finding candidate hosts. + */ + protected String migrationEquivalenceKey(VirtualMachine vm) { + List poolIds = volumeDao.findCreatedByInstance(vm.getId()).stream() + .map(VolumeVO::getPoolId) + .filter(Objects::nonNull) + .sorted() + .collect(Collectors.toList()); + List groupIds = affinityGroupVMMapDao.listByInstanceId(vm.getId()).stream() + .map(AffinityGroupVMMapVO::getAffinityGroupId) + .sorted() + .collect(Collectors.toList()); + return String.format("%s|%s|%s|%s|%s|%s", vm.getServiceOfferingId(), vm.getTemplateId(), vm.getHostId(), + vm.getHypervisorType(), poolIds, groupIds); + } + /** * Pre-fetch affinity group mappings for all eligible VMs (once, before iterations) * This allows us to skip expensive affinity processing for VMs without affinity groups diff --git a/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsEquivalenceTest.java b/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsEquivalenceTest.java new file mode 100644 index 000000000000..f2725acb9ca8 --- /dev/null +++ b/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsEquivalenceTest.java @@ -0,0 +1,121 @@ +// 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.cluster; + +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +import org.apache.cloudstack.affinity.AffinityGroupVMMapVO; +import org.apache.cloudstack.affinity.dao.AffinityGroupVMMapDao; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.MockitoJUnitRunner; + +import com.cloud.hypervisor.Hypervisor; +import com.cloud.storage.VolumeVO; +import com.cloud.storage.dao.VolumeDao; +import com.cloud.vm.VMInstanceVO; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; + +/** + * Working out where a VM could go is the expensive part of DRS planning, so VMs that would get the + * same answer share the work. These check what counts as the same answer. + */ +@RunWith(MockitoJUnitRunner.class) +public class ClusterDrsEquivalenceTest { + + @Mock + private VolumeDao volumeDao; + + @Mock + private AffinityGroupVMMapDao affinityGroupVMMapDao; + + @InjectMocks + private ClusterDrsServiceImpl service = new ClusterDrsServiceImpl(); + + private VMInstanceVO vm(long id, long offeringId, long templateId, Long hostId, Long... poolIds) { + VMInstanceVO vm = Mockito.mock(VMInstanceVO.class); + Mockito.lenient().when(vm.getId()).thenReturn(id); + Mockito.lenient().when(vm.getServiceOfferingId()).thenReturn(offeringId); + Mockito.lenient().when(vm.getTemplateId()).thenReturn(templateId); + Mockito.lenient().when(vm.getHostId()).thenReturn(hostId); + Mockito.lenient().when(vm.getHypervisorType()).thenReturn(Hypervisor.HypervisorType.KVM); + + List volumes = Arrays.stream(poolIds).map(poolId -> { + VolumeVO volume = Mockito.mock(VolumeVO.class); + Mockito.lenient().when(volume.getPoolId()).thenReturn(poolId); + return volume; + }).collect(java.util.stream.Collectors.toList()); + Mockito.lenient().when(volumeDao.findCreatedByInstance(id)).thenReturn(volumes); + Mockito.lenient().when(affinityGroupVMMapDao.listByInstanceId(id)).thenReturn(Collections.emptyList()); + return vm; + } + + @Test + public void testIdenticalVmsShareOnePass() { + assertEquals(service.migrationEquivalenceKey(vm(1L, 10L, 20L, 30L, 40L)), + service.migrationEquivalenceKey(vm(2L, 10L, 20L, 30L, 40L))); + } + + @Test + public void testVolumeOrderDoesNotMatter() { + assertEquals(service.migrationEquivalenceKey(vm(1L, 10L, 20L, 30L, 40L, 41L)), + service.migrationEquivalenceKey(vm(2L, 10L, 20L, 30L, 41L, 40L))); + } + + @Test + public void testDifferentOfferingIsADifferentAnswer() { + assertNotEquals(service.migrationEquivalenceKey(vm(1L, 10L, 20L, 30L, 40L)), + service.migrationEquivalenceKey(vm(2L, 11L, 20L, 30L, 40L))); + } + + @Test + public void testDifferentCurrentHostIsADifferentAnswer() { + assertNotEquals(service.migrationEquivalenceKey(vm(1L, 10L, 20L, 30L, 40L)), + service.migrationEquivalenceKey(vm(2L, 10L, 20L, 31L, 40L))); + } + + @Test + public void testDifferentStorageIsADifferentAnswer() { + assertNotEquals(service.migrationEquivalenceKey(vm(1L, 10L, 20L, 30L, 40L)), + service.migrationEquivalenceKey(vm(2L, 10L, 20L, 30L, 42L))); + } + + @Test + public void testDifferentTemplateIsADifferentAnswer() { + assertNotEquals(service.migrationEquivalenceKey(vm(1L, 10L, 20L, 30L, 40L)), + service.migrationEquivalenceKey(vm(2L, 10L, 21L, 30L, 40L))); + } + + @Test + public void testDifferentAffinityGroupsIsADifferentAnswer() { + VMInstanceVO grouped = vm(1L, 10L, 20L, 30L, 40L); + AffinityGroupVMMapVO mapping = Mockito.mock(AffinityGroupVMMapVO.class); + Mockito.when(mapping.getAffinityGroupId()).thenReturn(99L); + Mockito.when(affinityGroupVMMapDao.listByInstanceId(1L)).thenReturn(Collections.singletonList(mapping)); + + assertNotEquals("a VM in an affinity group cannot reuse an ungrouped VM's candidate hosts", + service.migrationEquivalenceKey(grouped), + service.migrationEquivalenceKey(vm(2L, 10L, 20L, 30L, 40L))); + } +} 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 685ce8eb33b5..46362cdfd3b9 100644 --- a/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java @@ -38,6 +38,7 @@ import com.cloud.org.Grouping; import com.cloud.server.ManagementServer; import com.cloud.service.ServiceOfferingVO; +import com.cloud.storage.dao.VolumeDao; import org.apache.cloudstack.affinity.AffinityGroupVMMapVO; import com.cloud.service.dao.ServiceOfferingDao; import com.cloud.utils.Pair; @@ -136,6 +137,9 @@ public class ClusterDrsServiceImplTest { @Mock private AsyncJobManager asyncJobManager; + @Mock + private VolumeDao volumeDao; + @Mock private VMInstanceDetailsDao vmInstanceDetailsDao; From ca0412bb92a30daa7bf43f4f35a002929a93ff3e Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 9 Sep 2026 23:24:12 +0000 Subject: [PATCH 11/15] Add 'weighted' DRS algorithm Opt-in per cluster via drs.algorithm. balanced and condensed are untouched and the default is unchanged. The existing algorithms balance one metric chosen by drs.metric, so whichever is not chosen goes unwatched: a cluster can sit inside its imbalance threshold on memory while CPU load varies several fold, and DRS correctly does nothing. Allocation is also a poor stand-in for load under overprovisioning, where a saturated host still reports a small percentage allocated. 'weighted' blends four figures per host - CPU and memory allocated, CPU and memory in use - and balances the result. Imbalance keeps its existing definition, standard deviation over the mean, so drs.imbalance still means what it did. - allocation is measured against what a host can hand out, so the overcommit ratio is applied - a migration is only worth making if the cluster ends up more even, and one that has to move storage costs more - hosts without utilisation samples fall back to allocation figures needsDrs decides whether to look at a cluster at all, and it was handed lists with no host identity, so measured load could not be attributed to a host. Added a map form alongside it; algorithms that only need the values keep the default and are unaffected. Signed-off-by: Brad House --- PendingReleaseNotes | 18 ++ .../main/java/com/cloud/host}/HostLoad.java | 2 +- .../java/com/cloud/host/HostLoadService.java | 32 +++ .../cluster/ClusterDrsAlgorithm.java | 17 ++ .../cloudstack/cluster/ClusterDrsService.java | 4 +- client/pom.xml | 5 + plugins/drs/cluster/weighted/pom.xml | 33 +++ .../apache/cloudstack/cluster/Weighted.java | 245 ++++++++++++++++++ .../weighted/spring-weighted-context.xml | 33 +++ .../cloudstack/cluster/WeightedTest.java | 192 ++++++++++++++ plugins/pom.xml | 1 + .../allocator/impl/HostLoadTracker.java | 5 +- .../allocator/impl/WeightedHostScorer.java | 1 + .../cluster/ClusterDrsServiceImpl.java | 3 +- .../allocator/impl/HostLoadTrackerTest.java | 2 + .../impl/WeightedHostScorerRankTest.java | 1 + .../impl/WeightedHostScorerTest.java | 1 + .../WeightedPlacementDistributionTest.java | 2 + .../cluster/ClusterDrsServiceImplTest.java | 24 +- 19 files changed, 603 insertions(+), 18 deletions(-) rename {server/src/main/java/com/cloud/agent/manager/allocator/impl => api/src/main/java/com/cloud/host}/HostLoad.java (97%) create mode 100644 api/src/main/java/com/cloud/host/HostLoadService.java create mode 100644 plugins/drs/cluster/weighted/pom.xml create mode 100644 plugins/drs/cluster/weighted/src/main/java/org/apache/cloudstack/cluster/Weighted.java create mode 100644 plugins/drs/cluster/weighted/src/main/resources/META-INF/cloudstack/weighted/spring-weighted-context.xml create mode 100644 plugins/drs/cluster/weighted/src/test/java/org/apache/cloudstack/cluster/WeightedTest.java diff --git a/PendingReleaseNotes b/PendingReleaseNotes index c6992f2cf7d8..1dd89f858ddc 100644 --- a/PendingReleaseNotes +++ b/PendingReleaseNotes @@ -120,6 +120,24 @@ example.ver.1 > example.ver.2: host.load.half.life. Until a management server has collected samples, ranking falls back to allocation figures alone. + * New DRS algorithm 'weighted' for drs.algorithm, alongside 'balanced' and 'condensed'. The + default is unchanged; the new one is opt-in per cluster. + + The existing algorithms balance a single metric chosen by drs.metric, so choosing one leaves + the other unwatched: a cluster can be even on memory while its CPU load varies several fold and + nothing moves. Allocation is also a poor stand-in for load under overprovisioning, where a + saturated host can still report a small percentage allocated. + + 'weighted' blends CPU and memory allocated with CPU and memory in use, and balances the result. + Imbalance keeps its existing meaning - standard deviation over the mean - so drs.imbalance + still means what it did. Tuned with the drs.weighted.* settings, all cluster scoped. Where no + utilisation samples are available it falls back to allocation figures. + + * DRS plan generation is considerably faster on large clusters. Working out where a VM could go + is now done once for each group of VMs that would get the same answer rather than once per VM, + and affinity is re-evaluated only for VMs affected by the migration just planned rather than + for every VM on every iteration. + KVM console: * Added the global setting `consoleproxy.kvm.multiple.viewers.enabled` to allow multiple users to simultaneously access the console of the same KVM user VM. diff --git a/server/src/main/java/com/cloud/agent/manager/allocator/impl/HostLoad.java b/api/src/main/java/com/cloud/host/HostLoad.java similarity index 97% rename from server/src/main/java/com/cloud/agent/manager/allocator/impl/HostLoad.java rename to api/src/main/java/com/cloud/host/HostLoad.java index 2e3d14ba8384..a2257c51bccb 100644 --- a/server/src/main/java/com/cloud/agent/manager/allocator/impl/HostLoad.java +++ b/api/src/main/java/com/cloud/host/HostLoad.java @@ -14,7 +14,7 @@ // KIND, either express or implied. See the License for the // specific language governing permissions and limitations // under the License. -package com.cloud.agent.manager.allocator.impl; +package com.cloud.host; /** * Smoothed view of what a host is actually doing, as opposed to what has been allocated on it. diff --git a/api/src/main/java/com/cloud/host/HostLoadService.java b/api/src/main/java/com/cloud/host/HostLoadService.java new file mode 100644 index 000000000000..d8e9e88cb27a --- /dev/null +++ b/api/src/main/java/com/cloud/host/HostLoadService.java @@ -0,0 +1,32 @@ +// 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 com.cloud.host; + +/** + * A smoothed view of what each host is actually doing, as opposed to what has been allocated on it. + * + * Allocation figures say what was promised; under overprovisioning they can be far from what a host + * is really carrying. Placement and rebalancing both need the second view. + */ +public interface HostLoadService { + + /** + * @return the host's smoothed load, or a value reporting itself unusable when the host has not + * been sampled recently enough to rank on + */ + HostLoad getLoad(long hostId); +} diff --git a/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsAlgorithm.java b/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsAlgorithm.java index 368487c2b9b0..722247e6fc51 100644 --- a/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsAlgorithm.java +++ b/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsAlgorithm.java @@ -62,6 +62,23 @@ public interface ClusterDrsAlgorithm extends Adapter { boolean needsDrs(Cluster cluster, List> cpuList, List> memoryList) throws ConfigurationException; + /** + * As above, but keeping host identity. + * + * An algorithm that considers anything beyond the figures in the two maps - measured load, for + * instance - cannot attribute it without knowing which host each entry belongs to. Algorithms + * that only need the values keep the default. + * + * @param hostCpuMap + * host id to a Ternary of used, reserved and total CPU + * @param hostMemoryMap + * host id to a Ternary of used, reserved and total memory + */ + default boolean needsDrs(Cluster cluster, Map> hostCpuMap, + Map> hostMemoryMap) throws ConfigurationException { + return needsDrs(cluster, new ArrayList<>(hostCpuMap.values()), new ArrayList<>(hostMemoryMap.values())); + } + /** * Calculates the metrics (improvement, cost, benefit) for migrating a VM to a destination host. Improvement is * calculated based on the change in cluster imbalance before and after the migration. diff --git a/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsService.java b/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsService.java index ba6a6464fc20..7323f7fb4916 100644 --- a/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsService.java +++ b/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsService.java @@ -51,9 +51,9 @@ public interface ClusterDrsService extends Manager, Configurable, Scheduler { true, ConfigKey.Scope.Cluster, null, "Maximum number of migrations for DRS", null, null, null); ConfigKey ClusterDrsAlgorithm = new ConfigKey<>(String.class, "drs.algorithm", - ConfigKey.CATEGORY_ADVANCED, "balanced", "The DRS algorithm to be executed on the cluster. Possible values are condensed, balanced.", + ConfigKey.CATEGORY_ADVANCED, "balanced", "The DRS algorithm to be executed on the cluster. Possible values are condensed, balanced, weighted. 'weighted' balances CPU and memory together, and on measured load as well as allocation, tuned with the drs.weighted.* settings.", true, ConfigKey.Scope.Cluster, null, "DRS algorithm", null, null, - null, ConfigKey.Kind.Select, "condensed,balanced"); + null, ConfigKey.Kind.Select, "condensed,balanced,weighted"); ConfigKey ClusterDrsImbalanceThreshold = new ConfigKey<>(Float.class, "drs.imbalance", ConfigKey.CATEGORY_ADVANCED, "0.4", diff --git a/client/pom.xml b/client/pom.xml index cc031a4912b1..ebbf812823ca 100644 --- a/client/pom.xml +++ b/client/pom.xml @@ -592,6 +592,11 @@ cloud-plugin-cluster-drs-condensed ${project.version} + + org.apache.cloudstack + cloud-plugin-cluster-drs-weighted + ${project.version} + org.apache.cloudstack cloud-plugin-database-quota diff --git a/plugins/drs/cluster/weighted/pom.xml b/plugins/drs/cluster/weighted/pom.xml new file mode 100644 index 000000000000..4c0fa3e6a902 --- /dev/null +++ b/plugins/drs/cluster/weighted/pom.xml @@ -0,0 +1,33 @@ + + + + + 4.0.0 + Apache CloudStack Plugin - Cluster DRS Algorithm - Weighted + cloud-plugin-cluster-drs-weighted + + org.apache.cloudstack + cloudstack-plugins + 24.0.0-SNAPSHOT + ../../../pom.xml + + diff --git a/plugins/drs/cluster/weighted/src/main/java/org/apache/cloudstack/cluster/Weighted.java b/plugins/drs/cluster/weighted/src/main/java/org/apache/cloudstack/cluster/Weighted.java new file mode 100644 index 000000000000..f136bfa1ad9d --- /dev/null +++ b/plugins/drs/cluster/weighted/src/main/java/org/apache/cloudstack/cluster/Weighted.java @@ -0,0 +1,245 @@ +// 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.cluster; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import javax.inject.Inject; +import javax.naming.ConfigurationException; + +import org.apache.cloudstack.framework.config.ConfigKey; +import org.apache.cloudstack.framework.config.Configurable; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +import com.cloud.dc.ClusterDetailsDao; +import com.cloud.dc.ClusterDetailsVO; +import com.cloud.host.Host; +import com.cloud.host.HostLoad; +import com.cloud.host.HostScoringWeights; +import com.cloud.host.HostLoadService; +import com.cloud.offering.ServiceOffering; +import com.cloud.org.Cluster; +import com.cloud.utils.Ternary; +import com.cloud.utils.component.AdapterBase; +import com.cloud.vm.VirtualMachine; +import com.cloud.vm.VmDetailConstants; + +/** + * Balances a cluster on CPU and memory together, and on what hosts are really doing rather than + * only on what has been allocated to them. + * + * The existing algorithms balance a single metric chosen by drs.metric. Choosing one leaves the + * other unwatched: a cluster can be even on memory while its CPU load varies several fold, and + * nothing moves. Allocation is also a poor stand-in for load under overprovisioning, where a + * saturated host can still report a small percentage allocated. + * + * This blends four figures per host - CPU and memory allocated, CPU and memory in use - and + * balances the result. The weights are the same host.weighted.* settings initial placement uses, on + * purpose: if the two weighted them differently they would disagree about which host is the better + * one, and rebalancing could move VMs off hosts that placement had just chosen. Imbalance keeps the same meaning as the other algorithms: the standard + * deviation of the per-host figure over its mean, so drs.imbalance still means what it did. + */ +public class Weighted extends AdapterBase implements ClusterDrsAlgorithm, Configurable { + + private static final Logger LOGGER = LogManager.getLogger(Weighted.class); + + @Inject + private HostLoadService hostLoadService; + + @Inject + private ClusterDetailsDao clusterDetailsDao; + + @Override + public String getName() { + return "weighted"; + } + + @Override + public boolean needsDrs(Cluster cluster, List> cpuList, + List> memoryList) throws ConfigurationException { + // without host identity, measured load cannot be attributed; the map form is what DRS calls + Map> cpuMap = new HashMap<>(); + Map> memoryMap = new HashMap<>(); + for (int i = 0; i < cpuList.size() && i < memoryList.size(); i++) { + cpuMap.put((long) -(i + 1), cpuList.get(i)); + memoryMap.put((long) -(i + 1), memoryList.get(i)); + } + return needsDrs(cluster, cpuMap, memoryMap); + } + + @Override + public boolean needsDrs(Cluster cluster, Map> hostCpuMap, + Map> hostMemoryMap) throws ConfigurationException { + double threshold = 1.0 - ClusterDrsService.ClusterDrsImbalanceThreshold.valueIn(cluster.getId()); + double imbalance = imbalanceOf(blendByHost(cluster, hostCpuMap, hostMemoryMap).values()); + boolean needed = imbalance > threshold; + LOGGER.debug("Cluster {} {} DRS. Imbalance: {} Threshold: {} Algorithm: {}", + cluster, needed ? "needs" : "does not need", imbalance, threshold, getName()); + return needed; + } + + @Override + public Ternary getMetrics(Cluster cluster, VirtualMachine vm, + ServiceOffering serviceOffering, Host destHost, + Map> hostCpuMap, Map> hostMemoryMap, + Boolean requiresStorageMotion, Double preImbalance, + double[] baseMetricsArray, Map hostIdToIndexMap) throws ConfigurationException { + + double before = imbalanceOf(blendByHost(cluster, hostCpuMap, hostMemoryMap).values()); + + long vmCpu = (long) serviceOffering.getCpu() * serviceOffering.getSpeed(); + long vmMemory = serviceOffering.getRamSize() * 1024L * 1024L; + Map> cpuAfter = withVmMoved(hostCpuMap, vm.getHostId(), destHost.getId(), vmCpu); + Map> memoryAfter = withVmMoved(hostMemoryMap, vm.getHostId(), destHost.getId(), vmMemory); + + double after = imbalanceOf(blendByHost(cluster, cpuAfter, memoryAfter).values()); + + double improvement = before - after; + // moving a VM costs something and buys nothing unless the cluster ends up more even, so a + // migration is only worth making when it measurably helps + double cost = Boolean.TRUE.equals(requiresStorageMotion) ? 1.0 : 0.0; + double benefit = improvement > 0 ? 1.0 + improvement : 0.0; + + LOGGER.trace("Cluster {} imbalance {} -> {} moving {} to {}", cluster, before, after, vm, destHost); + return new Ternary<>(improvement, cost, benefit); + } + + /** + * One figure per host, blending what is allocated with what is in use. + */ + protected Map blendByHost(Cluster cluster, Map> hostCpuMap, + Map> hostMemoryMap) { + float cpuOvercommit = overcommitRatio(cluster.getId(), VmDetailConstants.CPU_OVER_COMMIT_RATIO); + float memoryOvercommit = overcommitRatio(cluster.getId(), VmDetailConstants.MEMORY_OVER_COMMIT_RATIO); + + double cpuAllocatedWeight = weight(HostScoringWeights.CpuAllocatedWeight, cluster.getId()); + double memoryAllocatedWeight = weight(HostScoringWeights.MemoryAllocatedWeight, cluster.getId()); + double cpuUsedWeight = weight(HostScoringWeights.CpuUsedWeight, cluster.getId()); + double memoryUsedWeight = weight(HostScoringWeights.MemoryUsedWeight, cluster.getId()); + + Map blended = new HashMap<>(); + for (Map.Entry> entry : hostCpuMap.entrySet()) { + long hostId = entry.getKey(); + Ternary memory = hostMemoryMap.get(hostId); + if (memory == null) { + continue; + } + double cpuAllocated = fractionOf(entry.getValue(), cpuOvercommit); + double memoryAllocated = fractionOf(memory, memoryOvercommit); + + HostLoad load = hostLoadService == null ? HostLoad.UNKNOWN : hostLoadService.getLoad(hostId); + double usedCpuWeight = load.isUsable() ? cpuUsedWeight : 0; + double usedMemoryWeight = load.isUsable() ? memoryUsedWeight : 0; + + double sum = cpuAllocatedWeight + memoryAllocatedWeight + usedCpuWeight + usedMemoryWeight; + if (sum <= 0) { + blended.put(hostId, 0.0); + continue; + } + blended.put(hostId, (cpuAllocatedWeight * cpuAllocated + + memoryAllocatedWeight * memoryAllocated + + usedCpuWeight * load.getCpuUtilisation() + + usedMemoryWeight * load.getMemoryUtilisation()) / sum); + } + return blended; + } + + private Map> withVmMoved(Map> original, + Long sourceHostId, long destHostId, long amount) { + Map> copy = new HashMap<>(); + for (Map.Entry> entry : original.entrySet()) { + Ternary value = entry.getValue(); + long used = value.first(); + if (entry.getKey().equals(sourceHostId)) { + used -= amount; + } else if (entry.getKey() == destHostId) { + used += amount; + } + copy.put(entry.getKey(), new Ternary<>(used, value.second(), value.third())); + } + return copy; + } + + /** + * Used over what the host can hand out, which is its real total scaled by the overcommit ratio. + */ + private double fractionOf(Ternary capacity, float overcommit) { + double allocatable = (capacity.third() - capacity.second()) * (double) overcommit; + if (allocatable <= 0) { + return 0; + } + return clamp(capacity.first() / allocatable); + } + + protected float overcommitRatio(long clusterId, String key) { + ClusterDetailsVO detail = clusterDetailsDao.findDetail(clusterId, key); + if (detail == null || detail.getValue() == null) { + return 1f; + } + try { + float ratio = Float.parseFloat(detail.getValue()); + return ratio > 0 ? ratio : 1f; + } catch (NumberFormatException e) { + return 1f; + } + } + + private double weight(ConfigKey key, long clusterId) { + Double value = key.valueIn(clusterId); + if (value == null || value < 0) { + return 0; + } + return value; + } + + /** + * Standard deviation over the mean, the same definition the other algorithms use, so that + * drs.imbalance keeps its meaning. + */ + protected double imbalanceOf(java.util.Collection values) { + if (values == null || values.isEmpty()) { + return 0; + } + double[] array = values.stream().mapToDouble(Double::doubleValue).toArray(); + double mean = MEAN_CALCULATOR.evaluate(array); + if (mean == 0) { + return 0; + } + return STDDEV_CALCULATOR.evaluate(array, mean) / mean; + } + + private static double clamp(double value) { + if (Double.isNaN(value) || value < 0) { + return 0; + } + return Math.min(value, 1); + } + + @Override + public String getConfigComponentName() { + return Weighted.class.getSimpleName(); + } + + @Override + public ConfigKey[] getConfigKeys() { + // the four host.weighted.* weights are shared with initial placement, which registers them + return new ConfigKey[] {StorageMotionCost}; + } +} diff --git a/plugins/drs/cluster/weighted/src/main/resources/META-INF/cloudstack/weighted/spring-weighted-context.xml b/plugins/drs/cluster/weighted/src/main/resources/META-INF/cloudstack/weighted/spring-weighted-context.xml new file mode 100644 index 000000000000..903ae08087a1 --- /dev/null +++ b/plugins/drs/cluster/weighted/src/main/resources/META-INF/cloudstack/weighted/spring-weighted-context.xml @@ -0,0 +1,33 @@ + + + + + + + diff --git a/plugins/drs/cluster/weighted/src/test/java/org/apache/cloudstack/cluster/WeightedTest.java b/plugins/drs/cluster/weighted/src/test/java/org/apache/cloudstack/cluster/WeightedTest.java new file mode 100644 index 000000000000..f3c0fafbbec5 --- /dev/null +++ b/plugins/drs/cluster/weighted/src/test/java/org/apache/cloudstack/cluster/WeightedTest.java @@ -0,0 +1,192 @@ +// 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.cluster; + +import java.util.HashMap; +import java.util.Map; + +import javax.naming.ConfigurationException; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.MockitoJUnitRunner; + +import com.cloud.dc.ClusterDetailsDao; +import com.cloud.dc.ClusterDetailsVO; +import com.cloud.host.Host; +import com.cloud.host.HostLoad; +import com.cloud.host.HostLoadService; +import com.cloud.offering.ServiceOffering; +import com.cloud.org.Cluster; +import com.cloud.utils.Ternary; +import com.cloud.vm.VirtualMachine; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +@RunWith(MockitoJUnitRunner.class) +public class WeightedTest { + + private static final long CLUSTER_ID = 1L; + private static final long CPU_TOTAL = 192L * 2400L; + private static final long MEMORY_TOTAL = 1_132_000L * 1024L * 1024L; + private static final int CPU_OVERCOMMIT = 10; + private static final int MEMORY_OVERCOMMIT = 4; + + @Mock + private HostLoadService hostLoadService; + + @Mock + private ClusterDetailsDao clusterDetailsDao; + + @InjectMocks + private Weighted weighted = new Weighted(); + + private Cluster cluster; + private final Map> cpu = new HashMap<>(); + private final Map> memory = new HashMap<>(); + + @Before + public void setUp() { + cluster = Mockito.mock(Cluster.class); + Mockito.lenient().when(cluster.getId()).thenReturn(CLUSTER_ID); + Mockito.lenient().when(clusterDetailsDao.findDetail(CLUSTER_ID, "cpuOvercommitRatio")) + .thenReturn(new ClusterDetailsVO(CLUSTER_ID, "cpuOvercommitRatio", String.valueOf(CPU_OVERCOMMIT))); + Mockito.lenient().when(clusterDetailsDao.findDetail(CLUSTER_ID, "memoryOvercommitRatio")) + .thenReturn(new ClusterDetailsVO(CLUSTER_ID, "memoryOvercommitRatio", String.valueOf(MEMORY_OVERCOMMIT))); + } + + private void host(long id, double cpuAllocatedFraction, double memoryAllocatedFraction, HostLoad load) { + cpu.put(id, new Ternary<>((long) (CPU_TOTAL * CPU_OVERCOMMIT * cpuAllocatedFraction), 0L, CPU_TOTAL)); + memory.put(id, new Ternary<>((long) (MEMORY_TOTAL * MEMORY_OVERCOMMIT * memoryAllocatedFraction), 0L, MEMORY_TOTAL)); + Mockito.lenient().when(hostLoadService.getLoad(id)).thenReturn(load); + } + + @Test + public void testAnEvenClusterHasNoImbalance() { + host(1L, 0.4, 0.4, new HostLoad(0.4, 0.4, 10)); + host(2L, 0.4, 0.4, new HostLoad(0.4, 0.4, 10)); + + assertEquals(0.0, weighted.imbalanceOf(weighted.blendByHost(cluster, cpu, memory).values()), 1e-9); + } + + @Test + public void testCpuLoadImbalanceIsSeenWhenMemoryIsEven() { + // the case a memory-only metric misses entirely: memory even, CPU load three fold apart + host(1L, 0.30, 0.40, new HostLoad(0.90, 0.40, 10)); + host(2L, 0.30, 0.40, new HostLoad(0.30, 0.40, 10)); + + assertTrue("balancing on memory alone would call this cluster even", + weighted.imbalanceOf(weighted.blendByHost(cluster, cpu, memory).values()) > 0.15); + } + + @Test + public void testMemoryImbalanceIsSeenWhenCpuIsEven() { + host(1L, 0.30, 0.90, new HostLoad(0.30, 0.90, 10)); + host(2L, 0.30, 0.10, new HostLoad(0.30, 0.10, 10)); + + assertTrue(weighted.imbalanceOf(weighted.blendByHost(cluster, cpu, memory).values()) > 0.15); + } + + @Test + public void testAllocationBeyondPhysicalSizeStillDiscriminates() { + // at a factor of 10 both hosts are past their physical CPU; they must not both read as full + host(1L, 0.20, 0.20, HostLoad.UNKNOWN); + host(2L, 0.80, 0.20, HostLoad.UNKNOWN); + + Map blended = weighted.blendByHost(cluster, cpu, memory); + assertTrue(blended.get(2L) > blended.get(1L)); + } + + @Test + public void testUnmeasuredHostsFallBackToAllocation() { + host(1L, 0.20, 0.20, HostLoad.UNKNOWN); + host(2L, 0.60, 0.60, HostLoad.UNKNOWN); + + Map blended = weighted.blendByHost(cluster, cpu, memory); + assertEquals(0.20, blended.get(1L), 0.01); + assertEquals(0.60, blended.get(2L), 0.01); + } + + @Test + public void testMovingAVmOffTheBusyHostIsAnImprovement() throws ConfigurationException { + host(1L, 0.60, 0.60, new HostLoad(0.80, 0.60, 10)); + host(2L, 0.20, 0.20, new HostLoad(0.20, 0.20, 10)); + + VirtualMachine vm = Mockito.mock(VirtualMachine.class); + Mockito.when(vm.getHostId()).thenReturn(1L); + Host dest = Mockito.mock(Host.class); + Mockito.when(dest.getId()).thenReturn(2L); + ServiceOffering offering = Mockito.mock(ServiceOffering.class); + Mockito.when(offering.getCpu()).thenReturn(8); + Mockito.when(offering.getSpeed()).thenReturn(2400); + Mockito.when(offering.getRamSize()).thenReturn(32768); + + Ternary metrics = weighted.getMetrics(cluster, vm, offering, dest, + cpu, memory, false, null, new double[0], new HashMap<>()); + + assertTrue("moving away from the busier host should even the cluster out", metrics.first() > 0); + assertTrue("and should be considered worth doing", metrics.third() > metrics.second()); + } + + @Test + public void testMovingAVmOntoTheBusyHostIsNotAnImprovement() throws ConfigurationException { + host(1L, 0.60, 0.60, new HostLoad(0.80, 0.60, 10)); + host(2L, 0.20, 0.20, new HostLoad(0.20, 0.20, 10)); + + VirtualMachine vm = Mockito.mock(VirtualMachine.class); + Mockito.when(vm.getHostId()).thenReturn(2L); + Host dest = Mockito.mock(Host.class); + Mockito.when(dest.getId()).thenReturn(1L); + ServiceOffering offering = Mockito.mock(ServiceOffering.class); + Mockito.when(offering.getCpu()).thenReturn(8); + Mockito.when(offering.getSpeed()).thenReturn(2400); + Mockito.when(offering.getRamSize()).thenReturn(32768); + + Ternary metrics = weighted.getMetrics(cluster, vm, offering, dest, + cpu, memory, false, null, new double[0], new HashMap<>()); + + assertTrue("piling onto the busier host makes the cluster less even", metrics.first() < 0); + assertEquals("and must not be considered worth doing", 0.0, metrics.third(), 1e-9); + } + + @Test + public void testStorageMotionCostsMore() throws ConfigurationException { + host(1L, 0.60, 0.60, new HostLoad(0.80, 0.60, 10)); + host(2L, 0.20, 0.20, new HostLoad(0.20, 0.20, 10)); + + VirtualMachine vm = Mockito.mock(VirtualMachine.class); + Mockito.when(vm.getHostId()).thenReturn(1L); + Host dest = Mockito.mock(Host.class); + Mockito.when(dest.getId()).thenReturn(2L); + ServiceOffering offering = Mockito.mock(ServiceOffering.class); + Mockito.when(offering.getCpu()).thenReturn(8); + Mockito.when(offering.getSpeed()).thenReturn(2400); + Mockito.when(offering.getRamSize()).thenReturn(32768); + + double withoutStorage = weighted.getMetrics(cluster, vm, offering, dest, cpu, memory, false, + null, new double[0], new HashMap<>()).second(); + double withStorage = weighted.getMetrics(cluster, vm, offering, dest, cpu, memory, true, + null, new double[0], new HashMap<>()).second(); + + assertTrue("a migration that has to move storage should cost more", withStorage > withoutStorage); + } +} diff --git a/plugins/pom.xml b/plugins/pom.xml index 92768827f658..08b906e9a6b6 100755 --- a/plugins/pom.xml +++ b/plugins/pom.xml @@ -77,6 +77,7 @@ drs/cluster/balanced drs/cluster/condensed + drs/cluster/weighted event-bus/inmemory event-bus/kafka diff --git a/server/src/main/java/com/cloud/agent/manager/allocator/impl/HostLoadTracker.java b/server/src/main/java/com/cloud/agent/manager/allocator/impl/HostLoadTracker.java index 4543af9b8b14..cd51ae2d8686 100644 --- a/server/src/main/java/com/cloud/agent/manager/allocator/impl/HostLoadTracker.java +++ b/server/src/main/java/com/cloud/agent/manager/allocator/impl/HostLoadTracker.java @@ -28,6 +28,8 @@ import org.apache.cloudstack.framework.config.Configurable; import org.apache.cloudstack.managed.context.ManagedContextRunnable; +import com.cloud.host.HostLoad; +import com.cloud.host.HostLoadService; import com.cloud.host.HostStats; import com.cloud.host.HostVO; import com.cloud.host.Status; @@ -63,7 +65,7 @@ * * Memory is taken as used over total and is sound everywhere. */ -public class HostLoadTracker extends ManagerBase implements Configurable { +public class HostLoadTracker extends ManagerBase implements HostLoadService, Configurable { public static final ConfigKey HostLoadSampleInterval = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED, Integer.class, "host.load.sample.interval", "60", @@ -175,6 +177,7 @@ protected void record(long hostId, HostStats stats, long now) { : current.fold(cpu, memory, now, halfLife, stats)); } + @Override public HostLoad getLoad(long hostId) { return getLoad(hostId, System.currentTimeMillis()); } diff --git a/server/src/main/java/com/cloud/agent/manager/allocator/impl/WeightedHostScorer.java b/server/src/main/java/com/cloud/agent/manager/allocator/impl/WeightedHostScorer.java index 581c2052ffc8..9e236262f0ac 100644 --- a/server/src/main/java/com/cloud/agent/manager/allocator/impl/WeightedHostScorer.java +++ b/server/src/main/java/com/cloud/agent/manager/allocator/impl/WeightedHostScorer.java @@ -39,6 +39,7 @@ import com.cloud.dc.ClusterDetailsDao; import com.cloud.dc.ClusterDetailsVO; import com.cloud.host.Host; +import com.cloud.host.HostLoad; import com.cloud.host.HostScoringWeights; import com.cloud.utils.Ternary; import com.cloud.utils.component.AdapterBase; 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 614f56f7c070..8e0c0e5f2ff5 100644 --- a/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java +++ b/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java @@ -401,8 +401,7 @@ private List> getMigrationPlans( List> migrationPlan = new ArrayList<>(); Map vmToExcludesMap = null; Set staleAffinityVmIds = new HashSet<>(); - while (iteration < maxIterations && algorithm.needsDrs(cluster, new ArrayList<>(hostCpuMap.values()), - new ArrayList<>(hostMemoryMap.values()))) { + while (iteration < maxIterations && algorithm.needsDrs(cluster, hostCpuMap, hostMemoryMap)) { logger.debug("Starting DRS iteration {} for cluster {}", iteration + 1, cluster); // Affinity only changes for VMs that share a group with the one just moved, so after diff --git a/server/src/test/java/com/cloud/agent/manager/allocator/impl/HostLoadTrackerTest.java b/server/src/test/java/com/cloud/agent/manager/allocator/impl/HostLoadTrackerTest.java index 0be9b1310f04..0f8b38f695ea 100644 --- a/server/src/test/java/com/cloud/agent/manager/allocator/impl/HostLoadTrackerTest.java +++ b/server/src/test/java/com/cloud/agent/manager/allocator/impl/HostLoadTrackerTest.java @@ -17,6 +17,8 @@ package com.cloud.agent.manager.allocator.impl; import org.junit.Before; +import com.cloud.host.HostLoad; + import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.InjectMocks; diff --git a/server/src/test/java/com/cloud/agent/manager/allocator/impl/WeightedHostScorerRankTest.java b/server/src/test/java/com/cloud/agent/manager/allocator/impl/WeightedHostScorerRankTest.java index a597f3922c45..b597356a7c25 100644 --- a/server/src/test/java/com/cloud/agent/manager/allocator/impl/WeightedHostScorerRankTest.java +++ b/server/src/test/java/com/cloud/agent/manager/allocator/impl/WeightedHostScorerRankTest.java @@ -41,6 +41,7 @@ import com.cloud.dc.ClusterDetailsDao; import com.cloud.dc.ClusterDetailsVO; import com.cloud.host.Host; +import com.cloud.host.HostLoad; import com.cloud.utils.Ternary; import com.cloud.vm.dao.VMInstanceDao; diff --git a/server/src/test/java/com/cloud/agent/manager/allocator/impl/WeightedHostScorerTest.java b/server/src/test/java/com/cloud/agent/manager/allocator/impl/WeightedHostScorerTest.java index d5097d79d21a..ecbf36d831e5 100644 --- a/server/src/test/java/com/cloud/agent/manager/allocator/impl/WeightedHostScorerTest.java +++ b/server/src/test/java/com/cloud/agent/manager/allocator/impl/WeightedHostScorerTest.java @@ -30,6 +30,7 @@ import org.mockito.junit.MockitoJUnitRunner; import com.cloud.host.Host; +import com.cloud.host.HostLoad; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; diff --git a/server/src/test/java/com/cloud/agent/manager/allocator/impl/WeightedPlacementDistributionTest.java b/server/src/test/java/com/cloud/agent/manager/allocator/impl/WeightedPlacementDistributionTest.java index 22fa6c565839..de057c0b3831 100644 --- a/server/src/test/java/com/cloud/agent/manager/allocator/impl/WeightedPlacementDistributionTest.java +++ b/server/src/test/java/com/cloud/agent/manager/allocator/impl/WeightedPlacementDistributionTest.java @@ -22,6 +22,8 @@ import java.util.List; import java.util.Random; +import com.cloud.host.HostLoad; + import org.junit.Test; import static org.junit.Assert.assertTrue; 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 46362cdfd3b9..a31ce47e8766 100644 --- a/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java @@ -231,7 +231,7 @@ public void testGetDrsPlan() throws ConfigurationException { 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( + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap())).thenReturn( true, false); Mockito.doReturn(new Pair<>(vm1, host2)).when(clusterDrsService).getBestMigration( @@ -246,8 +246,8 @@ public void testGetDrsPlan() throws ConfigurationException { Mockito.verify(hostDao, Mockito.times(1)).findByClusterId(1L); Mockito.verify(vmInstanceDao, Mockito.times(1)).listByClusterId(1L); - Mockito.verify(balancedAlgorithm, Mockito.times(2)).needsDrs(Mockito.any(), Mockito.anyList(), - Mockito.anyList()); + Mockito.verify(balancedAlgorithm, Mockito.times(2)).needsDrs(Mockito.any(), Mockito.anyMap(), + Mockito.anyMap()); assertEquals(1, iterations.size()); } @@ -311,7 +311,7 @@ public void testGetDrsPlanWithSystemVMs() throws ConfigurationException { 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); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap())).thenReturn(true); Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); List> result = clusterDrsService.getDrsPlan(cluster, 5); @@ -353,7 +353,7 @@ public void testGetDrsPlanWithNonRunningVMs() throws ConfigurationException { 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); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap())).thenReturn(true); Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); List> result = clusterDrsService.getDrsPlan(cluster, 5); @@ -400,7 +400,7 @@ public void testGetDrsPlanWithSkipDrsFlag() throws ConfigurationException { 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); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap())).thenReturn(true); Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); List> result = clusterDrsService.getDrsPlan(cluster, 5); @@ -445,7 +445,7 @@ public void testGetDrsPlanWithNoCompatibleHosts() throws ConfigurationException 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); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap())).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 @@ -493,7 +493,7 @@ public void testGetDrsPlanWithExceptionInCompatibilityCheck() throws Configurati 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); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap())).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 @@ -542,7 +542,7 @@ public void testGetDrsPlanWithNoBestMigration() throws ConfigurationException { 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); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap())).thenReturn(true); Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(Mockito.anyLong(), Mockito.anyLong())).thenReturn(serviceOffering); Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); @@ -617,7 +617,7 @@ public void testGetDrsPlanWithMultipleIterations() throws ConfigurationException 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( + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap())).thenReturn( true, true, false); Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(Mockito.anyLong(), Mockito.anyLong())).thenReturn(serviceOffering); Mockito.when(hostJoinDao.searchByIds(1L, 2L)).thenReturn(List.of(hostJoin1, hostJoin2)); @@ -631,7 +631,7 @@ public void testGetDrsPlanWithMultipleIterations() throws ConfigurationException List> result = clusterDrsService.getDrsPlan(cluster, 5); assertEquals(2, result.size()); - Mockito.verify(balancedAlgorithm, Mockito.times(3)).needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList()); + Mockito.verify(balancedAlgorithm, Mockito.times(3)).needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap()); } @Test @@ -663,7 +663,7 @@ public void testGetDrsPlanWithMigrationToOriginalHost() throws ConfigurationExce 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); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap())).thenReturn(true); Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(Mockito.anyLong(), Mockito.anyLong())).thenReturn(serviceOffering); // Return migration to original host (host1) - should break the loop From ff30ba1c90b5d94fda09f2e81cbd177492f46f7e Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 10 Sep 2026 00:21:34 +0000 Subject: [PATCH 12/15] Fix defects found in review of the weighted DRS algorithm Two of these meant the algorithm could not run at all. The plugin had no module.properties, so it was never discovered, never instantiated and never registered. Selecting it would have failed every DRS run with "Invalid algorithm configured". The plugin injected a service that lives in a sibling Spring module and is therefore not visible to it, so once the module did load, the management server would fail to start. - the caller now works out the load once per plan and passes it in - which also fixes the cost of getting it: getMetrics runs for every candidate VM and host, and it was reading two cluster details and four settings from the database on each call Grouping VMs to share one candidate-host lookup was unsound. The key listed the inputs it thought mattered, and missed several that are held per VM rather than per offering - a custom offering's size, boot mode, device settings. Two VMs could then share a host list neither of them should have had. - only group VMs that carry none of those, rather than trying to enumerate everything that could matter - key on disk offering as well as pool, since storage tags come from the offering Reserved capacity was multiplied by the overcommit ratio instead of being subtracted from the scaled total, so a host reading 11% fuller than its peer read 67% fuller, and DRS would evacuate hosts merely for holding reserved capacity. Hosts were compared on different bases. A host with no utilisation samples was measured on allocation alone while its peers were measured on a blend, reporting a difference that was an artefact of the monitoring. Utilisation is now used only when every host has it. The cost and benefit terms cancelled out, so storage motion was free despite the claim otherwise. A migration that has to move storage now has to earn more than one that does not, by a configurable margin. Also: allocation beyond what a host can hand out is no longer flattened to a single value, and the settings that shape the other algorithms' single metric are documented as not applying here. Signed-off-by: Brad House --- PendingReleaseNotes | 15 +- .../cluster/ClusterDrsAlgorithm.java | 15 +- .../apache/cloudstack/cluster/Weighted.java | 133 ++++++++++++++---- .../cloudstack/weighted/module.properties | 18 +++ .../cloudstack/cluster/WeightedTest.java | 121 ++++++++++++++-- .../cluster/ClusterDrsServiceImpl.java | 65 +++++++-- .../cluster/ClusterDrsEquivalenceTest.java | 68 ++++++++- .../cluster/ClusterDrsServiceImplTest.java | 26 ++-- 8 files changed, 391 insertions(+), 70 deletions(-) create mode 100644 plugins/drs/cluster/weighted/src/main/resources/META-INF/cloudstack/weighted/module.properties diff --git a/PendingReleaseNotes b/PendingReleaseNotes index 1dd89f858ddc..62d9712c95b3 100644 --- a/PendingReleaseNotes +++ b/PendingReleaseNotes @@ -129,9 +129,18 @@ example.ver.1 > example.ver.2: saturated host can still report a small percentage allocated. 'weighted' blends CPU and memory allocated with CPU and memory in use, and balances the result. - Imbalance keeps its existing meaning - standard deviation over the mean - so drs.imbalance - still means what it did. Tuned with the drs.weighted.* settings, all cluster scoped. Where no - utilisation samples are available it falls back to allocation figures. + Imbalance keeps its existing shape - standard deviation over the mean - but it is computed over + a blend rather than over one metric, so drs.imbalance is not calibrated the same way and is + worth re-checking after switching. drs.metric, drs.metric.type and drs.metric.use.ratio choose + and shape the single metric the other algorithms balance; they do not apply to 'weighted' and + are ignored. + + Utilisation is only used when every host in the cluster has been sampled. Comparing a host + measured on utilisation against one measured on allocation alone would report a difference that + is an artefact of the monitoring rather than of the load, so the cluster falls back to + allocation figures until every host can be measured. + + Tuned with the drs.weighted.* settings, all cluster scoped. * DRS plan generation is considerably faster on large clusters. Working out where a VM could go is now done once for each group of VMs that would get the same answer rather than once per VM, diff --git a/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsAlgorithm.java b/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsAlgorithm.java index 722247e6fc51..3c8d89dad15e 100644 --- a/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsAlgorithm.java +++ b/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsAlgorithm.java @@ -20,6 +20,7 @@ package org.apache.cloudstack.cluster; import com.cloud.host.Host; +import com.cloud.host.HostLoad; import com.cloud.offering.ServiceOffering; import com.cloud.org.Cluster; import com.cloud.utils.Ternary; @@ -75,10 +76,22 @@ boolean needsDrs(Cluster cluster, List> cpuList, * host id to a Ternary of used, reserved and total memory */ default boolean needsDrs(Cluster cluster, Map> hostCpuMap, - Map> hostMemoryMap) throws ConfigurationException { + Map> hostMemoryMap, Map hostLoadMap) + throws ConfigurationException { return needsDrs(cluster, new ArrayList<>(hostCpuMap.values()), new ArrayList<>(hostMemoryMap.values())); } + /** + * Called once per plan, before any migration is considered, so that an algorithm can do work + * that would otherwise be repeated for every candidate VM and host. + * + * @param hostLoadMap + * measured load per host, empty when nothing has been sampled + */ + default void prepare(Cluster cluster, Map> hostCpuMap, + Map> hostMemoryMap, Map hostLoadMap) { + } + /** * Calculates the metrics (improvement, cost, benefit) for migrating a VM to a destination host. Improvement is * calculated based on the change in cluster imbalance before and after the migration. diff --git a/plugins/drs/cluster/weighted/src/main/java/org/apache/cloudstack/cluster/Weighted.java b/plugins/drs/cluster/weighted/src/main/java/org/apache/cloudstack/cluster/Weighted.java index f136bfa1ad9d..19feb328ea87 100644 --- a/plugins/drs/cluster/weighted/src/main/java/org/apache/cloudstack/cluster/Weighted.java +++ b/plugins/drs/cluster/weighted/src/main/java/org/apache/cloudstack/cluster/Weighted.java @@ -33,7 +33,6 @@ import com.cloud.host.Host; import com.cloud.host.HostLoad; import com.cloud.host.HostScoringWeights; -import com.cloud.host.HostLoadService; import com.cloud.offering.ServiceOffering; import com.cloud.org.Cluster; import com.cloud.utils.Ternary; @@ -53,19 +52,92 @@ * This blends four figures per host - CPU and memory allocated, CPU and memory in use - and * balances the result. The weights are the same host.weighted.* settings initial placement uses, on * purpose: if the two weighted them differently they would disagree about which host is the better - * one, and rebalancing could move VMs off hosts that placement had just chosen. Imbalance keeps the same meaning as the other algorithms: the standard - * deviation of the per-host figure over its mean, so drs.imbalance still means what it did. + * one, and rebalancing could move VMs off hosts that placement had just chosen. Imbalance keeps the same shape as the other algorithms - the standard + * deviation of the per-host figure over its mean - but it is computed over a blend rather than over + * one metric, so drs.imbalance is not calibrated the same way and is worth re-checking after + * switching. + * + * drs.metric, drs.metric.type and drs.metric.use.ratio choose and shape the single metric the other + * algorithms balance. They do not apply here and are ignored. */ public class Weighted extends AdapterBase implements ClusterDrsAlgorithm, Configurable { private static final Logger LOGGER = LogManager.getLogger(Weighted.class); - @Inject - private HostLoadService hostLoadService; + public static final ConfigKey StorageMotionCost = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED, + Double.class, "drs.weighted.storage.motion.cost", "0.02", + "How much a migration must improve the cluster's imbalance to be worth also moving the VM's " + + "storage. Migrations that do not need storage moved only have to improve it at all.", + true, ConfigKey.Scope.Cluster); @Inject private ClusterDetailsDao clusterDetailsDao; + /** + * Everything that is constant for one plan. getMetrics is called for every candidate VM and + * host - up to hundreds of thousands of times for a large cluster - so nothing in that path may + * hit the database or re-read settings. + */ + private static final class PlanContext { + private final float cpuOvercommit; + private final float memoryOvercommit; + private final double cpuAllocatedWeight; + private final double memoryAllocatedWeight; + private final double cpuUsedWeight; + private final double memoryUsedWeight; + private final Map hostLoadMap; + private final boolean everyHostMeasured; + + private PlanContext(float cpuOvercommit, float memoryOvercommit, double cpuAllocatedWeight, + double memoryAllocatedWeight, double cpuUsedWeight, double memoryUsedWeight, + Map hostLoadMap, boolean everyHostMeasured) { + this.everyHostMeasured = everyHostMeasured; + this.cpuOvercommit = cpuOvercommit; + this.memoryOvercommit = memoryOvercommit; + this.cpuAllocatedWeight = cpuAllocatedWeight; + this.memoryAllocatedWeight = memoryAllocatedWeight; + this.cpuUsedWeight = cpuUsedWeight; + this.memoryUsedWeight = memoryUsedWeight; + this.hostLoadMap = hostLoadMap; + } + + private HostLoad loadOf(long hostId) { + HostLoad load = hostLoadMap.get(hostId); + return load == null ? HostLoad.UNKNOWN : load; + } + } + + private final ThreadLocal context = new ThreadLocal<>(); + + @Override + public void prepare(Cluster cluster, Map> hostCpuMap, + Map> hostMemoryMap, Map hostLoadMap) { + long clusterId = cluster.getId(); + context.set(new PlanContext( + overcommitRatio(clusterId, VmDetailConstants.CPU_OVER_COMMIT_RATIO), + overcommitRatio(clusterId, VmDetailConstants.MEMORY_OVER_COMMIT_RATIO), + weight(HostScoringWeights.CpuAllocatedWeight, clusterId), + weight(HostScoringWeights.MemoryAllocatedWeight, clusterId), + weight(HostScoringWeights.CpuUsedWeight, clusterId), + weight(HostScoringWeights.MemoryUsedWeight, clusterId), + hostLoadMap == null ? new HashMap<>() : hostLoadMap, + hostLoadMap != null && !hostLoadMap.isEmpty() + && hostLoadMap.values().stream().allMatch(HostLoad::isUsable))); + } + + /** + * Falls back to reading everything when prepare has not been called, so the algorithm still + * works for a caller that does not know about it. + */ + private PlanContext contextFor(Cluster cluster) { + PlanContext prepared = context.get(); + if (prepared != null) { + return prepared; + } + prepare(cluster, null, null, null); + return context.get(); + } + @Override public String getName() { return "weighted"; @@ -81,12 +153,13 @@ public boolean needsDrs(Cluster cluster, List> cpuList cpuMap.put((long) -(i + 1), cpuList.get(i)); memoryMap.put((long) -(i + 1), memoryList.get(i)); } - return needsDrs(cluster, cpuMap, memoryMap); + return needsDrs(cluster, cpuMap, memoryMap, new HashMap<>()); } @Override public boolean needsDrs(Cluster cluster, Map> hostCpuMap, - Map> hostMemoryMap) throws ConfigurationException { + Map> hostMemoryMap, Map hostLoadMap) + throws ConfigurationException { double threshold = 1.0 - ClusterDrsService.ClusterDrsImbalanceThreshold.valueIn(cluster.getId()); double imbalance = imbalanceOf(blendByHost(cluster, hostCpuMap, hostMemoryMap).values()); boolean needed = imbalance > threshold; @@ -111,11 +184,12 @@ public Ternary getMetrics(Cluster cluster, VirtualMachin double after = imbalanceOf(blendByHost(cluster, cpuAfter, memoryAfter).values()); + // the caller migrates when benefit > cost, so expressing both in units of imbalance makes + // that comparison mean "is this worth what it costs". A migration that has to move storage + // has to earn more than one that does not. double improvement = before - after; - // moving a VM costs something and buys nothing unless the cluster ends up more even, so a - // migration is only worth making when it measurably helps - double cost = Boolean.TRUE.equals(requiresStorageMotion) ? 1.0 : 0.0; - double benefit = improvement > 0 ? 1.0 + improvement : 0.0; + double cost = Boolean.TRUE.equals(requiresStorageMotion) ? weight(StorageMotionCost, cluster.getId()) : 0.0; + double benefit = improvement; LOGGER.trace("Cluster {} imbalance {} -> {} moving {} to {}", cluster, before, after, vm, destHost); return new Ternary<>(improvement, cost, benefit); @@ -126,13 +200,7 @@ public Ternary getMetrics(Cluster cluster, VirtualMachin */ protected Map blendByHost(Cluster cluster, Map> hostCpuMap, Map> hostMemoryMap) { - float cpuOvercommit = overcommitRatio(cluster.getId(), VmDetailConstants.CPU_OVER_COMMIT_RATIO); - float memoryOvercommit = overcommitRatio(cluster.getId(), VmDetailConstants.MEMORY_OVER_COMMIT_RATIO); - - double cpuAllocatedWeight = weight(HostScoringWeights.CpuAllocatedWeight, cluster.getId()); - double memoryAllocatedWeight = weight(HostScoringWeights.MemoryAllocatedWeight, cluster.getId()); - double cpuUsedWeight = weight(HostScoringWeights.CpuUsedWeight, cluster.getId()); - double memoryUsedWeight = weight(HostScoringWeights.MemoryUsedWeight, cluster.getId()); + PlanContext ctx = contextFor(cluster); Map blended = new HashMap<>(); for (Map.Entry> entry : hostCpuMap.entrySet()) { @@ -141,20 +209,24 @@ protected Map blendByHost(Cluster cluster, Map> withVmMoved(Map capacity, float overcommit) { - double allocatable = (capacity.third() - capacity.second()) * (double) overcommit; + // overcommit scales the host's total; reserved is then taken off that, which is how + // CapacityManager computes free capacity everywhere else. Multiplying reserved by the ratio + // instead would make a host look fuller the more capacity it merely has reserved. + double allocatable = capacity.third() * (double) overcommit - capacity.second(); if (allocatable <= 0) { return 0; } - return clamp(capacity.first() / allocatable); + return capacity.first() / allocatable; } protected float overcommitRatio(long clusterId, String key) { @@ -239,7 +314,7 @@ public String getConfigComponentName() { @Override public ConfigKey[] getConfigKeys() { - // the four host.weighted.* weights are shared with initial placement, which registers them + // the four host.weighted.* weights are shared with initial placement and registered there return new ConfigKey[] {StorageMotionCost}; } } diff --git a/plugins/drs/cluster/weighted/src/main/resources/META-INF/cloudstack/weighted/module.properties b/plugins/drs/cluster/weighted/src/main/resources/META-INF/cloudstack/weighted/module.properties new file mode 100644 index 000000000000..636ecd1700e5 --- /dev/null +++ b/plugins/drs/cluster/weighted/src/main/resources/META-INF/cloudstack/weighted/module.properties @@ -0,0 +1,18 @@ +# 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. +name=weighted +parent=cluster diff --git a/plugins/drs/cluster/weighted/src/test/java/org/apache/cloudstack/cluster/WeightedTest.java b/plugins/drs/cluster/weighted/src/test/java/org/apache/cloudstack/cluster/WeightedTest.java index f3c0fafbbec5..e45dba7d3103 100644 --- a/plugins/drs/cluster/weighted/src/test/java/org/apache/cloudstack/cluster/WeightedTest.java +++ b/plugins/drs/cluster/weighted/src/test/java/org/apache/cloudstack/cluster/WeightedTest.java @@ -33,13 +33,13 @@ import com.cloud.dc.ClusterDetailsVO; import com.cloud.host.Host; import com.cloud.host.HostLoad; -import com.cloud.host.HostLoadService; import com.cloud.offering.ServiceOffering; import com.cloud.org.Cluster; import com.cloud.utils.Ternary; import com.cloud.vm.VirtualMachine; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @RunWith(MockitoJUnitRunner.class) @@ -51,9 +51,6 @@ public class WeightedTest { private static final int CPU_OVERCOMMIT = 10; private static final int MEMORY_OVERCOMMIT = 4; - @Mock - private HostLoadService hostLoadService; - @Mock private ClusterDetailsDao clusterDetailsDao; @@ -63,6 +60,7 @@ public class WeightedTest { private Cluster cluster; private final Map> cpu = new HashMap<>(); private final Map> memory = new HashMap<>(); + private final Map load = new HashMap<>(); @Before public void setUp() { @@ -74,10 +72,22 @@ public void setUp() { .thenReturn(new ClusterDetailsVO(CLUSTER_ID, "memoryOvercommitRatio", String.valueOf(MEMORY_OVERCOMMIT))); } - private void host(long id, double cpuAllocatedFraction, double memoryAllocatedFraction, HostLoad load) { - cpu.put(id, new Ternary<>((long) (CPU_TOTAL * CPU_OVERCOMMIT * cpuAllocatedFraction), 0L, CPU_TOTAL)); - memory.put(id, new Ternary<>((long) (MEMORY_TOTAL * MEMORY_OVERCOMMIT * memoryAllocatedFraction), 0L, MEMORY_TOTAL)); - Mockito.lenient().when(hostLoadService.getLoad(id)).thenReturn(load); + private void host(long id, double cpuAllocatedFraction, double memoryAllocatedFraction, HostLoad hostLoad) { + host(id, cpuAllocatedFraction, memoryAllocatedFraction, hostLoad, 0L, 0L); + } + + private void host(long id, double cpuAllocatedFraction, double memoryAllocatedFraction, HostLoad hostLoad, + long reservedCpu, long reservedMemory) { + cpu.put(id, new Ternary<>((long) (CPU_TOTAL * CPU_OVERCOMMIT * cpuAllocatedFraction), reservedCpu, CPU_TOTAL)); + memory.put(id, new Ternary<>((long) (MEMORY_TOTAL * MEMORY_OVERCOMMIT * memoryAllocatedFraction), + reservedMemory, MEMORY_TOTAL)); + load.put(id, hostLoad); + } + + /** DRS calls prepare once per plan; every test must do the same. */ + private Map blend() { + weighted.prepare(cluster, cpu, memory, load); + return weighted.blendByHost(cluster, cpu, memory); } @Test @@ -85,7 +95,7 @@ public void testAnEvenClusterHasNoImbalance() { host(1L, 0.4, 0.4, new HostLoad(0.4, 0.4, 10)); host(2L, 0.4, 0.4, new HostLoad(0.4, 0.4, 10)); - assertEquals(0.0, weighted.imbalanceOf(weighted.blendByHost(cluster, cpu, memory).values()), 1e-9); + assertEquals(0.0, weighted.imbalanceOf(blend().values()), 1e-9); } @Test @@ -95,7 +105,7 @@ public void testCpuLoadImbalanceIsSeenWhenMemoryIsEven() { host(2L, 0.30, 0.40, new HostLoad(0.30, 0.40, 10)); assertTrue("balancing on memory alone would call this cluster even", - weighted.imbalanceOf(weighted.blendByHost(cluster, cpu, memory).values()) > 0.15); + weighted.imbalanceOf(blend().values()) > 0.15); } @Test @@ -103,7 +113,7 @@ public void testMemoryImbalanceIsSeenWhenCpuIsEven() { host(1L, 0.30, 0.90, new HostLoad(0.30, 0.90, 10)); host(2L, 0.30, 0.10, new HostLoad(0.30, 0.10, 10)); - assertTrue(weighted.imbalanceOf(weighted.blendByHost(cluster, cpu, memory).values()) > 0.15); + assertTrue(weighted.imbalanceOf(blend().values()) > 0.15); } @Test @@ -112,7 +122,7 @@ public void testAllocationBeyondPhysicalSizeStillDiscriminates() { host(1L, 0.20, 0.20, HostLoad.UNKNOWN); host(2L, 0.80, 0.20, HostLoad.UNKNOWN); - Map blended = weighted.blendByHost(cluster, cpu, memory); + Map blended = blend(); assertTrue(blended.get(2L) > blended.get(1L)); } @@ -121,7 +131,7 @@ public void testUnmeasuredHostsFallBackToAllocation() { host(1L, 0.20, 0.20, HostLoad.UNKNOWN); host(2L, 0.60, 0.60, HostLoad.UNKNOWN); - Map blended = weighted.blendByHost(cluster, cpu, memory); + Map blended = blend(); assertEquals(0.20, blended.get(1L), 0.01); assertEquals(0.60, blended.get(2L), 0.01); } @@ -140,6 +150,7 @@ public void testMovingAVmOffTheBusyHostIsAnImprovement() throws ConfigurationExc Mockito.when(offering.getSpeed()).thenReturn(2400); Mockito.when(offering.getRamSize()).thenReturn(32768); + weighted.prepare(cluster, cpu, memory, load); Ternary metrics = weighted.getMetrics(cluster, vm, offering, dest, cpu, memory, false, null, new double[0], new HashMap<>()); @@ -161,11 +172,13 @@ public void testMovingAVmOntoTheBusyHostIsNotAnImprovement() throws Configuratio Mockito.when(offering.getSpeed()).thenReturn(2400); Mockito.when(offering.getRamSize()).thenReturn(32768); + weighted.prepare(cluster, cpu, memory, load); Ternary metrics = weighted.getMetrics(cluster, vm, offering, dest, cpu, memory, false, null, new double[0], new HashMap<>()); assertTrue("piling onto the busier host makes the cluster less even", metrics.first() < 0); - assertEquals("and must not be considered worth doing", 0.0, metrics.third(), 1e-9); + // the caller migrates when benefit > cost + assertFalse("and must not be considered worth doing", metrics.third() > metrics.second()); } @Test @@ -182,6 +195,7 @@ public void testStorageMotionCostsMore() throws ConfigurationException { Mockito.when(offering.getSpeed()).thenReturn(2400); Mockito.when(offering.getRamSize()).thenReturn(32768); + weighted.prepare(cluster, cpu, memory, load); double withoutStorage = weighted.getMetrics(cluster, vm, offering, dest, cpu, memory, false, null, new double[0], new HashMap<>()).second(); double withStorage = weighted.getMetrics(cluster, vm, offering, dest, cpu, memory, true, @@ -189,4 +203,81 @@ public void testStorageMotionCostsMore() throws ConfigurationException { assertTrue("a migration that has to move storage should cost more", withStorage > withoutStorage); } -} + + @Test + public void testReservedCapacityDoesNotMakeAHostLookFuller() { + // reserved is taken off the overcommitted total, not multiplied by the ratio. Getting this + // backwards made a host with reserved capacity read far fuller than an identical one + // without, and DRS would evacuate it for no reason. + host(1L, 0.10, 0.10, HostLoad.UNKNOWN, 0L, 0L); + host(2L, 0.10, 0.10, HostLoad.UNKNOWN, CPU_TOTAL / 10, MEMORY_TOTAL / 10); + + Map blended = blend(); + + assertEquals("a little reserved capacity should barely move the figure", + blended.get(1L), blended.get(2L), 0.02); + } + + @Test + public void testAllocationBeyondTheOvercommittedTotalIsNotFlattened() { + // hosts past what they can hand out must stay distinguishable, not both read as full + host(1L, 1.20, 0.20, HostLoad.UNKNOWN); + host(2L, 2.40, 0.20, HostLoad.UNKNOWN); + + Map blended = blend(); + + assertTrue("two oversubscribed hosts must not read identically", + blended.get(2L) > blended.get(1L)); + } + + @Test + public void testUtilisationIsDroppedWhenAnyHostCannotBeMeasured() { + // one host with broken telemetry must not read as loaded simply because it is measured on a + // different basis to its peers - that would evacuate whichever host stopped reporting + host(1L, 0.50, 0.50, new HostLoad(0.10, 0.10, 10)); + host(2L, 0.50, 0.50, HostLoad.UNKNOWN); + + Map blended = blend(); + + assertEquals("identical hosts must read identically when one cannot be measured", + blended.get(1L), blended.get(2L), 1e-9); + assertEquals(0.0, weighted.imbalanceOf(blended.values()), 1e-9); + } + + @Test + public void testUtilisationIsUsedWhenEveryHostIsMeasured() { + host(1L, 0.50, 0.50, new HostLoad(0.90, 0.50, 10)); + host(2L, 0.50, 0.50, new HostLoad(0.10, 0.50, 10)); + + assertTrue("with every host measured, load must separate them", + weighted.imbalanceOf(blend().values()) > 0.05); + } + + @Test + public void testStorageMotionMustEarnMoreThanAPlainMigration() throws ConfigurationException { + host(1L, 0.52, 0.50, new HostLoad(0.52, 0.50, 10)); + host(2L, 0.48, 0.50, new HostLoad(0.48, 0.50, 10)); + + VirtualMachine vm = Mockito.mock(VirtualMachine.class); + Mockito.when(vm.getHostId()).thenReturn(1L); + Host dest = Mockito.mock(Host.class); + Mockito.when(dest.getId()).thenReturn(2L); + ServiceOffering offering = Mockito.mock(ServiceOffering.class); + Mockito.when(offering.getCpu()).thenReturn(1); + Mockito.when(offering.getSpeed()).thenReturn(500); + Mockito.when(offering.getRamSize()).thenReturn(512); + + weighted.prepare(cluster, cpu, memory, load); + Ternary plain = weighted.getMetrics(cluster, vm, offering, dest, + cpu, memory, false, null, new double[0], new HashMap<>()); + weighted.prepare(cluster, cpu, memory, load); + Ternary withStorage = weighted.getMetrics(cluster, vm, offering, dest, + cpu, memory, true, null, new double[0], new HashMap<>()); + + // the caller migrates when benefit > cost + assertTrue("a small gain is worth taking without moving storage", + plain.third() > plain.second()); + assertFalse("the same small gain is not worth moving storage for", + withStorage.third() > withStorage.second()); + } +} \ No newline at end of file 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 8e0c0e5f2ff5..637eeb65d18f 100644 --- a/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java +++ b/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java @@ -34,6 +34,8 @@ import com.cloud.event.dao.EventDao; import com.cloud.exception.InvalidParameterValueException; import com.cloud.host.Host; +import com.cloud.host.HostLoad; +import com.cloud.host.HostLoadService; import com.cloud.host.HostVO; import com.cloud.host.dao.HostDao; import com.cloud.offering.ServiceOffering; @@ -44,7 +46,6 @@ import com.cloud.user.User; import com.cloud.utils.DateUtil; import com.cloud.utils.Pair; -import com.cloud.storage.VolumeVO; import com.cloud.storage.dao.VolumeDao; import com.cloud.utils.Ternary; import com.cloud.utils.component.ComponentContext; @@ -96,7 +97,6 @@ import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Set; import java.util.Timer; import java.util.TimerTask; @@ -152,6 +152,9 @@ public class ClusterDrsServiceImpl extends ManagerBase implements ClusterDrsServ @Inject VolumeDao volumeDao; + @Inject + HostLoadService hostLoadService; + List drsAlgorithms = new ArrayList<>(); Map drsAlgorithmMap = new HashMap<>(); @@ -401,7 +404,13 @@ private List> getMigrationPlans( List> migrationPlan = new ArrayList<>(); Map vmToExcludesMap = null; Set staleAffinityVmIds = new HashSet<>(); - while (iteration < maxIterations && algorithm.needsDrs(cluster, hostCpuMap, hostMemoryMap)) { + Map hostLoadMap = new HashMap<>(); + for (Long hostId : hostCpuMap.keySet()) { + hostLoadMap.put(hostId, hostLoadService.getLoad(hostId)); + } + algorithm.prepare(cluster, hostCpuMap, hostMemoryMap, hostLoadMap); + + while (iteration < maxIterations && algorithm.needsDrs(cluster, hostCpuMap, hostMemoryMap, hostLoadMap)) { logger.debug("Starting DRS iteration {} for cluster {}", iteration + 1, cluster); // Affinity only changes for VMs that share a group with the one just moved, so after @@ -607,17 +616,35 @@ private Pair>, Map>> get return new Pair<>(vmToCompatibleHostsCache, vmToStorageMotionCache); } + /** + * VM details that narrow which hosts a VM can run on. Matched as prefixes and case + * insensitively, so that related keys are covered without listing each one. + */ + private static final List PLACEMENT_AFFECTING_DETAIL_PREFIXES = List.of( + "uefi", "boot", "dpdk", "cpunumber", "cpuspeed", "memory", "rootdisk", "nic", "gpu", "vgpu", + "extraconfig", "hypervisortoolsversion", "kvm", "vmware", "hyperv"); + /** * Identifies VMs that would get the same answer from listHostsForMigrationOfVM. * - * The answer depends on what the VM asks for (its offering and template), where it is now, the - * volumes that would have to follow it, and the affinity groups it belongs to. Two VMs matching - * on all of those are interchangeable for the purpose of finding candidate hosts. + * The answer depends on what the VM asks for, where it is now, the volumes that would have to + * follow it, and its affinity groups. Rather than enumerate everything that could possibly + * matter and risk missing one, a VM is only grouped when it has none of the per-VM inputs that + * are known to change the answer - a custom offering whose size comes from the VM rather than + * the offering, a boot mode or device setting, and so on. Anything else is worked out on its + * own, which costs what it always did. + * + * @return a key shared with equivalent VMs, or one unique to this VM when it cannot be grouped */ protected String migrationEquivalenceKey(VirtualMachine vm) { - List poolIds = volumeDao.findCreatedByInstance(vm.getId()).stream() - .map(VolumeVO::getPoolId) - .filter(Objects::nonNull) + ServiceOffering offering = serviceOfferingDao.findByIdIncludingRemoved(vm.getId(), vm.getServiceOfferingId()); + if (offering == null || offering.isDynamic() || hasPlacementAffectingDetails(vm)) { + return "vm-" + vm.getId(); + } + + List volumes = volumeDao.findCreatedByInstance(vm.getId()).stream() + .filter(volume -> volume.getPoolId() != null) + .map(volume -> volume.getPoolId() + ":" + volume.getDiskOfferingId()) .sorted() .collect(Collectors.toList()); List groupIds = affinityGroupVMMapDao.listByInstanceId(vm.getId()).stream() @@ -625,7 +652,25 @@ protected String migrationEquivalenceKey(VirtualMachine vm) { .sorted() .collect(Collectors.toList()); return String.format("%s|%s|%s|%s|%s|%s", vm.getServiceOfferingId(), vm.getTemplateId(), vm.getHostId(), - vm.getHypervisorType(), poolIds, groupIds); + vm.getHypervisorType(), volumes, groupIds); + } + + /** + * Whether the VM carries any detail that narrows the hosts it can run on. Such a VM is never + * grouped with another, because the details are per VM and two VMs on the same offering can + * differ entirely. + */ + protected boolean hasPlacementAffectingDetails(VirtualMachine vm) { + Map details = vmInstanceDetailsDao.listDetailsKeyPairs(vm.getId()); + if (MapUtils.isEmpty(details)) { + return false; + } + for (String key : details.keySet()) { + if (PLACEMENT_AFFECTING_DETAIL_PREFIXES.stream().anyMatch(prefix -> key.toLowerCase().startsWith(prefix))) { + return true; + } + } + return false; } /** diff --git a/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsEquivalenceTest.java b/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsEquivalenceTest.java index f2725acb9ca8..0d596db0a387 100644 --- a/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsEquivalenceTest.java +++ b/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsEquivalenceTest.java @@ -30,6 +30,9 @@ import org.mockito.junit.MockitoJUnitRunner; import com.cloud.hypervisor.Hypervisor; +import com.cloud.service.ServiceOfferingVO; +import com.cloud.service.dao.ServiceOfferingDao; +import com.cloud.vm.dao.VMInstanceDetailsDao; import com.cloud.storage.VolumeVO; import com.cloud.storage.dao.VolumeDao; import com.cloud.vm.VMInstanceVO; @@ -50,6 +53,12 @@ public class ClusterDrsEquivalenceTest { @Mock private AffinityGroupVMMapDao affinityGroupVMMapDao; + @Mock + private ServiceOfferingDao serviceOfferingDao; + + @Mock + private VMInstanceDetailsDao vmInstanceDetailsDao; + @InjectMocks private ClusterDrsServiceImpl service = new ClusterDrsServiceImpl(); @@ -68,9 +77,25 @@ private VMInstanceVO vm(long id, long offeringId, long templateId, Long hostId, }).collect(java.util.stream.Collectors.toList()); Mockito.lenient().when(volumeDao.findCreatedByInstance(id)).thenReturn(volumes); Mockito.lenient().when(affinityGroupVMMapDao.listByInstanceId(id)).thenReturn(Collections.emptyList()); + + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.lenient().when(offering.isDynamic()).thenReturn(false); + Mockito.lenient().when(serviceOfferingDao.findByIdIncludingRemoved(id, offeringId)).thenReturn(offering); + Mockito.lenient().when(vmInstanceDetailsDao.listDetailsKeyPairs(id)).thenReturn(Collections.emptyMap()); return vm; } + private void offeringIsDynamic(long vmId, long offeringId) { + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offering.isDynamic()).thenReturn(true); + Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(vmId, offeringId)).thenReturn(offering); + } + + private void vmHasDetail(long vmId, String key, String value) { + Mockito.when(vmInstanceDetailsDao.listDetailsKeyPairs(vmId)) + .thenReturn(java.util.Map.of(key, value)); + } + @Test public void testIdenticalVmsShareOnePass() { assertEquals(service.migrationEquivalenceKey(vm(1L, 10L, 20L, 30L, 40L)), @@ -118,4 +143,45 @@ public void testDifferentAffinityGroupsIsADifferentAnswer() { service.migrationEquivalenceKey(grouped), service.migrationEquivalenceKey(vm(2L, 10L, 20L, 30L, 40L))); } -} + + @Test + public void testVolumesOnTheSamePoolButDifferentDiskOfferingsAreNotGrouped() { + VMInstanceVO a = vm(1L, 10L, 20L, 30L, 40L); + VMInstanceVO b = vm(2L, 10L, 20L, 30L, 40L); + VolumeVO differentOffering = Mockito.mock(VolumeVO.class); + Mockito.when(differentOffering.getPoolId()).thenReturn(40L); + Mockito.when(differentOffering.getDiskOfferingId()).thenReturn(99L); + Mockito.when(volumeDao.findCreatedByInstance(2L)).thenReturn(List.of(differentOffering)); + + assertNotEquals("storage tags come from the disk offering, so it changes the answer", + service.migrationEquivalenceKey(a), service.migrationEquivalenceKey(b)); + } + + @Test + public void testACustomOfferingIsNeverGrouped() { + // a dynamic offering takes its size from the VM, so two VMs on the same offering can be + // wildly different and must not share a candidate host list + VMInstanceVO a = vm(1L, 10L, 20L, 30L, 40L); + VMInstanceVO b = vm(2L, 10L, 20L, 30L, 40L); + offeringIsDynamic(1L, 10L); + offeringIsDynamic(2L, 10L); + + assertNotEquals(service.migrationEquivalenceKey(a), service.migrationEquivalenceKey(b)); + } + + @Test + public void testAVmWithABootModeIsNeverGrouped() { + VMInstanceVO a = vm(1L, 10L, 20L, 30L, 40L); + VMInstanceVO b = vm(2L, 10L, 20L, 30L, 40L); + vmHasDetail(1L, "UEFI", "SECURE"); + + assertNotEquals("a UEFI VM cannot run everywhere a BIOS VM can", + service.migrationEquivalenceKey(a), service.migrationEquivalenceKey(b)); + } + + @Test + public void testAVmWithNoDetailsIsStillGrouped() { + assertEquals(service.migrationEquivalenceKey(vm(1L, 10L, 20L, 30L, 40L)), + service.migrationEquivalenceKey(vm(2L, 10L, 20L, 30L, 40L))); + } +} \ No newline at end of file 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 a31ce47e8766..f1bc0f9c6b94 100644 --- a/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java @@ -28,6 +28,7 @@ import com.cloud.event.dao.EventDao; import com.cloud.exception.InvalidParameterValueException; import com.cloud.host.Host; +import com.cloud.host.HostLoadService; import com.cloud.host.HostVO; import com.cloud.deploy.DeploymentPlanner.ExcludeList; import com.cloud.deploy.DeploymentPlan; @@ -140,6 +141,9 @@ public class ClusterDrsServiceImplTest { @Mock private VolumeDao volumeDao; + @Mock + private HostLoadService hostLoadService; + @Mock private VMInstanceDetailsDao vmInstanceDetailsDao; @@ -231,7 +235,7 @@ public void testGetDrsPlan() throws ConfigurationException { Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList); Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); - Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap())).thenReturn( + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap(), Mockito.anyMap())).thenReturn( true, false); Mockito.doReturn(new Pair<>(vm1, host2)).when(clusterDrsService).getBestMigration( @@ -247,7 +251,7 @@ public void testGetDrsPlan() throws ConfigurationException { Mockito.verify(hostDao, Mockito.times(1)).findByClusterId(1L); Mockito.verify(vmInstanceDao, Mockito.times(1)).listByClusterId(1L); Mockito.verify(balancedAlgorithm, Mockito.times(2)).needsDrs(Mockito.any(), Mockito.anyMap(), - Mockito.anyMap()); + Mockito.anyMap(), Mockito.anyMap()); assertEquals(1, iterations.size()); } @@ -311,7 +315,7 @@ public void testGetDrsPlanWithSystemVMs() throws ConfigurationException { Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList); Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); - Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap())).thenReturn(true); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap(), Mockito.anyMap())).thenReturn(true); Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); List> result = clusterDrsService.getDrsPlan(cluster, 5); @@ -353,7 +357,7 @@ public void testGetDrsPlanWithNonRunningVMs() throws ConfigurationException { Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList); Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); - Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap())).thenReturn(true); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap(), Mockito.anyMap())).thenReturn(true); Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); List> result = clusterDrsService.getDrsPlan(cluster, 5); @@ -400,7 +404,7 @@ public void testGetDrsPlanWithSkipDrsFlag() throws ConfigurationException { Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList); Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); - Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap())).thenReturn(true); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap(), Mockito.anyMap())).thenReturn(true); Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); List> result = clusterDrsService.getDrsPlan(cluster, 5); @@ -445,7 +449,7 @@ public void testGetDrsPlanWithNoCompatibleHosts() throws ConfigurationException Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList); Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); - Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap())).thenReturn(true); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap(), Mockito.anyMap())).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 @@ -493,7 +497,7 @@ public void testGetDrsPlanWithExceptionInCompatibilityCheck() throws Configurati Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList); Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); - Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap())).thenReturn(true); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap(), Mockito.anyMap())).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 @@ -542,7 +546,7 @@ public void testGetDrsPlanWithNoBestMigration() throws ConfigurationException { Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList); Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); - Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap())).thenReturn(true); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap(), Mockito.anyMap())).thenReturn(true); Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(Mockito.anyLong(), Mockito.anyLong())).thenReturn(serviceOffering); Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); @@ -617,7 +621,7 @@ public void testGetDrsPlanWithMultipleIterations() throws ConfigurationException Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList); Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); - Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap())).thenReturn( + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap(), Mockito.anyMap())).thenReturn( true, true, false); Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(Mockito.anyLong(), Mockito.anyLong())).thenReturn(serviceOffering); Mockito.when(hostJoinDao.searchByIds(1L, 2L)).thenReturn(List.of(hostJoin1, hostJoin2)); @@ -631,7 +635,7 @@ public void testGetDrsPlanWithMultipleIterations() throws ConfigurationException List> result = clusterDrsService.getDrsPlan(cluster, 5); assertEquals(2, result.size()); - Mockito.verify(balancedAlgorithm, Mockito.times(3)).needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap()); + Mockito.verify(balancedAlgorithm, Mockito.times(3)).needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap(), Mockito.anyMap()); } @Test @@ -663,7 +667,7 @@ public void testGetDrsPlanWithMigrationToOriginalHost() throws ConfigurationExce Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList); Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); - Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap())).thenReturn(true); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap(), Mockito.anyMap())).thenReturn(true); Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(Mockito.anyLong(), Mockito.anyLong())).thenReturn(serviceOffering); // Return migration to original host (host1) - should break the loop From c15dd752956e362f5f319e35f66ef089ce87e629 Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 10 Sep 2026 00:25:42 +0000 Subject: [PATCH 13/15] Verify equivalent VMs share one candidate host lookup The grouping could have been disabled and every test would still have passed. Asserts the expensive call is made once for two interchangeable VMs. Signed-off-by: Brad House --- .../cluster/ClusterDrsServiceImplTest.java | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) 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 f1bc0f9c6b94..a4cb44a10d57 100644 --- a/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java @@ -1236,4 +1236,57 @@ public void testNonStrictHostAffinityOnTheCurrentHostDoesNotFreezeTheVm() { assertFalse(avoids(excludes, 11L)); } + + @Test + public void testEquivalentVmsShareOneCandidateHostLookup() throws ConfigurationException { + // the expensive call is listHostsForMigrationOfVM. Two interchangeable VMs must cost one + // pass, not two - otherwise the grouping is not actually doing anything. + ClusterVO cluster = Mockito.mock(ClusterVO.class); + Mockito.when(cluster.getId()).thenReturn(1L); + Mockito.when(cluster.getAllocationState()).thenReturn(Grouping.AllocationState.Enabled); + + HostVO host1 = Mockito.mock(HostVO.class); + Mockito.when(host1.getId()).thenReturn(1L); + + List vmList = new ArrayList<>(); + for (long id : new long[] {1L, 2L}) { + VMInstanceVO vm = Mockito.mock(VMInstanceVO.class); + Mockito.when(vm.getId()).thenReturn(id); + Mockito.when(vm.getHostId()).thenReturn(1L); + Mockito.when(vm.getType()).thenReturn(VirtualMachine.Type.User); + Mockito.when(vm.getState()).thenReturn(VirtualMachine.State.Running); + Mockito.when(vm.getServiceOfferingId()).thenReturn(9L); + Mockito.lenient().when(vm.getTemplateId()).thenReturn(8L); + vmList.add(vm); + } + + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offering.isDynamic()).thenReturn(false); + Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(Mockito.anyLong(), Mockito.anyLong())) + .thenReturn(offering); + Mockito.when(vmInstanceDetailsDao.listDetailsKeyPairs(Mockito.anyLong())) + .thenReturn(Collections.emptyMap()); + Mockito.when(volumeDao.findCreatedByInstance(Mockito.anyLong())).thenReturn(Collections.emptyList()); + + HostJoinVO hostJoin1 = Mockito.mock(HostJoinVO.class); + Mockito.when(hostJoin1.getId()).thenReturn(1L); + Mockito.when(hostJoin1.getCpus()).thenReturn(4); + Mockito.when(hostJoin1.getSpeed()).thenReturn(1000L); + Mockito.when(hostJoin1.getTotalMemory()).thenReturn(8192L); + + Mockito.when(hostDao.findByClusterId(1L)).thenReturn(List.of(host1)); + Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); + Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyMap(), Mockito.anyMap(), Mockito.anyMap())) + .thenReturn(false); + Mockito.when(managementServer.listHostsForMigrationOfVM(Mockito.any(), Mockito.anyLong(), + Mockito.anyLong(), Mockito.any(), Mockito.anyList())) + .thenReturn(new Ternary<>(new Pair<>(Collections.emptyList(), 0), + List.of(host1), Collections.emptyMap())); + + clusterDrsService.getDrsPlan(cluster, 5); + + Mockito.verify(managementServer, Mockito.times(1)).listHostsForMigrationOfVM( + Mockito.any(), Mockito.anyLong(), Mockito.anyLong(), Mockito.any(), Mockito.anyList()); + } } From 827fdec50da058a8a3c7bc39c72b97d793f26703 Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 10 Sep 2026 12:21:28 +0000 Subject: [PATCH 14/15] Declare the host load tracker where both its consumers can see it It was declared alongside the allocators, in a module context that is a descendant of the one the DRS service lives in. A bean in a descendant context is not visible to its parent, so injecting it into the DRS service failed and the management server did not start. Unit tests cannot catch this - the field is a mock there. It showed up as every integration test job running to its six hour limit waiting for a management server that was never coming up. Declared in the core context instead, which is an ancestor of the allocator module and the same context the DRS service is declared in. Also add the trailing newlines pre-commit asked for. Signed-off-by: Brad House --- .../java/org/apache/cloudstack/cluster/WeightedTest.java | 2 +- .../core/spring-server-core-managers-context.xml | 7 +++++++ .../server-allocator/spring-server-allocator-context.xml | 3 --- .../cloudstack/cluster/ClusterDrsEquivalenceTest.java | 2 +- 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/plugins/drs/cluster/weighted/src/test/java/org/apache/cloudstack/cluster/WeightedTest.java b/plugins/drs/cluster/weighted/src/test/java/org/apache/cloudstack/cluster/WeightedTest.java index e45dba7d3103..9a72df3ea51e 100644 --- a/plugins/drs/cluster/weighted/src/test/java/org/apache/cloudstack/cluster/WeightedTest.java +++ b/plugins/drs/cluster/weighted/src/test/java/org/apache/cloudstack/cluster/WeightedTest.java @@ -280,4 +280,4 @@ public void testStorageMotionMustEarnMoreThanAPlainMigration() throws Configurat assertFalse("the same small gain is not worth moving storage for", withStorage.third() > withStorage.second()); } -} \ No newline at end of file +} diff --git a/server/src/main/resources/META-INF/cloudstack/core/spring-server-core-managers-context.xml b/server/src/main/resources/META-INF/cloudstack/core/spring-server-core-managers-context.xml index 61b5b541bc75..547c42097f61 100644 --- a/server/src/main/resources/META-INF/cloudstack/core/spring-server-core-managers-context.xml +++ b/server/src/main/resources/META-INF/cloudstack/core/spring-server-core-managers-context.xml @@ -370,6 +370,13 @@ value="#{affinityProcessorsRegistry.registered}" /> + + + diff --git a/server/src/main/resources/META-INF/cloudstack/server-allocator/spring-server-allocator-context.xml b/server/src/main/resources/META-INF/cloudstack/server-allocator/spring-server-allocator-context.xml index 69aea2cf8451..3eaf9bb90bc5 100644 --- a/server/src/main/resources/META-INF/cloudstack/server-allocator/spring-server-allocator-context.xml +++ b/server/src/main/resources/META-INF/cloudstack/server-allocator/spring-server-allocator-context.xml @@ -31,9 +31,6 @@ - - diff --git a/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsEquivalenceTest.java b/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsEquivalenceTest.java index 0d596db0a387..7a3dc0fdae2f 100644 --- a/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsEquivalenceTest.java +++ b/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsEquivalenceTest.java @@ -184,4 +184,4 @@ public void testAVmWithNoDetailsIsStillGrouped() { assertEquals(service.migrationEquivalenceKey(vm(1L, 10L, 20L, 30L, 40L)), service.migrationEquivalenceKey(vm(2L, 10L, 20L, 30L, 40L))); } -} \ No newline at end of file +} From af6f78adf5e451be84209df240f9b0545822c38e Mon Sep 17 00:00:00 2001 From: Brad House Date: Mon, 9 Nov 2026 12:51:34 +0000 Subject: [PATCH 15/15] Sample host load for weighted DRS, not only for the weighted allocator The host load tracker only sampled while vm.allocation.algorithm was balancedweighted. The weighted DRS algorithm reads the same figures, but drs.algorithm is chosen per cluster and independently of the allocator. With the allocator at its default every host read as unmeasured, and weighted DRS balanced on allocation alone. In a lab run, a host at 98% CPU next to two at 4-8% gave an imbalance of 0.009 and no moves. Switching only the allocator gave 0.25 and four moves off the busy host. The tracker now also samples when drs.algorithm is weighted globally or on any cluster that has not been removed. That costs one query on cluster_details per sample interval rather than one per cluster, plus a lookup for each matching cluster, because deleting a cluster leaves its details behind. Weighted.prepare now logs once per plan when it drops the utilisation terms: WARN when no host has samples, INFO when only some do. Before this, a cluster stuck on allocation looked the same in the log as a measured one. The drs.algorithm description named drs.weighted.* settings as its tuning. Most of its tuning is in the shared host.weighted.* settings. Signed-off-by: Brad House --- .../cloudstack/cluster/ClusterDrsService.java | 2 +- .../apache/cloudstack/cluster/Weighted.java | 38 +++++- .../cloudstack/cluster/WeightedTest.java | 66 ++++++++++ .../allocator/impl/HostLoadTracker.java | 51 +++++++- .../allocator/impl/HostLoadTrackerTest.java | 116 ++++++++++++++++++ 5 files changed, 263 insertions(+), 10 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsService.java b/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsService.java index 7323f7fb4916..7f2955aa1ab0 100644 --- a/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsService.java +++ b/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsService.java @@ -51,7 +51,7 @@ public interface ClusterDrsService extends Manager, Configurable, Scheduler { true, ConfigKey.Scope.Cluster, null, "Maximum number of migrations for DRS", null, null, null); ConfigKey ClusterDrsAlgorithm = new ConfigKey<>(String.class, "drs.algorithm", - ConfigKey.CATEGORY_ADVANCED, "balanced", "The DRS algorithm to be executed on the cluster. Possible values are condensed, balanced, weighted. 'weighted' balances CPU and memory together, and on measured load as well as allocation, tuned with the drs.weighted.* settings.", + ConfigKey.CATEGORY_ADVANCED, "balanced", "The DRS algorithm to be executed on the cluster. Possible values are condensed, balanced, weighted. 'weighted' balances CPU and memory together, and on measured load as well as allocation, tuned with the host.weighted.{cpu,memory}.{allocated,used}.weight settings it shares with the 'balancedweighted' allocation algorithm and with drs.weighted.storage.motion.cost. Hosts are sampled for load while any cluster uses it, whatever vm.allocation.algorithm is.", true, ConfigKey.Scope.Cluster, null, "DRS algorithm", null, null, null, ConfigKey.Kind.Select, "condensed,balanced,weighted"); diff --git a/plugins/drs/cluster/weighted/src/main/java/org/apache/cloudstack/cluster/Weighted.java b/plugins/drs/cluster/weighted/src/main/java/org/apache/cloudstack/cluster/Weighted.java index 19feb328ea87..a1df535bda84 100644 --- a/plugins/drs/cluster/weighted/src/main/java/org/apache/cloudstack/cluster/Weighted.java +++ b/plugins/drs/cluster/weighted/src/main/java/org/apache/cloudstack/cluster/Weighted.java @@ -113,16 +113,46 @@ private HostLoad loadOf(long hostId) { public void prepare(Cluster cluster, Map> hostCpuMap, Map> hostMemoryMap, Map hostLoadMap) { long clusterId = cluster.getId(); + double cpuUsedWeight = weight(HostScoringWeights.CpuUsedWeight, clusterId); + double memoryUsedWeight = weight(HostScoringWeights.MemoryUsedWeight, clusterId); + boolean everyHostMeasured = hostLoadMap != null && !hostLoadMap.isEmpty() + && hostLoadMap.values().stream().allMatch(HostLoad::isUsable); + if (hostLoadMap != null && !everyHostMeasured && cpuUsedWeight + memoryUsedWeight > 0) { + logUnmeasured(cluster, hostLoadMap); + } context.set(new PlanContext( overcommitRatio(clusterId, VmDetailConstants.CPU_OVER_COMMIT_RATIO), overcommitRatio(clusterId, VmDetailConstants.MEMORY_OVER_COMMIT_RATIO), weight(HostScoringWeights.CpuAllocatedWeight, clusterId), weight(HostScoringWeights.MemoryAllocatedWeight, clusterId), - weight(HostScoringWeights.CpuUsedWeight, clusterId), - weight(HostScoringWeights.MemoryUsedWeight, clusterId), + cpuUsedWeight, + memoryUsedWeight, hostLoadMap == null ? new HashMap<>() : hostLoadMap, - hostLoadMap != null && !hostLoadMap.isEmpty() - && hostLoadMap.values().stream().allMatch(HostLoad::isUsable))); + everyHostMeasured)); + } + + /** + * Says why the utilisation terms are being dropped. Without this a cluster balancing on + * allocation only looks the same in the log as a measured one, apart from the size of the + * imbalance. prepare runs once per plan, so this is once per plan. + */ + private void logUnmeasured(Cluster cluster, Map hostLoadMap) { + long unmeasured = hostLoadMap.values().stream().filter(load -> !load.isUsable()).count(); + if (unmeasured == 0) { + // no hosts at all; nothing to balance and nothing worth saying + return; + } + if (unmeasured == hostLoadMap.size()) { + LOGGER.warn("No host in cluster {} has current load samples, so DRS algorithm {} is balancing " + + "on allocation only and ignoring {} and {}. Hosts are sampled every " + + "host.load.sample.interval seconds, so there are none yet just after a management server " + + "starts or this algorithm is first selected; otherwise no host is Up and reporting stats.", cluster, getName(), + HostScoringWeights.CpuUsedWeight.key(), HostScoringWeights.MemoryUsedWeight.key()); + } else { + LOGGER.info("{} of {} hosts in cluster {} have no current load samples (not yet sampled, not Up, " + + "or older than host.load.stale.after), so DRS algorithm {} is balancing every host on " + + "allocation only for this plan.", unmeasured, hostLoadMap.size(), cluster, getName()); + } } /** diff --git a/plugins/drs/cluster/weighted/src/test/java/org/apache/cloudstack/cluster/WeightedTest.java b/plugins/drs/cluster/weighted/src/test/java/org/apache/cloudstack/cluster/WeightedTest.java index 9a72df3ea51e..9dcbba94ad94 100644 --- a/plugins/drs/cluster/weighted/src/test/java/org/apache/cloudstack/cluster/WeightedTest.java +++ b/plugins/drs/cluster/weighted/src/test/java/org/apache/cloudstack/cluster/WeightedTest.java @@ -16,11 +16,19 @@ // under the License. package org.apache.cloudstack.cluster; +import java.util.ArrayList; import java.util.HashMap; +import java.util.List; import java.util.Map; import javax.naming.ConfigurationException; +import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.core.LogEvent; +import org.apache.logging.log4j.core.Logger; +import org.apache.logging.log4j.core.appender.AbstractAppender; +import org.apache.logging.log4j.core.config.Property; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -280,4 +288,62 @@ public void testStorageMotionMustEarnMoreThanAPlainMigration() throws Configurat assertFalse("the same small gain is not worth moving storage for", withStorage.third() > withStorage.second()); } + + /** Runs prepare and returns what Weighted logged at INFO or above while doing so. */ + private List logsFromPrepare() { + List events = new ArrayList<>(); + AbstractAppender appender = new AbstractAppender("WeightedTest", null, null, true, Property.EMPTY_ARRAY) { + @Override + public void append(LogEvent event) { + if (event.getLevel().isMoreSpecificThan(Level.INFO)) { + events.add(event.toImmutable()); + } + } + }; + Logger logger = (Logger) LogManager.getLogger(Weighted.class); + Level level = logger.getLevel(); + appender.start(); + logger.addAppender(appender); + logger.setLevel(Level.INFO); + try { + weighted.prepare(cluster, cpu, memory, load); + } finally { + logger.removeAppender(appender); + logger.setLevel(level); + appender.stop(); + } + return events; + } + + @Test + public void testFallingBackBecauseNoHostIsMeasuredIsAWarning() { + host(1L, 0.20, 0.20, HostLoad.UNKNOWN); + host(2L, 0.60, 0.60, HostLoad.UNKNOWN); + + List events = logsFromPrepare(); + + assertEquals(1, events.size()); + assertEquals(Level.WARN, events.get(0).getLevel()); + assertTrue(events.get(0).getMessage().getFormattedMessage().contains("allocation only")); + } + + @Test + public void testFallingBackBecauseSomeHostsAreUnmeasuredIsLoggedOnce() { + host(1L, 0.20, 0.20, new HostLoad(0.2, 0.2, 10)); + host(2L, 0.60, 0.60, HostLoad.UNKNOWN); + + List events = logsFromPrepare(); + + assertEquals(1, events.size()); + assertEquals(Level.INFO, events.get(0).getLevel()); + assertTrue(events.get(0).getMessage().getFormattedMessage().startsWith("1 of 2 hosts")); + } + + @Test + public void testNothingIsLoggedWhenEveryHostIsMeasured() { + host(1L, 0.20, 0.20, new HostLoad(0.2, 0.2, 10)); + host(2L, 0.60, 0.60, new HostLoad(0.6, 0.6, 10)); + + assertTrue(logsFromPrepare().isEmpty()); + } } diff --git a/server/src/main/java/com/cloud/agent/manager/allocator/impl/HostLoadTracker.java b/server/src/main/java/com/cloud/agent/manager/allocator/impl/HostLoadTracker.java index cd51ae2d8686..6e4a4e503932 100644 --- a/server/src/main/java/com/cloud/agent/manager/allocator/impl/HostLoadTracker.java +++ b/server/src/main/java/com/cloud/agent/manager/allocator/impl/HostLoadTracker.java @@ -24,10 +24,14 @@ import javax.inject.Inject; +import org.apache.cloudstack.cluster.ClusterDrsService; import org.apache.cloudstack.framework.config.ConfigKey; import org.apache.cloudstack.framework.config.Configurable; import org.apache.cloudstack.managed.context.ManagedContextRunnable; +import com.cloud.dc.ClusterDetailsDao; +import com.cloud.dc.ClusterDetailsVO; +import com.cloud.dc.dao.ClusterDao; import com.cloud.host.HostLoad; import com.cloud.host.HostLoadService; import com.cloud.host.HostStats; @@ -52,6 +56,9 @@ * host, so all of them converge on the same picture, and a restarted server simply reports nothing * usable until it has sampled - callers then fall back to allocation figures. * + * Two things read these figures: the 'balancedweighted' allocation algorithm and the 'weighted' DRS + * algorithm. Hosts are sampled while either is configured, and not otherwise. + * * What getCpuUtilization means depends on the hypervisor, and only KVM reports what this class * assumes: * @@ -69,8 +76,9 @@ public class HostLoadTracker extends ManagerBase implements HostLoadService, Con public static final ConfigKey HostLoadSampleInterval = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED, Integer.class, "host.load.sample.interval", "60", - "Seconds between samples of host CPU and memory utilisation, for placement algorithms that " + - "consider actual load. Should not be shorter than host.stats.interval.", + "Seconds between samples of host CPU and memory utilisation, for the 'balancedweighted' allocation " + + "algorithm and the 'weighted' DRS algorithm. Hosts are only sampled while one of them is " + + "configured. Should not be shorter than host.stats.interval.", false, ConfigKey.Scope.Global); public static final ConfigKey HostLoadStaleAfter = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED, @@ -86,12 +94,24 @@ public class HostLoadTracker extends ManagerBase implements HostLoadService, Con "slowly and are less affected by short spikes or by guests periodically releasing memory.", true, ConfigKey.Scope.Global); + /** + * The name the 'weighted' DRS algorithm registers under. It lives in a plugin, which this module + * cannot depend on. + */ + private static final String WEIGHTED_DRS_ALGORITHM = "weighted"; + @Inject private HostDao hostDao; @Inject private StatsCollector statsCollector; + @Inject + private ClusterDetailsDao clusterDetailsDao; + + @Inject + private ClusterDao clusterDao; + private final Map samples = new ConcurrentHashMap<>(); private ScheduledExecutorService executor; @@ -139,11 +159,32 @@ protected void sampleAllHosts() { } /** - * Only the placement algorithms that read these figures pay for collecting them. + * Only pay for collecting these figures while something reads them: the 'balancedweighted' + * allocator, or the 'weighted' DRS algorithm on any cluster. The DRS algorithm is chosen per + * cluster and independently of the allocator, so the allocator alone is not enough to decide. + * Without samples, 'weighted' DRS would balance on allocation only. */ protected boolean isInUse() { - return AllocationAlgorithm.balancedweighted.toString() - .equals(DeploymentClusterPlanner.VmAllocationAlgorithm.value()); + if (AllocationAlgorithm.balancedweighted.toString() + .equals(DeploymentClusterPlanner.VmAllocationAlgorithm.value())) { + return true; + } + // a cluster without its own value inherits the global one. Asked of cluster_details + // directly rather than per cluster: the config cache expires sooner than the sample + // interval, so a per-cluster loop would cost a query per cluster every time. If every + // cluster overrides a global 'weighted' away, hosts are sampled for nothing, which is + // harmless. + if (WEIGHTED_DRS_ALGORITHM.equals(ClusterDrsService.ClusterDrsAlgorithm.value())) { + return true; + } + for (ClusterDetailsVO detail : clusterDetailsDao.findDetails(ClusterDrsService.ClusterDrsAlgorithm.key(), + WEIGHTED_DRS_ALGORITHM, null)) { + // deleting a cluster only marks it removed and leaves its details behind + if (clusterDao.findById(detail.getResourceId()) != null) { + return true; + } + } + return false; } protected void record(long hostId, HostStats stats) { diff --git a/server/src/test/java/com/cloud/agent/manager/allocator/impl/HostLoadTrackerTest.java b/server/src/test/java/com/cloud/agent/manager/allocator/impl/HostLoadTrackerTest.java index 0f8b38f695ea..9ca1a3a4518b 100644 --- a/server/src/test/java/com/cloud/agent/manager/allocator/impl/HostLoadTrackerTest.java +++ b/server/src/test/java/com/cloud/agent/manager/allocator/impl/HostLoadTrackerTest.java @@ -16,16 +16,32 @@ // under the License. package com.cloud.agent.manager.allocator.impl; +import java.util.ArrayList; +import java.util.List; + +import org.apache.cloudstack.framework.config.ConfigKey; +import org.apache.cloudstack.framework.config.impl.ConfigDepotImpl; +import org.junit.After; import org.junit.Before; import com.cloud.host.HostLoad; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.InjectMocks; +import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; +import com.cloud.dc.ClusterDetailsDao; +import com.cloud.dc.ClusterDetailsVO; +import com.cloud.dc.ClusterVO; +import com.cloud.dc.dao.ClusterDao; +import com.cloud.host.Host; import com.cloud.host.HostStats; +import com.cloud.host.HostVO; +import com.cloud.host.Status; +import com.cloud.host.dao.HostDao; +import com.cloud.server.StatsCollector; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -37,6 +53,21 @@ public class HostLoadTrackerTest { private static final long HOST_ID = 1L; private static final long HALF_LIFE_MS = 300 * 1000L; + @Mock + private ClusterDetailsDao clusterDetailsDao; + + @Mock + private ClusterDao clusterDao; + + @Mock + private HostDao hostDao; + + @Mock + private StatsCollector statsCollector; + + @Mock + private ConfigDepotImpl configDepot; + @InjectMocks private HostLoadTracker tracker = new HostLoadTracker(); @@ -46,6 +77,17 @@ public class HostLoadTrackerTest { public void setUp() { tracker.clear(); now = 1_000_000L; + // unstubbed settings read as null, i.e. their defaults + ConfigKey.init(configDepot); + } + + @After + public void tearDown() { + ConfigKey.init(null); + } + + private void setting(String name, ConfigKey.Scope scope, Long scopeId, String value) { + Mockito.lenient().when(configDepot.getConfigStringValue(name, scope, scopeId)).thenReturn(value); } private HostStats stats(double cpuPercent, double usedMemoryFraction) { @@ -181,4 +223,78 @@ public void testOutOfRangeValuesAreClamped() { assertEquals(1.0, load.getCpuUtilisation(), 1e-6); assertEquals(1.0, load.getMemoryUtilisation(), 1e-6); } + + private void clustersWithWeightedDrs(long... clusterIds) { + List details = new ArrayList<>(); + for (long clusterId : clusterIds) { + details.add(new ClusterDetailsVO(clusterId, "drs.algorithm", "weighted")); + Mockito.lenient().when(clusterDao.findById(clusterId)).thenReturn(Mockito.mock(ClusterVO.class)); + } + Mockito.when(clusterDetailsDao.findDetails("drs.algorithm", "weighted", null)).thenReturn(details); + } + + @Test + public void testInUseWhenAClusterUsesWeightedDrsWithTheAllocatorAtItsDefault() { + clustersWithWeightedDrs(8L); + + assertTrue(tracker.isInUse()); + } + + @Test + public void testNotInUseWhenOnlyARemovedClusterUsedWeightedDrs() { + Mockito.when(clusterDetailsDao.findDetails("drs.algorithm", "weighted", null)) + .thenReturn(List.of(new ClusterDetailsVO(9L, "drs.algorithm", "weighted"))); + // findById does not return removed clusters + + assertFalse(tracker.isInUse()); + } + + @Test + public void testInUseWhenWeightedDrsIsTheGlobalDefaultWithTheAllocatorAtItsDefault() { + setting("drs.algorithm", ConfigKey.Scope.Global, null, "weighted"); + + assertTrue(tracker.isInUse()); + Mockito.verifyNoInteractions(clusterDetailsDao); + } + + @Test + public void testNotInUseWhenNeitherTheAllocatorNorAnyClusterReadsTheFigures() { + clustersWithWeightedDrs(); + + assertFalse(tracker.isInUse()); + } + + @Test + public void testInUseWhenTheAllocatorReadsTheFiguresWithoutLookingAtClusters() { + setting("vm.allocation.algorithm", ConfigKey.Scope.Global, null, "balancedweighted"); + + assertTrue(tracker.isInUse()); + Mockito.verifyNoInteractions(clusterDetailsDao); + } + + @Test + public void testHostsAreSampledForWeightedDrsWithTheAllocatorAtItsDefault() { + clustersWithWeightedDrs(7L); + HostVO host = Mockito.mock(HostVO.class); + Mockito.when(host.getId()).thenReturn(HOST_ID); + Mockito.when(host.getStatus()).thenReturn(Status.Up); + Mockito.when(hostDao.listByType(Host.Type.Routing)).thenReturn(List.of(host)); + HostStats reading = stats(40, 0.5); + Mockito.when(statsCollector.getHostStats(HOST_ID)).thenReturn(reading); + + tracker.sampleAllHosts(); + + assertTrue(tracker.getLoad(HOST_ID).isUsable()); + } + + @Test + public void testSamplesAreDroppedOnceNothingReadsThem() { + sample(40, 0.5, 0); + clustersWithWeightedDrs(); + + tracker.sampleAllHosts(); + + assertFalse(load().isUsable()); + Mockito.verifyNoInteractions(hostDao); + } }