From 28385be60998852f1419c5a77d3e0bba6e069807 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Flauzino?= Date: Mon, 6 Dec 2021 11:06:10 -0300 Subject: [PATCH] Fix metrics stats for VMs not running (#5633) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix metrics stats for VMs that are not running * Improves the way to get vmIdsToRemoveStats * Improves test Co-authored-by: José Flauzino --- .../main/java/com/cloud/vm/dao/UserVmDao.java | 6 ++ .../java/com/cloud/vm/dao/UserVmDaoImpl.java | 31 +++++++-- .../java/com/cloud/server/StatsCollector.java | 32 ++++++++- .../java/com/cloud/vm/UserVmManagerImpl.java | 7 ++ .../com/cloud/server/StatsCollectorTest.java | 65 +++++++++++++++++++ ...countManagerImplVolumeDeleteEventTest.java | 4 ++ 6 files changed, 139 insertions(+), 6 deletions(-) diff --git a/engine/schema/src/main/java/com/cloud/vm/dao/UserVmDao.java b/engine/schema/src/main/java/com/cloud/vm/dao/UserVmDao.java index 55d8fa02db1..c53936f7d2d 100644 --- a/engine/schema/src/main/java/com/cloud/vm/dao/UserVmDao.java +++ b/engine/schema/src/main/java/com/cloud/vm/dao/UserVmDao.java @@ -60,6 +60,12 @@ public interface UserVmDao extends GenericDao { */ public List listRunningByHostId(long hostId); + /** + * List all running VMs. + * @return the list of all VM instances that are running. + */ + public List listAllRunning(); + /** * List user vm instances with virtualized networking (i.e. not direct attached networking) for the given account and datacenter * @param accountId will search for vm instances belonging to this account diff --git a/engine/schema/src/main/java/com/cloud/vm/dao/UserVmDaoImpl.java b/engine/schema/src/main/java/com/cloud/vm/dao/UserVmDaoImpl.java index a903c3310bb..9cfa7a75c4d 100644 --- a/engine/schema/src/main/java/com/cloud/vm/dao/UserVmDaoImpl.java +++ b/engine/schema/src/main/java/com/cloud/vm/dao/UserVmDaoImpl.java @@ -67,6 +67,7 @@ public class UserVmDaoImpl extends GenericDaoBase implements Use protected SearchBuilder LastHostSearch; protected SearchBuilder HostUpSearch; protected SearchBuilder HostRunningSearch; + protected SearchBuilder RunningSearch; protected SearchBuilder StateChangeSearch; protected SearchBuilder AccountHostSearch; @@ -134,9 +135,8 @@ public class UserVmDaoImpl extends GenericDaoBase implements Use HostSearch.and("host", HostSearch.entity().getHostId(), SearchCriteria.Op.EQ); HostSearch.done(); - LastHostSearch = createSearchBuilder(); + LastHostSearch = createSearchBuilderWithStateCriteria(SearchCriteria.Op.EQ); LastHostSearch.and("lastHost", LastHostSearch.entity().getLastHostId(), SearchCriteria.Op.EQ); - LastHostSearch.and("state", LastHostSearch.entity().getState(), SearchCriteria.Op.EQ); LastHostSearch.done(); HostUpSearch = createSearchBuilder(); @@ -144,11 +144,13 @@ public class UserVmDaoImpl extends GenericDaoBase implements Use HostUpSearch.and("states", HostUpSearch.entity().getState(), SearchCriteria.Op.NIN); HostUpSearch.done(); - HostRunningSearch = createSearchBuilder(); + HostRunningSearch = createSearchBuilderWithStateCriteria(SearchCriteria.Op.EQ); HostRunningSearch.and("host", HostRunningSearch.entity().getHostId(), SearchCriteria.Op.EQ); - HostRunningSearch.and("state", HostRunningSearch.entity().getState(), SearchCriteria.Op.EQ); HostRunningSearch.done(); + RunningSearch = createSearchBuilderWithStateCriteria(SearchCriteria.Op.EQ); + RunningSearch.done(); + AccountPodSearch = createSearchBuilder(); AccountPodSearch.and("account", AccountPodSearch.entity().getAccountId(), SearchCriteria.Op.EQ); AccountPodSearch.and("pod", AccountPodSearch.entity().getPodIdToDeployIn(), SearchCriteria.Op.EQ); @@ -209,6 +211,19 @@ public class UserVmDaoImpl extends GenericDaoBase implements Use assert _updateTimeAttr != null : "Couldn't get this updateTime attribute"; } + /** + * Creates an {@link com.cloud.vm.UserVmVO UserVmVO} search builder with a + * {@link com.cloud.utils.db.SearchCriteria.Op SearchCriteria.Op} condition + * to the 'state' criteria already included. + * @param searchCriteria the {@link com.cloud.utils.db.SearchCriteria.Op SearchCriteria.Op} to 'state' criteria. + * @return the {@link com.cloud.vm.UserVmVO UserVmVO} search builder. + */ + protected SearchBuilder createSearchBuilderWithStateCriteria(SearchCriteria.Op searchCriteria) { + SearchBuilder genericSearchBuilderWithStateCriteria = createSearchBuilder(); + genericSearchBuilderWithStateCriteria.and("state", genericSearchBuilderWithStateCriteria.entity().getState(), searchCriteria); + return genericSearchBuilderWithStateCriteria; + } + @Override public List listByAccountAndPod(long accountId, long podId) { SearchCriteria sc = AccountPodSearch.create(); @@ -298,6 +313,14 @@ public class UserVmDaoImpl extends GenericDaoBase implements Use return listBy(sc); } + @Override + public List listAllRunning() { + SearchCriteria sc = RunningSearch.create(); + sc.setParameters("state", State.Running); + + return listBy(sc); + } + @Override public List listVirtualNetworkInstancesByAcctAndNetwork(long accountId, long networkId) { diff --git a/server/src/main/java/com/cloud/server/StatsCollector.java b/server/src/main/java/com/cloud/server/StatsCollector.java index c14f88c5a73..9b193c04b4a 100644 --- a/server/src/main/java/com/cloud/server/StatsCollector.java +++ b/server/src/main/java/com/cloud/server/StatsCollector.java @@ -238,7 +238,7 @@ public class StatsCollector extends ManagerBase implements ComponentMethodInterc @Inject private ClusterDao _clusterDao; @Inject - private UserVmDao _userVmDao; + protected UserVmDao _userVmDao; @Inject private VolumeDao _volsDao; @Inject @@ -291,7 +291,7 @@ public class StatsCollector extends ManagerBase implements ComponentMethodInterc private ImageStoreDetailsUtil imageStoreDetailsUtil; private ConcurrentHashMap _hostStats = new ConcurrentHashMap(); - private final ConcurrentHashMap _VmStats = new ConcurrentHashMap(); + protected ConcurrentHashMap _VmStats = new ConcurrentHashMap(); private final Map _volumeStats = new ConcurrentHashMap(); private ConcurrentHashMap _storageStats = new ConcurrentHashMap(); private ConcurrentHashMap _storagePoolStats = new ConcurrentHashMap(); @@ -619,6 +619,8 @@ public class StatsCollector extends ManagerBase implements ComponentMethodInterc } } + cleanUpVirtualMachineStats(); + } catch (Throwable t) { s_logger.error("Error trying to retrieve VM stats", t); } @@ -1485,6 +1487,32 @@ public class StatsCollector extends ManagerBase implements ComponentMethodInterc } } + /** + * Removes stats for a given virtual machine. + * @param vmId ID of the virtual machine to remove stats. + */ + public void removeVirtualMachineStats(Long vmId) { + s_logger.debug(String.format("Removing stats from VM with ID: %s .",vmId)); + _VmStats.remove(vmId); + } + + /** + * Removes stats of virtual machines that are not running from memory. + */ + protected void cleanUpVirtualMachineStats() { + List allRunningVmIds = new ArrayList(); + for (UserVmVO vm : _userVmDao.listAllRunning()) { + allRunningVmIds.add(vm.getId()); + } + + List vmIdsToRemoveStats = new ArrayList(_VmStats.keySet()); + vmIdsToRemoveStats.removeAll(allRunningVmIds); + + for (Long vmId : vmIdsToRemoveStats) { + removeVirtualMachineStats(vmId); + } + } + /** * Sends host metrics to a configured InfluxDB host. The metrics respects the following specification.
* Tags:vm_id, uuid, instance_name, data_center_id, host_id
diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index da6b55be32f..07d1f2b80a9 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -269,6 +269,7 @@ import com.cloud.resource.ResourceManager; import com.cloud.resource.ResourceState; import com.cloud.server.ManagementService; import com.cloud.server.ResourceTag; +import com.cloud.server.StatsCollector; import com.cloud.service.ServiceOfferingVO; import com.cloud.service.dao.ServiceOfferingDao; import com.cloud.service.dao.ServiceOfferingDetailsDao; @@ -552,6 +553,8 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir @Autowired @Qualifier("networkHelper") protected NetworkHelper nwHelper; + @Inject + private StatsCollector statsCollector; private ScheduledExecutorService _executor = null; private ScheduledExecutorService _vmIpFetchExecutor = null; @@ -5001,6 +5004,8 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir throw new InvalidParameterValueException("unable to find a virtual machine with id " + vmId); } + statsCollector.removeVirtualMachineStats(vmId); + _userDao.findById(userId); boolean status = false; try { @@ -5306,6 +5311,8 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir return vm; } + statsCollector.removeVirtualMachineStats(vmId); + boolean status; State vmState = vm.getState(); diff --git a/server/src/test/java/com/cloud/server/StatsCollectorTest.java b/server/src/test/java/com/cloud/server/StatsCollectorTest.java index 70416e886f3..4ee2099da6b 100644 --- a/server/src/test/java/com/cloud/server/StatsCollectorTest.java +++ b/server/src/test/java/com/cloud/server/StatsCollectorTest.java @@ -21,9 +21,11 @@ package com.cloud.server; import java.net.URI; import java.net.URISyntaxException; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; import org.influxdb.InfluxDB; @@ -34,6 +36,8 @@ import org.influxdb.dto.Point; import org.junit.Assert; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; import org.mockito.Mockito; import org.powermock.api.mockito.PowerMockito; import org.powermock.core.classloader.annotations.PrepareForTest; @@ -44,6 +48,9 @@ import com.cloud.agent.api.VmDiskStatsEntry; import com.cloud.server.StatsCollector.ExternalStatsProtocol; import com.cloud.user.VmDiskStatisticsVO; import com.cloud.utils.exception.CloudRuntimeException; +import com.cloud.vm.UserVmVO; +import com.cloud.vm.VmStats; +import com.cloud.vm.dao.UserVmDao; import com.tngtech.java.junit.dataprovider.DataProvider; import com.tngtech.java.junit.dataprovider.DataProviderRunner; @@ -51,6 +58,8 @@ import com.tngtech.java.junit.dataprovider.DataProviderRunner; @PowerMockRunnerDelegate(DataProviderRunner.class) @PrepareForTest({InfluxDBFactory.class, BatchPoints.class}) public class StatsCollectorTest { + + @InjectMocks private StatsCollector statsCollector = Mockito.spy(new StatsCollector()); private static final int GRAPHITE_DEFAULT_PORT = 2003; @@ -60,6 +69,18 @@ public class StatsCollectorTest { private static final String DEFAULT_DATABASE_NAME = "cloudstack"; + @Mock + ConcurrentHashMap vmStatsMock; + + @Mock + VmStats singleVmStatsMock; + + @Mock + UserVmDao userVmDaoMock; + + @Mock + UserVmVO userVmVOMock; + @Test public void createInfluxDbConnectionTest() { configureAndTestCreateInfluxDbConnection(true); @@ -218,4 +239,48 @@ public class StatsCollectorTest { boolean result = statsCollector.areAllDiskStatsZero(vmDiskStatsEntry); Assert.assertEquals(expected, result); } + + @Test + public void removeVirtualMachineStatsTestRemoveOneVmStats() { + Mockito.doReturn(new Object()).when(vmStatsMock).remove(Mockito.anyLong()); + + statsCollector.removeVirtualMachineStats(1l); + + Mockito.verify(vmStatsMock, Mockito.times(1)).remove(Mockito.anyLong()); + } + + @Test + public void cleanUpVirtualMachineStatsTestDoNothing() { + Mockito.doReturn(new ArrayList<>()).when(userVmDaoMock).listAllRunning(); + Mockito.doReturn(new ConcurrentHashMap(new HashMap<>()).keySet()) + .when(vmStatsMock).keySet(); + + statsCollector.cleanUpVirtualMachineStats(); + + Mockito.verify(statsCollector, Mockito.never()).removeVirtualMachineStats(Mockito.anyLong()); + } + + @Test + public void cleanUpVirtualMachineStatsTestRemoveOneVmStats() { + Mockito.doReturn(new ArrayList<>()).when(userVmDaoMock).listAllRunning(); + Mockito.doReturn(1l).when(userVmVOMock).getId(); + Mockito.doReturn(new ConcurrentHashMap(Map.of(1l, singleVmStatsMock)).keySet()) + .when(vmStatsMock).keySet(); + + statsCollector.cleanUpVirtualMachineStats(); + + Mockito.verify(vmStatsMock, Mockito.times(1)).remove(Mockito.anyLong()); + } + + @Test + public void cleanUpVirtualMachineStatsTestRemoveOnlyOneVmStats() { + Mockito.doReturn(1l).when(userVmVOMock).getId(); + Mockito.doReturn(Arrays.asList(userVmVOMock)).when(userVmDaoMock).listAllRunning(); + Mockito.doReturn(new ConcurrentHashMap(Map.of(1l, singleVmStatsMock, 2l, singleVmStatsMock)).keySet()) + .when(vmStatsMock).keySet(); + + statsCollector.cleanUpVirtualMachineStats(); + + Mockito.verify(vmStatsMock, Mockito.times(1)).remove(Mockito.anyLong()); + } } diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplVolumeDeleteEventTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplVolumeDeleteEventTest.java index ce0e7961ab5..37c48ad8bd7 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplVolumeDeleteEventTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplVolumeDeleteEventTest.java @@ -54,6 +54,7 @@ import com.cloud.event.UsageEventVO; import com.cloud.exception.AgentUnavailableException; import com.cloud.exception.CloudException; import com.cloud.exception.ConcurrentOperationException; +import com.cloud.server.StatsCollector; import com.cloud.service.ServiceOfferingVO; import com.cloud.storage.Volume.Type; import com.cloud.storage.VolumeVO; @@ -75,6 +76,8 @@ public class AccountManagerImplVolumeDeleteEventTest extends AccountManagetImplT @Mock private UserVmManager userVmManager; + @Mock + private StatsCollector statsCollectorMock; Map oldFields = new HashMap<>(); UserVmVO vm = mock(UserVmVO.class); @@ -201,6 +204,7 @@ public class AccountManagerImplVolumeDeleteEventTest extends AccountManagetImplT // volume. public void runningVMRootVolumeUsageEvent() throws SecurityException, IllegalArgumentException, ReflectiveOperationException, AgentUnavailableException, ConcurrentOperationException, CloudException { + Mockito.doNothing().when(statsCollectorMock).removeVirtualMachineStats(Mockito.anyLong()); Mockito.lenient().when(_vmMgr.destroyVm(nullable(Long.class), nullable(Boolean.class))).thenReturn(vm); List emittedEvents = deleteUserAccountRootVolumeUsageEvents(false); UsageEventVO event = emittedEvents.get(0);