Siyao Meng created HDDS-17067:
---------------------------------
Summary: 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
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]
