[
https://issues.apache.org/jira/browse/HDDS-17359?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17359:
------------------------------
Attachment: CR-1-keep-snapshot-diff-db-open-while-diff-tasks-run.patch
> OmSnapshotManager.close frees the snapshot diff DB while a snapshot diff job
> that outlived the 60 second wait still uses it
> ---------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17359
> URL: https://issues.apache.org/jira/browse/HDDS-17359
> 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: Minor
> Attachments:
> CR-1-keep-snapshot-diff-db-open-while-diff-tasks-run.patch,
> TestBugCR1SnapDiffDbFreedUnderRunningJob.java
>
>
> h3. Mechanism
> {{OmSnapshotManager.close}} first calls {{SnapshotDiffManager.close}}, which
> runs {{shutdownNow()}} on the snapshot diff executor and
> {{awaitTermination(60, SECONDS)}}, ignores the result, and returns at once if
> the closing thread is interrupted. It then closes the snapshot cache, shuts
> down {{SnapshotDiffCleanupService}} ({{BackgroundService.shutdown}}, the same
> ignored 60 second wait), closes the column family handles and closes
> {{snapshotDiffDb}}.
> Snapshot diff jobs (through {{RocksDbPersistentMap}} and the column family
> helpers) and the cleanup service (directly) use {{snapshotDiffDb}} as a raw
> {{ManagedRocksDB}}, with no reference count, and jobs never check their
> interrupt flag. A job still running when the wait ends therefore keeps using
> the DB after it is freed.
> Both callers stop the OM metadata manager before this close, so a job that
> next reads the OM DB fails and ends inside the wait. The jobs that outlive it
> are those in a long step that reads only snapshot DBs and {{snapshotDiffDb}},
> such as {{addToObjectIdMap}} over a large delta. When the wait ends, closing
> the snapshot cache makes such a job fail on its next read, and its failure
> path ({{updateJobStatusToFailed}}, then dropping its column families) races
> the free a few milliseconds later.
> h3. Trigger
> # A snapshot diff job is running on an OM, in a step that takes longer than
> the 60 second wait plus the time the OM spends before
> {{OmSnapshotManager.close}}.
> # The OM closes the snapshot manager: {{OzoneManager.stop}}, or
> {{OzoneManager.installCheckpoint}} on a follower. The latter closes snapshot
> DBs at its start, so a job there has to be in a step that reads neither
> snapshot DBs nor the OM DB for the whole install (argued from the code, not
> run).
> # The job reaches {{snapshotDiffDb}} after the free.
> No fault injection is needed, but the last step is a race.
> h3. Impact
> * The OM JVM aborts with SIGSEGV in {{RocksDB.get}} on the snapshot diff job
> thread, after {{OzoneManager.stop}} has already waited 60 seconds.
> * On a follower installing a checkpoint, an OM that should keep running can
> die instead (narrow, see above).
> * No data loss is expected: the OM recovers on restart.
> h3. Reproduction
> Crash on unmodified source, deterministic.
> [^TestBugCR1SnapDiffDbFreedUnderRunningJob.java]
> ({{hadoop-ozone/ozone-manager}}) starts one OM, creates a volume, a bucket
> and two snapshots, and submits a snapshot diff through the client protocol.
> The only stub makes the job's first metrics call wait, ignoring interrupts,
> until {{OzoneManager.stop}} returns, in place of a long step. Without the fix
> the forked JVM dies with SIGSEGV in {{RocksDB.get}} called from
> {{RocksDbPersistentMap.get}} in
> {{SnapshotDiffManager.updateJobStatusToFailed}}, 60 seconds after "Shutting
> down executorService: 'SnapDiffExecutor'". With the fix the test passes: the
> OM logs a warning and keeps the DB open, the job fails cleanly, records its
> status and ends. The test shows the free under a running job; it does not
> measure how often a real job loses the race.
> h3. Patch
> [^CR-1-keep-snapshot-diff-db-open-while-diff-tasks-run.patch], against
> bf3829e6c8ac69ae17585585703f91426dcdf6b3. It also applies to master at
> fa3c98f189e.
> {{OmSnapshotManager.close}} checks, after the existing bounded waits, whether
> the snapshot diff executor and the cleanup service executor have terminated,
> through a new {{isTerminated}} on {{SnapshotDiffManager}} and on
> {{BackgroundService}}. If either has not, it logs one warning and does not
> close the column family handles, the DB or its options.
> {{OmSnapshotLocalDataManager}} is now closed before that check so it is still
> closed in every case; it does not use the snapshot diff DB. Shutdown stays
> bounded: the patch chooses leaking an open native DB over freeing it under a
> thread that will use it. On {{OzoneManager.stop}} the process exits anyway.
> On {{installCheckpoint}} the reopen of the snapshot diff DB then fails on the
> RocksDB lock and the OM exits through its normal error path instead of
> crashing (argued from the code). Nothing changes when no task is running.
> Closing the snapshot cache before {{SnapshotDiffManager.close}} would further
> make most such jobs end inside the wait, because their snapshot DB iterators
> stop once the DB is closed; not tested and not part of this patch.
> Covered by {{testCloseKeepsSnapshotDiffDbWhileDiffJobRuns}} in the existing
> {{TestOmSnapshotManager}}: closing a snapshot manager whose snapshot diff
> executor still runs a task must leave the snapshot diff DB open, and closing
> it again after the task ends must close it. Without the change it fails
> because the DB is closed. With it {{TestOmSnapshotManager}},
> {{TestSnapshotDiffManager}}, {{TestSnapshotCache}},
> {{TestSnapshotDiffCleanupService}} and the other snapshot manager unit tests
> (121) and {{TestOzoneManagerHASnapshot}} on a mini cluster (8) pass,
> checkstyle is clean.
> HDDS-14856 covers raw {{ManagedRocksDB}} users that bypass the
> {{RocksDatabase}} reference count, including {{RocksDbPersistentMap}} and
> {{SnapshotDiffCleanupService}}. Reference counting the snapshot diff DB there
> would turn this crash into an exception and supersede this patch; this patch
> is a narrow guard for the shutdown path only and does not overlap with it.
> Found by code review of the RocksDB handle close and shutdown paths, as part
> of the TLA+ verification effort under HDDS-15926, on commit
> bf3829e6c8ac69ae17585585703f91426dcdf6b3. 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
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
