errose28 commented on code in PR #10678:
URL: https://github.com/apache/ozone/pull/10678#discussion_r3605918626
##########
hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/ozone/admin/upgrade/TestFinalizeSubCommand.java:
##########
@@ -119,6 +132,165 @@ public void
testNonZduServerPrintsErrorAndReturnsNonZero() throws Exception {
verify(omClient, never()).finalizeUpgrade();
}
+ @Test
+ public void testWithoutWaitFlagDoesNotPollStatus() throws Exception {
+ new CommandLine(cmd).parseArgs();
+ assertEquals(0, cmd.call());
+
+ verify(omClient, never()).queryUpgradeStatus();
+ }
+
+ @Test
+ public void testStatusCalledOnceWhenAlreadyFinalized() throws Exception {
+ when(omClient.queryUpgradeStatus()).thenReturn(finalizedStatus(3, 3));
+
+ new CommandLine(cmd).parseArgs("--wait");
+ assertEquals(0, cmd.call());
+
+ String output = outContent.toString(DEFAULT_ENCODING);
+ assertTrue(output.contains("Finalization complete."));
+ verify(omClient).finalizeUpgrade();
+ verify(omClient, times(1)).queryUpgradeStatus();
+ }
+
+ @Test
+ public void testWaitFlagPollsUntilFinalized() throws Exception {
+ when(omClient.queryUpgradeStatus())
+ .thenReturn(inProgressStatus(0, 3))
+ .thenReturn(inProgressStatus(2, 3))
+ .thenReturn(finalizedStatus(3, 3));
+
+ new CommandLine(cmd).parseArgs("--wait");
+ assertEquals(0, cmd.call());
+
+ String output = outContent.toString(DEFAULT_ENCODING);
+ assertTrue(output.contains("Waiting for finalization"));
+ assertTrue(output.contains("Finalization complete."));
+ verify(omClient, times(3)).queryUpgradeStatus();
+ }
+
+ @Test
+ public void testWaitFlagInterruptIsHandledCleanly() throws Exception {
+ // Make the poll interval long enough that the interrupt lands during the
sleep (after the first poll).
+ cmd.setPollIntervalMillis(60_000);
+ when(omClient.queryUpgradeStatus()).thenReturn(inProgressStatus(0, 3));
+
+ new CommandLine(cmd).parseArgs("--wait");
+
+ AtomicInteger result = new AtomicInteger(-1);
+ Thread runner = new Thread(() -> {
+ try {
+ result.set(cmd.call());
+ } catch (Exception ignored) {
+ // The command handles interruption internally and should not throw.
Review Comment:
If no exception is expected we should fail the test if one is encountered.
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/ozone/admin/upgrade/FinalizeSubCommand.java:
##########
@@ -51,11 +65,70 @@ public Integer call() throws Exception {
}
client.finalizeUpgrade();
out().println("Cluster finalization has been started. Monitor progress
with `ozone admin upgrade status`");
+
+ if (wait) {
Review Comment:
We should probably alter this message if `--wait` is provided to indicate
that we are waiting for finalization to complete.
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/ozone/admin/upgrade/FinalizeSubCommand.java:
##########
Review Comment:
Existing issue, but we can fix it here.
```suggestion
description = "Initiates the process to finalize a cluster upgrade. This
command is idempotent.",
```
##########
hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/ozone/admin/upgrade/TestStatusSubCommand.java:
##########
@@ -122,6 +134,96 @@ public void testNonZduServerPrintsErrorAndReturnsNonZero()
throws Exception {
verify(omClient, never()).queryUpgradeStatus();
}
+ @Test
+ public void testJsonFlagBasicOutput() throws Exception {
+ when(omClient.queryUpgradeStatus()).thenReturn(basicResponse(false, false,
1, 3));
+
+ new CommandLine(cmd).parseArgs("--json");
+ assertEquals(0, cmd.call());
+
+ JsonNode root = JSON.readTree(outContent.toString(DEFAULT_ENCODING));
+ assertFalse(root.path("omFinalized").asBoolean());
+ assertFalse(root.path("scmFinalized").asBoolean());
+ assertEquals(1, root.path("datanodesFinalized").asInt());
+ assertEquals(3, root.path("datanodesTotal").asInt());
+ }
+
+ @Test
+ public void testJsonFlagWithVerboseIncludesVersions() throws Exception {
Review 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.
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/ozone/admin/upgrade/FinalizeSubCommand.java:
##########
@@ -51,11 +65,70 @@ public Integer call() throws Exception {
}
client.finalizeUpgrade();
out().println("Cluster finalization has been started. Monitor progress
with `ozone admin upgrade status`");
+
+ if (wait) {
+ return waitForFinalization(client);
+ }
}
return 0;
}
+ /**
+ * Polls the cluster status until OM, SCM and all healthy datanodes report
finalized, the operator
+ * interrupts the command, or an RPC fails. {@code --wait} is safe to
re-run: if the cluster is already
+ * finalized, the first poll returns done and the command exits 0.
+ */
+ private int waitForFinalization(OzoneManagerProtocol client) {
+ while (true) {
+ QueryUpgradeStatusResponse status;
+ try {
+ status = client.queryUpgradeStatus();
+ } catch (Exception e) {
+ err().println("Failed to query upgrade status: " + e.getMessage()
+ + ". Use `ozone admin upgrade status` to monitor progress.");
+ return 1;
+ }
+
+ // Check if cluster is already finalized
+ if (isClusterFinalized(status)) {
+ out().println("Finalization complete.");
+ return 0;
+ }
+
+ if (isVerbose()) {
+ StatusSubCommand.printVerbose(status, out());
+ } else {
+ HddsProtos.UpgradeStatus hdds = status.getHddsStatus();
+ out().printf("Waiting for finalization: OM=%s, SCM=%s,
datanodes=%d/%d%n",
+ status.getOmFinalized(), hdds.getScmFinalized(),
+ hdds.getNumDatanodesFinalized(), hdds.getNumDatanodesTotal());
+ }
+ out().flush();
+
+ // Finalization checks before sleeping, so an already-finalized cluster
returns without waiting a poll interval.
+ try {
+ Thread.sleep(pollIntervalMillis);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ out().println("Waiting interrupted. Use `ozone admin upgrade status`
to monitor progress.");
+ return 0;
+ }
+ }
+ }
+
+ static boolean isClusterFinalized(QueryUpgradeStatusResponse status) {
+ HddsProtos.UpgradeStatus hdds = status.getHddsStatus();
+ return status.getOmFinalized()
+ && hdds.getScmFinalized()
+ && hdds.getNumDatanodesFinalized() == hdds.getNumDatanodesTotal();
+ }
Review 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.
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/ozone/admin/upgrade/FinalizeSubCommand.java:
##########
@@ -51,11 +65,70 @@ public Integer call() throws Exception {
}
client.finalizeUpgrade();
out().println("Cluster finalization has been started. Monitor progress
with `ozone admin upgrade status`");
+
+ if (wait) {
+ return waitForFinalization(client);
+ }
}
return 0;
}
+ /**
+ * Polls the cluster status until OM, SCM and all healthy datanodes report
finalized, the operator
+ * interrupts the command, or an RPC fails. {@code --wait} is safe to
re-run: if the cluster is already
+ * finalized, the first poll returns done and the command exits 0.
+ */
+ private int waitForFinalization(OzoneManagerProtocol client) {
+ while (true) {
+ QueryUpgradeStatusResponse status;
+ try {
+ status = client.queryUpgradeStatus();
+ } catch (Exception e) {
+ err().println("Failed to query upgrade status: " + e.getMessage()
+ + ". Use `ozone admin upgrade status` to monitor progress.");
+ return 1;
+ }
+
+ // Check if cluster is already finalized
+ if (isClusterFinalized(status)) {
+ out().println("Finalization complete.");
+ return 0;
+ }
+
+ if (isVerbose()) {
+ StatusSubCommand.printVerbose(status, out());
+ } else {
+ HddsProtos.UpgradeStatus hdds = status.getHddsStatus();
+ out().printf("Waiting for finalization: OM=%s, SCM=%s,
datanodes=%d/%d%n",
+ status.getOmFinalized(), hdds.getScmFinalized(),
+ hdds.getNumDatanodesFinalized(), hdds.getNumDatanodesTotal());
Review Comment:
Shouldn't we use the regular status output here instead of making a new one?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -1214,18 +1214,26 @@ public HddsProtos.UpgradeStatus queryUpgradeStatus()
throws IOException {
try {
getScm().checkAdminAccess(getRemoteUser(), true);
+ if (scm.getScmContext().isInSafeMode()) {
+ 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);
Review 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.
--
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]
