HDDS-15715. Add helpful CLI flags to finalization CLIs#10678
Conversation
errose28
left a comment
There was a problem hiding this comment.
Thanks for adding this @dombizita.
|
Thanks for the updates. Can you check on the CI failures? This should re-run against the base branch as well since we just merged from master. |
errose28
left a comment
There was a problem hiding this comment.
Thanks for the updates. Left some more comments.
Also here is an AI generated test we can add to cover the case of RPC failure during the wait loop. This assumes the desired behavior in this case is to exit the loop though, which I'm not sure is the case. It might be better to adjust the wait loop to print to stderr when this happens and retry automatically on the next interval, and update this test accordingly.
diff --git a/hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/ozone/admin/upgrade/TestFinalizeSubCommand.java b/hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/ozone/admin/upgrade/TestFinalizeSubCommand.java
index e6e3a5ed48..70bf533b11 100644
--- a/hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/ozone/admin/upgrade/TestFinalizeSubCommand.java
+++ b/hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/ozone/admin/upgrade/TestFinalizeSubCommand.java
@@ -169,6 +169,18 @@ public void testWaitFlagPollsUntilFinalized() throws Exception {
verify(omClient, times(3)).queryUpgradeStatus();
}
+ @Test
+ public void testWaitFlagRpcFailurePrintsMessageAndReturnsNonZero() throws Exception {
+ when(omClient.queryUpgradeStatus()).thenThrow(new IOException("RPC timeout"));
+
+ new CommandLine(cmd).parseArgs("--wait");
+ assertEquals(1, cmd.call());
+
+ String errOutput = errContent.toString(DEFAULT_ENCODING);
+ assertTrue(errOutput.contains("Failed to query upgrade status"));
+ assertTrue(errOutput.contains("RPC timeout"));
+ }
+
@Test
public void testWaitFlagInterruptIsHandledCleanly() throws Exception {
// Make the poll interval long enough that the interrupt lands during the sleep (after the first poll).
| try { | ||
| result.set(cmd.call()); | ||
| } catch (Exception ignored) { | ||
| // The command handles interruption internally and should not throw. |
There was a problem hiding this comment.
If no exception is expected we should fail the test if one is encountered.
| out().println("Cluster finalization has been started. Monitor progress with `ozone admin upgrade status`"); | ||
|
|
||
| if (wait) { |
There was a problem hiding this comment.
We should probably alter this message if --wait is provided to indicate that we are waiting for finalization to complete.
| */ | ||
| @CommandLine.Command( | ||
| name = "finalize", | ||
| description = "Initiates the the process to finalize a cluster upgrade.", |
There was a problem hiding this comment.
Existing issue, but we can fix it here.
| description = "Initiates the process to finalize a cluster upgrade. This command is idempotent.", |
| out().printf("Waiting for finalization: OM=%s, SCM=%s, datanodes=%d/%d%n", | ||
| status.getOmFinalized(), hdds.getScmFinalized(), | ||
| hdds.getNumDatanodesFinalized(), hdds.getNumDatanodesTotal()); |
There was a problem hiding this comment.
Shouldn't we use the regular status output here instead of making a new one?
| static boolean isClusterFinalized(QueryUpgradeStatusResponse status) { | ||
| HddsProtos.UpgradeStatus hdds = status.getHddsStatus(); | ||
| return status.getOmFinalized() | ||
| && hdds.getScmFinalized() | ||
| && hdds.getNumDatanodesFinalized() == hdds.getNumDatanodesTotal(); | ||
| } |
There was a problem hiding this comment.
Looking at this again, I think we should add a new field to the proto to represent this, with this computation happening on the server side, i.e. OM returns clusterFinalized = hdds.isFinalized() && omFinalized and the client just consumes clusterFinalized to make its decision. It can still print the counts, but this allows us to change the definition of finalized on the server side without impacting old clients.
This is similar to what we did with the shouldFinalize field that OM gets from SCM instead of checking the SCM and datanode counts directly.
| } | ||
|
|
||
| @Test | ||
| public void testJsonFlagWithVerboseIncludesVersions() throws Exception { |
There was a problem hiding this comment.
The names of this and the previous test make it seem like the verbose flag affects json output, which is doesn't. If this is intentional, then there should be one test for all json output and it should also assert that content is identical when verbose is passed.
| throw new SCMException("Cannot query upgrade status while SCM is in safe mode. Wait until SCM exits " | ||
| + "safe mode and try again.", ResultCodes.SAFE_MODE_EXCEPTION); |
There was a problem hiding this comment.
We should update OM in the PR as well to handle the SCM exception with this result code and return a propagate a coherent OMException back to the user, probably with result code UNSUPPORTED_OPERATION like #10777 does when the SCM versions don't match.
What changes were proposed in this pull request?
Added new CLI flags to the upgrade finalization CLI:
finalize --wait: the client will periodically poll the cluster for status from OM until the whole cluster is finalized. The finalize command is idempotent so waiting can be resumed at any timestatus --json: json version of the status output for scriptingstatus --verbose: includes internal information like OM and SCM apparent versions, the min and max DN apparent versions in the clusterUsed Claude Opus 4.8 for planning and coding, reviewed by me.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15715
How was this patch tested?
Added unit tests, green run on my fork: https://github.com/dombizita/ozone/actions/runs/28660447709