[jira] [Updated] (HDDS-16026) OM install-snapshot does not roll back a partial DB backup, leaving an empty om.db

["Siyao Meng (Jira)" <ji...@apache.org>]

     [ 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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org