[
https://issues.apache.org/jira/browse/HDDS-16119?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-16119:
------------------------------
Component/s: Ozone Datanode
Description:
{{DatanodeStorageMetrics}} (added in HDDS-13128) has two defects. The first is
a process-wide deadlock that is the root cause of the recent master
integration-job 90-minute timeouts; the second is a metrics-source and
volume-set leak in mini-cluster JVMs. Both are in the same class and are fixed
together.
h2. Defect 1 (Critical): metrics-system deadlock against the volume-set lock
{{getMetrics()}} reads storage totals by calling
{{MutableVolumeSet.getStorageReport()}}, which acquires the volume-set read
lock. That creates a lock-ordering inversion against the global
{{DefaultMetricsSystem}} monitor. Every HDDS service registers
{{PrometheusMetricsSink}} by default ({{HDDS_PROMETHEUS_ENABLED}} defaults to
true, via {{BaseHttpServer}}), so {{MetricsSystemImpl}} has at least one sink
and its periodic timer samples all sources. The two lock orders are opposite:
# Metrics sampler timer: {{MetricsSystemImpl.onTimerEvent()}} is synchronized
on the metrics-system monitor, then calls {{sampleMetrics()}} to
{{DatanodeStorageMetrics.getMetrics()}} to
{{MutableVolumeSet.getStorageReport()}} to the volume-set read lock. Holds the
monitor, wants the volume-set lock.
# Volume-failure handler: {{MutableVolumeSet.failVolume()}} (and
{{handleVolumeFailures()}}) takes the volume-set write lock, then calls
{{HddsVolume.failVolume()}} to {{VolumeIOStats.unregister()}} to
{{DefaultMetricsSystem.unregisterSource()}}, which is synchronized on the
metrics-system monitor. Holds the volume-set lock, wants the monitor.
When the sampler tick lands inside a {{failVolume}} critical section, the two
threads deadlock. The sampler then holds the global metrics monitor forever, so
every subsequent metrics register, unregister, or sample across the whole
process blocks. On a real datanode this hangs the metrics thread and the
Prometheus endpoint precisely when a data volume fails. In a mini-cluster (SCM,
OM, and datanodes share one {{DefaultMetricsSystem}} per JVM) it freezes the
entire cluster, which is the observed CI failure: an intermittent, silent hang
of a long-running test with a random cross-subsystem victim (SCM HA, OM HA,
snapshot, client, none of which changed), no slowdown on runs that miss the
race, and a sharp onset at the first integration coverage of HDDS-13128. Before
HDDS-13128 no sampled metrics source acquired the shared {{MutableVolumeSet}}
lock ({{VolumeInfoMetrics.getMetrics()}} reads only its own volume), so the
cycle did not exist.
h2. Defect 2 (Major, mini-cluster/test scope): source and volume-set leak
{{DatanodeStorageMetrics}} registers under a constant source name and keeps a
{{final MutableVolumeSet}}. Integration tests run many datanodes in one JVM
with {{DefaultMetricsSystem.setMiniClusterMode(true)}}, so the second and later
datanodes register as {{DatanodeStorageMetrics-1}}, {{-2}}, and so on, but
{{unregister()}} removes the constant base name. Every datanode past the first
leaks its source, and each leaked source pins a shut-down datanode's whole
{{MutableVolumeSet}}. This is heap and registry growth within a test class's
JVM. It is mini-cluster scoped (a production datanode has a single instance
created once and unregistered on stop). It does not cause the CI timeouts
(Surefire uses {{reuseForks=false}}, so it cannot accumulate across a split,
and completed-split durations are flat across the onset); it is a correctness
and hygiene bug fixed alongside Defect 1.
h2. Proposed fix
For Defect 1, make the sampling path lock-free so it never blocks on the
volume-set lock while the metrics monitor is held. Add
{{MutableVolumeSet.getStorageReportSnapshot()}}, which builds the report from
the existing {{ConcurrentHashMap}}s without taking the volume-set lock (a
weakly-consistent snapshot, the same guarantee already used by
{{getVolumesList()}}), and have {{getMetrics()}} call it instead of
{{getStorageReport()}}. The locking {{getStorageReport()}} is unchanged for the
node-report path. A transient one-sample miscount during a rare
volume-membership change is acceptable for a gauge and far preferable to a
process-wide deadlock.
For Defect 2, register and unregister under a per-instance unique source name
following the existing {{VolumeInfoMetrics}} pattern ({{SOURCE_BASENAME + '-' +
identifier}}), keeping register and unregister symmetric so no source or volume
set is leaked.
h2. Testing
* {{TestVolumeSet.testStorageReportSnapshotDoesNotBlockOnWriteLock}}: hold the
volume-set write lock, assert {{getStorageReportSnapshot()}} returns promptly
from another thread while the locking {{getStorageReport()}} blocks (times out).
* Leak test: in mini-cluster mode, repeated create plus unregister cycles leave
no residual {{DatanodeStorageMetrics}} sources, and shut-down
{{MutableVolumeSet}} instances become unreachable after GC (measured: 30 of 45
volume sets survived GC before the fix, 0 of 45 after).
* {{TestDatanodeStorageMetrics}} aggregation and zero-capacity behavior
unchanged. Container-service unit tests pass and checkstyle is clean.
was:
{{DatanodeStorageMetrics}} (added in HDDS-13128) registers with the metrics
system under a constant source name and keeps a {{final MutableVolumeSet}}
reference. {{OzoneContainer}} creates one instance per datanode.
Integration tests run many datanodes in a single JVM and enable
{{DefaultMetricsSystem.setMiniClusterMode(true)}} (see {{MiniOzoneClusterImpl}}
and {{MiniOzoneHAClusterImpl}}). In mini-cluster mode the metrics system
uniquifies duplicate source names, so the second and later datanodes register
as {{DatanodeStorageMetrics-1}}, {{DatanodeStorageMetrics-2}}, and so on.
However {{DatanodeStorageMetrics.unregister()}} calls
{{unregisterSource("DatanodeStorageMetrics")}} using the constant base name, so
only the first datanode's source is ever removed. Every datanode beyond the
first leaks its metrics source, and because each leaked source holds a
{{MutableVolumeSet}}, the entire volume set graph of the shut-down datanode
stays reachable and cannot be collected.
Within any single test class that stands up multiple datanodes (or builds
clusters back to back), the leaked sources and their pinned volume sets
accumulate in that class's JVM until the class finishes. This is a correctness
and heap-hygiene bug: shut-down datanode state that should be collectable stays
reachable, and stale {{DatanodeStorageMetrics-N}} sources pile up in the
metrics registry.
The older {{ContainerMetrics}} uses a similar constant-name register and
unregister and also leaks its source in mini-cluster mode, but it retains only
lightweight counters, so it was tolerated. {{DatanodeStorageMetrics}} is
qualitatively worse because it pins a full datanode volume set.
h3. Reproduction (unit level, no cluster)
With {{DefaultMetricsSystem.setMiniClusterMode(true)}}, create N
{{DatanodeStorageMetrics}} over real {{MutableVolumeSet}} instances, shut the
volume sets down, then call {{unregister()}} on each (as
{{OzoneContainer.stop()}} does), and force GC. Volume sets held only through
the leaked metrics sources remain reachable. Measured with 15 clusters of 3
datanodes: 30 of 45 volume sets survived GC with {{DatanodeStorageMetrics}},
versus 0 of 45 for the pre-change baseline.
h3. Proposed fix
Register and unregister under a per-instance unique source name, following the
existing {{VolumeInfoMetrics}} pattern ({{SOURCE_BASENAME + '-' +
identifier}}). This keeps register and unregister symmetric so no source or
volume set is leaked. With the fix the same reproduction retains 0 of 45 volume
sets.
h3. Testing
Unit test that asserts, in mini-cluster mode, that repeated create plus
unregister cycles leave no residual {{DatanodeStorageMetrics}} sources and that
shut-down {{MutableVolumeSet}} instances become unreachable after GC.
Priority: Critical (was: Major)
Summary: DatanodeStorageMetrics can deadlock the metrics system on
volume failure, and leaks its source in mini-cluster mode (was:
DatanodeStorageMetrics leaks a metrics source and its MutableVolumeSet per
datanode in mini-cluster JVMs)
> DatanodeStorageMetrics can deadlock the metrics system on volume failure, and
> leaks its source in mini-cluster mode
> -------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-16119
> URL: https://issues.apache.org/jira/browse/HDDS-16119
> Project: Apache Ozone
> Issue Type: Bug
> Components: Ozone Datanode
> Reporter: Siyao Meng
> Priority: Critical
>
> {{DatanodeStorageMetrics}} (added in HDDS-13128) has two defects. The first
> is a process-wide deadlock that is the root cause of the recent master
> integration-job 90-minute timeouts; the second is a metrics-source and
> volume-set leak in mini-cluster JVMs. Both are in the same class and are
> fixed together.
> h2. Defect 1 (Critical): metrics-system deadlock against the volume-set lock
> {{getMetrics()}} reads storage totals by calling
> {{MutableVolumeSet.getStorageReport()}}, which acquires the volume-set read
> lock. That creates a lock-ordering inversion against the global
> {{DefaultMetricsSystem}} monitor. Every HDDS service registers
> {{PrometheusMetricsSink}} by default ({{HDDS_PROMETHEUS_ENABLED}} defaults to
> true, via {{BaseHttpServer}}), so {{MetricsSystemImpl}} has at least one sink
> and its periodic timer samples all sources. The two lock orders are opposite:
> # Metrics sampler timer: {{MetricsSystemImpl.onTimerEvent()}} is synchronized
> on the metrics-system monitor, then calls {{sampleMetrics()}} to
> {{DatanodeStorageMetrics.getMetrics()}} to
> {{MutableVolumeSet.getStorageReport()}} to the volume-set read lock. Holds
> the monitor, wants the volume-set lock.
> # Volume-failure handler: {{MutableVolumeSet.failVolume()}} (and
> {{handleVolumeFailures()}}) takes the volume-set write lock, then calls
> {{HddsVolume.failVolume()}} to {{VolumeIOStats.unregister()}} to
> {{DefaultMetricsSystem.unregisterSource()}}, which is synchronized on the
> metrics-system monitor. Holds the volume-set lock, wants the monitor.
> When the sampler tick lands inside a {{failVolume}} critical section, the two
> threads deadlock. The sampler then holds the global metrics monitor forever,
> so every subsequent metrics register, unregister, or sample across the whole
> process blocks. On a real datanode this hangs the metrics thread and the
> Prometheus endpoint precisely when a data volume fails. In a mini-cluster
> (SCM, OM, and datanodes share one {{DefaultMetricsSystem}} per JVM) it
> freezes the entire cluster, which is the observed CI failure: an
> intermittent, silent hang of a long-running test with a random
> cross-subsystem victim (SCM HA, OM HA, snapshot, client, none of which
> changed), no slowdown on runs that miss the race, and a sharp onset at the
> first integration coverage of HDDS-13128. Before HDDS-13128 no sampled
> metrics source acquired the shared {{MutableVolumeSet}} lock
> ({{VolumeInfoMetrics.getMetrics()}} reads only its own volume), so the cycle
> did not exist.
> h2. Defect 2 (Major, mini-cluster/test scope): source and volume-set leak
> {{DatanodeStorageMetrics}} registers under a constant source name and keeps a
> {{final MutableVolumeSet}}. Integration tests run many datanodes in one JVM
> with {{DefaultMetricsSystem.setMiniClusterMode(true)}}, so the second and
> later datanodes register as {{DatanodeStorageMetrics-1}}, {{-2}}, and so on,
> but {{unregister()}} removes the constant base name. Every datanode past the
> first leaks its source, and each leaked source pins a shut-down datanode's
> whole {{MutableVolumeSet}}. This is heap and registry growth within a test
> class's JVM. It is mini-cluster scoped (a production datanode has a single
> instance created once and unregistered on stop). It does not cause the CI
> timeouts (Surefire uses {{reuseForks=false}}, so it cannot accumulate across
> a split, and completed-split durations are flat across the onset); it is a
> correctness and hygiene bug fixed alongside Defect 1.
> h2. Proposed fix
> For Defect 1, make the sampling path lock-free so it never blocks on the
> volume-set lock while the metrics monitor is held. Add
> {{MutableVolumeSet.getStorageReportSnapshot()}}, which builds the report from
> the existing {{ConcurrentHashMap}}s without taking the volume-set lock (a
> weakly-consistent snapshot, the same guarantee already used by
> {{getVolumesList()}}), and have {{getMetrics()}} call it instead of
> {{getStorageReport()}}. The locking {{getStorageReport()}} is unchanged for
> the node-report path. A transient one-sample miscount during a rare
> volume-membership change is acceptable for a gauge and far preferable to a
> process-wide deadlock.
> For Defect 2, register and unregister under a per-instance unique source name
> following the existing {{VolumeInfoMetrics}} pattern ({{SOURCE_BASENAME + '-'
> + identifier}}), keeping register and unregister symmetric so no source or
> volume set is leaked.
> h2. Testing
> * {{TestVolumeSet.testStorageReportSnapshotDoesNotBlockOnWriteLock}}: hold
> the volume-set write lock, assert {{getStorageReportSnapshot()}} returns
> promptly from another thread while the locking {{getStorageReport()}} blocks
> (times out).
> * Leak test: in mini-cluster mode, repeated create plus unregister cycles
> leave no residual {{DatanodeStorageMetrics}} sources, and shut-down
> {{MutableVolumeSet}} instances become unreachable after GC (measured: 30 of
> 45 volume sets survived GC before the fix, 0 of 45 after).
> * {{TestDatanodeStorageMetrics}} aggregation and zero-capacity behavior
> unchanged. Container-service unit tests pass and checkstyle is clean.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
