diff --git a/api/src/main/java/org/apache/cloudstack/affinity/AffinityProcessorBase.java b/api/src/main/java/org/apache/cloudstack/affinity/AffinityProcessorBase.java index 9995d8039e1f..550e61a68655 100644 --- a/api/src/main/java/org/apache/cloudstack/affinity/AffinityProcessorBase.java +++ b/api/src/main/java/org/apache/cloudstack/affinity/AffinityProcessorBase.java @@ -25,7 +25,9 @@ import com.cloud.vm.VirtualMachineProfile; import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; public class AffinityProcessorBase extends AdapterBase implements AffinityGroupProcessor { @@ -41,6 +43,23 @@ public void process(VirtualMachineProfile vm, DeploymentPlan plan, ExcludeList a } + + /** + * Indexes placements supplied by the caller. Callers such as DRS build a plan of several moves + * in memory and persist it only at the end, so during planning the database still shows the old + * host for every VM the plan has already moved. + */ + protected Map getVmIdVmMap(List vmList) { + Map vmIdVmMap = new HashMap<>(); + if (vmList == null) { + return vmIdVmMap; + } + for (VirtualMachine vm : vmList) { + vmIdVmMap.put(vm.getId(), vm); + } + return vmIdVmMap; + } + @Override public String getType() { return _type; diff --git a/plugins/affinity-group-processors/host-anti-affinity/src/main/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessor.java b/plugins/affinity-group-processors/host-anti-affinity/src/main/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessor.java index bd29a48f2588..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 @@ -17,6 +17,7 @@ package org.apache.cloudstack.affinity; import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.stream.Collectors; @@ -59,7 +60,7 @@ public class HostAntiAffinityProcessor extends AffinityProcessorBase implements protected AffinityGroupDao _affinityGroupDao; @Inject protected AffinityGroupVMMapDao _affinityGroupVMMapDao; - private int _vmCapacityReleaseInterval; + protected int _vmCapacityReleaseInterval; @Inject protected ConfigurationDao _configDao; @@ -82,7 +83,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 +91,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,24 +113,35 @@ 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()) { - 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..3ef05ce07bfb --- /dev/null +++ b/plugins/affinity-group-processors/host-anti-affinity/src/test/java/org/apache/cloudstack/affinity/HostAntiAffinityProcessorTest.java @@ -0,0 +1,233 @@ +// 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.Collections; +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.lenient; +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 long PLANNED_HOST_ID = 12L; + 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); + // 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 + 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)); + when(_affinityGroupVMMapDao.listVmIdsByAffinityGroup(AFFINITY_GROUP_ID)).thenReturn(ids); + + processor.processAffinityGroup(vmGroupMapping, avoid, vm); + + assertTrue(avoid.getHostsToAvoid() == null || avoid.getHostsToAvoid().isEmpty()); + } +} 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..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 @@ -17,6 +17,7 @@ package org.apache.cloudstack.affinity; import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.Map; @@ -67,13 +68,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 +95,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); 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); + } } 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..ac7f621625f1 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(); @@ -761,6 +787,47 @@ void processPlans() { } } + /** + * 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. + * + * 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 + * @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, + 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 (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); + } + /** * 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. @@ -783,23 +850,51 @@ 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 { - 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, dispatchedSourceHosts)) { + 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; + } + 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 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 6390b29097b5..c5d0c47809bd 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,16 @@ import com.cloud.exception.InvalidParameterValueException; 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; 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; @@ -61,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; @@ -70,6 +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; @@ -77,6 +86,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) @@ -123,6 +133,9 @@ public class ClusterDrsServiceImplTest { @Mock private AffinityGroupVMMapDao affinityGroupVMMapDao; + @Mock + private AsyncJobManager asyncJobManager; + @Mock private VMInstanceDetailsDao vmInstanceDetailsDao; @@ -951,4 +964,176 @@ 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 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(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) + .applyAffinityConstraints(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()); + } + + @Test + public void testDestinationRefusedWhenAffinityExcludesIt() { + VMInstanceVO vm = vmWithAffinityGroup(1L, 10L); + affinityExcludes(20L); + + assertTrue(clusterDrsService.destinationViolatesAffinity(vm, host(20L), + Collections.emptyList(), Collections.emptyList())); + } + + @Test + public void testDestinationAllowedWhenAffinityExcludesSomewhereElse() { + VMInstanceVO vm = vmWithAffinityGroup(1L, 10L); + affinityExcludes(21L); + + assertFalse(clusterDrsService.destinationViolatesAffinity(vm, host(20L), + Collections.emptyList(), Collections.emptyList())); + } + + @Test + public void testDestinationRefusedWhenVmIsNoLongerRunning() { + VMInstanceVO vm = Mockito.mock(VMInstanceVO.class); + Mockito.when(vm.getHostId()).thenReturn(null); + + assertTrue("a plan for a VM that has since stopped is out of date", + clusterDrsService.destinationViolatesAffinity(vm, host(20L), + Collections.emptyList(), Collections.emptyList())); + } + + @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); + + // 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 + 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 host20 = host(20L); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(hostDao.findById(20L)).thenReturn(host20); + + Mockito.doReturn(true).when(clusterDrsService) + .destinationViolatesAffinity(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()); + + clusterDrsService.executeDrsPlan(plan); + + Mockito.verify(migration).setStatus(JobInfo.Status.CANCELLED); + 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)); + } }