HDDS-16026. Restore the OM DB directory when the pre-install backup fails part way - #10904
Draft
smengcl wants to merge 2 commits into
Draft
HDDS-16026. Restore the OM DB directory when the pre-install backup fails part way#10904smengcl wants to merge 2 commits into
smengcl wants to merge 2 commits into
Conversation
…ails part way replaceOMDBWithCheckpoint's backup loop had no catch, so an IOException part way through left dbDir missing every item already moved into om.db.backup.*. The caller logs and proceeds to reloadOMState(), which succeeds rather than failing because DBStoreBuilder only checks the parent directory and DBProfile sets createIfMissing -- so RocksDB re-creates om.db empty and the OM comes back on a namespace that no longer matches the index its state machine claims. Extract the rollback that already existed in moveCheckpointFiles into restoreFromBackup and call it from a new catch on the backup loop before rethrowing. The extracted body is unchanged; the transient marker path is recomputed inside the helper rather than passed in, which is a no-op on the backup-loop path where the marker was never created, as is the delete-replaced-items loop. The helper declares throws IOException because exitManager.exitSystem does; both call sites already declare it. No behaviour change on any currently-succeeding path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
testInstallSnapshotFailedBackupRestoresDbDir drives the real install path on a 3-OM MiniOzoneHACluster, fails the second backup move, and asserts that the metadata directory is unchanged afterwards. Reverting the preceding commit makes it fail with a different inode at metaDir/om.db, showing reloadOMState created a fresh empty database while the original sat in the backup directory. The failure is injected through the existing FaultInjector idiom (see RDBSnapshotProvider), which needs a hook in the backup loop: a nullable checkpointBackupInjector field plus a @VisibleForTesting setter. It is a no-op in production. A filesystem-permission trick was considered instead, but the item processed first depends on Files.list ordering, which would make the test flaky; the injector fails a chosen attempt regardless of order. Both orderings are covered: the name-set assertion catches a lost db.snapshots, and the inode assertion catches a replaced om.db (the name alone is recreated by RocksDB, so the set check is not sufficient on its own). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes snapshot install rollback when the pre-install OM DB backup fails mid-loop, ensuring the metadata directory is restored instead of being left partially moved (and potentially re-created empty by RocksDB).
Changes:
- Add an
IOExceptioncatch around the pre-install backup loop to restore moved items before rethrowing. - Extract rollback logic into
restoreFromBackup(...)and reuse it from both backup and checkpoint-move failure paths. - Add a test-only
FaultInjectorhook and a new integration test to deterministically fail the Nth backup move and assert the metadata dir is unchanged.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java | Adds rollback on backup-loop failure, extracts restore helper, and introduces a test-only fault injector hook. |
| hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java | Adds an integration test that injects a mid-backup failure and verifies the DB dir is restored (including inode stability). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+5044
to
+5047
| @VisibleForTesting | ||
| public void setCheckpointBackupInjector(FaultInjector injector) { | ||
| checkpointBackupInjector = injector; | ||
| } |
Comment on lines
+494
to
+495
| /** Test-only hook to fail a checkpoint-install DB backup part way through. */ | ||
| private FaultInjector checkpointBackupInjector; |
Comment on lines
+615
to
+633
| followerOM.setCheckpointBackupInjector(new ThrowOnNthPauseFaultInjector(2, | ||
| "Simulated backup move failure for test")); | ||
| followerOM.setExitManagerForTesting(new DummyExitManager()); | ||
| try { | ||
| TermIndex termIndex = followerOM.installCheckpoint( | ||
| leaderOMNodeId, leaderCheckpointLocation, leaderCheckpointTrxnInfo); | ||
| assertNull(termIndex, "Install should have been reported as failed"); | ||
|
|
||
| // Everything present before the aborted install must still be present. | ||
| assertThat(topLevelNames(followerMetaDir)).containsAll(namesBefore); | ||
| assertTrue(Files.exists(sentinel), | ||
| "Sentinel under " + OM_SNAPSHOT_DIR + " was relocated and never restored"); | ||
| assertEquals("keep-me", new String(Files.readAllBytes(sentinel), UTF_8)); | ||
| // The original om.db must be the one still in place, not a fresh empty DB. | ||
| assertEquals(dbInodeBefore, getINode(followerDbDir), | ||
| OM_DB_NAME + " was replaced rather than restored"); | ||
| } finally { | ||
| followerOM.setCheckpointBackupInjector(null); | ||
| } |
Comment on lines
+592
to
+595
| Path omDbDir = leaderCheckpointLocation.resolve(OM_DB_NAME); | ||
| assertTrue(omDbDir.toFile().mkdir()); | ||
| moveCheckpointContentsToOmDbDir(leaderCheckpointLocation, omDbDir); | ||
| Files.createDirectory(leaderCheckpointLocation.resolve(OM_SNAPSHOT_DIR)); |
sadanand48
approved these changes
Jul 31, 2026
sadanand48
left a comment
sadanand48
left a comment
Contributor
There was a problem hiding this comment.
Thanks @smengcl for the patch. LGTM
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
OzoneManager.replaceOMDBWithCheckpointmoves the existing metadata-directory contents intoom.db.backup.<index>_<ts>before moving the checkpoint into place. The backup loop has nocatch, so anIOExceptionafter the first move leaves the DB directory partially emptied. The rollback for this exists only inmoveCheckpointFiles's catch block, which runs after the loop that threw.Nothing then fails: the
dbInconsistentMarkerguard is written insidemoveCheckpointFiles, so a backup-loop failure never sets it, andreloadOMState()succeeds by re-creatingom.dbempty. Full impact analysis is on the Jira.This PR extracts that rollback into
restoreFromBackup(dbDir, dbBackupDir, backedUpItems)and calls it from a newcatch (IOException)on the backup loop before rethrowing. The extracted body is unchanged apart from indentation, and no currently-succeeding path changes behaviour.The second commit adds a nullable
checkpointBackupInjectorand a@VisibleForTestingsetter onOzoneManagerso the test can fail a chosen backup move, following the existingFaultInjectoridiom. It is a no-op in production. A filesystem-permission injection was tried first, but it depended onFiles.listordering and made the test flaky.Found by a TLA+ model of the follower snapshot-install path, confirmed by code reading, and reproduced by the test below.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16026
How was this patch tested?
New
TestOMRatisSnapshots#testInstallSnapshotFailedBackupRestoresDbDirdrives the real install path on a 3-OMMiniOzoneHACluster, fails the second backup move, and asserts the metadata directory is unchanged. Reverting the first commit makes it fail with a different inode atmetaDir/om.db:That is the original DB relocated and a fresh empty one created in its place. Names alone are not enough, since RocksDB re-creates
om.db, so the test compares inodes; its name-set assertion covers the oppositeFiles.listordering, wheredb.snapshotsis lost instead.mvn -pl :ozone-integration-test test -Dtest=TestOMRatisSnapshots— 6 tests, 0 failures (176.7s), includingtestInstallCorruptedCheckpointFailure, which exercises the extracted rollback, and both happy-path installs.checkstyle.sh— 0 violations.Not tested: whether a separate volume or bind mount under the OM metadata directory makes the failure deterministic rather than sporadic. That needs a mount; it is recorded as unverified on the Jira.
Generated-by: Claude Code (Opus 5)