HDDS-15960. Hadoop metrics leak detection - #10941
Draft
ivandika3 wants to merge 15 commits into
Draft
Conversation
Add MetricsLeakAssertion which inspects the DefaultMetricsSystem's private allSources map via reflection and fails the test if any metrics source is still registered after MiniOzoneCluster shutdown. The check verifies the field exists and logs a WARN (skipping) if the underlying Hadoop metrics implementation changes, rather than failing spuriously. Also enable the metrics percentile-interval configs by default in the mini cluster builder so quantile code paths are exercised by every integration test. Generated-by: Codex (GPT-5)
Throw AssertionError instead of logging a WARN and skipping when the metrics system's allSources field cannot be found, is not a Map, or cannot be read, so a broken leak check fails the test loudly rather than being silently ignored. Generated-by: Codex (GPT-5)
Checking allSources before DefaultMetricsSystem.shutdown() makes the assertion independent of whether Hadoop clears allSources on shutdown (it currently does not), and asserts the invariant while the metrics system is still fully populated. Well-behaved sources unregister themselves during cluster stop(), so anything still registered at this point is a genuine leak. Generated-by: Codex (GPT-5)
Add an EXPECTED_LEFTOVER_SOURCES allowlist for metrics that are registered once per JVM or per service and intentionally never unregistered (JvmMetrics/JvmMetricsCpu from HddsServerUtil, UgiMetrics, ManagedRocksObjectMetrics, ContainerCacheMetrics). Entries are matched by prefix to cover the numeric suffixes the metrics system appends for repeated registrations. This keeps the assertion focused on genuine per-instance service metrics leaks; verified to reduce the leftover count from 50 to 41 on TestMiniOzoneCluster. Generated-by: Codex (GPT-5)
ContainerCacheMetrics has a real per-cluster lifecycle: it is registered in ContainerCache.getInstance() and ContainerCache.shutdownCache() is its natural teardown. Add an unregister() and call it from shutdownCache(), and drop ContainerCacheMetrics from the leak-assertion allowlist since it no longer leaks. The remaining allowlisted Ozone sources (JvmMetrics, JvmMetricsCpu, ManagedRocksObjectMetrics) are JVM-scoped singletons with no per-service owner, so they are correctly left on the allowlist alongside Hadoop's UgiMetrics. Generated-by: Codex (GPT-5)
Generated-by: Codex (GPT-5)
…y subsystem Running the assertion across the full integration suite surfaced a broad set of pre-existing metrics leaks (JVM singletons, per-service sources, and Ratis/Hadoop infrastructure metrics). Allowlisting all of them so the suite stays green while the assertion guards against new leaks; each group is commented and is meant to be burned down in follow-up issues. Matching now supports a leading '*' (suffix match) in addition to a trailing '*' (prefix match), to cover names that embed a table-specific prefix (keyTableCache-1), a random id (CSMMetricsgroup-...), an absolute path (VolumeIOStats-/...), or a port (RpcActivityForPort15000). Verified against all 4423 distinct leftover source names from the failed CI run: zero remain unmatched. TestMiniOzoneCluster (5 tests) and TestOzoneIntegrationNonHA (318 tests) pass. Generated-by: Codex (GPT-5)
Split the leftover-sources list into EXPECTED_LEFTOVER_SOURCES (JVM-level singletons that are intentionally never unregistered) and TODO_LEFTOVER_SOURCES (genuine per-service leaks to be fixed and removed). This makes the intent of each entry explicit and keeps the burn-down list self-documenting. Also correct the ContainerCacheMetrics handling: it is registered once per JVM by the ContainerCache singleton (whose reference is never reset), so it cannot be unregistered per-cluster. Running the full TestMiniOzoneCluster showed that a single leftover ContainerCacheMetrics appears when an earlier test in the shared JVM creates the singleton. Move it back to EXPECTED_LEFTOVER_SOURCES and drop the unregister() call, keeping only the SOURCE_NAME constant cleanup. Verified: all 4424 distinct leftover names from CI are covered; TestMiniOzoneCluster (5 tests) passes. Generated-by: Codex (GPT-5)
ManagedRocksObjectMetrics.assertNoLeaks() reports RocksDB objects that were GC'd without being closed, which is often the root cause of a failing shutdown. Run it before MetricsLeakAssertion.assertNoLeaks() so the more actionable failure surfaces first. Both assertions still run before DefaultMetricsSystem.shutdown(). Generated-by: Codex (GPT-5)
HddsVolume.failVolume() unregistered VolumeIOStats but not VolumeInfoMetrics, while shutdown() unregisters both. Make failVolume() symmetric so a failed volume does not leak its VolumeInfoMetrics source. Add TestHddsVolume.testFailVolumeUnregistersMetrics which verifies both sources are removed; the test fails without the fix. Generated-by: Codex (GPT-5)
This reverts commit 85ddb76.
…umes VolumeInfoMetrics is deliberately kept registered on a failed volume (HDDS-7086) so its FAILED state is visible via JMX / the DataNode UI, so a leftover VolumeInfoMetrics is only a leak for healthy volumes that were not shut down. Document this so follow-up work does not try to unregister it from failVolume(). Generated-by: Codex (GPT-5)
…trics MutableVolumeSet.shutdown() only shut down healthy volumes in volumeMap and never touched failedVolumeMap. A failed volume registers a VolumeInfoMetrics source when it is created (HDDS-7086 keeps it registered so the FAILED state is visible while the datanode runs), but it was never unregistered because shutdown() skipped failed volumes, leaking the source for the life of the JVM. Shut down and clear failedVolumeMap in shutdown() as well. HddsVolume and StorageVolume shutdown() are null-safe for failed volumes (volumeIOStats / volumeUsage are null), so this only unregisters the info metrics and is safe. Extend TestVolumeSet.testFailVolumes to assert the failed volume's VolumeInfoMetrics is registered before shutdown and removed after; it fails without the fix. Generated-by: Codex (GPT-5)
The metrics leak assertion reflects over the JVM-wide DefaultMetricsSystem registry, so it cannot attribute a source to a specific cluster. Tests that run clusters concurrently via MiniOzoneClusterProvider (which builds a reserve cluster in the background while another is still running) would otherwise see a concurrent cluster's sources flagged as leaks. Add a setMetricsLeakAssertEnabled(boolean) flag to MiniOzoneCluster.Builder (default true); when false, MiniOzoneClusterImpl.shutdown() skips the assertion. Disable it in the three tests that use MiniOzoneClusterProvider: TestSafeMode, TestHDDSUpgrade, and TestDecommissionAndMaintenance. Verified: TestSafeMode, TestDecommissionAndMaintenance (9 tests), and TestMiniOzoneCluster (5 tests) pass; 0 checkstyle violations. Generated-by: Codex (GPT-5)
OmSnapshotInternalMetrics is created in the OzoneManager constructor and has an unregister() method, but OzoneManager.stop() never called it, so the source stayed registered after a single-OM mini cluster shut down. Add the unregister call alongside the existing DeletingServiceMetrics and OMPerformanceMetrics unregisters. Verified: a single-OM cluster no longer leaks OmSnapshotInternalMetrics (passes with the source removed from the leak-assertion allowlist). Note: in OM HA (multiple OMs in one JVM) the metrics system registers OmSnapshotInternalMetrics-1, -2, ... for the additional OMs, and those suffixed registrations still leak because unregister() only removes the base name. That shared per-OM registration problem affects many OM metrics uniformly and is tracked separately. Generated-by: Codex (GPT-5)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Add guardrails to detect Hadoop metrics leak similar to
ManagedRocksObjectMetrics.INSTANCE.assertNoLeaks.One idea is enable all the metrics related configuration in all the integration tests (e.g. percentiles configuration) and after the MiniOzoneCluster are shut down, check the metric registry to ensure that there are no more metrics that are still registered. We will fail the test so that we can fail loudly.
The main class is MetricsLeakAssertion, we have two list of leaky Metric sources:
(Generated-by: Kimi K3)
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15960
How was this patch tested?
CI.