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: [email protected]
For additional commands, e-mail: [email protected]
