smengcl opened a new pull request, #10943: URL: https://github.com/apache/ozone/pull/10943
Generated-by: Claude Code (Opus 5) ## What changes were proposed in this pull request? Two functions write the in-memory transaction info of the OM, and they write different values. `loadSnapshotInfoFromDB()` reads the value from the installed checkpoint DB, and that value is correct. [installCheckpoint](https://github.com/apache/ozone/blob/40cd903440e7b84017461eccdaf4075f72f58a89/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L4390) writes the TermIndex from before the installation. But the next line [unpauses](https://github.com/apache/ozone/blob/40cd903440e7b84017461eccdaf4075f72f58a89/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L4391) the state machine at the index of the checkpoint. The oneline change makes `installCheckpoint` write the index of the checkpoint. **No failure is demonstrated, but one path has no protection.** Ratis runs `StateMachineUpdater.reload()` on a different thread. Thus the field holds the old index from the unpause until that reload. `reload()` calls `reinitialize()` before it reads `getLatestSnapshot()`, and the OM obeys that rule with `loadSnapshotInfoFromDB()`. Thus the value is correct again after the reload. All the callers of `getLatestSnapshot()` are examined during this window: * `getSnapshotIndex()` and `containsTermIndex()` use `ServerState.latestInstalledSnapshot`. Ratis sets that value in the same thread. These two functions stay correct. * `getLogInfo()` only makes a report. * The snapshot-creation decision refuses to run during an installation. * `getLastEntry()` has no protection. It uses the snapshot value when the last entry of the Raft log is null, and it does not use the `max`. `decideVote()` calls `getLastEntry()`. It grants a vote when the last entry of this server is less than the last entry of the candidate. Thus an index that is too low can grant a vote that the server must refuse. The vote path does not test `getInProgressInstallSnapshotIndex()`, and thus an installation does not stop it. `RaftLog.onSnapshotInstalled` can make the log empty, and `SegmentedRaftLogCache.getLastTermIndex()` gives null for an empty log. This sequence is unlikely. The window is probably less than one millisecond, and the election timeout is approximately one second. The Raft log must also be empty, and the position of the candidate must be between the old index and the correct index. I did not measure the window, and I did not cause this sequence in a test. The change writes the correct value before the unpause. Thus the field is correct for all of the window. On the path where the DB replacement fails, the change makes no difference. The code writes `term` and `lastAppliedIndex` again only at line 4362-4363, and those two lines are in the `try` block that failed. Also, `valueOf(long, long)` calls `valueOf(TermIndex)` ([TransactionInfo.java:81](https://github.com/apache/ozone/blob/40cd903440e7b84017461eccdaf4075f72f58a89/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/TransactionInfo.java#L81)). On that path this call is necessary, because no other function writes the field. Thus I corrected the call, and I did not remove it. A TLA+ model of the follower snapshot-installation path found this issue. The model checks that the three views of the transaction index agree after an installation. ## What is the link to the Apache JIRA https://issues.apache.org/jira/browse/HDDS-16086 ## How was this patch tested? * New test `TestOMRatisSnapshots#testInstallCheckpointPublishesNewTransactionInfo` calls `installCheckpoint` directly. Then it compares the transaction info with the TermIndex that the function returns. The test does not start a reload of Ratis. Thus the value before the change is `INITIAL_VALUE`, which is more incorrect than a true installation gives. The test must do the comparison immediately, because `takeSnapshotImpl` also calculates the field again. * Without the change, the new test fails on this base and gives: `In-memory transaction info must match the index the state machine was unpaused at ==> expected: <(t:1, i:102)> but was: <<INITIAL_VALUE>>`. With the change it passes. (The test does not exercise the vote path. For that, a test must cause an election inside a window of less than one millisecond. I did not find a stable method to do this.) -- 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]
