Siyao Meng created HDDS-17259:
---------------------------------
Summary: Compaction DAG is populated without the lock its readers
take, so snapshot diff can miss keys and backup SST pruning can silently stop
Key: HDDS-17259
URL: https://issues.apache.org/jira/browse/HDDS-17259
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments:
MC-1-synchronize-compaction-dag-population-with-dag-walks.patch,
TestBugMC1DagDiffDuringCompactionListener.java
h3. Mechanism
{{RocksDBCheckpointDiffer.newCompactionCompletedListener}} calls
{{CompactionDag.populateCompactionDAG}} inside {{synchronized (this)}}, but the
block is in an anonymous {{AbstractEventListener}}, so it locks the listener
object. Every DAG walk ({{getSSTDiffList}}, {{internalGetSSTDiffList}}) and the
DAG pruning tasks ({{pruneSstFiles}}, {{pruneSstFileNodesFromDag}}) lock the
differ instead. Nothing keeps them from running while the listener is adding a
compaction to the DAG, a Guava {{MutableGraph}} that is not thread safe. The
comment in {{pruneSstFiles}} shows the reader lock was meant to exclude the
compaction thread.
Two consequences follow.
*SST file pruning.* {{pruneSstFiles}} streams over the forward DAG's nodes
while the listener can add nodes. The scan can then throw
{{ConcurrentModificationException}}. The task is scheduled with
{{Scheduler.scheduleWithFixedDelay}} and has no exception handling, so the
first such exception cancels it for good, and nothing is logged.
*Snapshot diff.* {{populateCompactionDAG}} first adds the output node O, then
the edges from O to each input, one at a time. If a walk runs after the edge to
an input A that is in the older snapshot but before the edge to a newer input
B, A counts as unchanged, B is never visited, the reachability check passes,
and the result is a present but empty delta. {{CompositeDeltaDiffComputer}}
falls back to a full diff only when the DAG walk returns no result (an empty
{{Optional}}) or throws, so the keys that exist only in B are missing from the
diff. If B's edge comes first, A is unreachable and the diff falls back to a
full diff (safe). The edge order follows the {{HashMap}} order of the SST paths.
h3. Trigger
SST file pruning: a tracked compaction (an OM with at least one snapshot)
completes while the {{pruneSstFiles}} scan runs. The scan runs every
{{ozone.om.snapshot.compaction.dag.prune.daemon.run.interval}} (default 10m)
and takes time proportional to the DAG size. No unusual scheduling is needed,
only that overlap.
Snapshot diff:
# The older snapshot contains SST A.
# A key k2 is written and flushed into SST B.
# RocksDB compacts A and B into O; the completed listener starts adding O to
the DAG.
# A newer snapshot is created while the listener is between the edge to A and
the edge to B.
# A snapshot diff between the two reaches the DAG walk before the listener
finishes.
The unsafe window for the diff is only the in-memory edge loop of
{{populateCompactionDAG}}. The part of the listener before it is safe (a node
not yet in the DAG is read whole). So in practice the diff omission needs the
compaction thread to be descheduled inside that loop for at least as long as
creating a snapshot and starting a diff job. It was not observed without an
injected pause.
h3. Impact
* Once the {{pruneSstFiles}} task is cancelled, backup SST files of compacted
outputs are no longer removed from the compaction SST backup directory until
the OM restarts or reopens its DB. Only the DAG pruning of entries older than
{{ozone.om.snapshot.compaction.dag.max.time.allowed}} (default 30d) still
removes them, so the directory holds up to that much compaction history on
disk. Shown with a unit level probe against the real differ and its scheduler
(prune interval 20 ms, 20000 compactions added to the DAG under a separate
monitor in about 3 s): the task stopped on unmodified source and kept running
with the patch. Not run on a cluster.
* When the diff window is hit, the snapshot diff report silently omits the keys
that were only in the newer input (CREATE k2 above). The finished job serves
the stored report; nothing revalidates it. Exercised at the differ level only,
not with an end to end report on a cluster.
* Argued from code, not run:
{{SnapshotDefragService.performIncrementalDefragmentation}} uses the same
{{CompositeDeltaDiffComputer}}, so a defragmentation that hit the same window
would build the snapshot without those keys.
h3. Reproduction
PASS on unmodified source, deterministic up to the edge order.
[^TestBugMC1DagDiffDuringCompactionListener.java]
({{hadoop-hdds/rocksdb-checkpoint-differ}}) shows the diff omission. It uses a
real RocksDB with the differ's real listeners, a real {{compactRange}} and real
checkpoints, and issues the diff as {{RDBDifferComputer}} does. The only
intervention is timing: a delegating proxy on the forward DAG holds the
listener after its first edge until the diff has finished or is waiting. Every
graph call is passed through unchanged. It repeats the scenario with shifted
SST numbers until the omission order occurs. Output of the run on the
unmodified source, round 1: {{diff during listener: present, files=[]
keys=[]}}, then {{diff after listener: present, files=[000013] keys=[k2]}}. A
passing test means the defect is present. The pruning consequence is covered by
the regression test in the patch and the probe described above, not by this
attachment.
h3. Suggested fix
Locking the differ in the listeners ({{synchronized
(RocksDBCheckpointDiffer.this)}}) would also close both windows, but the differ
monitor is held across blocking work: {{pruneSstFileValues}} rewrites backup
SST files and acquires the bootstrap lock inside it, and RocksDB writes to the
compaction log table happen under it. RocksDB compaction threads would then
wait for that work. The attached patch uses a lock that only guards in-memory
DAG operations instead.
h3. Patch
[^MC-1-synchronize-compaction-dag-population-with-dag-walks.patch], against
b10a7d427bdf83807a7e48a1be507d3b02d5a503. It also applies to master at
d3b5f518556.
{{CompactionDag.populateCompactionDAG}} and {{CompactionDag.pruneNodesFromDag}}
become synchronized, {{RocksDBCheckpointDiffer.getSSTDiffList}} walks the DAG
inside {{synchronized (compactionDag)}}, and the node scan in {{pruneSstFiles}}
takes the same lock instead of the differ, which removes the
{{ConcurrentModificationException}} that cancels the pruning task. A walk or
scan then sees a compaction either not yet added or with all its edges. Holders
of this lock only touch in-memory DAG structures, so the compaction thread
waits at most for a walk or a prune in progress. No config, protocol or on disk
change.
Covered by two tests in the existing {{TestRocksDBCheckpointDiffer}} that hold
{{populateCompactionDAG}} after the first edge.
{{testGetSSTDiffListDuringCompactionDagPopulation}} runs {{getSSTDiffList}}
meanwhile, for both edge orders: without the change the omission order returns
an empty delta and the other order an empty {{Optional}}; with it both return
the newer input. {{testPruneSstFilesDuringCompactionDagPopulation}} runs
{{pruneSstFiles}} meanwhile: without the change (or with only the
{{pruneSstFiles}} line reverted) the scan runs during the population; with it
the scan waits and then prunes the backup file of the new output. With the
change the tests of {{rocksdb-checkpoint-differ}} (123 run, 2 native library
tests skipped), the OM differ users {{TestRDBDifferComputer}},
{{TestCompositeDeltaDiffComputer}}, {{TestSnapshotDiffManager}} and
{{TestOMSnapshotDirectoryMetrics}} (89), and the mini cluster suites
{{TestOMSnapshotDAG}} and {{TestSnapshotBackgroundServices}} (6) pass, and
checkstyle is clean. With the patch the reproduction fails, because the diff
waits for the listener in every round and always contains k2.
Found by TLA+ model checking and code review of the OM snapshot diff paths
under HDDS-15926, on commit b10a7d427bdf83807a7e48a1be507d3b02d5a503. 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]
