[
https://issues.apache.org/jira/browse/HDDS-16119?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-16119:
------------------------------
Attachment: (was: HDDS-16119.001.patch)
> 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
> Assignee: Siyao Meng
> Priority: Critical
> Labels: pull-request-available
>
> {{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]
