HDDS-11470. Report OM snapshot installation failures to Ratis

[andyhuangdev]

https://issues.apache.org/jira/browse/HDDS-11470

When OM checkpoint installation fails, the snapshot-install callback previously
completed normally with a null result. Ratis could therefore treat the request
as completed without a successful snapshot installation.

Complete the callback exceptionally when OM reports a failed installation.
Add unit coverage for both successful and failed callback completion.

Test:

• mvn -pl :ozone-manager -am test -Dtest=TestOzoneManagerStateMachine -DskipShade -DskipRecon -DskipDocs -Dsurefire.failIfNoSpecifiedTests=false
• ./hadoop-ozone/dev-support/checks/checkstyle.sh

[chungen0126]

Thanks @andyhuangdev for working on this.

[chungen0126]

I don't think this is the correct approach. A null return value for termIndex shouldn't always result in an exception.

[andyhuangdev]

Thanks for pointing this out. I agree that treating every null TermIndex as an exception is too broad, since null is also used for cases where snapshot installation is unavailable or should not proceed.

I propose moving the failure handling to OzoneManager.installSnapshotFromLeader(). A genuine installCheckpoint() failure would be propagated as an IOException, while the existing non-exceptional null cases would retain their current semantics. OzoneManagerStateMachine already converts an IOException into an exceptional future for Ratis.

I will also update the tests to verify that:

• a null result still completes normally;
• an IOException completes the future exceptionally; and
• a checkpoint installation failure is propagated as an IOException.

Does this approach align with what you had in mind?

[szetszwo]

@andyhuangdev , I agree your approach and have tried it a little bit as below (TODO: we should change all LOG.error to throw an excepiton)

diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
index 4254a634b3..1806ec3d42 100644
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
@@ -4260,7 +4260,7 @@ public synchronized TermIndex installSnapshotFromLeader(String leaderId) throws
       }
       termIndex = installCheckpoint(leaderId, checkpointLocation);
     } catch (Exception ex) {
-      LOG.error("Failed to install snapshot from Leader OM.", ex);
+      throw new IOException("Failed to install snapshot from Leader " + leaderId, ex);
     } finally {
       cleanupCheckpoint(omDBCheckpoint);
     }
@@ -4317,6 +4317,7 @@ TermIndex installCheckpoint(String leaderId, Path checkpointLocation,
     long startTime = Time.monotonicNow();
     File oldDBLocation = metadataManager.getStore().getDbLocation();
     Path omDbPath = Paths.get(checkpointLocation.toString(), OM_DB_NAME);
+    Exception exception = null;
     try {
       // Stop Background services
       keyManager.stop();
@@ -4328,13 +4329,11 @@ TermIndex installCheckpoint(String leaderId, Path checkpointLocation,
       // pending transactions in the buffer, they are discarded.
       omRatisServer.getOmStateMachine().pause();
     } catch (Exception e) {
-      LOG.error("Failed to stop/ pause the services. Cannot proceed with " +
-          "installing the new checkpoint.");
       // Stop the checkpoint install process and restart the services.
       keyManager.start(configuration);
       startSecretManagerIfNecessary();
       startTrashEmptier(configuration);
-      throw e;
+      throw new IOException("Failed to installCheckpoint " + checkpointTrxnInfo + ": Cannot stop/pause services.");
     }
 
     File dbBackup = null;
@@ -4380,9 +4379,8 @@ TermIndex installCheckpoint(String leaderId, Path checkpointLocation,
             "index: {}, time: {} ms", leaderId, term, lastAppliedIndex,
             Time.monotonicNow() - time);
       } catch (Exception e) {
-        LOG.error("Failed to install Snapshot from {} as OM failed to replace" +
-            " DB with downloaded checkpoint. Reloading old OM state.",
-            leaderId, e);
+        exception = new IOException("Failed to installCheckpoint " + checkpointTrxnInfo
+            + ": Cannot replace DB.");
       }
     } else {
       LOG.warn("Cannot proceed with InstallSnapshot as OM is at TermIndex {} " +
@@ -4449,6 +4447,9 @@ TermIndex installCheckpoint(String leaderId, Path checkpointLocation,
           dbBackup, e);
     }
 
+    if (exception != null) {
+      throw exception;
+    }
     if (lastAppliedIndex != checkpointTrxnInfo.getTransactionIndex()) {
       // Install Snapshot failed and old state was reloaded. Return null to
       // Ratis to indicate that installation failed.

[andyhuangdev]

@szetszwo
Thanks for the guidance. Updated in e6e998a.

The state machine no longer treats every null TermIndex as an exception. A null result retains its existing non-exceptional semantics.

Actual checkpoint installation failures are now propagated as IOException. For a DB replacement failure, OM first restores the previous state and restarts the required services, then throws the saved exception with the original cause.

I also updated the tests to cover:

• null results completing normally;
• IOException completing the Ratis future exceptionally; and
• DB replacement failure being propagated after the original OM state is restored.

Validation:

• TestOzoneManagerStateMachine: 49 tests passed
• TestOMRatisSnapshots#testInstallSnapshotFailedBackupRestoresDbDir passed
• checkstyle passed

[szetszwo]

+1 the change looks good.