[
https://issues.apache.org/jira/browse/HDDS-17259?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17259:
------------------------------
Attachment: MC-1-synchronize-compaction-dag-population-with-dag-walks.patch
> 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
> Affects Versions: 1.4.0, 2.0.0, 1.4.1, 2.1.0, 2.2.0, 2.1.1, 2.1.2, 2.2.1
> Reporter: Siyao Meng
> Priority: Major
> 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]
