[
https://issues.apache.org/jira/browse/HDDS-17226?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17226:
------------------------------
Attachment: TestBugCR12ResumedAdminDropsQueuedRecommission.java
> 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
> Priority: Major
> 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: [email protected]
For additional commands, e-mail: [email protected]
