[
https://issues.apache.org/jira/browse/HDDS-17221?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17221:
------------------------------
Attachment: TestBugCR8MaintenanceEndNotUpdated.java
> 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
> Priority: Minor
> Attachments:
> CR-8-update-maintenance-end-on-repeated-maintenance.patch,
> TestBugCR8MaintenanceEndNotUpdated.java
>
>
> 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]
