HDDS-16300. Allow OM to dynamically reconfigure its SCM node list without a restart

[hani-fouladgar]

What changes were proposed in this pull request?

When an SCM is added to (or removed from) an SCM-HA ring, the OM only learns the new membership from ozone.scm.nodes.<serviceId> / ozone.scm.address.<serviceId>.<nodeId> at startup. The OM's SCM block and container failover proxy providers build their node list once in their constructor, so reaching a newly added SCM previously required restarting the OM. This makes SCM scale-out/migration disruptive for the OM.

Please describe your PR in detail:
Approach
Make the OM's SCM proxy providers reloadable and drive the reload through the existing reconfiguration mechanism, so no restart is needed.

• SCMFailoverProxyProviderBase.changeConfig() (new): reloads the node list and addresses from the (already-updated) configuration.
  • loadConfigs() now builds the new node list / proxy-info map in temporaries and commits them only after the whole config parses successfully. Adding an SCM requires two properties (the node list and the new node's address); they may be applied in either order, and if the node list is updated first the reload throws and leaves the previous state intact so the operator can retry.
  • Cached proxies for removed nodes, or nodes whose address changed, are stopped (RPC.stopProxy) so the next call dials the fresh address.
  • The current-proxy pointer is kept valid: it re-syncs its index to the rebuilt list, falling back to the first node if the node it referenced was removed.
• HAUtils: added overloads of getScmBlockClient / getScmContainerClient that accept a caller-supplied proxy provider, so the OM can keep the reference needed to reload it.
• ScmClient.reloadScmNodes() (new): delegates to changeConfig() on the block and container providers; a no-op when providers are absent (e.g. mock-constructed clients).
• OzoneManager: constructs and retains the block/container proxy providers, passes them to ScmClient, and — only when an SCM service id is configured (SCM HA) — registers ozone.scm.nodes.<serviceId> and, as a prefix, ozone.scm.address.<serviceId>. as reconfigurable. The reconfScmNodes callback rejects an empty node list and otherwise triggers reloadScmNodes(). The address keys are registered as a prefix because a newly added node's key does not exist at startup and cannot be registered by name in advance.

This mirrors the existing datanode-side SCM reconfiguration (HDDS-16.x, TestDatanodeSCMNodesReconfiguration) and keeps service boundaries intact — the OM only reloads its own client proxies.

What is the link to the Apache JIRA

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

How was this patch tested?

Testing

• TestSCMFailoverProxyProviderChangeConfig (unit): changeConfig() adds a node, removes a node while keeping the current-proxy pointer valid, and fails cleanly (leaving state intact) when a referenced node's address is missing.
• TestScmClient (unit): reloadScmNodes() delegates to both providers; no-op when providers are null.
• TestOmSCMNodesReconfiguration (integration, SCM-HA MiniOzoneCluster): the SCM node list and per-node address prefix are reconfigurable on a running OM; an empty node list is rejected; a valid reconfigure drives the proxy reload end-to-end.

Manual Testing

1. Manually shrink the SCM node list 3 -> 2

bash-5.1$ sed -i 's/scm1,scm2,scm3/scm1,scm2/' /etc/hadoop/ozone-site.xmlp/ozone-site.xml

2. Get the value of ozone.scm.nodes.scmservice from OM (in memory)

bash-5.1$ curl -s http://localhost:9874/conf | tr -d '\n' | grep -oE '<name>ozone\.scm\.(address|nodes)\.scmservice</name>\s*<value>[^<]+</value>'
<name>ozone.scm.nodes.scmservice</name><value>scm1,scm2,scm3</value>

3. Trigger reconfig the OM to get the new value from the config

bash-5.1$ ozone admin reconfig --service OM --address om1:9862 start   
OM: Started reconfiguration task on node [om1:9862].

4. Check the reconfig status

bash-5.1$ ozone admin reconfig --service OM --address om1:9862 status 2>&1 | grep -vE "TracingUtil|Sampling" 
OM: Reconfiguring status for node [om1:9862]: started at Thu Sep 03 20:53:38 UTC 2026 and finished at Thu Sep 03 20:53:38 UTC 2026.
SUCCESS: Changed property ozone.scm.nodes.scmservice
	From: "scm1,scm2,scm3"
	To: "scm1,scm2"

5. Get the value of ozone.scm.nodes.scmservice from OM (in memory)

bash-5.1$ curl -s http://localhost:9874/conf | tr -d '\n' | grep -oE '<name>ozone\.scm\.(address|nodes)\.scmservice</name>\s*<value>[^<]+</value>'
<name>ozone.scm.nodes.scmservice</name><value>scm1,scm2</value>

6. Add scm3 back

bash-5.1$ sed -i 's/scm1,scm2/scm1,scm2,scm3/' /etc/hadoop/ozone-site.xml

7. Get the value of ozone.scm.nodes.scmservice from OM (in memory)

bash-5.1$ curl -s http://localhost:9874/conf | tr -d '\n' | grep -oE '<name>ozone\.scm\.(address|nodes)\.scmservice</name>\s*<value>[^<]+</value>'
<name>ozone.scm.nodes.scmservice</name><value>scm1,scm2</value>

8. Trigger reconfig the OM to get the new value from the config

bash-5.1$ ozone admin reconfig --service OM --address om1:9862 start 
OM: Started reconfiguration task on node [om1:9862].

9. Check the reconfig status

bash-5.1$ ozone admin reconfig --service OM --address om1:9862 status 2>&1 | grep -vE "TracingUtil|Sampling" 
OM: Reconfiguring status for node [om1:9862]: started at Thu Sep 03 21:01:59 UTC 2026 and finished at Thu Sep 03 21:01:59 UTC 2026.
SUCCESS: Changed property ozone.scm.nodes.scmservice
	From: "scm1,scm2"
	To: "scm1,scm2,scm3"

10. Get the value of ozone.scm.nodes.scmservice from OM (in memory)
    curl -s http://localhost:9874/conf | tr -d '\n' | grep -oE 'ozone.scm.(address|nodes).scmservice\s*[^<]+'
    ozone.scm.nodes.scmservicescm1,scm2,scm3
    bash-5.1$

[chihsuan]

Thanks for working on this! @hani-fouladgar I left a few inline comments, please see them for details.

Also, should the PR description say secure clusters still need an OM restart for a full SCM migration?

[chihsuan]

Does stopping the proxy guarantee that the next call uses the updated endpoint? The retry handler keeps the old proxy until a failover, and that proxy can still make calls. So a removed but online SCM keeps serving OM calls. Could we add a regression test for this?

[hani-fouladgar]

Good catch — it does not, and I've documented the limitation on changeConfig: the wrapping RetryInvocationHandler keeps its last-fetched proxy and re-fetches only on failover, so a removed-but-online SCM can keep serving in-flight calls until the next failover. The new endpoint is guaranteed only from the next fetch. Added testAddressChangeEvictsCachedProxy to cover the provider-level eviction.

[chihsuan]

Could we resolve addresses and stop proxies outside the lock, like refreshProxyAddressIfChanged does? If DNS is slow for a new SCM hostname, other SCM calls may be blocked.

[hani-fouladgar]

Done. Split out a pure buildConfigs() that does the DNS resolution and touches no shared state; changeConfig() now calls it before taking the monitor, and defers RPC.stopProxy until after the monitor is released. A slow resolver for a new SCM host no longer blocks concurrent SCM calls. Mirrors refreshProxyAddressIfChanged.

[chihsuan]

I noticed changeCurrentProxy moves to the next node, so the current proxy here is actually scm1, not scm3. Should we assert it is scm3 before the reload, so the removal path is really tested?

[hani-fouladgar]

Fixed. changeCurrentProxy advances to the next ring node, so I now target the node before scm3 and assertEquals("scm3", provider.getCurrentProxySCMNodeId()) before reload, so the removal path is actually exercised.

[chihsuan]

Just curious, is changing only an address key expected to not take effect? The configuration is updated and reconfig reports success, but OM still connects to the old address. Also, adding a node may need a second reconfig start if the node list is applied before the address.

[hani-fouladgar]

Real bug — fixed. Prefix-registered address keys have no per-key reload function, so an address-only change was never applied. reconfScmNodes now defers on ConfigurationException (keeps the value instead of rolling back), and a new reconfiguration-complete callback reloadScmProxiesOnReconfig reloads once the whole batch is applied. So an address-only change takes effect, and adding a node works in a single reconfig start regardless of key order. Covered by the new testReconfigureScmAddressReloadsProxies integration test.

[chihsuan]

Thanks for the fix! I tried the defer path. With the address key missing, status still says SUCCESS but the providers keep the old list. The live conf also keeps a node without an address, so getServiceList() starts throwing. Could we throw here instead, like the datanode side, so it shows FAILED and can be retried?

[hani-fouladgar]

You were right: the defer left a node without an address in the live conf while reporting SUCCESS, breaking getServiceList(). reconfScmNodes now captures the previous node-list value, and on a reload ConfigurationException it restores the previous value and rethrows. ReconfigurationHandler wraps that into a ReconfigurationException, so the reconfig is reported FAILED and is retriable, and the live conf never keeps an SCM without a resolvable address. This mirrors the datanode's "don't record what didn't take effect" semantics (the datanode returns only its effective node set; the OM can't skip per-node because changeConfig() rebuilds the whole proxy list atomically, so roll-back-and-fail is the equivalent).

