[
https://issues.apache.org/jira/browse/HDDS-16119?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-16119:
------------------------------
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.
was:
{{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.
> 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.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
