diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/ContainerCacheMetrics.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/ContainerCacheMetrics.java index c4ca78ffb41c..a7917d4d35d9 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/ContainerCacheMetrics.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/ContainerCacheMetrics.java @@ -28,6 +28,8 @@ */ public final class ContainerCacheMetrics { + private static final String SOURCE_NAME = "ContainerCacheMetrics"; + @Metric("Rate to measure the db open latency") private MutableRate dbOpenLatency; @@ -54,9 +56,7 @@ private ContainerCacheMetrics() { public static ContainerCacheMetrics create() { MetricsSystem ms = DefaultMetricsSystem.instance(); - String name = "ContainerCacheMetrics"; - - return ms.register(name, "null", new ContainerCacheMetrics()); + return ms.register(SOURCE_NAME, "null", new ContainerCacheMetrics()); } public void incNumDbGetOps() { diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/MutableVolumeSet.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/MutableVolumeSet.java index a79a06b6541f..e9c2f9df17a1 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/MutableVolumeSet.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/MutableVolumeSet.java @@ -360,6 +360,17 @@ public void shutdown() { } volumeMap.clear(); + // Shut down failed volumes too: their VolumeInfoMetrics is registered on + // creation (HDDS-7086) and must be unregistered on shutdown. + for (StorageVolume volume : failedVolumeMap.values()) { + try { + volume.shutdown(); + } catch (Exception ex) { + LOG.error("Failed to shutdown failed volume : " + volume.getStorageDir(), ex); + } + } + failedVolumeMap.clear(); + if (volumeHealthMetrics != null) { volumeHealthMetrics.unregister(); } diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestVolumeSet.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestVolumeSet.java index 932101dc526b..0bfc88f8ac8f 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestVolumeSet.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestVolumeSet.java @@ -25,6 +25,8 @@ import static org.assertj.core.api.Assumptions.assumeThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.File; @@ -39,6 +41,8 @@ import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.scm.ScmConfigKeys; import org.apache.hadoop.metrics2.MetricsRecordBuilder; +import org.apache.hadoop.metrics2.MetricsSystem; +import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem; import org.apache.hadoop.ozone.OzoneConfigKeys; import org.apache.hadoop.ozone.container.common.utils.HddsVolumeUtil; import org.junit.jupiter.api.AfterEach; @@ -191,7 +195,16 @@ void testFailVolumes(@TempDir File readOnlyVolumePath, @TempDir File volumePath) assertEquals(readOnlyVolumePath, volSet.getFailedVolumesList().get(0) .getStorageDir()); assertNumVolumes(volSet, 1, 1); + + // The failed volume's VolumeInfoMetrics is registered on creation + // (HDDS-7086) and must be unregistered when the volume set shuts down. + MetricsSystem ms = DefaultMetricsSystem.instance(); + String failedVolumeMetrics = VolumeInfoMetrics.class.getSimpleName() + + "-" + readOnlyVolumePath.getAbsolutePath(); + assertNotNull(ms.getSource(failedVolumeMetrics)); + volSet.shutdown(); + assertNull(ms.getSource(failedVolumeMetrics)); } @Test diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestSafeMode.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestSafeMode.java index 1300c61b1f8d..ef0867829482 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestSafeMode.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestSafeMode.java @@ -59,7 +59,9 @@ class TestSafeMode { static void setup() { OzoneConfiguration conf = new OzoneConfiguration(); clusterProvider = new MiniOzoneClusterProvider( - MiniOzoneCluster.newBuilder(conf), 2); + // Clusters overlap via MiniOzoneClusterProvider, so the metrics leak + // assertion would see a concurrent cluster's sources. + MiniOzoneCluster.newBuilder(conf).setMetricsLeakAssertEnabled(false), 2); } @BeforeEach diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/node/TestDecommissionAndMaintenance.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/node/TestDecommissionAndMaintenance.java index 5260c973ba06..4e0f36f316bf 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/node/TestDecommissionAndMaintenance.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/node/TestDecommissionAndMaintenance.java @@ -148,7 +148,10 @@ public static void init() { conf.setFromObject(replicationConf); MiniOzoneCluster.Builder builder = MiniOzoneCluster.newBuilder(conf) - .setNumDatanodes(DATANODE_COUNT); + .setNumDatanodes(DATANODE_COUNT) + // Clusters overlap via MiniOzoneClusterProvider, so the metrics leak + // assertion would see a concurrent cluster's sources. + .setMetricsLeakAssertEnabled(false); clusterProvider = new MiniOzoneClusterProvider(builder, 9); } diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/upgrade/TestHDDSUpgrade.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/upgrade/TestHDDSUpgrade.java index c0435088d256..1ecdb5226f69 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/upgrade/TestHDDSUpgrade.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/upgrade/TestHDDSUpgrade.java @@ -161,6 +161,9 @@ public static void initClass() { builder.setNumOfStorageContainerManagers(NUM_SCMS) .setSCMConfigurator(scmConfigurator) .setNumDatanodes(NUM_DATA_NODES) + // Clusters overlap via MiniOzoneClusterProvider, so the metrics leak + // assertion would see a concurrent cluster's sources. + .setMetricsLeakAssertEnabled(false) .setDatanodeFactory(UniformDatanodesFactory.newBuilder() .setLayoutVersion(HDDSLayoutFeature.INITIAL_VERSION.layoutVersion()) .build()); diff --git a/hadoop-ozone/mini-cluster/src/main/java/org/apache/hadoop/ozone/MetricsLeakAssertion.java b/hadoop-ozone/mini-cluster/src/main/java/org/apache/hadoop/ozone/MetricsLeakAssertion.java new file mode 100644 index 000000000000..f6274f2d3208 --- /dev/null +++ b/hadoop-ozone/mini-cluster/src/main/java/org/apache/hadoop/ozone/MetricsLeakAssertion.java @@ -0,0 +1,246 @@ +/* + * 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.hadoop.ozone; + +import java.lang.reflect.Field; +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.TreeSet; +import org.apache.hadoop.metrics2.MetricsSource; +import org.apache.hadoop.metrics2.MetricsSystem; +import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem; + +/** + * Asserts that no metrics sources remain registered in the + * {@link DefaultMetricsSystem} after a mini cluster is shut down. + * + *

Hadoop's {@code MetricsSystemImpl} does not remove registered sources on + * {@code stop()} or {@code shutdown()}; a metrics class that forgets to call + * {@code unregisterSource(...)} leaks its registration silently. This helper + * inspects the private {@code allSources} map via reflection and fails the + * test if anything is still registered. + */ +public final class MetricsLeakAssertion { + + private static final String ALL_SOURCES_FIELD = "allSources"; + + /** + * Metrics sources that are registered once per JVM (or once per service) + * and intentionally never unregistered, so they are expected to still be + * present after a mini cluster shuts down. These are JVM-level singletons + * with no per-service owner; do not add per-service metrics here. + * + *

For the matching rules see {@link #isExpectedLeftover(String)}. + */ + private static final List EXPECTED_LEFTOVER_SOURCES = Arrays.asList( + "JvmMetrics*", // per service, via HddsServerUtil.initializeMetrics + "JvmMetricsCpu", + "UgiMetrics", // Hadoop security UGI metrics + "ManagedRocksObjectMetrics", + "ContainerCacheMetrics" // ContainerCache singleton, created once per JVM + ); + + /** + * Metrics sources that leak: they should be unregistered when the mini + * cluster shuts down but currently are not. This is a transitional list to + * keep the integration suite green while the leaks are being fixed. Each + * entry should be removed once the corresponding source unregisters + * properly; do not add new entries, fix the leak instead. + * + *

For the matching rules see {@link #isExpectedLeftover(String)}. + */ + private static final List TODO_LEFTOVER_SOURCES = Arrays.asList( + // RPC / HTTP server metrics (name embeds the listening port). + "RpcActivityForPort*", + "RpcDetailedActivityForPort*", + "HttpServer2*", + "LocalJobRunnerMetrics*", + + // Datanode per-instance metrics. + "StorageContainerMetrics", + "ContainerDataScannerMetrics*", // name embeds the volume path + "ContainerMetadataScannerMetrics", + "On-demand container scanner metrics", + "BackgroundVolumeScannerMetrics", + "VolumeHealthMetrics-*", + "VolumeIOStats-*", // name embeds the volume path + // Name embeds the volume path. Also intentionally kept registered for + // failed volumes (HDDS-7086), so only the healthy-volume case is a leak. + "VolumeInfoMetrics-*", + "CommandHandlerMetrics", + "HddsDispatcher", + "ECReconstructionMetrics", + "ReplicationSupervisorMetrics", + "ContainerReplicator/push", + "BlockDeletingService", + "GrpcMetrics", + + // SCM metrics. + "SCMNodeMetrics", + "SCMContainerManagerMetrics", + "SCMContainerMetrics", + "SCMMetrics", + "SafeModeMetrics", + "ContainerBalancerMetrics", + "NodeDecommissionMetrics", + "SCMDatanodeProtocol", + "ScmBlockLocationProtocol", + "ScmContainerLocationProtocol", + "ScmSecurityProtocol", + "EventQueue*", // per event/handler pair, registered by SCM EventQueue + + // OM metrics. + "OMMetrics", + "OMPerformanceMetrics", + "OMLockMetrics", + "OMHAMetrics", + "OMSnapshotDirectoryMetrics", + "OmSnapshotInternalMetrics", + "OmSnapshotMetrics", + "OmClientProtocol", + "DeletingServiceMetrics", + "KeyLifecycleServiceMetrics", + "BucketUtilizationMetrics", + "DelegationTokenSecretManagerMetrics", + + // OM / SCM RocksDB table cache and DB metrics (name embeds path/uuid). + "Rocksdb_*", + "SSTFilePruningMetrics-*", + "DBCheckpointMetrics", + "*TableCache", + + // Ratis / third-party infrastructure metrics. + "CSMMetricsgroup-*", // Ratis container state machine, embeds a random id + "CacheMetrics-*", // Hadoop cache metrics (XceiverClientManager, etc.) + "NettyMetrics*", + "ContainerClientMetrics1", + "ReconTaskMetrics", + "ReconTaskControllerMetrics" + ); + + private MetricsLeakAssertion() { + } + + /** + * Throws an {@link AssertionError} if any metrics sources are still + * registered with the default metrics system, or if the expected + * {@code allSources} field cannot be found or read (e.g. a Hadoop version + * change restructured {@code MetricsSystemImpl}), so that a broken or + * missing check fails loudly instead of going unnoticed. + */ + public static void assertNoLeaks() { + MetricsSystem ms = DefaultMetricsSystem.instance(); + Field field = findAllSourcesField(ms.getClass()); + if (field == null) { + throw new AssertionError("Cannot check for metrics leaks: '" + ALL_SOURCES_FIELD + + "' field not found on " + ms.getClass().getName() + + ". The metrics system implementation may have changed."); + } + final Map allSources; + try { + field.setAccessible(true); + Object value = field.get(ms); + if (!(value instanceof Map)) { + throw new AssertionError("Cannot check for metrics leaks: '" + ALL_SOURCES_FIELD + + "' on " + ms.getClass().getName() + " is not a Map."); + } + @SuppressWarnings("unchecked") + Map sources = (Map) value; + allSources = sources; + } catch (IllegalAccessException e) { + throw new AssertionError("Cannot check for metrics leaks: unable to access '" + + ALL_SOURCES_FIELD + "' on " + ms.getClass().getName() + ".", e); + } + Set leaked = new TreeSet<>(allSources.keySet()); + leaked.removeIf(MetricsLeakAssertion::isExpectedLeftover); + if (!leaked.isEmpty()) { + throw new AssertionError("Found " + leaked.size() + + " metrics source(s) still registered after cluster shutdown: " + leaked); + } + } + + /** + * Matching rules for {@link #EXPECTED_LEFTOVER_SOURCES} and + * {@link #TODO_LEFTOVER_SOURCES}: + *

+ */ + private static boolean isExpectedLeftover(String name) { + return matches(EXPECTED_LEFTOVER_SOURCES, name) || matches(TODO_LEFTOVER_SOURCES, name); + } + + private static boolean matches(List entries, String name) { + for (String entry : entries) { + if (entry.startsWith("*")) { + // Suffix match, also tolerating a trailing numeric suffix (-N). + String suffix = entry.substring(1); + if (name.endsWith(suffix) || stripNumericSuffix(name).endsWith(suffix)) { + return true; + } + } else if (entry.endsWith("*")) { + if (name.startsWith(entry.substring(0, entry.length() - 1))) { + return true; + } + } else if (name.equals(entry) || name.startsWith(entry + "-")) { + return true; + } + } + return false; + } + + private static String stripNumericSuffix(String name) { + int idx = name.lastIndexOf('-'); + if (idx > 0 && idx < name.length() - 1) { + String tail = name.substring(idx + 1); + boolean numeric = true; + for (int i = 0; i < tail.length(); i++) { + if (!Character.isDigit(tail.charAt(i))) { + numeric = false; + break; + } + } + if (numeric) { + return name.substring(0, idx); + } + } + return name; + } + + private static Field findAllSourcesField(Class clazz) { + Class current = clazz; + while (current != null) { + try { + return current.getDeclaredField(ALL_SOURCES_FIELD); + } catch (NoSuchFieldException e) { + current = current.getSuperclass(); + } + } + return null; + } +} diff --git a/hadoop-ozone/mini-cluster/src/main/java/org/apache/hadoop/ozone/MiniOzoneCluster.java b/hadoop-ozone/mini-cluster/src/main/java/org/apache/hadoop/ozone/MiniOzoneCluster.java index 8765c2aaaae6..68fa05e13b23 100644 --- a/hadoop-ozone/mini-cluster/src/main/java/org/apache/hadoop/ozone/MiniOzoneCluster.java +++ b/hadoop-ozone/mini-cluster/src/main/java/org/apache/hadoop/ozone/MiniOzoneCluster.java @@ -262,6 +262,7 @@ abstract class Builder { protected int numOfDatanodes = 3; protected boolean startDataNodes = true; + protected boolean metricsLeakAssertEnabled = true; protected CertificateClient certClient; protected SecretKeyClient secretKeyClient; protected DatanodeFactory dnFactory = UniformDatanodesFactory.newBuilder().build(); @@ -354,6 +355,17 @@ public Builder setStartDataNodes(boolean nodes) { return this; } + /** + * Whether to assert that no metrics sources leak on cluster shutdown. + * Disable for tests that run multiple clusters concurrently (e.g. via + * MiniOzoneClusterProvider), because the metrics registry is shared + * JVM-wide and a concurrent cluster's sources would be flagged as leaks. + */ + public Builder setMetricsLeakAssertEnabled(boolean enabled) { + this.metricsLeakAssertEnabled = enabled; + return this; + } + public Builder setCertificateClient(CertificateClient client) { this.certClient = client; return this; diff --git a/hadoop-ozone/mini-cluster/src/main/java/org/apache/hadoop/ozone/MiniOzoneClusterImpl.java b/hadoop-ozone/mini-cluster/src/main/java/org/apache/hadoop/ozone/MiniOzoneClusterImpl.java index e13719dde6e5..afbf5952c713 100644 --- a/hadoop-ozone/mini-cluster/src/main/java/org/apache/hadoop/ozone/MiniOzoneClusterImpl.java +++ b/hadoop-ozone/mini-cluster/src/main/java/org/apache/hadoop/ozone/MiniOzoneClusterImpl.java @@ -113,6 +113,9 @@ public class MiniOzoneClusterImpl implements MiniOzoneCluster { private OzoneConfiguration conf; private final SCMConfigurator scmConfigurator; + // Whether to assert no metrics sources leak on shutdown. Tests running + // multiple clusters concurrently disable this via the builder. + private boolean metricsLeakAssertEnabled = true; private StorageContainerManager scm; private OzoneManager ozoneManager; private final List hddsDatanodes; @@ -165,6 +168,10 @@ protected void setConf(OzoneConfiguration newConf) { this.conf = newConf; } + void setMetricsLeakAssertEnabled(boolean enabled) { + this.metricsLeakAssertEnabled = enabled; + } + public void waitForSCMToBeReady() throws TimeoutException, InterruptedException { GenericTestUtils.waitFor(scm::checkLeader, @@ -390,8 +397,16 @@ public void shutdown() { stop(); FileUtils.deleteDirectory(baseDir); ContainerCache.getInstance(conf).shutdownCache(); - DefaultMetricsSystem.shutdown(); + // RocksDB object leaks are often the root cause; check them first. ManagedRocksObjectMetrics.INSTANCE.assertNoLeaks(); + // Assert before tearing down the metrics system: Hadoop does not clear + // allSources on shutdown, but checking before shutdown makes the + // assertion independent of that behavior. Tests running multiple + // clusters concurrently disable this because the registry is JVM-wide. + if (metricsLeakAssertEnabled) { + MetricsLeakAssertion.assertNoLeaks(); + } + DefaultMetricsSystem.shutdown(); } catch (Exception e) { LOG.error("Exception while shutting down the cluster.", e); } @@ -526,6 +541,7 @@ public MiniOzoneCluster build() throws IOException { MiniOzoneClusterImpl cluster = new MiniOzoneClusterImpl(conf, scmConfigurator, om, scm, hddsDatanodes, getServices()); + cluster.setMetricsLeakAssertEnabled(metricsLeakAssertEnabled); cluster.startServices(); cluster.setCAClient(certClient); @@ -617,6 +633,12 @@ protected void initializeConfiguration() throws IOException { // pipeline. conf.setInt(HddsConfigKeys.HDDS_SCM_SAFEMODE_MIN_DATANODE, numOfDatanodes >= 3 ? 3 : 1); + // Enable metrics percentile collection so that the quantile code paths + // are exercised by every integration test. + conf.setIfUnset(OzoneConfigKeys.OZONE_GPRC_METRICS_PERCENTILES_INTERVALS_KEY, "60,300"); + conf.setIfUnset(OzoneConfigKeys.OZONE_XCEIVER_CLIENT_METRICS_PERCENTILES_INTERVALS_SECONDS_KEY, "60,300"); + conf.setIfUnset(HddsConfigKeys.HDDS_METRICS_PERCENTILES_INTERVALS_KEY, "60,300"); + conf.setIfUnset(HddsConfigKeys.OZONE_DATANODE_IO_METRICS_PERCENTILES_INTERVALS_SECONDS_KEY, "60,300"); configureHostAndRackTopology(); } diff --git a/hadoop-ozone/mini-cluster/src/main/java/org/apache/hadoop/ozone/MiniOzoneHAClusterImpl.java b/hadoop-ozone/mini-cluster/src/main/java/org/apache/hadoop/ozone/MiniOzoneHAClusterImpl.java index 8df0f587c605..f0272737263c 100644 --- a/hadoop-ozone/mini-cluster/src/main/java/org/apache/hadoop/ozone/MiniOzoneHAClusterImpl.java +++ b/hadoop-ozone/mini-cluster/src/main/java/org/apache/hadoop/ozone/MiniOzoneHAClusterImpl.java @@ -502,6 +502,7 @@ public MiniOzoneHAClusterImpl build() throws IOException { MiniOzoneHAClusterImpl cluster = new MiniOzoneHAClusterImpl(conf, scmConfigurator, omService, scmService, hddsDatanodes, path, getServices()); + cluster.setMetricsLeakAssertEnabled(metricsLeakAssertEnabled); try { cluster.startServices(); } catch (Exception e) { diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java index 101cbbfadb05..53335faf56a3 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java @@ -2491,6 +2491,7 @@ public boolean stop() { serviceManager.stop(); DeletingServiceMetrics.unregister(); OMPerformanceMetrics.unregister(); + OmSnapshotInternalMetrics.unregister(); RatisDropwizardExports.clear(ratisMetricsMap, ratisReporterList); scmClient.close(); if (certClient != null) {