[chihsuan]

nit: Could we trim the comments a bit? The same reasoning is repeated in a few places, here and in OzoneManager for example. Keeping each point once would be enough.

[hani-fouladgar]

Done. Removed the duplicated "applied in either order" reasoning here; it now lives in one place — the reconfScmNodes javadoc in OzoneManager.

[hani-fouladgar]

Thanks for working on this! @hani-fouladgar I left a few inline comments, please see them for details.

Also, should the PR description say secure clusters still need an OM restart for a full SCM migration?

Added to the PR description.

[chihsuan]

Thanks for the updates! @hani-fouladgar The provider side looks good now. 👍

I left a follow-up on the defer thread and two inline notes.

[chihsuan]

Should we catch and log here? If the reload throws, the remaining complete callbacks (tracing, logging) are skipped.

[hani-fouladgar]

Done!

[chihsuan]

Could we also cover the nodes-before-address case? reconfigureProperty never fires the complete callback, so its wiring is not exercised by these tests.

[hani-fouladgar]

Done!

[chihsuan]

Thanks! I think this still sets both keys before calling the callback, so the nodes-before-address order in a real batch is not exercised yet.

[hani-fouladgar]

You were right: my old test set both keys then called the callback, which reproduces the address-first end state, not the nodes-before-address ordering. Replaced it with testReconfigureAddScmNodeNodesBeforeAddress, which applies the keys in the real batch order:

