HDDS-14108. Provide option in ‘scm safemode status’ to show status of all SCM nodes - #9611
Conversation
|
@octachoron would you like to take a look at it if you have time? It's related to what we discussed recently :) |
|
@dombizita, absolutely, thank you! I don't think my vote is enough to merge though. |
octachoron
left a comment
There was a problem hiding this comment.
Thank you @sreejasahithi for the patch. I added a few thoughts and questions inline. 🙂
octachoron
left a comment
There was a problem hiding this comment.
Thank you, the changes look good to me. Do you think there is a good way to write tests for the feature? (I do not see straightforward precedent other than actual integration tests, but that does not mean there isn't a way. 🙂)
ashishkumar50
left a comment
There was a problem hiding this comment.
@sreejasahithi Thanks for working on this.
| } | ||
|
|
||
| private void executeForSpecificNodeInHA(ScmClient scmClient, String serviceId) throws IOException { | ||
| String scmAddress = getScmOption().getScm(); |
There was a problem hiding this comment.
scmAddress is not mandatory option.
| } else if (StringUtils.isNotEmpty(getScmOption().getScm()) && serviceId != null) { | ||
| executeForSpecificNodeInHA(scmClient, serviceId); | ||
| } else { | ||
| executeForSingleNode(scmClient); |
There was a problem hiding this comment.
In normal or existing behaviour we need safemode status from leader node most of the time. When no scm address is passed, whether we are getting safe mode status from leader node or not? Because now follower also can accept safemode and can return the status.
There was a problem hiding this comment.
Thanks @ashishkumar50 for finding this bug, you are right now that we are allowing follower to also accept status command there can be a possibility where when we run safemode status command with no additional option it can return the status of the follower.
I have fixed this issue.
priyeshkaratha
left a comment
There was a problem hiding this comment.
Thanks @sreejasahithi for working on this. I have one minor comment on handling audit logs.
| target.getAddress().equals(nodeAddr.getAddress()); | ||
| } catch (Exception e) { | ||
| // If address resolution fails, no match | ||
| return false; |
There was a problem hiding this comment.
nit : Log the exception here before returning false
There was a problem hiding this comment.
I have removed the logging here because it creates unwanted noise in the CLI output. Instead, I have ensured that actual errors are properly surfaced when no leader can be determined or when the node specified in --scm option doesn't match, clear error messages are thrown to the user.
priyeshkaratha
left a comment
There was a problem hiding this comment.
Thanks @sreejasahithi for improving the PR. Changes looks good to me.
aryangupta1998
left a comment
There was a problem hiding this comment.
Thanks for the patch @sreejasahithi, some minor comments inline!
aryangupta1998
left a comment
There was a problem hiding this comment.
Thanks for updating the patch @sreejasahithi, LGTM!
|
@sumitagrawl could you please review this patch. |
| public Map<String, Pair<Boolean, String>> getSafeModeRuleStatuses() | ||
| throws IOException { | ||
| GetSafeModeRuleStatusesRequestProto request = | ||
| GetSafeModeRuleStatusesRequestProto.getDefaultInstance(); | ||
| GetSafeModeRuleStatusesResponseProto response = | ||
| submitRequest(Type.GetSafeModeRuleStatuses, | ||
| builder -> builder.setGetSafeModeRuleStatusesRequest(request)) | ||
| .getGetSafeModeRuleStatusesResponse(); | ||
| Map<String, Pair<Boolean, String>> map = new HashMap(); | ||
| for (SafeModeRuleStatusProto statusProto : | ||
| response.getSafeModeRuleStatusesProtoList()) { | ||
| map.put(statusProto.getRuleName(), | ||
| return buildSafeModeRuleStatusesMap(response); | ||
| } | ||
|
|
||
| /** | ||
| * Helper method to build a map from GetSafeModeRuleStatusesResponseProto. | ||
| * Extracts rule names and their status information. | ||
| */ | ||
| private Map<String, Pair<Boolean, String>> buildSafeModeRuleStatusesMap( | ||
| GetSafeModeRuleStatusesResponseProto response) { | ||
| Map<String, Pair<Boolean, String>> ruleStatuses = new HashMap<>(); | ||
| for (SafeModeRuleStatusProto statusProto : response.getSafeModeRuleStatusesProtoList()) { | ||
| ruleStatuses.put(statusProto.getRuleName(), | ||
| Pair.of(statusProto.getValidate(), statusProto.getStatusText())); | ||
| } | ||
| return map; | ||
| return ruleStatuses; | ||
| } |
There was a problem hiding this comment.
@sreejasahithi , Using Map + Pair is an incorrect choice of data structure. Just want to let you know but not blaming your since it was the existing code before this PR. See
There was a problem hiding this comment.
Thanks @szetszwo for pointing this out. Agreed Map<String, Pair<Boolean, String>> is not needed given the use of SafeModeRuleStatusProto.
Will take a look at HDDS-16130
What changes were proposed in this pull request?
This PR provides an option
--allto show the safemode status of each SCM node in the cluster.If verbose, It also provides the status of each safemode exit rule for each SCM node.
This PR also fixes the bug stated in HDDS-13832 where when
--scmoption is used in HA it always shows the status of the leader SCM and silently ignores the node specified via the option.What is the link to the Apache JIRA
HDDS-14108
How was this patch tested?
This patch was tested locally in a docker ozone-ha cluster:
if scm1 goes down then we can see that the command curretly picks up the new leader when we run the command with no explicit option (--all or --scm) provided.
if scm node scm2 also goes down then it clearly states that leader could not be determined.
when invalid scm node provided:
Green CI : https://github.com/sreejasahithi/ozone/actions/runs/20842284515