[jira] [Created] (HDDS-17226) A recommission is silently lost when the datanode admin workflow is resumed before the next monitor run, and the cancelled request completes

["Siyao Meng (Jira)" <ji...@apache.org>]

Siyao Meng created HDDS-17226:
---------------------------------

             Summary: A recommission is silently lost when the datanode admin workflow is resumed before the next monitor run, and the cancelled request completes
                 Key: HDDS-17226
                 URL: https://issues.apache.org/jira/browse/HDDS-17226
             Project: Apache Ozone
          Issue Type: Bug
            Reporter: Siyao Meng
         Attachments: CR-12-keep-queued-recommission-when-resuming-admin.patch, TestBugCR12ResumedAdminDropsQueuedRecommission.java

h3. Mechanism
{{NodeDecommissionManager.recommission}} does not change the op state of the node. It calls {{DatanodeAdminMonitorImpl.stopMonitoring}}, which queues the node in {{cancelledNodes}}, and the next monitor run ({{processCancelledNodes}}, every {{ozone.scm.datanode.admin.monitor.interval}}, default 30s) puts the node back to IN_SERVICE and stops tracking it.

Until that run the node still reads DECOMMISSIONING, ENTERING_MAINTENANCE or IN_MAINTENANCE. {{NodeDecommissionManager.continueAdminForNode}} decides from that op state alone, and for those three states it calls {{DatanodeAdminMonitor.startMonitoring}}, which first removes the node from {{cancelledNodes}}. The queued recommission is dropped, and the node is put back into the admin workflow with its admin state unchanged. The recommission RPC had already returned an empty error list.

h3. Trigger
# {{ozone admin datanode decommission <dn>}} (or {{maintenance}}), and the monitor runs, so the node is tracked.
# {{ozone admin datanode recommission <dn>}}, which returns no error.
# Within the same monitor interval, one of the paths that resume the admin workflow runs for that node:
#* the datanode re-registers with a changed IP address or hostname. {{SCMNodeManager.register}} fires NODE_ADDRESS_UPDATE and {{NodeAddressUpdateHandler}} calls {{continueAdminForNode}}.
#* this SCM steps down and is elected leader again. {{NodeDecommissionManager.onBecomeLeader}} calls {{continueAdminForNode}} for every node, and the monitor queue survives on the same process.
# The queued recommission is gone and the node is tracked again, still DECOMMISSIONING or in maintenance.

{{NewNodeHandler}} calls {{continueAdminForNode}} as well, for a newly registered node whose persisted state is not IN_SERVICE. A queued recommission for a node that is new to this SCM is unlikely (the queue is empty after a restart), and that path was not exercised.

h3. Impact
* The recommission is lost with no error anywhere: the CLI reports success, and nothing restores it, because the node is no longer queued and its op state was never changed.
* For a decommission, the workflow runs on to its end and the node becomes DECOMMISSIONED, which is terminal. This is the same for both triggers. An operator who cancelled a decommission sees it complete anyway, and has to notice and recommission again.
* For maintenance, the cancelled request proceeds as well: the node enters IN_MAINTENANCE and stays out of service until the end time given with the original request expires, or indefinitely if none was given (argued from the code, not run).
* No data is at risk in either case.

h3. Affected versions
{{continueAdminForNode}} and {{onBecomeLeader}} arrived with HDDS-5090, first released in 1.2.0, and the NODE_ADDRESS_UPDATE trigger with HDDS-5916 in 1.3.0. {{continueAdminForNode}}, {{recommission}} and the {{cancelledNodes}} removal in {{startMonitoring}} are unchanged in every release since, up to and including 2.2.1, and on master at 20d95ed37fe.