1. Node list applied first (address not set yet) → the property fails with ReconfigurationException and rolls back → asserts the node is not added.
2. The new SCM's address applied (prefix key, no reload of its own).
3. Node list reapplied → now resolves → asserts the node is added on both providers.

[chihsuan]

Thanks @hani-fouladgar I think two cases remain, both from validating one key at a time.

Adding an SCM can fail if the node list is applied before its address, since reconfScmNodes only sees the live conf at that point. An invalid address change can also report SUCCESS.

I wonder if these keys should be validated as one group before anything is written. That would need a hook in ReconfigurationHandler / ReconfigurableBase. Since it touches the shared mechanism, I think a committer's take would help on whether to do it here or in a follow-up.

[chihsuan]

nit: Could we import SCMFailoverProxyProviderBase here instead of the fully qualified name?

[hani-fouladgar]

Added import org.apache.hadoop.hdds.scm.proxy.SCMFailoverProxyProviderBase; and changed the assertResolvedHost signature to use the short name.

[chihsuan]

Thanks! I think this still sets both keys before calling the callback, so the nodes-before-address order in a real batch is not exercised yet.

[hani-fouladgar]

Adding an SCM can fail if the node list is applied before it

Thanks, that's a fair read — both cases are inherent to per-key validation, and I agree grouped/atomic validation of ozone.scm.nodes. together with the ozone.scm.address..* keys is the clean fix.

What this PR guarantees today is the safety property: an unresolvable node list fails and is rolled back (visible as FAILED, retriable), so the live configuration never ends up holding an SCM without an address, and getServiceList() stays consistent. The two residuals are usability, not correctness:

• node-list-before-address needs the address set first (or a retry) — same two-pass behavior as the datanode, which skips an unresolvable add;
• an unresolvable address-only change is logged and the previous proxies are kept, but the status still reads SUCCESS because a reconfiguration-complete callback can't fail the batch.

