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