Siyao Meng created HDDS-17015:
---------------------------------
Summary: ConcurrentModificationException in
DbVolume.closeAllDbStore leaves a failed DB volume active and stops the
periodic disk check
Key: HDDS-17015
URL: https://issues.apache.org/jira/browse/HDDS-17015
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments: CR-11-use-concurrent-map-for-db-volume-store-paths.patch,
TestBugCR11DbVolumeMapRace.java
h3. Mechanism
{{DbVolume.hddsDbStorePathMap}} is a {{HashMap}}. {{HddsVolume.createDbStore}}
adds to it through {{addHddsDbStorePath}} while SchemaV3 finalization holds the
data volume set lock. {{DbVolume.failVolume}} iterates it in
{{closeAllDbStore}} while the DB volume set lock is held. No lock is shared, so
an add during the loop makes the iterator throw
{{ConcurrentModificationException}}.
The exception leaves {{MutableVolumeSet.failVolume}} before the volume is moved
to the failed set, then {{handleVolumeFailures}}, {{checkAllVolumes}} and
{{StorageVolumeChecker.checkAllVolumeSets}}, which only catches
{{IOException}}. That method is the body of the {{scheduleWithFixedDelay}} task
of the periodic disk check, and a scheduled task that throws is not run again.
h3. Trigger
A datanode with dedicated DB volumes ({{hdds.datanode.container.db.dir}})
finalizes SchemaV3 while the periodic disk check fails one of its DB volumes,
and a {{createDbStore}} adds a path to that DB volume during the close loop.
Before finalization completes each new DB is closed right after creation, so
the loop only does cache lookups and the window is very short.
h3. Impact
* The DB volume is FAILED but stays in the active DB volume set. The rest of
the finalize loop can still place new DBs on it, and the DB load that ends the
finalization ({{HddsVolumeUtil.loadAllHddsVolumeDbStore}}) maps its data
volumes back to it and reopens their DBs there (by code reading).
* The periodic disk check stops for all volume sets until the datanode
restarts, so later disk failures are not detected by it. The exception is never
logged, because nothing reads the scheduled future; the operator only sees the
earlier log lines that mark the DB volume as failed. On demand checks after I/O
errors still run.
* Rare: needs a DB volume failure during the one-time finalization, and the
window was not hit naturally.
h3. Reproduction
PASS on unmodified source. [^TestBugCR11DbVolumeMapRace.java]
({{hadoop-hdds/container-service}}, target path and Maven command in the class
comment) registers a real DB volume set with a real {{StorageVolumeChecker}}
and runs the real {{checkAllVolumeSets}} as a {{scheduleWithFixedDelay}} task.
The disk check verdict is stubbed to report the DB volume as failed, and
{{DatanodeStoreCache}} is mocked so that the first {{removeDB}} of the close
loop registers one more DB path, as a concurrent {{createDbStore}} would.
Output: {{periodic check runs=1 dbVolumeState=FAILED stillInActiveSet=true
failedSet=0}}, and the scheduled future completes with the
{{ConcurrentModificationException}} from {{DbVolume.closeAllDbStore}} and is
not run again. A passing test means the defect is present.
h3. Patch
[^CR-11-use-concurrent-map-for-db-volume-store-paths.patch], against
cce1ca28944886218132f01c0e271c1fd0700078. It also applies to master at
2536fe9cbc3.
The map becomes a {{ConcurrentHashMap}}. Its iterators do not throw on
concurrent changes, so {{failVolume}} completes and the periodic task keeps
running. Keys and values are never null, and the other uses ({{put}},
{{clear}}, {{keySet}}, {{values}}) behave the same.
Covered by {{testFailVolumeWhileDbStorePathIsAdded}} in the existing
{{TestDbVolume}}, which adds a path from the first {{removeDB}} call of
{{failVolume}} and asserts that the volume is failed and every earlier path was
closed. Without the change it fails with {{ConcurrentModificationException}}.
With it the DB volume, volume, volume set, checker and upgrade suites (83
tests) pass and checkstyle is clean. With the patch the reproduction fails
because the periodic task keeps running.
{{checkAllVolumeSets}} still lets any other {{RuntimeException}} end the
periodic check; catching it there could be a follow up.
Found by code review of the datanode volume and per-disk RocksDB lifecycle
paths, as part of the TLA+ verification effort under HDDS-15926, on commit
cce1ca28944886218132f01c0e271c1fd0700078. Checked against HDDS issues and
apache/ozone pull requests for duplicates before filing. The attached patch is
a proposal for review. Generated with Specula (Claude Opus 5.5).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
