From 4261c3d54d942aba069934618630d4e7c0eec2c3 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Mon, 20 Jul 2026 12:40:07 +0200 Subject: [PATCH 1/4] 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 78358812b2b7d11e7c7805322b1d70badbc0cdcb Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Wed, 22 Jul 2026 11:24:55 +0200 Subject: [PATCH 2/4] 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 8e3694b1184c1192da361a6a5889447cceea82bd Mon Sep 17 00:00:00 2001 From: Prashant Bhanage Date: Thu, 23 Jul 2026 17:34:38 +0530 Subject: [PATCH 3/4] prometheus: fix scrape duration growing unbounded (#13667) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three changes scoped exclusively to the prometheus exporter plugin: 1. Bounded HTTP executor — give the HttpServer a FixedThreadPool(2) instead of the JDK default single-threaded executor; shut it down cleanly in stop(). 2. TTL guard on updateMetrics() — add a synchronized guard with a last-run timestamp so scrapes arriving faster than the configurable minimum interval (prometheus.exporter.metrics.min.refresh.interval, default 5 s) reuse the previously computed metrics instead of triggering a new recomputation. 3. Timing instrumentation — wrap updateMetrics() body with System.nanoTime() start/end and log elapsed wall-clock time at info level on every run (including exception path), so slow sub-collectors can be identified from logs. Fixes: #13667 Ref: #13586 --- .../metrics/PrometheusExporterImpl.java | 16 ++++- .../metrics/PrometheusExporterServer.java | 3 + .../metrics/PrometheusExporterServerImpl.java | 12 +++- .../metrics/PrometheusExporterImplTest.java | 71 +++++++++++++++++++ 4 files changed, 100 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..f67a68019061 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 @@ -108,6 +113,10 @@ public boolean stop() { httpServer.stop(0); logger.debug("Stopped Prometheus exporter http server"); } + if (httpExecutor != null) { + httpExecutor.shutdownNow(); + logger.debug("Shut down Prometheus exporter http executor"); + } return true; } @@ -122,7 +131,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 8637b6d70577043c01e22ec7f0622780e1f1caf7 Mon Sep 17 00:00:00 2001 From: Prashant Bhanage Date: Sat, 25 Jul 2026 18:03:48 +0530 Subject: [PATCH 4/4] prometheus: null out executor references safely on shutdown Scope httpServer.setExecutor(null) inside the existing httpServer null-check to avoid a NullPointerException if start() never created a server (e.g. exporter disabled, or the IOException path was taken). Also null out the httpExecutor field after shutdown for defensive hygiene, per review feedback on PR #13696. --- .../apache/cloudstack/metrics/PrometheusExporterServerImpl.java | 2 ++ 1 file changed, 2 insertions(+) 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 f67a68019061..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 @@ -110,6 +110,7 @@ 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"); } @@ -117,6 +118,7 @@ public boolean stop() { httpExecutor.shutdownNow(); logger.debug("Shut down Prometheus exporter http executor"); } + httpExecutor = null; return true; }