Closing both properly means adding a batch/pre-validation hook to ReconfigurableBase/ReconfigurationHandler, which is shared by the datanode, OM, and SCM. Since that changes the shared mechanism, I agree it's better as a separate change with a committer's input rather than expanding this PR's scope. I'll file a follow-up HDDS Jira for grouped SCM-config validation and link it here — happy to take it on once there's a committer's steer on the approach. Does deferring those two cases to that follow-up sound reasonable for this PR?

[chihsuan]

I'll file a follow-up HDDS Jira for grouped SCM-config validation and link it here, happy to take it on once there's a committer's steer on the approach. Does deferring those two cases to that follow-up sound reasonable for this PR?

Thanks @hani-fouladgar Sounds reasonable to me. I left three inline notes, otherwise looks good to me, pending a committer's review.

[chihsuan]

Could we widen this catch to RuntimeException? I noticed NetUtils.createSocketAddr throws IllegalArgumentException for an address like host:9860. That skips the rollback, so the live conf keeps the bad node list.

[hani-fouladgar]

Done!

[chihsuan]

Same here, should this catch RuntimeException too? A malformed address-only change would still escape and skip the tracing and logging callbacks.

[hani-fouladgar]

Done!

[chihsuan]

I wonder if we could drive this through the real complete-callback path instead? Calling the method directly means the registerCompleteCallback line is not covered, and it is also what forces reloadScmProxiesOnReconfig to be public.

[hani-fouladgar]

Good question — I looked into driving this through the real complete-callback path, but it isn't reachable from a mini-cluster. The reconfiguration-complete callbacks only fire from the async ReconfigurationThread (startReconfigurationTask()), which reads a fresh new OzoneConfiguration() off ozone-site.xml from disk. The mini-cluster never rewrites that file, so an address-only change made in-memory on the live conf is invisible to that path — the callback would diff two identical on-disk configs and do nothing.

The synchronous reconfigureProperty path (which the other tests use) doesn't fire complete callbacks at all — it only runs the per-property function, and the address keys are prefix-registered with identity(), so there's no per-key hook to exercise. That's why an address-only change is applied by the complete callback rather than the per-property path.

So calling reloadScmProxiesOnReconfig directly is the only way to cover the address-only reload in this harness, and it's what keeps the method @VisibleForTesting. The registerCompleteCallback(...) wiring line itself is exercised by the other reconfiguration tests that go through the handler; the node-list reload path (reconfScmNodes) is covered end-to-end. Happy to add a framework-level test hook if you'd prefer the method be private, but that touches shared ReconfigurationHandler infrastructure used by the datanode and SCM too, so I left it out of this PR.

[chihsuan]

Thanks for the explanation, that makes sense. Could we still add two cases here? One with an unrelated key, to check no reload happens, and one with a malformed address, to check the failure is caught.

[hani-fouladgar]

