[
https://issues.apache.org/jira/browse/HDDS-16026?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-16026:
------------------------------
Description:
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)
was:
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|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#L4534],
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 gets
stuck re-requesting install-snapshot (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 it can win leadership and
serve the truncated namespace as authoritative.
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 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)
> 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
>
> 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]
