Skip to content

HDDS-16138. Add compatibility coverage for SCM Ratis role output over IPv6 - #11049

Open
rjgoyln wants to merge 2 commits into
apache:masterfrom
rjgoyln:HDDS-16138
Open

HDDS-16138. Add compatibility coverage for SCM Ratis role output over IPv6#11049
rjgoyln wants to merge 2 commits into
apache:masterfrom
rjgoyln:HDDS-16138

Conversation

@rjgoyln

@rjgoyln rjgoyln commented Aug 18, 2026

Copy link
Copy Markdown

What changes were proposed in this pull request?

The colon-delimited SCM Ratis role string is a wire format with four independent readers — the JMX view behind the SCM web UI, ozone admin scm roles, leader detection in ozone admin safemode status, and Recon's snapshot download — and none of them own it. HDDS-15774 made the encoding and the shared parser IPv6-safe but left most of those readers untested, and the JMX view, which reorders the fields, had no test at all. A change to the encoding today shifts its columns and breaks leader selection silently, surfacing as a runtime failure in an IPv6 deployment rather than as a test failure.

This pins the format and covers each reader. The producer's output is asserted verbatim instead of being round-tripped through the parser, which is what actually holds the brackets and the field order in place; the DNS case asserts only the fields the encoding owns, so it does not depend on what the name resolves to. The existing IPv6 table test moves from substring matching to pinning every column. The JMX and safe-mode tests also cover malformed role strings because these consumers handle parser failures differently.

No behaviour changes. SafeModeCheckSubcommand.findLeaderNode was unreachable from a test because the subcommand builds its own ScmClient, so it now takes the node list as a parameter rather than reading the field, and the stateless matchesAddress becomes static with it.

Two things are left for reviewers to weigh: findLeaderNode now reads only other objects' data and arguably belongs beside SCMNodeInfo, and the positional String[] indexing remains the underlying awkwardness — HDDS-15774 raised a structured representation as the alternative, which this JIRA scopes out.

What is the link to the Apache JIRA

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

How was this patch tested?

Unit tests only: TestSCMRatisServerImpl, TestStorageContainerManagerRatisRoles (new), TestGetScmRatisRolesSubcommand, TestSafeModeCheckSubcommand (new), TestStorageContainerServiceProviderImpl. Checkstyle, RAT and SpotBugs are clean across the three modules touched.

Generated-by: Claude Code (Opus 5)

… IPv6

The encoded SCM Ratis role string is a wire format shared by the roles
CLI, safe-mode leader detection, the JMX view behind the SCM web UI, and
Recon's snapshot download. After HDDS-15774 made the encoding IPv6-safe,
only the producer and the roles table were covered, so a regression in
the encoding or in any of the other consumers would surface as a runtime
failure in an IPv6 deployment rather than as a test failure.

Leader detection in `ozone admin safemode status` could not be reached
from a test because the subcommand builds its own client, so
findLeaderNode now takes the node list as a parameter instead of reading
the field.
Copilot AI lite review requested due to automatic review settings August 18, 2026 15:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@jojochuang
jojochuang requested review from smengcl and a lite review from Copilot August 18, 2026 15:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

@rjgoyln
rjgoyln marked this pull request as ready for review August 21, 2026 17:46
@smengcl

smengcl commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Thanks @rjgoyln for the additions.

Can you enable GH actions in your fork so CI can run? https://github.com/rjgoyln/ozone/settings/actions

SCM sets an address on every RaftPeer it builds, so describing the empty
entry as producer output overstated a path the code does not reach. The
cases stay as malformed-input coverage.
@rjgoyln

rjgoyln commented Aug 22, 2026

Copy link
Copy Markdown
Author

Thanks @rjgoyln for the additions.

Can you enable GH actions in your fork so CI can run? https://github.com/rjgoyln/ozone/settings/actions

Thank you! Actions are enabled on the fork now, and build-branch is running against the latest push.

@smengcl smengcl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. pending CI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants