From 9623b17ca695052a7b414a35f6d674122b638292 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Mon, 20 Jul 2026 12:40:07 +0200 Subject: [PATCH 01/10] optimise prometheus scraping workflow --- .../metrics/PrometheusExporterImpl.java | 12 ++++--- .../com/cloud/alert/AlertManagerImpl.java | 33 ++++++++++++++----- 2 files changed, 33 insertions(+), 12 deletions(-) diff --git a/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterImpl.java b/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterImpl.java index b49f11c77745..1d540cee5bea 100644 --- a/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterImpl.java +++ b/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterImpl.java @@ -32,7 +32,6 @@ import org.apache.cloudstack.storage.datastore.db.ImageStoreDao; import org.apache.commons.lang3.StringUtils; -import com.cloud.alert.AlertManager; import com.cloud.api.ApiDBUtils; import com.cloud.api.query.dao.DomainJoinDao; import com.cloud.api.query.dao.StoragePoolJoinDao; @@ -126,8 +125,6 @@ public String toString() { @Inject private DomainJoinDao domainDao; @Inject - private AlertManager alertManager; - @Inject DedicatedResourceDao _dedicatedDao; @Inject private AccountDao _accountDao; @@ -494,10 +491,17 @@ private void addVMsBySizeMetrics(final List metricsList, final long dcId, public void updateMetrics() { final List latestMetricsItems = new ArrayList(); try { + // NOTE: capacity data is refreshed independently by AlertManagerImpl's own + // periodic CapacityChecker timer (see AlertManagerImpl#start()). Do NOT force a + // synchronous recalculateCapacity() here: it spins up a fresh thread pool per host + // and per storage pool across ALL zones on every single scrape, so with Z zones a + // single Prometheus scrape triggered Z redundant full recalculations. That extra, + // uncoordinated load compounds over time (thread churn + overlapping runs with the + // timer) and was the cause of https://github.com/apache/cloudstack/issues/13586 + // (scrape_duration_seconds climbing until a management-server restart). for (final DataCenterVO dc : dcDao.listAll()) { final String zoneName = dc.getName(); final String zoneUuid = dc.getUuid(); - alertManager.recalculateCapacity(); addHostMetrics(latestMetricsItems, dc.getId(), zoneName, zoneUuid); addVMMetrics(latestMetricsItems, dc.getId(), zoneName, zoneUuid); addVolumeMetrics(latestMetricsItems, dc.getId(), zoneName, zoneUuid); diff --git a/server/src/main/java/com/cloud/alert/AlertManagerImpl.java b/server/src/main/java/com/cloud/alert/AlertManagerImpl.java index 7bf00037ee4b..c85b3aa0117a 100644 --- a/server/src/main/java/com/cloud/alert/AlertManagerImpl.java +++ b/server/src/main/java/com/cloud/alert/AlertManagerImpl.java @@ -161,6 +161,8 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi private final ExecutorService _executor; + private ExecutorService _capacityExecutorService; + protected SMTPMailSender mailSender; protected String[] recipients = null; protected String senderAddress = null; @@ -249,6 +251,9 @@ public boolean start() { @Override public boolean stop() { _timer.cancel(); + if (_capacityExecutorService != null) { + _capacityExecutorService.shutdown(); + } return true; } @@ -281,6 +286,24 @@ public void sendAlert(AlertType alertType, long dataCenterId, Long podId, String } } + /** + * Shared, long-lived pool for capacity recalculation, reused across every + * recalculateHostCapacities()/recalculateStorageCapacities() call instead of creating and + * tearing down a new thread pool per invocation. Repeatedly creating/shutting down pools was + * unnecessary overhead under frequent callers (e.g. the Prometheus exporter used to trigger a + * full recalculation on every scrape, see https://github.com/apache/cloudstack/issues/13586). + * Lazily created so this remains safe for callers that invoke the recalculate methods directly + * without going through configure()/start() (e.g. unit tests). + */ + private synchronized ExecutorService getCapacityExecutorService() { + if (_capacityExecutorService == null || _capacityExecutorService.isShutdown()) { + _capacityExecutorService = Executors.newFixedThreadPool( + Math.max(1, CapacityManager.CapacityCalculateWorkers.value()), + new NamedThreadFactory("Capacity-Calculator")); + } + return _capacityExecutorService; + } + /** * Recalculates the capacities of hosts, including CPU and RAM. */ @@ -290,10 +313,8 @@ protected void recalculateHostCapacities() { return; } ConcurrentHashMap> futures = new ConcurrentHashMap<>(); - ExecutorService executorService = Executors.newFixedThreadPool(Math.max(1, - Math.min(CapacityManager.CapacityCalculateWorkers.value(), hostIds.size()))); for (Long hostId : hostIds) { - futures.put(hostId, executorService.submit(() -> { + futures.put(hostId, getCapacityExecutorService().submit(() -> { final HostVO host = hostDao.findById(hostId); _capacityMgr.updateCapacityForHost(host); return null; @@ -307,7 +328,6 @@ protected void recalculateHostCapacities() { entry.getKey(), e.getMessage()), e); } } - executorService.shutdown(); } protected void recalculateStorageCapacities() { @@ -316,10 +336,8 @@ protected void recalculateStorageCapacities() { return; } ConcurrentHashMap> futures = new ConcurrentHashMap<>(); - ExecutorService executorService = Executors.newFixedThreadPool(Math.max(1, - Math.min(CapacityManager.CapacityCalculateWorkers.value(), storagePoolIds.size()))); for (Long poolId: storagePoolIds) { - futures.put(poolId, executorService.submit(() -> { + futures.put(poolId, getCapacityExecutorService().submit(() -> { Transaction.execute(new TransactionCallbackNoReturn() { @Override public void doInTransactionWithoutResult(TransactionStatus status) { @@ -343,7 +361,6 @@ public void doInTransactionWithoutResult(TransactionStatus status) { entry.getKey(), e.getMessage()), e); } } - executorService.shutdown(); } @Override From 91b541a2e370c0980d813927172e12164b3236af Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Wed, 22 Jul 2026 11:24:55 +0200 Subject: [PATCH 02/10] move comment to javadoc --- .../cloudstack/metrics/PrometheusExporter.java | 16 ++++++++++++++++ .../metrics/PrometheusExporterImpl.java | 8 -------- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporter.java b/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporter.java index 6361f0edc6b5..8ad7f1d1164a 100644 --- a/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporter.java +++ b/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporter.java @@ -18,7 +18,23 @@ public interface PrometheusExporter { + /** + * Update the Prometheus metrics in text format. + * + * NOTE: capacity data is refreshed independently by {@code AlertManagerImpl}'s own + * periodic {@code CapacityChecker} timer. Do NOT force a synchronous + * {@code recalculateCapacity()} call here: it spins up a fresh thread pool per host + * and per storage pool across ALL zones on every single scrape, so with Z zones a + * single Prometheus scrape triggered Z redundant full recalculations. That extra, + * uncoordinated load compounds over time and can lead to {@code scrape_duration_seconds} + * climbing until a management-server restart. + * + * @see PrometheusExporterImpl#updateMetrics() + */ void updateMetrics(); + /** + * @return the latest Prometheus metrics refreshed by {@link #updateMetrics()}. + */ String getMetrics(); } diff --git a/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterImpl.java b/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterImpl.java index 1d540cee5bea..f737bad25ca2 100644 --- a/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterImpl.java +++ b/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterImpl.java @@ -491,14 +491,6 @@ private void addVMsBySizeMetrics(final List metricsList, final long dcId, public void updateMetrics() { final List latestMetricsItems = new ArrayList(); try { - // NOTE: capacity data is refreshed independently by AlertManagerImpl's own - // periodic CapacityChecker timer (see AlertManagerImpl#start()). Do NOT force a - // synchronous recalculateCapacity() here: it spins up a fresh thread pool per host - // and per storage pool across ALL zones on every single scrape, so with Z zones a - // single Prometheus scrape triggered Z redundant full recalculations. That extra, - // uncoordinated load compounds over time (thread churn + overlapping runs with the - // timer) and was the cause of https://github.com/apache/cloudstack/issues/13586 - // (scrape_duration_seconds climbing until a management-server restart). for (final DataCenterVO dc : dcDao.listAll()) { final String zoneName = dc.getName(); final String zoneUuid = dc.getUuid(); From 828857a23a3fd7efd3e899ee6c7bea280af43aef Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Tue, 4 Aug 2026 12:04:39 +0200 Subject: [PATCH 03/10] renaming to newer convention --- .../main/java/com/cloud/alert/AlertManagerImpl.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/com/cloud/alert/AlertManagerImpl.java b/server/src/main/java/com/cloud/alert/AlertManagerImpl.java index c85b3aa0117a..9141acc994a7 100644 --- a/server/src/main/java/com/cloud/alert/AlertManagerImpl.java +++ b/server/src/main/java/com/cloud/alert/AlertManagerImpl.java @@ -161,7 +161,7 @@ public class AlertManagerImpl extends ManagerBase implements AlertManager, Confi private final ExecutorService _executor; - private ExecutorService _capacityExecutorService; + private ExecutorService capacityExecutorService; protected SMTPMailSender mailSender; protected String[] recipients = null; @@ -251,8 +251,8 @@ public boolean start() { @Override public boolean stop() { _timer.cancel(); - if (_capacityExecutorService != null) { - _capacityExecutorService.shutdown(); + if (capacityExecutorService != null) { + capacityExecutorService.shutdown(); } return true; } @@ -296,12 +296,12 @@ public void sendAlert(AlertType alertType, long dataCenterId, Long podId, String * without going through configure()/start() (e.g. unit tests). */ private synchronized ExecutorService getCapacityExecutorService() { - if (_capacityExecutorService == null || _capacityExecutorService.isShutdown()) { - _capacityExecutorService = Executors.newFixedThreadPool( + if (capacityExecutorService == null || capacityExecutorService.isShutdown()) { + capacityExecutorService = Executors.newFixedThreadPool( Math.max(1, CapacityManager.CapacityCalculateWorkers.value()), new NamedThreadFactory("Capacity-Calculator")); } - return _capacityExecutorService; + return capacityExecutorService; } /** From 8b1daf02fc1188700ae17d8481fe897a193a072b Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Tue, 4 Aug 2026 12:15:54 +0200 Subject: [PATCH 04/10] more tests --- .../com/cloud/alert/AlertManagerImplTest.java | 113 ++++++++++++++++++ 1 file changed, 113 insertions(+) diff --git a/server/src/test/java/com/cloud/alert/AlertManagerImplTest.java b/server/src/test/java/com/cloud/alert/AlertManagerImplTest.java index d34d0b5873f2..b69178b11d9d 100644 --- a/server/src/test/java/com/cloud/alert/AlertManagerImplTest.java +++ b/server/src/test/java/com/cloud/alert/AlertManagerImplTest.java @@ -17,7 +17,10 @@ package com.cloud.alert; import java.io.UnsupportedEncodingException; +import java.lang.reflect.Field; import java.util.List; +import java.util.Timer; +import java.util.concurrent.ExecutorService; import javax.mail.MessagingException; @@ -219,4 +222,114 @@ public void testRecalculateStorageCapacities() { Mockito.verify(storageManager, Mockito.times(2)).createCapacityEntry(sharedPool, Capacity.CAPACITY_TYPE_STORAGE_ALLOCATED, 10L); Mockito.verify(storageManager, Mockito.times(1)).createCapacityEntry(nonSharedPool, Capacity.CAPACITY_TYPE_LOCAL_STORAGE, 20L); } + + @Test + public void testRecalculateHostCapacitiesWithEmptyHostList() throws Exception { + Mockito.when(hostDao.listIdsByType(Host.Type.Routing)).thenReturn(List.of()); + alertManagerImplMock.recalculateHostCapacities(); + Mockito.verify(hostDao, Mockito.never()).findById(Mockito.anyLong()); + Mockito.verify(capacityManager, Mockito.never()).updateCapacityForHost(Mockito.any()); + assertNull("executor should never be created when there is nothing to submit", getCapacityExecutorService()); + } + + @Test + public void testRecalculateStorageCapacitiesWithEmptyPoolList() throws Exception { + Mockito.when(primaryDataStoreDao.listAllIds()).thenReturn(List.of()); + alertManagerImplMock.recalculateStorageCapacities(); + Mockito.verify(primaryDataStoreDao, Mockito.never()).findById(Mockito.anyLong()); + Mockito.verify(storageManager, Mockito.never()).createCapacityEntry(Mockito.any(), Mockito.anyShort(), Mockito.anyLong()); + assertNull("executor should never be created when there is nothing to submit", getCapacityExecutorService()); + } + + @Test + public void testRecalculateHostCapacitiesLogsAndContinuesOnTaskFailure() { + Mockito.when(hostDao.listIdsByType(Host.Type.Routing)).thenReturn(List.of(1L, 2L, 3L)); + HostVO host1 = Mockito.mock(HostVO.class); + HostVO host2 = Mockito.mock(HostVO.class); + HostVO host3 = Mockito.mock(HostVO.class); + Mockito.when(hostDao.findById(1L)).thenReturn(host1); + Mockito.when(hostDao.findById(2L)).thenReturn(host2); + Mockito.when(hostDao.findById(3L)).thenReturn(host3); + Mockito.doThrow(new RuntimeException("boom")).when(capacityManager).updateCapacityForHost(host2); + + alertManagerImplMock.recalculateHostCapacities(); + + Mockito.verify(capacityManager).updateCapacityForHost(host1); + Mockito.verify(capacityManager).updateCapacityForHost(host2); + Mockito.verify(capacityManager).updateCapacityForHost(host3); + Mockito.verify(alertManagerImplMock.logger).error(Mockito.anyString(), Mockito.any(Throwable.class)); + } + + @Test + public void testRecalculateHostCapacitiesReusesExecutorAcrossCalls() throws Exception { + Mockito.when(hostDao.listIdsByType(Host.Type.Routing)).thenReturn(List.of(1L)); + Mockito.when(hostDao.findById(Mockito.anyLong())).thenReturn(Mockito.mock(HostVO.class)); + + alertManagerImplMock.recalculateHostCapacities(); + ExecutorService firstExecutor = getCapacityExecutorService(); + assertNotNull(firstExecutor); + + Mockito.when(primaryDataStoreDao.listAllIds()).thenReturn(List.of(101L)); + StoragePoolVO pool = Mockito.mock(StoragePoolVO.class); + Mockito.when(primaryDataStoreDao.findById(101L)).thenReturn(pool); + alertManagerImplMock.recalculateStorageCapacities(); + + assertEquals("host and storage recalculation should share the same long-lived pool", + firstExecutor, getCapacityExecutorService()); + } + + @Test + public void testRecalculateHostCapacitiesRecreatesExecutorAfterShutdown() throws Exception { + Mockito.when(hostDao.listIdsByType(Host.Type.Routing)).thenReturn(List.of(1L)); + Mockito.when(hostDao.findById(Mockito.anyLong())).thenReturn(Mockito.mock(HostVO.class)); + + alertManagerImplMock.recalculateHostCapacities(); + ExecutorService firstExecutor = getCapacityExecutorService(); + firstExecutor.shutdown(); + + alertManagerImplMock.recalculateHostCapacities(); + ExecutorService secondExecutor = getCapacityExecutorService(); + + Assert.assertNotEquals("a shut down executor should be replaced rather than reused", firstExecutor, secondExecutor); + Assert.assertFalse(secondExecutor.isShutdown()); + } + + @Test + public void testStopShutsDownCapacityExecutorServiceWhenPresent() throws Exception { + Timer timerMock = Mockito.mock(Timer.class); + setTimer(timerMock); + Mockito.when(hostDao.listIdsByType(Host.Type.Routing)).thenReturn(List.of(1L)); + Mockito.when(hostDao.findById(Mockito.anyLong())).thenReturn(Mockito.mock(HostVO.class)); + alertManagerImplMock.recalculateHostCapacities(); + + boolean result = alertManagerImplMock.stop(); + + Assert.assertTrue(result); + Mockito.verify(timerMock).cancel(); + Assert.assertTrue(getCapacityExecutorService().isShutdown()); + } + + @Test + public void testStopDoesNotThrowWhenCapacityExecutorServiceNeverCreated() throws Exception { + Timer timerMock = Mockito.mock(Timer.class); + setTimer(timerMock); + + boolean result = alertManagerImplMock.stop(); + + Assert.assertTrue(result); + Mockito.verify(timerMock).cancel(); + assertNull(getCapacityExecutorService()); + } + + private ExecutorService getCapacityExecutorService() throws Exception { + Field field = AlertManagerImpl.class.getDeclaredField("capacityExecutorService"); + field.setAccessible(true); + return (ExecutorService) field.get(alertManagerImplMock); + } + + private void setTimer(Timer timer) throws Exception { + Field field = AlertManagerImpl.class.getDeclaredField("_timer"); + field.setAccessible(true); + field.set(alertManagerImplMock, timer); + } } From 7dfa353cc8e6e1235852b97a81625fc0876ddb12 Mon Sep 17 00:00:00 2001 From: prrssshhhh Date: Tue, 4 Aug 2026 23:54:28 +0530 Subject: [PATCH 05/10] prometheus: fix scrape duration growing unbounded (#13667) (#13696) --- .../metrics/PrometheusExporterImpl.java | 16 ++++- .../metrics/PrometheusExporterServer.java | 3 + .../metrics/PrometheusExporterServerImpl.java | 14 +++- .../metrics/PrometheusExporterImplTest.java | 71 +++++++++++++++++++ 4 files changed, 102 insertions(+), 2 deletions(-) diff --git a/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterImpl.java b/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterImpl.java index f737bad25ca2..82483d62a67d 100644 --- a/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterImpl.java +++ b/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterImpl.java @@ -23,6 +23,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; import javax.inject.Inject; @@ -101,6 +102,7 @@ public String toString() { } private static List metricsItems = new ArrayList<>(); + private volatile long lastMetricsUpdateTime = 0L; @Inject private DataCenterDao dcDao; @@ -488,7 +490,15 @@ private void addVMsBySizeMetrics(final List metricsList, final long dcId, } @Override - public void updateMetrics() { + public synchronized void updateMetrics() { + final long minIntervalMs = TimeUnit.SECONDS.toMillis(PrometheusExporterServer.PrometheusExporterMinRefreshInterval.value()); + final long now = System.currentTimeMillis(); + if (now - lastMetricsUpdateTime < minIntervalMs) { + logger.debug("Skipping metrics recomputation, last update was " + (now - lastMetricsUpdateTime) + "ms ago (min interval: " + minIntervalMs + "ms)"); + return; + } + + final long startNanos = System.nanoTime(); final List latestMetricsItems = new ArrayList(); try { for (final DataCenterVO dc : dcDao.listAll()) { @@ -508,8 +518,12 @@ public void updateMetrics() { addDomainResourceCount(latestMetricsItems); } catch (Exception e) { logger.warn("Getting metrics failed ", e); + } finally { + final long elapsedMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startNanos); + logger.info("Prometheus metrics update completed in " + elapsedMs + " ms"); } metricsItems = latestMetricsItems; + lastMetricsUpdateTime = System.currentTimeMillis(); } @Override diff --git a/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterServer.java b/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterServer.java index f0f5e3c6987b..f171b9ca4d7f 100644 --- a/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterServer.java +++ b/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterServer.java @@ -33,4 +33,7 @@ public interface PrometheusExporterServer extends Manager { ConfigKey PrometheusExporterOfferingCountLimit = new ConfigKey<>("Advanced", Integer.class, "prometheus.exporter.offering.output.limit", "-1", "Limit the number of output for cloudstack_vms_total_by_size to the provided value. -1 for unlimited output.", true); + + ConfigKey PrometheusExporterMinRefreshInterval = new ConfigKey<>("Advanced", Integer.class, "prometheus.exporter.metrics.min.refresh.interval", "5", + "Minimum interval in seconds between metrics recomputations. Scrapes arriving faster than this interval reuse the previously computed metrics.", true, EnablePrometheusExporter.key()); } diff --git a/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterServerImpl.java b/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterServerImpl.java index d9f25d2f5772..63aaa3b6a703 100644 --- a/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterServerImpl.java +++ b/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterServerImpl.java @@ -29,10 +29,13 @@ import java.net.InetSocketAddress; import java.nio.charset.StandardCharsets; import java.util.Arrays; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; public class PrometheusExporterServerImpl extends ManagerBase implements PrometheusExporterServer, Configurable { private static HttpServer httpServer; + private ExecutorService httpExecutor; @Inject private PrometheusExporter prometheusExporter; @@ -79,6 +82,8 @@ public boolean start() { if (EnablePrometheusExporter.value()) { try { httpServer = HttpServer.create(new InetSocketAddress(PrometheusExporterServerPort.value()), 0); + httpExecutor = Executors.newFixedThreadPool(2); + httpServer.setExecutor(httpExecutor); httpServer.createContext("/metrics", new ExporterHandler(prometheusExporter)); httpServer.createContext("/", new HttpHandler() { @Override @@ -105,9 +110,15 @@ public void handle(HttpExchange httpExchange) throws IOException { @Override public boolean stop() { if (httpServer != null) { + httpServer.setExecutor(null); httpServer.stop(0); logger.debug("Stopped Prometheus exporter http server"); } + if (httpExecutor != null) { + httpExecutor.shutdownNow(); + logger.debug("Shut down Prometheus exporter http executor"); + } + httpExecutor = null; return true; } @@ -122,7 +133,8 @@ public ConfigKey[] getConfigKeys() { EnablePrometheusExporter, PrometheusExporterServerPort, PrometheusExporterAllowedAddresses, - PrometheusExporterOfferingCountLimit + PrometheusExporterOfferingCountLimit, + PrometheusExporterMinRefreshInterval }; } } diff --git a/plugins/integrations/prometheus/src/test/java/org/apache/cloudstack/metrics/PrometheusExporterImplTest.java b/plugins/integrations/prometheus/src/test/java/org/apache/cloudstack/metrics/PrometheusExporterImplTest.java index 40490c46f56e..c6e5afc6b95f 100644 --- a/plugins/integrations/prometheus/src/test/java/org/apache/cloudstack/metrics/PrometheusExporterImplTest.java +++ b/plugins/integrations/prometheus/src/test/java/org/apache/cloudstack/metrics/PrometheusExporterImplTest.java @@ -18,6 +18,15 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.lang.reflect.Field; +import java.util.Collections; + +import com.cloud.dc.dao.DataCenterDao; import org.junit.Test; @@ -105,4 +114,66 @@ public void testItemHostCertExpiryContainsTimestampValue() { assertTrue("Metric should contain correct timestamp value", metricsString.endsWith(" " + CERT_EXPIRY_EPOCH)); } + + /** + * Two rapid calls to updateMetrics() within the min refresh interval + * should result in only one actual recomputation (one call to dcDao.listAll()). + */ + @Test + public void testUpdateMetricsTTLGuardSkipsSecondCall() throws Exception { + PrometheusExporterImpl exporter = new PrometheusExporterImpl(); + + DataCenterDao mockDcDao = mock(DataCenterDao.class); + when(mockDcDao.listAll()).thenReturn(Collections.emptyList()); + setField(exporter, "dcDao", mockDcDao); + + // First call should trigger recomputation + exporter.updateMetrics(); + // Second immediate call should be skipped by the TTL guard + exporter.updateMetrics(); + + verify(mockDcDao, times(1)).listAll(); + } + + /** + * After the min refresh interval has elapsed, updateMetrics() should + * trigger a fresh recomputation. + */ + @Test + public void testUpdateMetricsTTLGuardAllowsAfterInterval() throws Exception { + PrometheusExporterImpl exporter = new PrometheusExporterImpl(); + + DataCenterDao mockDcDao = mock(DataCenterDao.class); + when(mockDcDao.listAll()).thenReturn(Collections.emptyList()); + setField(exporter, "dcDao", mockDcDao); + + // First call + exporter.updateMetrics(); + + // Simulate that the min interval has already elapsed by resetting lastMetricsUpdateTime + setField(exporter, "lastMetricsUpdateTime", 0L); + + // Second call should now trigger recomputation + exporter.updateMetrics(); + + verify(mockDcDao, times(2)).listAll(); + } + + private static void setField(Object target, String fieldName, Object value) throws Exception { + Field field = null; + Class clazz = target.getClass(); + while (clazz != null) { + try { + field = clazz.getDeclaredField(fieldName); + break; + } catch (NoSuchFieldException e) { + clazz = clazz.getSuperclass(); + } + } + if (field == null) { + throw new NoSuchFieldException(fieldName); + } + field.setAccessible(true); + field.set(target, value); + } } From 2d2c40bebce4e7bbbaadeb90610bdafec8f0a473 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Wed, 5 Aug 2026 17:09:07 +0200 Subject: [PATCH 06/10] sonarcube --- .../cloudstack/metrics/PrometheusExporterImpl.java | 4 ++-- .../test/java/com/cloud/alert/AlertManagerImplTest.java | 9 ++++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterImpl.java b/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterImpl.java index 82483d62a67d..f0eae1998a82 100644 --- a/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterImpl.java +++ b/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterImpl.java @@ -494,7 +494,7 @@ public synchronized void updateMetrics() { final long minIntervalMs = TimeUnit.SECONDS.toMillis(PrometheusExporterServer.PrometheusExporterMinRefreshInterval.value()); final long now = System.currentTimeMillis(); if (now - lastMetricsUpdateTime < minIntervalMs) { - logger.debug("Skipping metrics recomputation, last update was " + (now - lastMetricsUpdateTime) + "ms ago (min interval: " + minIntervalMs + "ms)"); + logger.debug("Skipping metrics recomputation, last update was {}ms ago (min interval: {}ms)", now - lastMetricsUpdateTime, minIntervalMs); return; } @@ -520,7 +520,7 @@ public synchronized void updateMetrics() { logger.warn("Getting metrics failed ", e); } finally { final long elapsedMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startNanos); - logger.info("Prometheus metrics update completed in " + elapsedMs + " ms"); + logger.info("Prometheus metrics update completed in {} ms", elapsedMs); } metricsItems = latestMetricsItems; lastMetricsUpdateTime = System.currentTimeMillis(); diff --git a/server/src/test/java/com/cloud/alert/AlertManagerImplTest.java b/server/src/test/java/com/cloud/alert/AlertManagerImplTest.java index b69178b11d9d..38ba645f1bf3 100644 --- a/server/src/test/java/com/cloud/alert/AlertManagerImplTest.java +++ b/server/src/test/java/com/cloud/alert/AlertManagerImplTest.java @@ -263,7 +263,8 @@ public void testRecalculateHostCapacitiesLogsAndContinuesOnTaskFailure() { @Test public void testRecalculateHostCapacitiesReusesExecutorAcrossCalls() throws Exception { Mockito.when(hostDao.listIdsByType(Host.Type.Routing)).thenReturn(List.of(1L)); - Mockito.when(hostDao.findById(Mockito.anyLong())).thenReturn(Mockito.mock(HostVO.class)); + HostVO hostMock = Mockito.mock(HostVO.class); + Mockito.when(hostDao.findById(Mockito.anyLong())).thenReturn(hostMock); alertManagerImplMock.recalculateHostCapacities(); ExecutorService firstExecutor = getCapacityExecutorService(); @@ -281,7 +282,8 @@ public void testRecalculateHostCapacitiesReusesExecutorAcrossCalls() throws Exce @Test public void testRecalculateHostCapacitiesRecreatesExecutorAfterShutdown() throws Exception { Mockito.when(hostDao.listIdsByType(Host.Type.Routing)).thenReturn(List.of(1L)); - Mockito.when(hostDao.findById(Mockito.anyLong())).thenReturn(Mockito.mock(HostVO.class)); + HostVO hostMock = Mockito.mock(HostVO.class); + Mockito.when(hostDao.findById(Mockito.anyLong())).thenReturn(hostMock); alertManagerImplMock.recalculateHostCapacities(); ExecutorService firstExecutor = getCapacityExecutorService(); @@ -299,7 +301,8 @@ public void testStopShutsDownCapacityExecutorServiceWhenPresent() throws Excepti Timer timerMock = Mockito.mock(Timer.class); setTimer(timerMock); Mockito.when(hostDao.listIdsByType(Host.Type.Routing)).thenReturn(List.of(1L)); - Mockito.when(hostDao.findById(Mockito.anyLong())).thenReturn(Mockito.mock(HostVO.class)); + HostVO hostMock = Mockito.mock(HostVO.class); + Mockito.when(hostDao.findById(Mockito.anyLong())).thenReturn(hostMock); alertManagerImplMock.recalculateHostCapacities(); boolean result = alertManagerImplMock.stop(); From 3bd4902f0ab4baa47446f5730ac67b01b9cfd8c1 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Sun, 23 Aug 2026 13:29:22 +0200 Subject: [PATCH 07/10] sonarqube/-cloud warnings --- .../com/cloud/alert/AlertManagerImplTest.java | 161 +++++++++--------- 1 file changed, 85 insertions(+), 76 deletions(-) diff --git a/server/src/test/java/com/cloud/alert/AlertManagerImplTest.java b/server/src/test/java/com/cloud/alert/AlertManagerImplTest.java index 38ba645f1bf3..2cb6dc36edce 100644 --- a/server/src/test/java/com/cloud/alert/AlertManagerImplTest.java +++ b/server/src/test/java/com/cloud/alert/AlertManagerImplTest.java @@ -35,7 +35,6 @@ import org.mockito.ArgumentCaptor; import org.mockito.InjectMocks; import org.mockito.Mock; -import org.mockito.Mockito; import org.mockito.Spy; import org.mockito.junit.MockitoJUnitRunner; @@ -57,7 +56,17 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.anyShort; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; @RunWith(MockitoJUnitRunner.class) public class AlertManagerImplTest { @@ -110,15 +119,15 @@ public void setUp() { private void sendMessage() { try { - DataCenterVO zone = Mockito.mock(DataCenterVO.class); - Mockito.when(zone.getId()).thenReturn(0L); - Mockito.when(_dcDao.findById(0L)).thenReturn(zone); - HostPodVO pod = Mockito.mock(HostPodVO.class); - Mockito.when(pod.getId()).thenReturn(1L); - Mockito.when(_podDao.findById(1L)).thenReturn(pod); - ClusterVO cluster = Mockito.mock(ClusterVO.class); - Mockito.when(cluster.getId()).thenReturn(1L); - Mockito.when(_clusterDao.findById(1L)).thenReturn(cluster); + DataCenterVO zone = mock(DataCenterVO.class); + when(zone.getId()).thenReturn(0L); + when(_dcDao.findById(0L)).thenReturn(zone); + HostPodVO pod = mock(HostPodVO.class); + when(pod.getId()).thenReturn(1L); + when(_podDao.findById(1L)).thenReturn(pod); + ClusterVO cluster = mock(ClusterVO.class); + when(cluster.getId()).thenReturn(1L); + when(_clusterDao.findById(1L)).thenReturn(cluster); alertManagerImplMock.sendAlert(AlertManager.AlertType.ALERT_TYPE_CPU, 0, 1L, 1L, "", ""); } catch (UnsupportedEncodingException | MessagingException e) { @@ -128,39 +137,39 @@ private void sendMessage() { @Test public void sendAlertTestSendMail() { - Mockito.doReturn(null).when(_alertDao).getLastAlert(Mockito.anyShort(), Mockito.anyLong(), - Mockito.anyLong(), Mockito.anyLong()); - Mockito.doReturn(null).when(_alertDao).persist(any()); + doReturn(null).when(_alertDao).getLastAlert(anyShort(), anyLong(), + anyLong(), anyLong()); + doReturn(null).when(_alertDao).persist(any()); alertManagerImplMock.recipients = new String[]{""}; sendMessage(); - Mockito.verify(alertManagerImplMock).sendMessage(any()); + verify(alertManagerImplMock).sendMessage(any()); } @Test public void sendAlertTestDebugLogging() { - Mockito.doReturn(0).when(alertVOMock).getSentCount(); - Mockito.doReturn(alertVOMock).when(_alertDao).getLastAlert(Mockito.anyShort(), Mockito.anyLong(), - Mockito.anyLong(), Mockito.anyLong()); + doReturn(0).when(alertVOMock).getSentCount(); + doReturn(alertVOMock).when(_alertDao).getLastAlert(anyShort(), anyLong(), + anyLong(), anyLong()); sendMessage(); - Mockito.verify(alertManagerImplMock.logger).debug(Mockito.anyString()); - Mockito.verify(alertManagerImplMock, Mockito.never()).sendMessage(any()); + verify(alertManagerImplMock.logger).debug(anyString()); + verify(alertManagerImplMock, never()).sendMessage(any()); } @Test public void sendAlertTestWarnLogging() { - Mockito.doReturn(null).when(_alertDao).getLastAlert(Mockito.anyShort(), Mockito.anyLong(), - Mockito.anyLong(), Mockito.anyLong()); - Mockito.doReturn(null).when(_alertDao).persist(Mockito.any()); + doReturn(null).when(_alertDao).getLastAlert(anyShort(), anyLong(), + anyLong(), anyLong()); + doReturn(null).when(_alertDao).persist(any()); alertManagerImplMock.recipients = null; sendMessage(); - Mockito.verify(alertManagerImplMock.logger, Mockito.times(2)).warn(Mockito.anyString()); - Mockito.verify(alertManagerImplMock, Mockito.never()).sendMessage(any()); + verify(alertManagerImplMock.logger, times(2)).warn(anyString()); + verify(alertManagerImplMock, never()).sendMessage(any()); } @Test @@ -196,83 +205,83 @@ public void testSendAlertWithNullAlertType() throws MessagingException, Unsuppor @Test public void testRecalculateHostCapacities() { List mockHostIds = List.of(1L, 2L, 3L); - Mockito.when(hostDao.listIdsByType(Host.Type.Routing)).thenReturn(mockHostIds); - HostVO host = Mockito.mock(HostVO.class); - Mockito.when(hostDao.findById(Mockito.anyLong())).thenReturn(host); - Mockito.doNothing().when(capacityManager).updateCapacityForHost(host); + when(hostDao.listIdsByType(Host.Type.Routing)).thenReturn(mockHostIds); + HostVO host = mock(HostVO.class); + when(hostDao.findById(anyLong())).thenReturn(host); + doNothing().when(capacityManager).updateCapacityForHost(host); alertManagerImplMock.recalculateHostCapacities(); - Mockito.verify(hostDao, Mockito.times(3)).findById(Mockito.anyLong()); - Mockito.verify(capacityManager, Mockito.times(3)).updateCapacityForHost(host); + verify(hostDao, times(3)).findById(anyLong()); + verify(capacityManager, times(3)).updateCapacityForHost(host); } @Test public void testRecalculateStorageCapacities() { List mockPoolIds = List.of(101L, 102L, 103L); - Mockito.when(primaryDataStoreDao.listAllIds()).thenReturn(mockPoolIds); - StoragePoolVO sharedPool = Mockito.mock(StoragePoolVO.class); - Mockito.when(sharedPool.isShared()).thenReturn(true); - Mockito.when(primaryDataStoreDao.findById(mockPoolIds.get(0))).thenReturn(sharedPool); - Mockito.when(primaryDataStoreDao.findById(mockPoolIds.get(1))).thenReturn(sharedPool); - StoragePoolVO nonSharedPool = Mockito.mock(StoragePoolVO.class); - Mockito.when(nonSharedPool.isShared()).thenReturn(false); - Mockito.when(primaryDataStoreDao.findById(mockPoolIds.get(2))).thenReturn(nonSharedPool); - Mockito.when(capacityManager.getAllocatedPoolCapacity(sharedPool, null)).thenReturn(10L); - Mockito.when(capacityManager.getAllocatedPoolCapacity(nonSharedPool, null)).thenReturn(20L); + when(primaryDataStoreDao.listAllIds()).thenReturn(mockPoolIds); + StoragePoolVO sharedPool = mock(StoragePoolVO.class); + when(sharedPool.isShared()).thenReturn(true); + when(primaryDataStoreDao.findById(mockPoolIds.get(0))).thenReturn(sharedPool); + when(primaryDataStoreDao.findById(mockPoolIds.get(1))).thenReturn(sharedPool); + StoragePoolVO nonSharedPool = mock(StoragePoolVO.class); + when(nonSharedPool.isShared()).thenReturn(false); + when(primaryDataStoreDao.findById(mockPoolIds.get(2))).thenReturn(nonSharedPool); + when(capacityManager.getAllocatedPoolCapacity(sharedPool, null)).thenReturn(10L); + when(capacityManager.getAllocatedPoolCapacity(nonSharedPool, null)).thenReturn(20L); alertManagerImplMock.recalculateStorageCapacities(); - Mockito.verify(storageManager, Mockito.times(2)).createCapacityEntry(sharedPool, Capacity.CAPACITY_TYPE_STORAGE_ALLOCATED, 10L); - Mockito.verify(storageManager, Mockito.times(1)).createCapacityEntry(nonSharedPool, Capacity.CAPACITY_TYPE_LOCAL_STORAGE, 20L); + verify(storageManager, times(2)).createCapacityEntry(sharedPool, Capacity.CAPACITY_TYPE_STORAGE_ALLOCATED, 10L); + verify(storageManager, times(1)).createCapacityEntry(nonSharedPool, Capacity.CAPACITY_TYPE_LOCAL_STORAGE, 20L); } @Test public void testRecalculateHostCapacitiesWithEmptyHostList() throws Exception { - Mockito.when(hostDao.listIdsByType(Host.Type.Routing)).thenReturn(List.of()); + when(hostDao.listIdsByType(Host.Type.Routing)).thenReturn(List.of()); alertManagerImplMock.recalculateHostCapacities(); - Mockito.verify(hostDao, Mockito.never()).findById(Mockito.anyLong()); - Mockito.verify(capacityManager, Mockito.never()).updateCapacityForHost(Mockito.any()); + verify(hostDao, never()).findById(anyLong()); + verify(capacityManager, never()).updateCapacityForHost(any()); assertNull("executor should never be created when there is nothing to submit", getCapacityExecutorService()); } @Test public void testRecalculateStorageCapacitiesWithEmptyPoolList() throws Exception { - Mockito.when(primaryDataStoreDao.listAllIds()).thenReturn(List.of()); + when(primaryDataStoreDao.listAllIds()).thenReturn(List.of()); alertManagerImplMock.recalculateStorageCapacities(); - Mockito.verify(primaryDataStoreDao, Mockito.never()).findById(Mockito.anyLong()); - Mockito.verify(storageManager, Mockito.never()).createCapacityEntry(Mockito.any(), Mockito.anyShort(), Mockito.anyLong()); + verify(primaryDataStoreDao, never()).findById(anyLong()); + verify(storageManager, never()).createCapacityEntry(any(), anyShort(), anyLong()); assertNull("executor should never be created when there is nothing to submit", getCapacityExecutorService()); } @Test public void testRecalculateHostCapacitiesLogsAndContinuesOnTaskFailure() { - Mockito.when(hostDao.listIdsByType(Host.Type.Routing)).thenReturn(List.of(1L, 2L, 3L)); - HostVO host1 = Mockito.mock(HostVO.class); - HostVO host2 = Mockito.mock(HostVO.class); - HostVO host3 = Mockito.mock(HostVO.class); - Mockito.when(hostDao.findById(1L)).thenReturn(host1); - Mockito.when(hostDao.findById(2L)).thenReturn(host2); - Mockito.when(hostDao.findById(3L)).thenReturn(host3); - Mockito.doThrow(new RuntimeException("boom")).when(capacityManager).updateCapacityForHost(host2); + when(hostDao.listIdsByType(Host.Type.Routing)).thenReturn(List.of(1L, 2L, 3L)); + HostVO host1 = mock(HostVO.class); + HostVO host2 = mock(HostVO.class); + HostVO host3 = mock(HostVO.class); + when(hostDao.findById(1L)).thenReturn(host1); + when(hostDao.findById(2L)).thenReturn(host2); + when(hostDao.findById(3L)).thenReturn(host3); + doThrow(new RuntimeException("boom")).when(capacityManager).updateCapacityForHost(host2); alertManagerImplMock.recalculateHostCapacities(); - Mockito.verify(capacityManager).updateCapacityForHost(host1); - Mockito.verify(capacityManager).updateCapacityForHost(host2); - Mockito.verify(capacityManager).updateCapacityForHost(host3); - Mockito.verify(alertManagerImplMock.logger).error(Mockito.anyString(), Mockito.any(Throwable.class)); + verify(capacityManager).updateCapacityForHost(host1); + verify(capacityManager).updateCapacityForHost(host2); + verify(capacityManager).updateCapacityForHost(host3); + verify(alertManagerImplMock.logger).error(anyString(), any(Throwable.class)); } @Test public void testRecalculateHostCapacitiesReusesExecutorAcrossCalls() throws Exception { - Mockito.when(hostDao.listIdsByType(Host.Type.Routing)).thenReturn(List.of(1L)); - HostVO hostMock = Mockito.mock(HostVO.class); - Mockito.when(hostDao.findById(Mockito.anyLong())).thenReturn(hostMock); + when(hostDao.listIdsByType(Host.Type.Routing)).thenReturn(List.of(1L)); + HostVO hostMock = mock(HostVO.class); + when(hostDao.findById(anyLong())).thenReturn(hostMock); alertManagerImplMock.recalculateHostCapacities(); ExecutorService firstExecutor = getCapacityExecutorService(); assertNotNull(firstExecutor); - Mockito.when(primaryDataStoreDao.listAllIds()).thenReturn(List.of(101L)); - StoragePoolVO pool = Mockito.mock(StoragePoolVO.class); - Mockito.when(primaryDataStoreDao.findById(101L)).thenReturn(pool); + when(primaryDataStoreDao.listAllIds()).thenReturn(List.of(101L)); + StoragePoolVO pool = mock(StoragePoolVO.class); + when(primaryDataStoreDao.findById(101L)).thenReturn(pool); alertManagerImplMock.recalculateStorageCapacities(); assertEquals("host and storage recalculation should share the same long-lived pool", @@ -281,9 +290,9 @@ public void testRecalculateHostCapacitiesReusesExecutorAcrossCalls() throws Exce @Test public void testRecalculateHostCapacitiesRecreatesExecutorAfterShutdown() throws Exception { - Mockito.when(hostDao.listIdsByType(Host.Type.Routing)).thenReturn(List.of(1L)); - HostVO hostMock = Mockito.mock(HostVO.class); - Mockito.when(hostDao.findById(Mockito.anyLong())).thenReturn(hostMock); + when(hostDao.listIdsByType(Host.Type.Routing)).thenReturn(List.of(1L)); + HostVO hostMock = mock(HostVO.class); + when(hostDao.findById(anyLong())).thenReturn(hostMock); alertManagerImplMock.recalculateHostCapacities(); ExecutorService firstExecutor = getCapacityExecutorService(); @@ -298,29 +307,29 @@ public void testRecalculateHostCapacitiesRecreatesExecutorAfterShutdown() throws @Test public void testStopShutsDownCapacityExecutorServiceWhenPresent() throws Exception { - Timer timerMock = Mockito.mock(Timer.class); + Timer timerMock = mock(Timer.class); setTimer(timerMock); - Mockito.when(hostDao.listIdsByType(Host.Type.Routing)).thenReturn(List.of(1L)); - HostVO hostMock = Mockito.mock(HostVO.class); - Mockito.when(hostDao.findById(Mockito.anyLong())).thenReturn(hostMock); + when(hostDao.listIdsByType(Host.Type.Routing)).thenReturn(List.of(1L)); + HostVO hostMock = mock(HostVO.class); + when(hostDao.findById(anyLong())).thenReturn(hostMock); alertManagerImplMock.recalculateHostCapacities(); boolean result = alertManagerImplMock.stop(); Assert.assertTrue(result); - Mockito.verify(timerMock).cancel(); + verify(timerMock).cancel(); Assert.assertTrue(getCapacityExecutorService().isShutdown()); } @Test public void testStopDoesNotThrowWhenCapacityExecutorServiceNeverCreated() throws Exception { - Timer timerMock = Mockito.mock(Timer.class); + Timer timerMock = mock(Timer.class); setTimer(timerMock); boolean result = alertManagerImplMock.stop(); Assert.assertTrue(result); - Mockito.verify(timerMock).cancel(); + verify(timerMock).cancel(); assertNull(getCapacityExecutorService()); } From 5f8ca5342de2cdf03477755f6f1bdeddcafa55ec Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Tue, 25 Aug 2026 11:17:33 +0200 Subject: [PATCH 08/10] shutdown exception --- .../apache/cloudstack/metrics/PrometheusExporterServerImpl.java | 1 - 1 file changed, 1 deletion(-) diff --git a/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterServerImpl.java b/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterServerImpl.java index 63aaa3b6a703..93279658177d 100644 --- a/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterServerImpl.java +++ b/plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterServerImpl.java @@ -110,7 +110,6 @@ public void handle(HttpExchange httpExchange) throws IOException { @Override public boolean stop() { if (httpServer != null) { - httpServer.setExecutor(null); httpServer.stop(0); logger.debug("Stopped Prometheus exporter http server"); } From bde422a117d657fc68b928f58fe1c9972ed53d06 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Tue, 25 Aug 2026 11:18:07 +0200 Subject: [PATCH 09/10] tests --- .../PrometheusExporterServerImplTest.java | 271 ++++++++++++++++++ 1 file changed, 271 insertions(+) create mode 100644 plugins/integrations/prometheus/src/test/java/org/apache/cloudstack/metrics/PrometheusExporterServerImplTest.java diff --git a/plugins/integrations/prometheus/src/test/java/org/apache/cloudstack/metrics/PrometheusExporterServerImplTest.java b/plugins/integrations/prometheus/src/test/java/org/apache/cloudstack/metrics/PrometheusExporterServerImplTest.java new file mode 100644 index 000000000000..bfb8b5417cb9 --- /dev/null +++ b/plugins/integrations/prometheus/src/test/java/org/apache/cloudstack/metrics/PrometheusExporterServerImplTest.java @@ -0,0 +1,271 @@ +// 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.metrics; + +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.isNull; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.io.ByteArrayOutputStream; +import java.io.InputStream; +import java.lang.reflect.Field; +import java.net.ConnectException; +import java.net.HttpURLConnection; +import java.net.Socket; +import java.net.URL; +import java.nio.charset.StandardCharsets; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.ThreadPoolExecutor; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import com.sun.net.httpserver.HttpServer; + +import org.apache.cloudstack.framework.config.ConfigKey; +import org.apache.cloudstack.framework.config.impl.ConfigDepotImpl; + +public class PrometheusExporterServerImplTest { + + private PrometheusExporterServerImpl server; + private ConfigDepotImpl mockDepot; + + @Before + public void setUp() throws Exception { + server = new PrometheusExporterServerImpl(); + mockDepot = mock(ConfigDepotImpl.class); + setConfigDepot(mockDepot); + // EnablePrometheusExporter is a non-dynamic ConfigKey, so its cached _value survives + // across tests (and even across test classes sharing this JVM) unless cleared here: + // isDynamic()==false means value() only re-reads the (mocked) depot while _value==null. + resetConfigKeyValue(PrometheusExporterServer.EnablePrometheusExporter); + setStaticField("httpServer", null); + setInstanceField(server, "httpExecutor", null); + } + + @After + public void tearDown() throws Exception { + server.stop(); + setConfigDepot(null); + resetConfigKeyValue(PrometheusExporterServer.EnablePrometheusExporter); + setStaticField("httpServer", null); + } + + @Test + public void testStartWhenDisabledDoesNotCreateServerOrExecutor() throws Exception { + stubConfigValue(PrometheusExporterServer.EnablePrometheusExporter, "false"); + + boolean result = server.start(); + + assertTrue("start() should always return true", result); + assertNull("httpServer should not be created when the exporter is disabled", getStaticField("httpServer")); + assertNull("httpExecutor should not be created when the exporter is disabled", getInstanceField(server, "httpExecutor")); + } + + @Test + public void testStopWhenNeverStartedDoesNotThrow() throws Exception { + boolean result = server.stop(); + + assertTrue("stop() should return true even if the server was never started", result); + } + + @Test + public void testStopShutsDownExecutorAndClosesServer() throws Exception { + stubConfigValue(PrometheusExporterServer.EnablePrometheusExporter, "true"); + stubConfigValue(PrometheusExporterServer.PrometheusExporterServerPort, "0"); + stubConfigValue(PrometheusExporterServer.PrometheusExporterAllowedAddresses, "127.0.0.1"); + + assertTrue(server.start()); + + HttpServer startedHttpServer = (HttpServer) getStaticField("httpServer"); + ExecutorService startedExecutor = (ExecutorService) getInstanceField(server, "httpExecutor"); + int port = startedHttpServer.getAddress().getPort(); + + assertFalse("Executor should be alive right after start()", startedExecutor.isShutdown()); + + server.stop(); + + assertTrue("stop() should shut down the http executor", startedExecutor.isShutdown()); + assertNull("httpExecutor field should be cleared after stop()", getInstanceField(server, "httpExecutor")); + + try { + new Socket("127.0.0.1", port).close(); + org.junit.Assert.fail("Server socket should no longer accept connections after stop()"); + } catch (ConnectException expected) { + // expected: the listening socket was closed by stop() + } + } + + @Test + public void testStartCreatesFixedThreadPoolOfTwoAndWiresItToTheServer() throws Exception { + stubConfigValue(PrometheusExporterServer.EnablePrometheusExporter, "true"); + stubConfigValue(PrometheusExporterServer.PrometheusExporterServerPort, "0"); + stubConfigValue(PrometheusExporterServer.PrometheusExporterAllowedAddresses, "127.0.0.1"); + + assertTrue(server.start()); + + Object executor = getInstanceField(server, "httpExecutor"); + assertTrue("httpExecutor should be a ThreadPoolExecutor", executor instanceof ThreadPoolExecutor); + assertEquals("httpExecutor should be a fixed pool of 2 threads", 2, ((ThreadPoolExecutor) executor).getMaximumPoolSize()); + } + + @Test + public void testAllowedRemoteAddressReceivesMetrics() throws Exception { + PrometheusExporter mockExporter = mock(PrometheusExporter.class); + when(mockExporter.getMetrics()).thenReturn("cloudstack_test_metric 1"); + setInstanceField(server, "prometheusExporter", mockExporter); + + stubConfigValue(PrometheusExporterServer.EnablePrometheusExporter, "true"); + stubConfigValue(PrometheusExporterServer.PrometheusExporterServerPort, "0"); + stubConfigValue(PrometheusExporterServer.PrometheusExporterAllowedAddresses, "127.0.0.1"); + + assertTrue(server.start()); + int port = ((HttpServer) getStaticField("httpServer")).getAddress().getPort(); + + HttpURLConnection connection = (HttpURLConnection) new URL("http://127.0.0.1:" + port + "/metrics").openConnection(); + try { + assertEquals(200, connection.getResponseCode()); + String body = readFully(connection.getInputStream()); + assertEquals("cloudstack_test_metric 1", body); + } finally { + connection.disconnect(); + } + + verify(mockExporter, times(1)).updateMetrics(); + verify(mockExporter, times(1)).getMetrics(); + } + + @Test + public void testDisallowedRemoteAddressReceivesForbidden() throws Exception { + PrometheusExporter mockExporter = mock(PrometheusExporter.class); + setInstanceField(server, "prometheusExporter", mockExporter); + + stubConfigValue(PrometheusExporterServer.EnablePrometheusExporter, "true"); + stubConfigValue(PrometheusExporterServer.PrometheusExporterServerPort, "0"); + stubConfigValue(PrometheusExporterServer.PrometheusExporterAllowedAddresses, "10.0.0.1"); + + assertTrue(server.start()); + int port = ((HttpServer) getStaticField("httpServer")).getAddress().getPort(); + + HttpURLConnection connection = (HttpURLConnection) new URL("http://127.0.0.1:" + port + "/metrics").openConnection(); + try { + assertEquals(403, connection.getResponseCode()); + String body = readFully(connection.getErrorStream()); + assertEquals("Forbidden", body); + } finally { + connection.disconnect(); + } + + verify(mockExporter, times(0)).updateMetrics(); + } + + @Test + public void testRootPathReturnsLandingPage() throws Exception { + stubConfigValue(PrometheusExporterServer.EnablePrometheusExporter, "true"); + stubConfigValue(PrometheusExporterServer.PrometheusExporterServerPort, "0"); + stubConfigValue(PrometheusExporterServer.PrometheusExporterAllowedAddresses, "127.0.0.1"); + + assertTrue(server.start()); + int port = ((HttpServer) getStaticField("httpServer")).getAddress().getPort(); + + HttpURLConnection connection = (HttpURLConnection) new URL("http://127.0.0.1:" + port + "/").openConnection(); + try { + assertEquals(200, connection.getResponseCode()); + String body = readFully(connection.getInputStream()); + assertTrue("Landing page should link to /metrics", body.contains("/metrics")); + } finally { + connection.disconnect(); + } + } + + @Test + public void testGetConfigComponentName() { + assertEquals("PrometheusExporter", server.getConfigComponentName()); + } + + @Test + public void testGetConfigKeysIncludesMinRefreshIntervalAddedByTheScrapeThrottlingFix() { + ConfigKey[] keys = server.getConfigKeys(); + + assertArrayEquals(new ConfigKey[]{ + PrometheusExporterServer.EnablePrometheusExporter, + PrometheusExporterServer.PrometheusExporterServerPort, + PrometheusExporterServer.PrometheusExporterAllowedAddresses, + PrometheusExporterServer.PrometheusExporterOfferingCountLimit, + PrometheusExporterServer.PrometheusExporterMinRefreshInterval + }, keys); + } + + private static String readFully(InputStream inputStream) throws Exception { + ByteArrayOutputStream buffer = new ByteArrayOutputStream(); + byte[] chunk = new byte[1024]; + int bytesRead; + while ((bytesRead = inputStream.read(chunk)) != -1) { + buffer.write(chunk, 0, bytesRead); + } + return buffer.toString(StandardCharsets.UTF_8.name()); + } + + private void stubConfigValue(ConfigKey configKey, String value) { + when(mockDepot.getConfigStringValue(eq(configKey.key()), eq(ConfigKey.Scope.Global), isNull())).thenReturn(value); + } + + private static void setConfigDepot(ConfigDepotImpl depot) throws Exception { + Field field = ConfigKey.class.getDeclaredField("s_depot"); + field.setAccessible(true); + field.set(null, depot); + } + + private static void resetConfigKeyValue(ConfigKey configKey) throws Exception { + Field field = ConfigKey.class.getDeclaredField("_value"); + field.setAccessible(true); + field.set(configKey, null); + } + + private static void setStaticField(String fieldName, Object value) throws Exception { + Field field = PrometheusExporterServerImpl.class.getDeclaredField(fieldName); + field.setAccessible(true); + field.set(null, value); + } + + private static Object getStaticField(String fieldName) throws Exception { + Field field = PrometheusExporterServerImpl.class.getDeclaredField(fieldName); + field.setAccessible(true); + return field.get(null); + } + + private static void setInstanceField(Object target, String fieldName, Object value) throws Exception { + Field field = PrometheusExporterServerImpl.class.getDeclaredField(fieldName); + field.setAccessible(true); + field.set(target, value); + } + + private static Object getInstanceField(Object target, String fieldName) throws Exception { + Field field = PrometheusExporterServerImpl.class.getDeclaredField(fieldName); + field.setAccessible(true); + return field.get(target); + } +} From 15c8fa163c484b191a367ccd42678cbe2676819d Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Tue, 25 Aug 2026 21:53:50 +0200 Subject: [PATCH 10/10] sonarqube --- .../cloudstack/metrics/PrometheusExporterServerImplTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/integrations/prometheus/src/test/java/org/apache/cloudstack/metrics/PrometheusExporterServerImplTest.java b/plugins/integrations/prometheus/src/test/java/org/apache/cloudstack/metrics/PrometheusExporterServerImplTest.java index bfb8b5417cb9..5f23be3febb5 100644 --- a/plugins/integrations/prometheus/src/test/java/org/apache/cloudstack/metrics/PrometheusExporterServerImplTest.java +++ b/plugins/integrations/prometheus/src/test/java/org/apache/cloudstack/metrics/PrometheusExporterServerImplTest.java @@ -86,7 +86,7 @@ public void testStartWhenDisabledDoesNotCreateServerOrExecutor() throws Exceptio } @Test - public void testStopWhenNeverStartedDoesNotThrow() throws Exception { + public void testStopWhenNeverStartedDoesNotThrow() { boolean result = server.stop(); assertTrue("stop() should return true even if the server was never started", result);