Copilot commented on code in PR #10904:
URL: https://github.com/apache/ozone/pull/10904#discussion_r3681263282
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -490,6 +491,8 @@ public final class OzoneManager extends
ServiceRuntimeInfoImpl
private final boolean isS3MultiTenancyEnabled;
private final boolean isStrictS3;
private ExitManager exitManager;
+ /** Test-only hook to fail a checkpoint-install DB backup part way through.
*/
+ private FaultInjector checkpointBackupInjector;
Review Comment:
The injector is written via a setter and read during checkpoint install; if
install/backup occurs on a different thread than the test that sets the
injector, updates may not be safely published. Marking
`checkpointBackupInjector` as `volatile` (or using an `AtomicReference`) would
provide the necessary visibility guarantees.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -5010,6 +5041,11 @@ private OmBucketInfo resolveBucketLink(
allowDanglingBuckets, aclEnabled);
}
+ @VisibleForTesting
+ public void setCheckpointBackupInjector(FaultInjector injector) {
+ checkpointBackupInjector = injector;
+ }
Review Comment:
This setter is `public` but appears to be intended strictly for tests. Since
the integration test is in the same package (`org.apache.hadoop.ozone.om`),
consider making the method package-private (remove `public`) to avoid exposing
a test hook as part of the public surface area.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -557,6 +560,88 @@ public void testInstallCorruptedCheckpointFailure() throws
Exception {
assertLogCapture(logCapture, msg);
}
+ /**
+ * When the pre-install backup loop in replaceOMDBWithCheckpoint fails part
way
+ * through, every item it already relocated into om.db.backup.* must be put
back.
+ * Today the loop has no catch, so the items stay in the backup directory:
the
+ * one that was moved first is lost, and if that item is om.db then
reloadOMState
+ * silently re-creates an empty one.
+ */
+ @Test
+ public void testInstallSnapshotFailedBackupRestoresDbDir() throws Exception {
+ final String leaderOMNodeId =
OmTestUtil.getCurrentOmProxyNodeId(objectStore);
+ OzoneManager leaderOM = cluster.getOzoneManager(leaderOMNodeId);
+ OzoneManagerRatisServer leaderRatisServer = leaderOM.getOmRatisServer();
+
+ // Find the inactive OM, so the checkpoint index is ahead of its applied
index
+ // and canProceed lets the replacement start.
+ String followerNodeId = leaderOM.getPeerNodes().get(0).getNodeId();
+ if (cluster.isOMActive(followerNodeId)) {
+ followerNodeId = leaderOM.getPeerNodes().get(1).getNodeId();
+ }
+ OzoneManager followerOM = cluster.getOzoneManager(followerNodeId);
+
+ writeKeysToIncreaseLogIndex(leaderRatisServer, 100);
+
+ // Build a checkpoint whose top level holds two entries, so the backup loop
+ // performs two moves and can fail on the second.
+ DBCheckpoint leaderDbCheckpoint =
+ leaderOM.getMetadataManager().getStore().getCheckpoint(false);
+ Path leaderCheckpointLocation = leaderDbCheckpoint.getCheckpointLocation();
+ assertNotNull(leaderCheckpointLocation);
+ Path omDbDir = leaderCheckpointLocation.resolve(OM_DB_NAME);
+ assertTrue(omDbDir.toFile().mkdir());
+ moveCheckpointContentsToOmDbDir(leaderCheckpointLocation, omDbDir);
+ Files.createDirectory(leaderCheckpointLocation.resolve(OM_SNAPSHOT_DIR));
Review Comment:
`mkdir()` + `assertTrue(...)` provides limited failure diagnostics and
`Files.createDirectory(...)` will throw if the directory already exists. To
make the test more robust/future-proof (e.g., if checkpoints start containing
`OM_SNAPSHOT_DIR` by default), prefer `Files.createDirectories(...)` for the
snapshot dir and consider using `Files.createDirectory(...)` (or
`createDirectories`) for `omDbDir` to get clearer exceptions on failure.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -557,6 +560,88 @@ public void testInstallCorruptedCheckpointFailure() throws
Exception {
assertLogCapture(logCapture, msg);
}
+ /**
+ * When the pre-install backup loop in replaceOMDBWithCheckpoint fails part
way
+ * through, every item it already relocated into om.db.backup.* must be put
back.
+ * Today the loop has no catch, so the items stay in the backup directory:
the
+ * one that was moved first is lost, and if that item is om.db then
reloadOMState
+ * silently re-creates an empty one.
+ */
+ @Test
+ public void testInstallSnapshotFailedBackupRestoresDbDir() throws Exception {
+ final String leaderOMNodeId =
OmTestUtil.getCurrentOmProxyNodeId(objectStore);
+ OzoneManager leaderOM = cluster.getOzoneManager(leaderOMNodeId);
+ OzoneManagerRatisServer leaderRatisServer = leaderOM.getOmRatisServer();
+
+ // Find the inactive OM, so the checkpoint index is ahead of its applied
index
+ // and canProceed lets the replacement start.
+ String followerNodeId = leaderOM.getPeerNodes().get(0).getNodeId();
+ if (cluster.isOMActive(followerNodeId)) {
+ followerNodeId = leaderOM.getPeerNodes().get(1).getNodeId();
+ }
+ OzoneManager followerOM = cluster.getOzoneManager(followerNodeId);
+
+ writeKeysToIncreaseLogIndex(leaderRatisServer, 100);
+
+ // Build a checkpoint whose top level holds two entries, so the backup loop
+ // performs two moves and can fail on the second.
+ DBCheckpoint leaderDbCheckpoint =
+ leaderOM.getMetadataManager().getStore().getCheckpoint(false);
+ Path leaderCheckpointLocation = leaderDbCheckpoint.getCheckpointLocation();
+ assertNotNull(leaderCheckpointLocation);
+ Path omDbDir = leaderCheckpointLocation.resolve(OM_DB_NAME);
+ assertTrue(omDbDir.toFile().mkdir());
+ moveCheckpointContentsToOmDbDir(leaderCheckpointLocation, omDbDir);
+ Files.createDirectory(leaderCheckpointLocation.resolve(OM_SNAPSHOT_DIR));
+
+ TransactionInfo leaderCheckpointTrxnInfo =
+ OzoneManagerRatisUtils.getTrxnInfoFromCheckpoint(conf, omDbDir);
+
+ // Give the follower a matching second entry with a sentinel inside it, so
the
+ // loop finds two items in the follower's metadata dir to relocate.
+ File followerMetaDir = OMStorage.getOmDbDir(followerOM.getConfiguration());
+ Path followerDbDir = Paths.get(followerMetaDir.toString(), OM_DB_NAME);
+ Path followerSnapshotDir = Paths.get(followerMetaDir.toString(),
OM_SNAPSHOT_DIR);
+ Files.createDirectories(followerSnapshotDir);
+ Path sentinel = followerSnapshotDir.resolve("sentinel");
+ Files.write(sentinel, "keep-me".getBytes(UTF_8));
+
+ Set<String> namesBefore = topLevelNames(followerMetaDir);
+ assertThat(namesBefore).contains(OM_DB_NAME, OM_SNAPSHOT_DIR);
+ Object dbInodeBefore = getINode(followerDbDir);
+
+ // Fail the second of the two backup moves. The first has already
succeeded,
+ // so the DB directory is now missing whichever item was relocated first.
+ 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);
+ }
Review Comment:
The test mutates the OM's `exitManager` but does not restore it in the
`finally` block. If the same `OzoneManager` instance is reused by later tests
in the suite, leaving `DummyExitManager` installed can mask real fail-fast
behavior. Consider saving the original exit manager (or resetting to the
default) in `finally`, alongside clearing the injector.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
