[
https://issues.apache.org/jira/browse/HDDS-17330?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17330:
------------------------------
Attachment: TestBugCR2MetricsMapFirstTaskOfType.java
> ReplicationSupervisorMetrics can throw ConcurrentModificationException when
> the first task of a new type is added during a metrics snapshot
> -------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17330
> URL: https://issues.apache.org/jira/browse/HDDS-17330
> Project: Apache Ozone
> Issue Type: Bug
> Affects Versions: 2.0.0, 2.1.0, 2.2.0, 2.1.1, 2.1.2, 2.2.1
> Reporter: Siyao Meng
> Priority: Minor
> Attachments:
> CR-2-concurrent-replication-supervisor-metrics-map.patch,
> TestBugCR2MetricsMapFirstTaskOfType.java
>
>
> h3. Mechanism
> {{ReplicationSupervisor}} keeps the metric names of the task types it has
> seen in {{METRICS_MAP}}, a static plain {{HashMap}}.
> {{ReplicationSupervisor.initCounters}} puts a new entry when the first task
> of a type is added, under {{synchronized (this)}}.
> {{ReplicationSupervisor.getMetricsMap}} returns an unmodifiable view of the
> same map, and {{ReplicationSupervisorMetrics.getMetrics}} iterates it with
> {{forEach}} without any lock. A {{put}} during that loop makes
> {{HashMap.forEach}} throw {{ConcurrentModificationException}} after the loop,
> so the gauges added after it ({{numInflight*}}) are missing from the record.
> h3. Trigger
> # A datanode has already run a task of one type (container replication, EC
> reconstruction or container reconciliation), so the map is not empty.
> # The first task of another type is added (for example the first reconcile
> command) while a metrics snapshot is running: the sampling timer when a sink
> such as the Prometheus endpoint is registered, a JMX read, or
> {{publishMetricsNow}}.
> There are three task types, so this can happen at most twice per datanode
> process, and only if the {{put}} falls inside the few microseconds the
> snapshot spends in the loop. No fault is needed, but it is very unlikely on a
> real cluster.
> h3. Impact
> Minor. Hadoop's {{MetricsSourceAdapter.getMetrics}} catches the exception and
> logs {{Error getting metrics from source ReplicationSupervisorMetrics}} with
> the stack trace at ERROR. The partial record is still published: the per type
> gauges are in it, the {{numInflight*}} gauges are missing for that one
> sample. The next snapshot is complete. No task, counter or replication is
> affected. (The adapter behaviour is from reading hadoop-common 3.4.3; my
> reproduction calls {{getMetrics}} directly.) In {{MiniOzoneCluster}}, where
> several datanodes share the static map under different locks, the first two
> inserts can also race and lose one entry, so that type's gauges are not
> published in that JVM; this is argued from the code, not run.
> h3. Reproduction
> PASS on unmodified source, deterministic.
> [^TestBugCR2MetricsMapFirstTaskOfType.java]
> ({{hadoop-hdds/container-service}}) uses only real task types: it adds a
> {{ReplicationTask}}, then calls {{ReplicationSupervisorMetrics.getMetrics}}
> with a mock record builder that adds the first {{ReconcileContainerTask}}
> while the replication gauges are being added, that is inside the {{forEach}}.
> {{getMetrics}} throws {{ConcurrentModificationException}} and no
> {{numInflight*}} gauge is published. The overlap is forced on the snapshot
> thread instead of a command handler thread; the map sees the same {{put}}
> inside the same loop. The test needs its own JVM because the map is static. A
> passing test means the defect is present. A concurrent run with real timing
> was not attempted, since there are at most two windows per process.
> h3. Patch
> [^CR-2-concurrent-replication-supervisor-metrics-map.patch], against
> 07512078fe639f387acd52104491d5a7f3edb84c. It also applies to master at
> 2851f5a3057.
> {{METRICS_MAP}} becomes a {{ConcurrentHashMap}}. Its {{forEach}} is weakly
> consistent and never throws, and concurrent inserts from several supervisors
> are safe. The map stays static and nothing else changes, so the published
> metric names are the same as before. No consumer depends on gauge order.
> Covered by {{testMetricsSnapshotWhileFirstTaskOfNewTypeIsAdded}} in the
> existing {{TestReplicationSupervisor}}, which adds a task of a new type from
> inside a snapshot and checks that the snapshot completes with its
> {{numInflight}} gauge and that the next one publishes the new type. Without
> the change it fails with {{ConcurrentModificationException}} at
> {{ReplicationSupervisorMetrics.getMetrics}}. With it
> {{TestReplicationSupervisor}} and the replicate, reconstruct and reconcile
> command handler tests (62 tests) pass and checkstyle is clean. With the patch
> the reproduction fails, because the snapshot no longer throws.
> Found by code review of the datanode ReplicationSupervisor thread pool resize
> and metrics paths, as part of the TLA+ verification effort under HDDS-15926,
> on commit 07512078fe639f387acd52104491d5a7f3edb84c. Checked against HDDS
> issues and apache/ozone pull requests for duplicates before filing (HDDS-6314
> fixed the same pattern in {{SCMContainerMetrics}}). The attached patch is a
> proposal for review. Generated with Specula (Claude Opus 5.5).
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
