Siyao Meng created HDDS-17221:
---------------------------------
Summary: Starting maintenance again on a datanode already in
maintenance reports success but keeps the old maintenance end
Key: HDDS-17221
URL: https://issues.apache.org/jira/browse/HDDS-17221
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments: CR-8-update-maintenance-end-on-repeated-maintenance.patch
h3. Mechanism
{{NodeDecommissionManager.startMaintenance}} computes the end from {{--end}},
but for a node in ENTERING_MAINTENANCE or IN_MAINTENANCE it only logs "Nothing
to do" and returns. The new end is dropped and no {{DatanodeAdminError}} is
added, so {{startMaintenanceNodes}} returns an empty error list. With
{{--force}} omitted the result is the same: {{checkIfMaintenancePossible}} only
leaves the node out of its capacity check, and the node still reaches the same
branch. The method carries a TODO from the first version of the feature: "If
startMaintenance is called on a host already in maintenance, then we should
update the end time?".
The monitor reads the end from the node status ({{operationalStateExpired}})
and ends maintenance at the old end. {{putIntoMaintenance}} carries the old end
over to IN_MAINTENANCE.
h3. Trigger
# {{ozone admin datanode maintenance --end=1 <dn>}}.
# Before that hour has passed, {{ozone admin datanode maintenance --end=100
<dn>}} to extend, with or without {{--force}}.
# At the first end the monitor puts the node back to IN_SERVICE.
No document or test describes what a repeated request should do.
{{testNodesCanBePutIntoMaintenanceAndRecommissioned}} repeats the request with
the same end and only checks that no error is raised.
h3. Impact
* The extension is acknowledged ("Entering maintenance mode on datanode(s)"
with no errors) but ignored. Maintenance ends at the end of the first request,
and the operator is not told.
* If the node is still down at that time it becomes a normal dead node and its
containers are re-replicated, which is what maintenance is meant to avoid
(argued from code and docs, not run). No data is at risk.
* The workaround, recommission followed by a new maintenance request, loses the
new request if both are sent within one monitor interval, which is HDDS-17220.
With this patch that case keeps the node in maintenance with the new end,
because the branch now puts the node back into the monitor and so takes it out
of the cancelled queue.
h3. Reproduction
PASS on the unmodified source, deterministic.
[^TestBugCR8MaintenanceEndNotUpdated.java] ({{hadoop-hdds/server-scm}}) uses
the setup of {{TestNodeDecommissionManager}} (with a mocked
{{ReplicationManager}}, so it also runs on master) and calls
{{startMaintenanceNodes}}, the method {{SCMClientProtocolServer}} calls. One
test uses only that call. The other shows the effect without waiting an hour:
it moves the end left by the first request to 2 seconds from now with
{{setNodeOperationalState}}, requests 100 hours, and runs the monitor after the
old end. Output on the pin:
{noformat}
first window shortened to 2s: state=ENTERING_MAINTENANCE end=1792555380 (2s
from now)
maintenance --end=100 --force: state=ENTERING_MAINTENANCE end=1792555380 (2s
from now)
monitor run after the old end: state=IN_SERVICE end=0 (none)
maintenance --end=1: state=ENTERING_MAINTENANCE end=1792558982 (3600s from now)
maintenance --end=100 --force: state=ENTERING_MAINTENANCE end=1792558982 (3600s
from now)
maintenance --end=100: state=ENTERING_MAINTENANCE end=1792558982 (3600s from
now)
{noformat}
A passing test means the defect is present. With the patch both tests fail: the
end becomes 100 hours away and the node stays in maintenance.
h3. Patch
[^CR-8-update-maintenance-end-on-repeated-maintenance.patch], against
b0aa6475b7856d2c7f467dd3db12b9a1dd116f63. It also applies to master at
20d95ed37fe.
For a node already in maintenance, a request with {{--end}} now updates the end
and nothing else. The new {{NodeManager.setMaintenanceEnd}}
({{NodeStateMap.updateMaintenanceEnd}} in {{SCMNodeManager}}) checks under the
node state write lock that the node is still in a maintenance state and sets
the end, keeping its current op state, so a monitor transition at the same time
is not undone. If the node has left maintenance in the meantime, the request
returns an error. A request without {{--end}} keeps the current end: the server
cannot tell an omitted {{--end}} from {{--end=0}}, and clearing an end by
accident would leave the node in maintenance indefinitely. The node is then
passed to {{monitor.startMonitoring}}, as {{continueAdminForNode}} does for a
node in maintenance, so a node the monitor has just released is tracked again.
The heartbeat reconciliation in {{SCMNodeManager}} already compares the end
with the datanode's persisted value and sends it to the datanode. The new end
reaches the datanode and the follower SCMs with the next heartbeat; until then
a leader change keeps the old end. The TODO is removed. HDFS
{{DatanodeAdminManager.startMaintenance}} also updates the expiry of a node
already in maintenance.
The monitor's own write when a node moves from ENTERING_MAINTENANCE to
IN_MAINTENANCE ({{DatanodeAdminMonitorImpl.setNodeOpState}}) read the end and
wrote it back as two steps, which would have dropped an end stored in between,
so it now goes through a new {{NodeStateMap}} method that writes the state and
keeps the current end in one step under the same lock. Without that, "no error"
would still not mean that the new end is in effect.
This patch and the patch of HDDS-17220 change the same lines of
{{startMaintenance}}, so whichever lands second needs a rebase: a plain {{git
apply}} of either one on top of the other fails on the hunk that removes the
TODO, while {{git apply -3}} and a merge of the two apply cleanly. The changes
themselves are independent, HDDS-17220 withdraws a queued recommission for
every decommission or maintenance request and writes no end, this one updates
the end of a node already in maintenance. With both applied, the maintenance
branch puts the node back into the monitor twice, which is harmless because the
tracked set is keyed on the datanode.
Covered by {{testMaintenanceEndIsUpdatedForNodeInMaintenance}} in the existing
{{TestNodeDecommissionManager}} (ENTERING_MAINTENANCE and an untracked
IN_MAINTENANCE node, with and without {{--force}}, both tracked and in the same
state after a monitor run, a request without {{--end}} keeping the end, and no
end set on an IN_SERVICE node), and by
{{testNodeOperationalStateCanBeUpdatedKeepingTheEnd}} in {{TestNodeStateMap}}
for the write the monitor now uses. Without the change the first test fails
because the end stays 1 hour away. With it {{TestNodeDecommissionManager}},
{{TestDatanodeAdminMonitor}}, {{TestNodeDecommissionMetrics}},
{{TestNodeStateManager}}, {{TestNodeStateMap}} and {{TestSCMNodeManager}} (134
tests) pass, {{TestDecommissionAndMaintenance}} (9 tests, mini cluster) passes
with this patch alone, 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]
