[jira] [Updated] (HDDS-17219) Decommission monitor on a stepped down SCM can overwrite the operational state reported by a datanode, so a later re-promotion drops the decommission

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

     [ https://issues.apache.org/jira/browse/HDDS-17219?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Siyao Meng updated HDDS-17219:
------------------------------
    Attachment: TestBugMC7StaleFollowerMonitor.java

> Decommission monitor on a stepped down SCM can overwrite the operational state reported by a datanode, so a later re-promotion drops the decommission
> -----------------------------------------------------------------------------------------------------------------------------------------------------
>
>                 Key: HDDS-17219
>                 URL: https://issues.apache.org/jira/browse/HDDS-17219
>             Project: Apache Ozone
>          Issue Type: Bug
>            Reporter: Siyao Meng
>            Priority: Minor
>         Attachments: MC-7-skip-admin-monitor-op-state-writes-on-follower-scm.patch, TestBugMC7StaleFollowerMonitor.java
>
>
> h3. Mechanism
> The operational state of a datanode is not replicated through Ratis. The leader SCM keeps it in memory and, when a heartbeat reports a different persisted state, sends {{SetNodeOperationalStateCommand}} to the datanode. A follower SCM copies the persisted state from each heartbeat into its own memory ({{SCMNodeManager.updateDatanodeOpState}}).
> {{DatanodeAdminMonitorImpl}} runs on every SCM, and after a step down it keeps the nodes it was tracking as leader. In {{processTransitioningNodes}} it reads the node status, decides, and writes the new state later through {{setNodeOpState}}, without checking that the SCM is still leader or that the state is still the one it read. On a follower, when the read returns IN_SERVICE (the datanode has not applied the decommission yet), the monitor aborts the workflow and writes IN_SERVICE. If a heartbeat reporting DECOMMISSIONING is applied between that read and the write, the write replaces DECOMMISSIONING with IN_SERVICE and the node is no longer tracked.
> If this SCM becomes leader again before the next heartbeat from that datanode, {{NodeDecommissionManager.onBecomeLeader}} ({{continueAdminForNode}}) sees IN_SERVICE and does not resume the workflow, and the next heartbeat makes it send IN_SERVICE to the datanode, which applies it. The decommission was accepted and persisted by the datanode, and is now dropped.
> h3. Trigger
> # SCM A, as leader, accepts a decommission, sets DECOMMISSIONING and sends the command to the datanode; its monitor starts tracking the node.
> # A steps down before the datanode reports DECOMMISSIONING. A heartbeat still reporting IN_SERVICE is copied on A. This also needs the datanode to hold the command past its next heartbeat to A: it normally applies a command about a second after the heartbeat round that delivered it, so it has to be delayed (for example a busy command thread, or an early heartbeat round triggered by other events).
> # A monitor tick on A reads IN_SERVICE. The datanode has meanwhile applied the command, and its heartbeat reporting DECOMMISSIONING is applied on A before the tick writes IN_SERVICE.
> # A becomes leader again before the datanode's next heartbeat reaches it.
> The interim leader must not process a heartbeat reporting DECOMMISSIONING while it is leader; otherwise it sends IN_SERVICE itself and the decommission is lost anyway. HDDS-5090, which added the follower copy and {{onBecomeLeader}}, documents and accepts that loss for a failover before the state reaches the followers; here the state has already reached the datanode and this SCM, which is what {{onBecomeLeader}} was added to preserve. HDDS-3204 (a new leader starts from its own view) is related. The datanode must also take the command off its queue before any heartbeat response from a leader of a newer term, because that response carries the new term and {{StateContext.getNextCommand}} then drops queued commands of an older term; in practice no newer leader may answer the datanode in that heartbeat round, for example after a failed election that A wins again (code reading). The read to write window is short (a log line and an in-memory update) per tracked node and monitor tick, and A has to lose and regain leadership within about one heartbeat interval (30s by default). This is rare.
> h3. Impact
> An accepted decommission is silently undone: the datanode returns to IN_SERVICE and the operator has to issue the decommission again. No data is lost. The other writes of the monitor on a follower (DECOMMISSIONED, IN_MAINTENANCE, IN_SERVICE at the end of maintenance, for a dead node or on recommission) do not need the heartbeat race; they are only sent if the SCM is re-promoted before the next heartbeat, and each is the decision the leader would make on its next tick anyway. Only the abort write racing a heartbeat has a harmful effect (code reading, not run).
> h3. Reproduction
> PASS, deterministic, unmodified source. [^TestBugMC7StaleFollowerMonitor.java] ({{hadoop-hdds/server-scm}}) uses the real {{SCMNodeManager}} (from {{HddsTestUtils.getScm}}), {{NodeDecommissionManager}}, {{DatanodeAdminMonitorImpl}} and {{SCMContext}}. Leadership is changed with {{SCMContext.updateLeaderAndTerm}} and {{onBecomeLeader}}, heartbeats are {{processHeartbeat}} calls carrying the persisted state a datanode would report, and there is no datanode process. The monitor sees the node manager through a Mockito spy that, once, runs a heartbeat to completion on another thread right after the monitor's status read; that is the only timing help. The first test shows the state overwritten to IN_SERVICE and the re-promoted SCM sending IN_SERVICE to the datanode. Two controls show the correct outcome (the decommission resumes and completes) when the heartbeat arrives after the tick, or when one more heartbeat arrives before re-promotion. The interim leader is not modelled. A passing test means the defect is present.
> h3. Patch
> [^MC-7-skip-admin-monitor-op-state-writes-on-follower-scm.patch], against b0aa6475b7856d2c7f467dd3db12b9a1dd116f63. It applies to master at 20d95ed37fe with a three way merge ({{git apply -3}}), because the lines next to the monitor construction in {{NodeDecommissionManager}} changed.
> {{DatanodeAdminMonitorImpl}} now gets the {{SCMContext}} and {{setNodeOpState}}, the single place where the monitor writes the operational state, returns without writing when {{SCMContext.isLeader()}} is false, as {{continueAdminForNode}} already does. On a follower the state then always comes from the datanode heartbeats, the monitor still stops tracking nodes as before, and {{onBecomeLeader}} resumes the workflow from the reported state. Behaviour on the leader and without SCM HA is unchanged. The check is at the write, not at the start of the tick, so it also covers a tick that is running when leadership is lost. One follower side change: an admin request that the monitor had not yet applied when the SCM stepped down (for example a recommission still queued for its next tick) is now dropped on that SCM as well, as it already is on any other new leader (HDDS-5090); the operator repeats it. Before the patch the follower wrote IN_SERVICE locally, and a re-promotion before the next heartbeat pushed it to the datanode.
> Covered by {{testFollowerDoesNotOverwriteOpStateReportedByHeartbeat}} in the existing {{TestDatanodeAdminMonitor}}, which delivers the heartbeat right after the monitor's status read on a follower and then checks that the workflow resumes and completes after re-promotion. Without the leader check it fails with IN_SERVICE instead of DECOMMISSIONING. With it {{TestDatanodeAdminMonitor}}, {{TestNodeDecommissionManager}}, {{TestNodeDecommissionMetrics}} and {{TestSCMNodeManager}} pass, the mini cluster {{TestDecommissionAndMaintenance}} passes (9 tests, three of them, the SCM restart cases, on a rerun after a port clash on the shared test machine; it runs a single SCM, so it covers the leader path only), and checkstyle is clean. With the patch the first test of the reproduction fails because the state stays DECOMMISSIONING, and the two controls still pass.
> Found by TLA+ model checking and code review of the SCM datanode decommission and maintenance admin paths 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).



--
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