Siyao Meng created HDDS-17330:
---------------------------------
Summary: 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
Reporter: Siyao Meng
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]
