[
https://issues.apache.org/jira/browse/HDDS-16026?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-16026:
------------------------------
Fix Version/s: 2.3.0
Resolution: Fixed
Status: Resolved (was: Patch Available)
> OM install-snapshot does not roll back a partial DB backup, leaving an empty
> om.db
> ----------------------------------------------------------------------------------
>
> Key: HDDS-16026
> URL: https://issues.apache.org/jira/browse/HDDS-16026
> Project: Apache Ozone
> Issue Type: Bug
> Components: Ozone Manager
> Reporter: Siyao Meng
> Assignee: Siyao Meng
> Priority: Major
> Labels: pull-request-available
> Fix For: 2.3.0
>
>
> Verified and reproduced at master 8acc0413a9. Source links below are
> permalinks pinned to that commit, so the line numbers stay valid as master
> moves.
> *Problem*
> A follower OM installing a checkpoint first moves its existing
> metadata-directory contents aside into {{om.db.backup.<index>_<ts>}}, then
> moves the checkpoint into their place. If an I/O error interrupts that first
> step, the OM can be left with neither its original database nor the
> checkpoint: whatever was already moved aside stays in the backup directory,
> and nothing puts it back. When the item already moved is {{om.db}}, the OM
> proceeds to serve requests from a database that no longer holds its namespace.
> The [backup
> loop|https://github.com/apache/ozone/blob/8acc0413a9548462df8a699133d009a230dff2c5/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L4485-L4509]
> is a try-with-resources with no {{catch}}, so an {{IOException}} on any item
> after the [first
> move|https://github.com/apache/ozone/blob/8acc0413a9548462df8a699133d009a230dff2c5/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L4505]
> propagates with the DB directory partially emptied. Code to roll this back
> does exist -- but only in [moveCheckpointFiles's catch
> block|https://github.com/apache/ozone/blob/8acc0413a9548462df8a699133d009a230dff2c5/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L4566-L4602],
> which is [invoked after the loop that
> threw|https://github.com/apache/ozone/blob/8acc0413a9548462df8a699133d009a230dff2c5/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L4513],
> so it is never reached.
> *Impact*
> The OM does not fail on this path even though the codebase has a guard for
> exactly this condition: [OmMetadataManagerImpl.start() terminates the process
> when dbInconsistentMarker is
> present|https://github.com/apache/ozone/blob/8acc0413a9548462df8a699133d009a230dff2c5/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java#L440-L450],
> but [the marker is written inside
> moveCheckpointFiles|https://github.com/apache/ozone/blob/8acc0413a9548462df8a699133d009a230dff2c5/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L4535],
> after the backup loop -- so a backup-loop failure never sets it.
> {{reloadOMState()}} therefore succeeds with a fresh empty {{om.db}}, because
> [DBStoreBuilder checks only the parent
> directory|https://github.com/apache/ozone/blob/8acc0413a9548462df8a699133d009a230dff2c5/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/DBStoreBuilder.java#L224-L225]
> and [DBProfile sets
> createIfMissing|https://github.com/apache/ozone/blob/8acc0413a9548462df8a699133d009a230dff2c5/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/DBProfile.java#L66],
> and {{unpause}} runs with the pre-install term/index, since [those are only
> reassigned on
> success|https://github.com/apache/ozone/blob/8acc0413a9548462df8a699133d009a230dff2c5/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L4355-L4356].
> The state machine is left claiming an applied index the DB does not back, so
> Ratis feeds it subsequent entries and it builds a namespace missing
> everything before that index. Failed installs are logged, and a later process
> restart should get stuck re-requesting install-snapshot, since the empty DB
> has no TRANSACTION_INFO_KEY and the log cannot be replayed from zero -- so
> this is not silent. But nothing repairs the DB, and once the replica's log
> catches up nothing prevents it winning leadership and serving the truncated
> namespace as authoritative.
> Those last two statements are reasoned rather than traced: the restart
> behaviour from the empty DB's missing TRANSACTION_INFO_KEY, and the
> leadership one from Raft's up-to-date-log requirement. Neither was followed
> through the Ratis restart or election code, so they are the two claims here
> most worth an independent check.
> A freshly bootstrapping OM is unaffected: it has no populated DB to relocate.
> The exposure is a follower catching up past log retention, with at least two
> items in the checkpoint (typically {{om.db}} and {{db.snapshots}}).
> Likelihood is low as a random event: both paths are under the same directory,
> so the move is a metadata-only rename, and the failure has to land after at
> least one item has moved. One configuration would make it deterministic
> instead -- if a separate volume or bind mount sits under the OM metadata
> directory (for example {{db.snapshots}} on its own device), {{rename()}} on
> that mountpoint fails every time, so the Ratis retry never recovers.
> UNVERIFIED: that case needs a mount and is not covered by the test below.
> *Fix*
> Extract the rollback from {{moveCheckpointFiles}} into
> {{restoreFromBackup(dbDir, dbBackupDir, backedUpItems)}} and call it from a
> new {{catch (IOException)}} on the backup loop before rethrowing. The
> extracted body is unchanged apart from indentation; 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.
> The catch is scoped to {{IOException}} deliberately.
> {{Files.list(...).collect(...)}} can raise {{UncheckedIOException}}, but it
> runs before any move, so nothing has been relocated and there is nothing to
> restore; {{getRatisLogDirectory()}} is called per iteration but reads
> immutable config, so it would fail on the first iteration, also before any
> move.
> The patch also adds a nullable {{checkpointBackupInjector}} field and a
> {{@VisibleForTesting}} setter on {{OzoneManager}} so the test can fail a
> chosen backup move. This follows the existing [FaultInjector
> idiom|https://github.com/apache/ozone/blob/8acc0413a9548462df8a699133d009a230dff2c5/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/RDBSnapshotProvider.java#L225-L248]
> already used for the OM snapshot provider, and is a no-op in production. A
> filesystem-permission approach was tried first, but which item is processed
> first depends on {{Files.list}} ordering, which made the test flaky.
> *How found and reproduced*
> Found by a TLA+ model of the follower snapshot-install path (property: an
> aborted install must leave the follower at its pre-install state), then
> confirmed by code reading.
> Reproduced in
> {{TestOMRatisSnapshots#testInstallSnapshotFailedBackupRestoresDbDir}} on a
> 3-OM MiniOzoneHACluster: the test builds a checkpoint with two top-level
> entries, fails the second backup move, and asserts the metadata directory is
> unchanged afterwards. On unfixed master it fails with a different inode at
> {{metaDir/om.db}} -- direct evidence that the original database was relocated
> and a fresh empty one created in its place. Comparing names is not
> sufficient, because RocksDB re-creates {{om.db}}; the test compares inodes,
> and its name-set assertion covers the opposite ordering, where
> {{db.snapshots}} is the item lost.
> With the fix the test passes, and the whole
> [TestOMRatisSnapshots|https://github.com/apache/ozone/blob/8acc0413a9548462df8a699133d009a230dff2c5/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java]
> class passes (6 tests), including {{testInstallCorruptedCheckpointFailure}},
> which exercises the rollback and {{exitSystem}} path that was extracted.
> Checkstyle reports no violations.
> Generated-by: Claude Code (Opus 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]
