From 68c31a42ad79802bd5f33eb78d649bc5a018325c Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Mon, 10 Aug 2026 15:59:51 -0700 Subject: [PATCH 1/6] HDDS-16119. Fix DatanodeStorageMetrics metrics-system deadlock and mini-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) --- .../common/volume/DatanodeStorageMetrics.java | 31 ++++++++++--- .../common/volume/MutableVolumeSet.java | 43 +++++++++++++++---- .../volume/TestDatanodeStorageMetrics.java | 43 ++++++++++++++++++- .../common/volume/TestVolumeSet.java | 27 ++++++++++++ ...TestDatanodeStorageMetricsIntegration.java | 9 +++- 5 files changed, 133 insertions(+), 20 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/DatanodeStorageMetrics.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/DatanodeStorageMetrics.java index fc1edbee11ed..6def4acfefed 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/DatanodeStorageMetrics.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/DatanodeStorageMetrics.java @@ -30,7 +30,7 @@ /** * Node-level storage totals for a DataNode, aggregated over its HDDS data volumes only - * ({@code VolumeType.DATA_VOLUME}) via {@link MutableVolumeSet#getStorageReport()}. + * ({@code VolumeType.DATA_VOLUME}) via {@link MutableVolumeSet#getStorageReportSnapshot()}. * This is the same scope as the {@code storageReport} entries produced by * {@code OzoneContainer.getNodeReport()}; meta and DB volumes are excluded. * Registered as {@code Hadoop:service=HddsDatanode,name=DatanodeStorageMetrics}. @@ -53,10 +53,22 @@ public final class DatanodeStorageMetrics implements MetricsSource { private final MetricsRegistry registry; private final MutableVolumeSet volumeSet; + private final String sourceName; private DatanodeStorageMetrics(MutableVolumeSet volumeSet) { this.volumeSet = volumeSet; - this.registry = new MetricsRegistry(SOURCE_NAME); + // In mini-cluster mode many datanodes share one metrics system, so the + // metrics system uniquifies the constant source name on registration + // (DatanodeStorageMetrics-1, -2, ...). Unregistering by the constant base + // name would then leak every source past the first, and each leaked source + // pins a shut-down datanode's MutableVolumeSet. Make the name unique per + // datanode up front so register and unregister stay symmetric. In + // production there is one instance per JVM, so keep the plain name for + // stable JMX and Prometheus metric names. + this.sourceName = DefaultMetricsSystem.inMiniClusterMode() + ? SOURCE_NAME + '-' + volumeSet.getDatanodeUuid() + : SOURCE_NAME; + this.registry = new MetricsRegistry(sourceName); } /** @@ -65,8 +77,8 @@ private DatanodeStorageMetrics(MutableVolumeSet volumeSet) { */ public static DatanodeStorageMetrics create(MutableVolumeSet volumeSet) { DatanodeStorageMetrics datanodeStorageMetrics = new DatanodeStorageMetrics(volumeSet); - DefaultMetricsSystem.instance().register( - SOURCE_NAME, "DataNode node-level storage totals", datanodeStorageMetrics); + DefaultMetricsSystem.instance().register(datanodeStorageMetrics.sourceName, + "DataNode node-level storage totals", datanodeStorageMetrics); return datanodeStorageMetrics; } @@ -74,7 +86,7 @@ public static DatanodeStorageMetrics create(MutableVolumeSet volumeSet) { * Unregisters this source from the Metrics2 system. */ public void unregister() { - DefaultMetricsSystem.instance().unregisterSource(SOURCE_NAME); + DefaultMetricsSystem.instance().unregisterSource(sourceName); } /** @@ -83,12 +95,17 @@ public void unregister() { */ @Override public void getMetrics(MetricsCollector collector, boolean all) { - MetricsRecordBuilder builder = collector.addRecord(SOURCE_NAME); + MetricsRecordBuilder builder = collector.addRecord(sourceName); registry.snapshot(builder, all); long capacity = 0L; long used = 0L; - for (StorageLocationReport report : volumeSet.getStorageReport()) { + // getMetrics() runs while the DefaultMetricsSystem monitor is held. Read a + // lock-free snapshot instead of getStorageReport(), which takes the + // volume-set lock: a volume-failure handler holds that lock while + // unregistering volume metrics (which needs the same monitor), so locking + // here can deadlock the metrics system. + for (StorageLocationReport report : volumeSet.getStorageReportSnapshot()) { capacity = Math.addExact(capacity, report.getCapacity()); used = Math.addExact(used, report.getScmUsed()); } 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..434a35f813de 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 @@ -21,6 +21,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import java.io.IOException; +import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.Map; @@ -407,20 +408,44 @@ public boolean hasEnoughVolumes() { public StorageLocationReport[] getStorageReport() { this.readLock(); try { - StorageLocationReport[] reports = new StorageLocationReport[volumeMap.size() + failedVolumeMap.size()]; - int counter = 0; - for (StorageVolume volume : volumeMap.values()) { - reports[counter++] = volume.getReport(); - } - for (StorageVolume volume : failedVolumeMap.values()) { - reports[counter++] = volume.getReport(); - } - return reports; + return buildStorageReport(); } finally { this.readUnlock(); } } + /** + * Lock-free variant of {@link #getStorageReport()}. Both {@code volumeMap} + * and {@code failedVolumeMap} are {@link ConcurrentHashMap}s, so this returns + * a weakly-consistent snapshot (mirroring {@link #getVolumesList()}) without + * acquiring the volume-set lock. + * + *

Use this from callers that must not block on the volume-set lock. In + * particular, metrics sampling runs while the {@code DefaultMetricsSystem} + * monitor is held, and a volume-failure handler holds the volume-set write + * lock while unregistering volume metrics (which needs that same monitor); + * acquiring the volume-set lock from the sampling thread can therefore + * deadlock the whole metrics system. + */ + public StorageLocationReport[] getStorageReportSnapshot() { + return buildStorageReport(); + } + + private StorageLocationReport[] buildStorageReport() { + List reports = new ArrayList<>(volumeMap.size() + failedVolumeMap.size()); + for (StorageVolume volume : volumeMap.values()) { + reports.add(volume.getReport()); + } + for (StorageVolume volume : failedVolumeMap.values()) { + reports.add(volume.getReport()); + } + return reports.toArray(new StorageLocationReport[0]); + } + + public String getDatanodeUuid() { + return datanodeUuid; + } + public StorageVolume.VolumeType getVolumeType() { return volumeType; } diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestDatanodeStorageMetrics.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestDatanodeStorageMetrics.java index d54a8b0f9ea8..555cdd59e0d4 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestDatanodeStorageMetrics.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestDatanodeStorageMetrics.java @@ -21,9 +21,12 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import java.util.UUID; import org.apache.hadoop.metrics2.AbstractMetric; import org.apache.hadoop.metrics2.impl.MetricsCollectorImpl; import org.apache.hadoop.metrics2.impl.MetricsRecordImpl; +import org.apache.hadoop.metrics2.impl.MetricsSystemImpl; +import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem; import org.apache.hadoop.ozone.container.common.impl.StorageLocationReport; import org.junit.jupiter.api.Test; @@ -51,7 +54,7 @@ void testAggregationAcrossTwoVolumes() { .build(); MutableVolumeSet volumeSet = mock(MutableVolumeSet.class); - when(volumeSet.getStorageReport()) + when(volumeSet.getStorageReportSnapshot()) .thenReturn(new StorageLocationReport[]{vol1, vol2}); DatanodeStorageMetrics metrics = DatanodeStorageMetrics.create(volumeSet); @@ -78,7 +81,7 @@ void testAggregationAcrossTwoVolumes() { void testZeroCapacityReturnsZeroPercentage() { // No volumes → capacity=0, used=0; OzoneUsedPercentage must be 0.0, not NaN. MutableVolumeSet volumeSet = mock(MutableVolumeSet.class); - when(volumeSet.getStorageReport()).thenReturn(new StorageLocationReport[0]); + when(volumeSet.getStorageReportSnapshot()).thenReturn(new StorageLocationReport[0]); DatanodeStorageMetrics metrics = DatanodeStorageMetrics.create(volumeSet); try { @@ -94,6 +97,42 @@ void testZeroCapacityReturnsZeroPercentage() { } } + @Test + void testNoSourceLeakInMiniClusterMode() { + // In mini-cluster mode many datanodes share one metrics system. Register + // and unregister must be symmetric so no source (and its pinned volume set) + // leaks. The pre-fix code registered a constant name (uniquified to -N) but + // unregistered the base name, leaking every datanode past the first. + boolean prev = DefaultMetricsSystem.inMiniClusterMode(); + DefaultMetricsSystem.setMiniClusterMode(true); + try { + MetricsSystemImpl ms = (MetricsSystemImpl) DefaultMetricsSystem.instance(); + String uuidA = "dn-" + UUID.randomUUID(); + String uuidB = "dn-" + UUID.randomUUID(); + String nameA = DatanodeStorageMetrics.SOURCE_NAME + '-' + uuidA; + String nameB = DatanodeStorageMetrics.SOURCE_NAME + '-' + uuidB; + + DatanodeStorageMetrics a = DatanodeStorageMetrics.create(mockVolumeSet(uuidA)); + DatanodeStorageMetrics b = DatanodeStorageMetrics.create(mockVolumeSet(uuidB)); + assertThat(ms.getSource(nameA)).isNotNull(); + assertThat(ms.getSource(nameB)).isNotNull(); + + a.unregister(); + b.unregister(); + assertThat(ms.getSource(nameA)).isNull(); + assertThat(ms.getSource(nameB)).isNull(); + } finally { + DefaultMetricsSystem.setMiniClusterMode(prev); + } + } + + private static MutableVolumeSet mockVolumeSet(String datanodeUuid) { + MutableVolumeSet volumeSet = mock(MutableVolumeSet.class); + when(volumeSet.getDatanodeUuid()).thenReturn(datanodeUuid); + when(volumeSet.getStorageReportSnapshot()).thenReturn(new StorageLocationReport[0]); + return volumeSet; + } + private static long findLong(Iterable metrics, String name) { for (AbstractMetric m : metrics) { if (name.equals(m.name())) { 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..64e1c00e70e4 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,7 @@ 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.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.File; @@ -34,6 +35,9 @@ import java.util.ArrayList; import java.util.List; import java.util.UUID; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import org.apache.commons.io.FileUtils; import org.apache.hadoop.hdds.HddsConfigKeys; import org.apache.hadoop.hdds.conf.OzoneConfiguration; @@ -194,6 +198,29 @@ void testFailVolumes(@TempDir File readOnlyVolumePath, @TempDir File volumePath) volSet.shutdown(); } + @Test + public void testStorageReportSnapshotDoesNotBlockOnWriteLock() throws Exception { + // Regression test: DatanodeStorageMetrics.getMetrics() samples the volume + // set while the DefaultMetricsSystem monitor is held. It must not block on + // the volume-set lock, otherwise it deadlocks with a volume-failure handler + // that holds the write lock while unregistering volume metrics. + volumeSet.writeLock(); + try { + CompletableFuture snapshot = CompletableFuture.supplyAsync( + () -> volumeSet.getStorageReportSnapshot().length); + // The snapshot must return without waiting for the write lock. + assertEquals(2, snapshot.get(10, TimeUnit.SECONDS)); + + // Sanity check that the lock is genuinely held: the locking variant does + // block behind the write lock (it must time out here). + CompletableFuture locked = CompletableFuture.supplyAsync( + () -> volumeSet.getStorageReport().length); + assertThrows(TimeoutException.class, () -> locked.get(2, TimeUnit.SECONDS)); + } finally { + volumeSet.writeUnlock(); + } + } + @Test public void testInterrupt() throws Exception { Method method = this.volumeSet.getClass() diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/TestDatanodeStorageMetricsIntegration.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/TestDatanodeStorageMetricsIntegration.java index 076889a7e8f1..774184e8689b 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/TestDatanodeStorageMetricsIntegration.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/TestDatanodeStorageMetricsIntegration.java @@ -123,7 +123,12 @@ void storageMetricsReflectWrittenData() throws Exception { * Each call re-reads the underlying storage reports — do not mix values * from different calls when checking invariants across gauges. */ - private static MetricsRecordBuilder storageMetrics() { - return getMetrics(DatanodeStorageMetrics.SOURCE_NAME); + private MetricsRecordBuilder storageMetrics() { + // In mini-cluster mode the source name is made unique per datanode (to keep + // metrics registration and unregistration symmetric and avoid the + // shared-JVM source leak), so look it up by the per-datanode name. + MutableVolumeSet volumeSet = cluster.getHddsDatanodes().get(0) + .getDatanodeStateMachine().getContainer().getVolumeSet(); + return getMetrics(DatanodeStorageMetrics.SOURCE_NAME + '-' + volumeSet.getDatanodeUuid()); } } From 870c68a154f5d26a0b59f78403bb96ad5d4602d7 Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Mon, 10 Aug 2026 16:07:26 -0700 Subject: [PATCH 2/6] HDDS-16119. Harden leak test: unregister in finally, drop MetricsSystemImpl 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) --- .../volume/TestDatanodeStorageMetrics.java | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestDatanodeStorageMetrics.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestDatanodeStorageMetrics.java index 555cdd59e0d4..05d14410a4b3 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestDatanodeStorageMetrics.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestDatanodeStorageMetrics.java @@ -23,9 +23,9 @@ import java.util.UUID; import org.apache.hadoop.metrics2.AbstractMetric; +import org.apache.hadoop.metrics2.MetricsSystem; import org.apache.hadoop.metrics2.impl.MetricsCollectorImpl; import org.apache.hadoop.metrics2.impl.MetricsRecordImpl; -import org.apache.hadoop.metrics2.impl.MetricsSystemImpl; import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem; import org.apache.hadoop.ozone.container.common.impl.StorageLocationReport; import org.junit.jupiter.api.Test; @@ -105,15 +105,16 @@ void testNoSourceLeakInMiniClusterMode() { // unregistered the base name, leaking every datanode past the first. boolean prev = DefaultMetricsSystem.inMiniClusterMode(); DefaultMetricsSystem.setMiniClusterMode(true); + MetricsSystem ms = DefaultMetricsSystem.instance(); + String uuidA = "dn-" + UUID.randomUUID(); + String uuidB = "dn-" + UUID.randomUUID(); + String nameA = DatanodeStorageMetrics.SOURCE_NAME + '-' + uuidA; + String nameB = DatanodeStorageMetrics.SOURCE_NAME + '-' + uuidB; + DatanodeStorageMetrics a = null; + DatanodeStorageMetrics b = null; try { - MetricsSystemImpl ms = (MetricsSystemImpl) DefaultMetricsSystem.instance(); - String uuidA = "dn-" + UUID.randomUUID(); - String uuidB = "dn-" + UUID.randomUUID(); - String nameA = DatanodeStorageMetrics.SOURCE_NAME + '-' + uuidA; - String nameB = DatanodeStorageMetrics.SOURCE_NAME + '-' + uuidB; - - DatanodeStorageMetrics a = DatanodeStorageMetrics.create(mockVolumeSet(uuidA)); - DatanodeStorageMetrics b = DatanodeStorageMetrics.create(mockVolumeSet(uuidB)); + a = DatanodeStorageMetrics.create(mockVolumeSet(uuidA)); + b = DatanodeStorageMetrics.create(mockVolumeSet(uuidB)); assertThat(ms.getSource(nameA)).isNotNull(); assertThat(ms.getSource(nameB)).isNotNull(); @@ -122,6 +123,14 @@ void testNoSourceLeakInMiniClusterMode() { assertThat(ms.getSource(nameA)).isNull(); assertThat(ms.getSource(nameB)).isNull(); } finally { + // Do not leak sources into other tests if an assertion above fails. + // unregister() is idempotent, so a repeat after the happy path is a no-op. + if (a != null) { + a.unregister(); + } + if (b != null) { + b.unregister(); + } DefaultMetricsSystem.setMiniClusterMode(prev); } } From 4c8d430206fac804f405d5563b8900ed85107dad Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Mon, 10 Aug 2026 16:26:24 -0700 Subject: [PATCH 3/6] HDDS-16119. Address review: keep pre-sized array on locked path, document 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) --- .../common/volume/DatanodeStorageMetrics.java | 3 +++ .../common/volume/MutableVolumeSet.java | 16 +++++++++++----- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/DatanodeStorageMetrics.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/DatanodeStorageMetrics.java index 6def4acfefed..7949209c21aa 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/DatanodeStorageMetrics.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/DatanodeStorageMetrics.java @@ -34,6 +34,9 @@ * This is the same scope as the {@code storageReport} entries produced by * {@code OzoneContainer.getNodeReport()}; meta and DB volumes are excluded. * Registered as {@code Hadoop:service=HddsDatanode,name=DatanodeStorageMetrics}. + * In mini-cluster mode, where many datanodes share one metrics system, the name + * is suffixed with the datanode UUID ({@code DatanodeStorageMetrics-}) so + * registration and unregistration stay unique per datanode. */ @Metrics(about = "Ozone DataNode node-level storage totals", context = OzoneConsts.OZONE) 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 434a35f813de..b6837d4a90cc 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 @@ -408,7 +408,15 @@ public boolean hasEnoughVolumes() { public StorageLocationReport[] getStorageReport() { this.readLock(); try { - return buildStorageReport(); + StorageLocationReport[] reports = new StorageLocationReport[volumeMap.size() + failedVolumeMap.size()]; + int counter = 0; + for (StorageVolume volume : volumeMap.values()) { + reports[counter++] = volume.getReport(); + } + for (StorageVolume volume : failedVolumeMap.values()) { + reports[counter++] = volume.getReport(); + } + return reports; } finally { this.readUnlock(); } @@ -428,10 +436,8 @@ public StorageLocationReport[] getStorageReport() { * deadlock the whole metrics system. */ public StorageLocationReport[] getStorageReportSnapshot() { - return buildStorageReport(); - } - - private StorageLocationReport[] buildStorageReport() { + // No lock is held, so the map sizes can change concurrently; collect into a + // list instead of indexing into a pre-sized array. List reports = new ArrayList<>(volumeMap.size() + failedVolumeMap.size()); for (StorageVolume volume : volumeMap.values()) { reports.add(volume.getReport()); From b1ef717293e3a8ec9f00fcc96df509dcc66adad0 Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Mon, 10 Aug 2026 16:37:02 -0700 Subject: [PATCH 4/6] HDDS-16119. Document getStorageReport() metrics deadlock caveat Co-Authored-By: Claude Opus 4.8 (1M context) --- .../container/common/volume/MutableVolumeSet.java | 10 ++++++++++ 1 file changed, 10 insertions(+) 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 b6837d4a90cc..51c682ba5cdd 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 @@ -405,6 +405,16 @@ public boolean hasEnoughVolumes() { return hasEnoughVolumes; } + /** + * Returns a consistent snapshot of the storage reports under the volume-set + * read lock. + * + *

Do not call this from metrics collection (or any caller that may hold the + * {@code DefaultMetricsSystem} monitor): a volume-failure handler holds the + * volume-set write lock while unregistering volume metrics under that same + * monitor, so taking the read lock here can deadlock the metrics system. Use + * {@link #getStorageReportSnapshot()} from those paths instead. + */ public StorageLocationReport[] getStorageReport() { this.readLock(); try { From 440d3c99b133085a7b885eb4305bfe34b72774f0 Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Mon, 10 Aug 2026 22:15:51 -0700 Subject: [PATCH 5/6] HDDS-16119. Simplify getStorageReportSnapshot() to a stream over the concurrent maps Co-Authored-By: Claude Opus 4.8 (1M context) --- .../common/volume/MutableVolumeSet.java | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) 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 51c682ba5cdd..4d60015a050e 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 @@ -21,7 +21,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import java.io.IOException; -import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.Map; @@ -29,6 +28,7 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.function.Function; +import java.util.stream.Stream; import org.apache.hadoop.hdds.conf.ConfigurationSource; import org.apache.hadoop.hdds.fs.SpaceUsageCheckFactory; import org.apache.hadoop.hdds.scm.ScmConfigKeys; @@ -446,16 +446,12 @@ public StorageLocationReport[] getStorageReport() { * deadlock the whole metrics system. */ public StorageLocationReport[] getStorageReportSnapshot() { - // No lock is held, so the map sizes can change concurrently; collect into a - // list instead of indexing into a pre-sized array. - List reports = new ArrayList<>(volumeMap.size() + failedVolumeMap.size()); - for (StorageVolume volume : volumeMap.values()) { - reports.add(volume.getReport()); - } - for (StorageVolume volume : failedVolumeMap.values()) { - reports.add(volume.getReport()); - } - return reports.toArray(new StorageLocationReport[0]); + // volumeMap and failedVolumeMap are ConcurrentHashMaps; their value streams + // are weakly consistent, so no lock is needed here (same guarantee as + // getVolumesList()). + return Stream.concat(volumeMap.values().stream(), failedVolumeMap.values().stream()) + .map(StorageVolume::getReport) + .toArray(StorageLocationReport[]::new); } public String getDatanodeUuid() { From 667aa5bed5aec4294b9d0d3e7dd100c455c32702 Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Tue, 11 Aug 2026 03:07:32 -0700 Subject: [PATCH 6/6] HDDS-16119. Compute sourceName as a local in create() for consistency 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) --- .../common/volume/DatanodeStorageMetrics.java | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/DatanodeStorageMetrics.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/DatanodeStorageMetrics.java index 7949209c21aa..b0fdd9036fa7 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/DatanodeStorageMetrics.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/DatanodeStorageMetrics.java @@ -58,8 +58,17 @@ public final class DatanodeStorageMetrics implements MetricsSource { private final MutableVolumeSet volumeSet; private final String sourceName; - private DatanodeStorageMetrics(MutableVolumeSet volumeSet) { + private DatanodeStorageMetrics(MutableVolumeSet volumeSet, String sourceName) { this.volumeSet = volumeSet; + this.sourceName = sourceName; + this.registry = new MetricsRegistry(sourceName); + } + + /** + * Creates a new {@code DatanodeStorageMetrics} instance and registers it + * with the default Metrics2 system. + */ + public static DatanodeStorageMetrics create(MutableVolumeSet volumeSet) { // In mini-cluster mode many datanodes share one metrics system, so the // metrics system uniquifies the constant source name on registration // (DatanodeStorageMetrics-1, -2, ...). Unregistering by the constant base @@ -68,19 +77,11 @@ private DatanodeStorageMetrics(MutableVolumeSet volumeSet) { // datanode up front so register and unregister stay symmetric. In // production there is one instance per JVM, so keep the plain name for // stable JMX and Prometheus metric names. - this.sourceName = DefaultMetricsSystem.inMiniClusterMode() + String sourceName = DefaultMetricsSystem.inMiniClusterMode() ? SOURCE_NAME + '-' + volumeSet.getDatanodeUuid() : SOURCE_NAME; - this.registry = new MetricsRegistry(sourceName); - } - - /** - * Creates a new {@code DatanodeStorageMetrics} instance and registers it - * with the default Metrics2 system. - */ - public static DatanodeStorageMetrics create(MutableVolumeSet volumeSet) { - DatanodeStorageMetrics datanodeStorageMetrics = new DatanodeStorageMetrics(volumeSet); - DefaultMetricsSystem.instance().register(datanodeStorageMetrics.sourceName, + DatanodeStorageMetrics datanodeStorageMetrics = new DatanodeStorageMetrics(volumeSet, sourceName); + DefaultMetricsSystem.instance().register(sourceName, "DataNode node-level storage totals", datanodeStorageMetrics); return datanodeStorageMetrics; }