h3. Reproduction
PASS on the unmodified source, deterministic (two identical runs). [^TestBugCR12ResumedAdminDropsQueuedRecommission.java] ({{hadoop-hdds/server-scm}}) runs one real SCM per test with its own decommission manager and event queue, driven through {{SCMNodeManager.register}} and {{processHeartbeat}}, the {{decommissionNodes}} and {{recommissionNodes}} methods that {{SCMClientProtocolServer}} calls, the real NODE_ADDRESS_UPDATE handler and {{DatanodeAdminMonitorImpl.run}}. The scheduled monitor is set to 1h and each tick is run by hand in place of the 30s interval. The simulated datanode changes its persisted state only when SCM sends {{SetNodeOperationalStateCommand}}, as {{SetNodeOperationalStateCommandHandler}} does. Output on the unmodified source:
{noformat}
recommission (queued): state=DECOMMISSIONING tracked=true queuedRecommissions=1 pending=0
address change: state=DECOMMISSIONING tracked=true queuedRecommissions=0 pending=1
monitor run after the address change: state=DECOMMISSIONED tracked=false queuedRecommissions=0 pending=0
recommission (queued): state=DECOMMISSIONING tracked=true queuedRecommissions=1 pending=0
leader change: state=DECOMMISSIONING tracked=true queuedRecommissions=0 pending=1
monitor run after the leader change: state=DECOMMISSIONED tracked=false queuedRecommissions=0 pending=0
{noformat}
A control without a resume in that window ends IN_SERVICE and untracked. A passing test means the defect is present. With the patch the two defect tests fail on the queued recommission count right after the resume ({{expected: 0 but was: 1}}), and the control passes. Not run on a cluster with real datanodes.

h3. Patch
[^CR-12-keep-queued-recommission-when-resuming-admin.patch], against b0aa6475b7856d2c7f467dd3db12b9a1dd116f63. It also applies to master at 20d95ed37fe.

{{DatanodeAdminMonitor}} gets {{isStopMonitoringQueued}}, which reports under the monitor lock whether a request queued by {{stopMonitoring}} is still waiting, and {{continueAdminForNode}} returns early when it is. A node with a queued recommission is one the monitor still knows about and is about to put back IN_SERVICE, so there is nothing to resume for it. The queue check and the application of the recommission both run under the monitor lock ({{isStopMonitoringQueued}} is synchronized, {{processCancelledNodes}} runs inside the synchronized block of {{run()}}), so the check cannot find an empty queue while the op state is still the admin state; and if the monitor applies the recommission between the check and the status read, that read returns IN_SERVICE and nothing is tracked again. No op state is written on this path, {{recommission}}, {{startMonitoring}} and {{stopMonitoring}} are unchanged, and a node with no queued recommission is tracked again exactly as before.

Op state and the monitor queues live in the leader SCM's memory and are not in the Ratis log, so a real failover to another SCM process loses a queued recommission before and after this patch. The case this patch covers is a step down and re-election of the same process, where the queue survives.

HDDS-17220 fixes the other consumer of the same queued state ({{startDecommission}} and {{startMaintenance}} take a "Nothing to do" branch while a recommission is queued). Both fixes are needed and they compose: that patch withdraws the queued recommission before it reads the node status, after which the guard added here is a no op, and its re-queue on a rejected request is honoured by the guard. The two patches insert their new monitor method at the same place in {{DatanodeAdminMonitor}} and {{DatanodeAdminMonitorImpl}}, so whichever lands second needs that one hunk moved; keeping both methods is the whole resolution.

Covered by {{testQueuedRecommissionSurvivesContinueAdmin}} in the existing {{TestNodeDecommissionManager}}: a decommissioning, an entering maintenance and an in maintenance node are recommissioned, both resume entry points are called before the next monitor run, and after that run all three have to be IN_SERVICE and untracked while a fourth node that is DECOMMISSIONING with no queued recommission has to be tracked. Without the change it fails with {{expected: <IN_SERVICE> but was: <DECOMMISSIONING>}}. With it {{TestNodeDecommissionManager}}, {{TestDatanodeAdminMonitor}}, {{TestNodeDecommissionMetrics}}, {{TestSCMNodeManager}}, {{TestNodeStateManager}} and {{TestDeadNodeHandler}} (128 tests) pass, {{TestDecommissionAndMaintenance}} (9 tests, mini cluster) passes, and checkstyle is clean.

Found by code review of the SCM datanode decommission and maintenance admin paths, as part of the TLA+ verification effort under HDDS-15926, on commit b0aa6475b7856d2c7f467dd3db12b9a1dd116f63. Checked against HDDS issues and apache/ozone pull requests for duplicates before filing. The attached patch is a proposal for review. Generated with Specula (Claude Opus 5.5 and Claude Opus 5).



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org