Added both: testReloadScmProxiesOnReconfigIgnoresUnrelatedKey (unrelated key → no reload, providers keep the original address) and testReloadScmProxiesOnReconfigCatchesMalformedAddress (malformed address → callback catches the failure, doesn't throw, membership unchanged). Thanks for the suggestion.

[chihsuan]

Thanks for the follow-up! @hani-fouladgar I looked a bit more and left a few small notes. Also, I just noticed the two new keys are not listed in Reconfigurability.md. Should we add them?

[chihsuan]

Heads-up for the follow-up Jira rather than a change request. If an address key is unset, or set to host:port, the bad value stays in the live conf. getServiceList() then throws, so new clients can't reach this OM until fixed.

[chihsuan]

Could we reword this? When the node list is applied before the address, it is rolled back, so the callback reloads the old list. Adding a node then needs a second reconfig start. Same for the reloadScmProxiesOnReconfig javadoc.

[hani-fouladgar]

Reworded both the reconfScmNodes and reloadScmProxiesOnReconfig javadocs: if the node list is applied before the address it's rolled back, the complete callback reloads the old list, and the node is only picked up on a second reconfig start once the address is stored.

[chihsuan]

nit: Would it be worth calling getScmInfo() after the reload here? The tests only compare node ids, so a proxy that was stopped but still in use would not be caught.

[hani-fouladgar]

Good call. Added a live getScmInfo() RPC through both rebuilt providers after the reload, so a proxy that was stopped but left in use is caught rather than just matching node ids.

[hani-fouladgar]

Thanks for the follow-up! @hani-fouladgar I looked a bit more and left a few small notes. Also, I just noticed the two new keys are not listed in Reconfigurability.md. Should we add them?

Good catch — added ozone.scm.nodes.<scmServiceId> and ozone.scm.address.<scmServiceId>.<scmNodeId> to the OM table in Reconfigurability.md.

[szetszwo]

@hani-fouladgar , is this AI generated/assisted? If yes, please add the AI model (ASF requirement). Please also shorten the long javadoc/comments since Javadoc/comments should be as concise as possible.

[hani-fouladgar]

@hani-fouladgar , is this AI generated/assisted? If yes, please add the AI model (ASF requirement). Please also shorten the long javadoc/comments since Javadoc/comments should be as concise as possible.

I added Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> to commits' comments. I also shortened the javadoc/comments as much as I could.

[szetszwo]

@hani-fouladgar , Thanks for working on this!

I need you to review this AI generated PR first:

• Revert formatting and unnecessary changes (as an example, see the comment inlined)
• Review the comments/javadocs -- they are still too long and hard to maintain.

[szetszwo]

The else is unnecessarily removed here and the comment is reformatted. Please revert them and minimize the change.

It took me 10 min to figure out such a simple change here.

+  private ScmProxyConfig buildConfigs() {
     List<SCMNodeInfo> scmNodeInfoList = SCMNodeInfo.buildNodeInfo(conf);
-    scmNodeIds = new ArrayList<>();
-
+    final List<String> newScmNodeIds = new ArrayList<>();
+    final Map<String, SCMProxyInfo> newScmProxyInfoMap = new HashMap<>();
 
     for (SCMNodeInfo scmNodeInfo : scmNodeInfoList) {
       String protocolAddress = getProtocolAddress(scmNodeInfo);
@@ -188,16 +199,96 @@ protected synchronized void loadConfigs() {
 
         String scmServiceId = scmNodeInfo.getServiceId();
         String scmNodeId = scmNodeInfo.getNodeId();
-        scmNodeIds.add(scmNodeId);
+        newScmNodeIds.add(scmNodeId);
         // Preserve the original config string so DNS can be re-resolved
         // on connection failure when the SCM peer is rescheduled to a
         // new IP (Kubernetes pod-IP-change recovery). See
         // refreshProxyAddressIfChanged(String).
         SCMProxyInfo scmProxyInfo = new SCMProxyInfo(scmServiceId, scmNodeId,
             protocolAddr, protocolAddress);
-        scmProxyInfoMap.put(scmNodeId, scmProxyInfo);
+        newScmProxyInfoMap.put(scmNodeId, scmProxyInfo);
+      }
+    }
+
+    return new ScmProxyConfig(newScmNodeIds, newScmProxyInfoMap);
+  }

[hani-fouladgar]

Reverted to else as requested, though it's technically unnecessary since the throw exits the method. Kept the newScmProxyInfoMap/newScmNodeIds locals because buildConfigs() must not touch shared state — it returns a fresh holder so changeConfig() can resolve addresses off-lock, and a mid-loop throw leaves the live config untouched.

[hani-fouladgar]

• comments/javadocs

Done — shortened the comments/javadocs and added the else back.

[szetszwo]

@hani-fouladgar , thanks for the update!

Reviewing SCMFailoverProxyProviderBase; please see the comments inilned.

[szetszwo]

• Call indexOf(..) first to avoid calling contains(..).
• In this else case, does it need to update currentProxySCMNodeId?

[hani-fouladgar]

Done, single indexOf() now. And no, the else case doesn't need to touch currentProxySCMNodeId: when indexOf() finds the node, scmNodeIds.get(index) is by definition the same ID we searched for, so only the index needs re-syncing.

[szetszwo]

Could they be updated without copying the map?

Map<String, SCMProxyInfo> oldProxyInfoMap = scmProxyInfoMap;
scmProxyInfoMap = newConfig.proxyInfoMap;

[hani-fouladgar]

Good catch — done.

[szetszwo]

Why not just stop the stale proxies in the loop above?

[hani-fouladgar]

RPC.stopProxy() can block while the IPC client tears down its connections, and doing that inside the synchronized block would stall every concurrent getProxy() / shouldRetry() caller on the provider monitor. Same reason we resolve DNS before locking. refreshProxyAddressIfChanged() already uses the same evict-under-lock / stop-after-unlock shape.

[szetszwo]

+1 the change looks good.