[
https://issues.apache.org/jira/browse/HDDS-17067?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17067:
------------------------------
Attachment: TestBugMC5DeadHandlerErasesReturnedNode.java
> Registration does not refresh a known datanode's heartbeat time, so the dead
> node handler keeps or erases its replicas wrongly
> ------------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17067
> URL: https://issues.apache.org/jira/browse/HDDS-17067
> Project: Apache Ozone
> Issue Type: Bug
> Reporter: Siyao Meng
> Priority: Major
> Attachments:
> MC-4-refresh-heartbeat-on-register-and-recheck-dead-node-before-replica-removal.patch,
> TestBugMC4DeadNodeRegisterReport.java,
> TestBugMC5DeadHandlerErasesReturnedNode.java
>
>
> h3. Mechanism
> {{SCMNodeManager.register}} refreshes nothing for a datanode it already
> knows. A new node gets a fresh heartbeat time from the {{DatanodeInfo}}
> constructor, and a node that registers with a changed address, port set or
> version gets one from {{updateNode}}, but a plain re-registration leaves the
> old heartbeat time in place. Health is computed from that time by
> {{NodeStateManager.checkNodesHealth}}, so a registered node keeps the health
> of its last heartbeat before the outage until its next heartbeat is
> processed. {{SCMDatanodeProtocolServer.register}} then fires the registration
> report, and {{ContainerReportHandler}} applies it whatever the node's health
> is.
> {{DeadNodeHandler.onMessage}} reads the node status once when it starts, then
> removes every replica of the node ({{removeContainerReplicas}}) after closing
> its containers and destroying its pipelines, without reading the health again
> and without the {{DatanodeInfo}} monitor that {{ContainerReportHandler}} and
> {{IncrementalContainerReportHandler}} hold while they apply a report.
> DEAD_NODE and CONTAINER_REPORT are handled on different threads. DEAD_NODE
> fires only on the transition into DEAD (and again when the operational state
> of a DEAD node changes).
> Recon uses the same code ({{ReconNodeManager.register}} delegates to it,
> {{ReconDeadNodeHandler}} extends {{DeadNodeHandler}}). In Recon the window is
> wider: when a heartbeat arrives after a gap of at least 3 x
> {{hdds.heartbeat.recon.interval}}, {{ReconNodeManager.processHeartbeat}}
> answers with {{ReregisterCommand}} and updates only its own heartbeat map,
> not the heartbeat time of {{NodeStateManager}}. The datanode registers again
> at once and sends its next heartbeat one {{hdds.heartbeat.interval}} (30s)
> later, so in Recon a returning node keeps its old health for about 30s after
> it has registered.
> This gives three wrong outcomes:
> * A DEAD node registers, its report re-adds its replicas, and it stops again
> before its next heartbeat. It never leaves DEAD, DEAD_NODE does not fire
> again, and the replicas stay.
> * A node comes back while STALE, shortly before its dead deadline, and
> registers. The deadline still applies to its old heartbeat time, so it is
> declared DEAD after its registration report has been applied, and
> {{DeadNodeHandler}} removes the replicas of a live node.
> * A node comes back while {{DeadNodeHandler}} for it is still running (or
> queued). The handler removes the replicas of the registration report that was
> applied in the meantime.
> h3. Trigger
> A datanode that restarts, or loses its network, for about the dead interval
> ({{ozone.scm.dead.node.interval}}, 10 minutes by default). For the first
> outcome it must then stop again within about 30s of registering with Recon
> (about 2s with SCM, the time from registration to the first heartbeat,
> {{hdds.heartbeat.initial-interval}}). For the second it must come back within
> about 30s before Recon's dead deadline. For the third the DEAD_NODE handling
> must overlap the registration. No client action is involved.
> h3. Impact
> Reproduced in Recon with real node manager timers and handlers (see
> Reproduction); the SCM side is argued from the shared code, not run.
> * *Recon reports a container HEALTHY that has no live replica* (first
> outcome): after the node stopped again and was DEAD, the replica on it was
> still in Recon's container manager and {{ReconReplicationManager}} reported
> the container HEALTHY instead of MISSING. It stays that way while the node is
> down. [^TestBugMC4DeadNodeRegisterReport.java]
> * *Recon reports a container MISSING on a HEALTHY datanode* (second and third
> outcomes): the replica is removed after the registration report, the node is
> HEALTHY again at its next heartbeat, and the container stays MISSING until
> the node's next full container report. After a restart that is
> {{hdds.container.report.interval}} plus a random delay of up to the same
> interval, 60 to 120 minutes by default; an incremental report restores single
> containers earlier. [^TestBugMC5DeadHandlerErasesReturnedNode.java]
> * *SCM (argued, not run):* {{RatisContainerReplicaCount}} does not look at
> node health, so a datanode that stops within about 2s of registering after a
> restart would keep its replicas on a DEAD node, and {{ReplicationManager}}
> would not re-replicate them for as long as the node stays down. The 2s is
> only the trigger window. A replica removed from a live node would cause
> unnecessary replication until the next full report.
> h3. Reproduction
> Both attached classes go in {{hadoop-ozone/recon}} (package
> {{org.apache.hadoop.ozone.recon.scm}}, Maven command in the class comment)
> and pass on unmodified source when the bug is present. They run a real
> {{ReconNodeManager}} with short stale and dead intervals,
> {{ReconDeadNodeHandler}}, {{ReconContainerReportHandler}} and
> {{ReconReplicationManager.processAll}}, and drive the datanode side through
> {{register}} and {{processHeartbeat}}. {{TestBugMC4DeadNodeRegisterReport}}
> and {{returnsShortlyBeforeDeadDeadline}} in
> {{TestBugMC5DeadHandlerErasesReturnedNode}} use no hooks.
> {{comesBackWhileDeadHandlerRuns}} holds the dead node handler at its first
> container lookup until the node has come back, which only changes timing.
> h3. Patch
> [^MC-4-refresh-heartbeat-on-register-and-recheck-dead-node-before-replica-removal.patch],
> against 916aa1d9949cbb10f2750a40c8704340893131a2. It also applies to master
> at 14e0611fdd5.
> Two parts, both needed:
> * {{SCMNodeManager.register}} calls {{DatanodeInfo.updateLastHeartbeatTime}}
> for an already registered node. A registered node then leaves STALE or DEAD
> at the next health check, and if it stops again it goes through DEAD again
> and its replicas are removed. This closes the first and second outcomes.
> {{ReconNodeManager.loadExistingNodes}} no longer registers nodes it already
> knows again when an SCM DB snapshot sync reloads its node table, so the
> reload does not bring dead nodes back. The table holds what those nodes
> registered with, so the reload had nothing to apply to them.
> * {{DeadNodeHandler.removeContainerReplicas}} takes the {{DatanodeInfo}}
> monitor that the report handlers hold, checks under it that the node is still
> DEAD and only then removes the replicas; otherwise it logs that it keeps
> them. This closes the third outcome.
> Applying the registration report was kept on purpose: it is the only full
> report a returning datanode sends for 60 to 120 minutes, so skipping reports
> from DEAD nodes would lose replicas instead. Behaviour change in SCM and
> Recon: a known datanode that registers with the same address, ports and
> version now leaves STALE or DEAD at the next health check (3s) instead of
> after its first heartbeat, through the usual transition and its events. New
> nodes, and registrations with a changed address, ports or version, already
> get a fresh heartbeat time at registration, and fenced registrations are
> rejected before the refresh. Not covered: a dead node handler that rechecks
> in the seconds between the registration and that health check, and a
> registration report that waits in the report queue for longer than the dead
> interval.
> Covered by {{testRegisterMovesDeadNodeOutOfDead}} in {{TestSCMNodeManager}}
> (DEAD, HEALTHY after registering again, DEAD again after it stops),
> {{testDeadNodeHandlerKeepsReplicasOfNodeThatCameBack}} in
> {{TestDeadNodeHandler}} and {{testReinitializeKeepsDeadNodeDead}} in
> {{TestReconNodeManager}}. Without the change the first two fail with
> "expected: HEALTHY but was: DEAD" and with a replica removal that should not
> happen; without the {{loadExistingNodes}} change the third fails with
> "expected: DEAD but was: HEALTHY". With it the {{hadoop-hdds/server-scm}}
> node, server and container tests pass (369 tests), as do the unit tests in
> the Recon {{scm}} and {{fsck}} packages and {{TestReconTasks}}, checkstyle is
> clean, and the attached reproductions no longer show the bug.
> Found by TLA+ model checking and code review of the datanode container report
> and Recon container tracking paths under HDDS-15926, on commit
> 916aa1d9949cbb10f2750a40c8704340893131a2. Checked against HDDS issues and
> apache/ozone pull requests for duplicates before filing. Related but not
> duplicates: HDDS-16633 (re-registration racing DEAD handling in the network
> topology), HDDS-16632 (container list lost on re-registration with a new
> address), HDDS-14834 and HDDS-16623 (resolved, network topology after DEAD in
> the same handler), HDDS-14950 (NodeManager concurrency in general). 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]
