HDDS-16119. Fix DatanodeStorageMetrics metrics-system deadlock and mini-cluster source leak by smengcl · Pull Request #10988 · apache/ozone

smengcl · 2026-08-10T23:14:56Z

Generated-by: Claude Code (Opus 4.8)

What changes were proposed in this pull request?

DatanodeStorageMetrics (added in HDDS-13128) has two defects, both fixed here.

Defect 1 (deadlock). getMetrics() reads storage totals via MutableVolumeSet.getStorageReport(), which takes the volume-set read lock. The metrics sampler timer calls getMetrics() while holding the global DefaultMetricsSystem monitor (every HDDS service registers PrometheusMetricsSink by default, so the timer samples all sources). Meanwhile a volume-failure handler (MutableVolumeSet.failVolume()) holds the volume-set write lock and then calls VolumeIOStats.unregister(), which needs the same monitor. The two lock orders are opposite, so when a sample tick lands inside a failVolume critical section the two threads deadlock. The sampler then holds the metrics monitor forever and every metrics register, unregister, or sample across the process blocks. On a datanode this hangs the metrics thread and the Prometheus endpoint when a data volume fails; in a mini-cluster (where SCM, OM, and datanodes share one metrics system per JVM) it freezes the whole cluster.

The fix adds MutableVolumeSet.getStorageReportSnapshot(), a lock-free read from the existing ConcurrentHashMaps (the same weakly-consistent guarantee that getVolumesList() already provides), and getMetrics() uses it. The locking getStorageReport() is unchanged for the node-report path. DatanodeStorageMetrics was the only sampled metrics source that reached into the shared volume-set lock (VolumeInfoMetrics.getMetrics() reads only its own volume), so removing this edge breaks the cycle.

Defect 2 (source leak, mini-cluster and test scope). The source was registered under a constant name (uniquified to -N in mini-cluster mode) but unregistered by the base name, so every datanode past the first leaked its source and pinned a shut-down datanode's MutableVolumeSet. In mini-cluster mode the source is now registered and unregistered under a per-datanode name so the two are symmetric. Production keeps the plain DatanodeStorageMetrics name (one instance per JVM) so JMX and Prometheus metric names are unchanged.

This deadlock is also the root cause of the recent master integration-job 90 minute timeouts: an intermittent, silent hang of a long-running test with a random cross-subsystem victim, no slowdown on runs that miss the race, and a sharp onset at the first integration coverage of HDDS-13128.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-16119

How was this patch tested?

New and updated unit and integration tests, all run locally:

TestVolumeSet#testStorageReportSnapshotDoesNotBlockOnWriteLock: holds the volume-set write lock and asserts the snapshot returns promptly from another thread while the locking getStorageReport() times out.
TestDatanodeStorageMetrics#testNoSourceLeakInMiniClusterMode: in mini-cluster mode, asserts create then unregister leaves no residual source.
TestDatanodeStorageMetricsIntegration: updated to look up the per-datanode source name; passes on a real single-datanode MiniOzoneCluster.

mvn -pl :hdds-container-service test -Dtest=TestDatanodeStorageMetrics,TestVolumeSet and the integration test pass, and checkstyle.sh is clean on both changed modules.

And tests are not timing out anymore: https://github.com/smengcl/hadoop-ozone/actions/runs/31443047833

…ni-cluster source leak DatanodeStorageMetrics (added in HDDS-13128) has two defects, both fixed here. Defect 1 (deadlock): getMetrics() read storage totals via MutableVolumeSet.getStorageReport(), which takes the volume-set lock. Because the metrics sampler timer calls getMetrics() while holding the global DefaultMetricsSystem monitor, and a volume-failure handler holds the volume-set write lock while unregistering volume metrics (which needs that monitor), the two lock orders are opposite and can deadlock. A wedged metrics monitor then freezes the whole process. Fix: add MutableVolumeSet.getStorageReportSnapshot(), a lock-free read from the ConcurrentHashMaps (same weakly-consistent guarantee as getVolumesList()), and have getMetrics() use it. Defect 2 (leak, mini-cluster/test scope): the source was registered under a constant name (uniquified to -N in mini-cluster mode) but unregistered by the base name, leaking every datanode past the first and pinning its volume set. Fix: in mini-cluster mode, register/unregister under a per-datanode name so the two are symmetric; production keeps the plain name for stable JMX and Prometheus metric names. Tests: TestVolumeSet asserts the snapshot does not block on the write lock; TestDatanodeStorageMetrics asserts no source leak in mini-cluster mode; TestDatanodeStorageMetricsIntegration looks up the per-datanode source name. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…emImpl cast Review follow-ups to testNoSourceLeakInMiniClusterMode: - Unregister both sources in a finally block so a failing assertion does not leak metrics sources into other tests in the JVM (unregister is idempotent). - Use the MetricsSystem interface (getSource is declared there) instead of casting DefaultMetricsSystem.instance() to MetricsSystemImpl. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Fixes two issues in DataNode storage metrics: a metrics-system deadlock caused by lock-order inversion between DefaultMetricsSystem sampling and MutableVolumeSet locking, and a metrics source leak in mini-cluster/test JVMs due to asymmetric register/unregister naming.

Changes:

Add MutableVolumeSet#getStorageReportSnapshot() (lock-free, weakly-consistent) and switch DatanodeStorageMetrics#getMetrics() to use it to break the deadlock cycle.
Make DatanodeStorageMetrics registration/unregistration symmetric in mini-cluster mode by using a per-datanode source name (DatanodeStorageMetrics-<uuid>), preventing source leaks and pinned MutableVolumeSets.
Update unit/integration tests to validate the non-blocking snapshot behavior and the corrected per-datanode source naming in mini-cluster mode.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/MutableVolumeSet.java	Adds lock-free storage report snapshot API used by metrics to avoid deadlock.
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/DatanodeStorageMetrics.java	Uses snapshot API for sampling; fixes mini-cluster source naming to prevent leaks.
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestVolumeSet.java	Adds regression coverage ensuring snapshot doesn’t block behind the volume-set write lock.
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestDatanodeStorageMetrics.java	Updates unit tests to validate snapshot sampling and symmetric source unregister in mini-cluster mode.
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/TestDatanodeStorageMetricsIntegration.java	Updates integration test to locate the metrics source via the per-datanode name in mini-cluster mode.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…ment mini-cluster naming - getStorageReport() (node-report path, under the read lock) keeps its original pre-sized-array implementation to avoid the extra ArrayList allocation; the ArrayList is used only in the lock-free getStorageReportSnapshot() where the map sizes can change concurrently. - Update DatanodeStorageMetrics class Javadoc to note the per-datanode source name suffix used in mini-cluster mode. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

yandrey321 · 2026-08-11T00:26:14Z

@jojochuang please take a look

yandrey321 · 2026-08-11T00:34:44Z

+   * {@link #getStorageReportSnapshot()} from those paths instead.
+   */
  public StorageLocationReport[] getStorageReport() {
    this.readLock();


why do we need read lock here?

The lock was added way back in HDDS-354 to fix NPE. Without the lock, you get inconsistent view and hit NPE in getNodeReport() with the current impl. Note: method was renamed from getNodeReport() to getStorageReport() in HDDS-3807.

…concurrent maps Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

priyeshkaratha

Thanks @smengcl for working on this. Changes overall LGTM. Please check the nit

Address review comment: create() registered with datanodeStorageMetrics.sourceName while unregister() and getMetrics() use the sourceName field directly. Compute the source name as a local in create() and pass it to the constructor, so create() registers with a bare sourceName like the other operations. Pure refactor, no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

priyeshkaratha

Thanks for updating the PR. Changes LGTM

smengcl · 2026-08-11T17:10:43Z

Thanks @yandrey321 @priyeshkaratha for the reviews.

smengcl and others added 2 commits August 10, 2026 15:59

Copilot AI lite review requested due to automatic review settings August 10, 2026 23:14

smengcl added bug Something isn't working metrics AI-gen labels Aug 10, 2026

Copilot started reviewing on behalf of smengcl August 10, 2026 23:15 View session

smengcl mentioned this pull request Aug 10, 2026

HDDS-13128. Expose per DN space utilisation as JMX metrics #10913

Merged

Copilot AI reviewed Aug 10, 2026

View reviewed changes

Comment thread ...-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/MutableVolumeSet.java

Comment thread ...ce/src/main/java/org/apache/hadoop/ozone/container/common/volume/DatanodeStorageMetrics.java

smengcl and others added 2 commits August 10, 2026 16:26

HDDS-16119. Document getStorageReport() metrics deadlock caveat

b1ef717

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

yandrey321 reviewed Aug 11, 2026

View reviewed changes

Comment thread ...-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/MutableVolumeSet.java Outdated

yandrey321 reviewed Aug 11, 2026

View reviewed changes

HDDS-16119. Simplify getStorageReportSnapshot() to a stream over the …

440d3c9

…concurrent maps Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

smengcl requested a review from jojochuang August 11, 2026 05:29

smengcl mentioned this pull request Aug 11, 2026

HDDS-15542. Closed-container reconciliation advances BCSID past a hole, masking missing chunks #10496

Merged

smengcl marked this pull request as ready for review August 11, 2026 07:47

priyeshkaratha reviewed Aug 11, 2026

View reviewed changes

Comment thread ...ce/src/main/java/org/apache/hadoop/ozone/container/common/volume/DatanodeStorageMetrics.java Outdated

priyeshkaratha approved these changes Aug 11, 2026

View reviewed changes

smengcl merged commit bd0f23b into apache:master Aug 11, 2026
45 checks passed

smengcl deleted the HDDS-16119 branch August 11, 2026 17:10

Navigation Menu

Uh oh!

HDDS-16119. Fix DatanodeStorageMetrics metrics-system deadlock and mini-cluster source leak - #10988

HDDS-16119. Fix DatanodeStorageMetrics metrics-system deadlock and mini-cluster source leak#10988
smengcl merged 6 commits into
apache:masterfrom
smengcl:HDDS-16119

smengcl commented Aug 10, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

yandrey321 commented Aug 11, 2026

Uh oh!

Uh oh!

yandrey321 Aug 11, 2026

Uh oh!

smengcl Aug 11, 2026

Uh oh!

priyeshkaratha left a comment

Uh oh!

Uh oh!

priyeshkaratha left a comment

Uh oh!

Uh oh!

smengcl commented Aug 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

smengcl commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

yandrey321 commented Aug 11, 2026

Uh oh!

Uh oh!

yandrey321 Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

smengcl Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

priyeshkaratha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

priyeshkaratha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smengcl commented Aug 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

smengcl commented Aug 10, 2026 •

edited

Loading