Siyao Meng created HDDS-17518:
---------------------------------
Summary: StaleNodeHandler closes pipelines of a datanode that is
HEALTHY again when it handles a late STALE_NODE event
Key: HDDS-17518
URL: https://issues.apache.org/jira/browse/HDDS-17518
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments: CR-3-skip-stale-node-event-for-healthy-node.patch,
TestBugCR3StaleNodeEventClosesHealthyNodePipeline.java
h3. Mechanism
{{StaleNodeHandler.onMessage}} closes every pipeline that
{{NodeManager.getPipelines}} returns when the handler runs. It does not look at
the current health of the datanode. SCM's {{EventQueue}} gives each handler of
each event type its own single thread executor, so a STALE_NODE event can be
handled after the UNHEALTHY_TO_HEALTHY_NODE event that followed it. By then the
datanode is HEALTHY, {{BackgroundPipelineCreator}} may have placed a new
pipeline on it, and the handler closes that pipeline too.
{{DeadNodeHandler}} has the same kind of late event and skips a datanode that
is no longer DEAD (HDDS-14834). {{StaleNodeHandler}} has no such check.
HDDS-14950 (revisit SCM NodeManager concurrency) is related.
h3. Trigger
The STALE_NODE event is handled after the datanode has been restored to HEALTHY
and a pipeline has been placed on it. Two ways to get there:
# Several datanodes go STALE together, for example in a rack or switch outage.
The STALE_NODE events are handled one by one (the handler has a single thread,
and each {{closePipeline}} closes the open containers of the pipeline and
updates the pipeline through SCM Ratis). The events of the other datanodes
close the pipelines this datanode shared with them. The datanode heartbeats
again and the next health check restores it while its own event is still
queued, and {{BackgroundPipelineCreator}} places a new pipeline on it. The new
pipeline needs room below {{ozone.scm.datanode.pipeline.limit}}: pipelines that
are not closed yet count against it, so a datanode whose old pipelines are all
still open gets no new one, and the late event then only closes the old ones.
# Maintenance ends or the datanode is recommissioned while it is STALE and its
heartbeat has just come back. {{NodeStateManager.setNodeOperationalState}}
reads the health, changes the operational state and then fires the event for
the health it read. If the health check restores the datanode in between,
STALE_NODE is fired after UNHEALTHY_TO_HEALTHY_NODE for a datanode that is
already HEALTHY and IN_SERVICE. The window is a few instructions, and the event
does harm only if the handler is busy long enough for a pipeline to be placed
first, as in the first case.
Both were run at unit level with the event delivery order chosen by the test.
The time the handler can lag on a real cluster was not measured.
h3. Impact
A pipeline of a HEALTHY, IN_SERVICE datanode is closed, together with the
containers open on it. Writes on it fail over to other pipelines, and
{{BackgroundPipelineCreator}} creates replacements, so no data is lost. When no
new pipeline was placed on the datanode the late event only closes pipelines
that were already closed or that belonged to the stale period.
h3. Reproduction
PASS, deterministic, unmodified source.
[^TestBugCR3StaleNodeEventClosesHealthyNodePipeline.java]
({{hadoop-hdds/server-scm}}) uses a real {{SCMNodeManager}},
{{NodeStateManager}} and {{StaleNodeHandler}} with a mock {{PipelineManager}}
that records closes. The scheduled health check is stopped and the test calls
{{NodeStateManager.checkNodesHealth}} itself, setting heartbeat times with
{{DatanodeInfo.updateLastHeartbeatTime}}. Fired events are recorded and
STALE_NODE is delivered to the handler by the test after the recovery. A new
pipeline is added through {{NodeManager.addPipeline}}, the call
{{PipelineManagerImpl.createPipeline}} makes.
* {{staleEventHandledAfterRecovery}}: events {{[Stale_Node,
UNHEALTHY_TO_HEALTHY_NODE]}}, the pipeline created after the recovery is closed.
* {{maintenanceEndRacingRecovery}}: the health check runs inside
{{setNodeOperationalState}} (through a spy on the node state map, before the
operational state update). Events {{[UNHEALTHY_TO_HEALTHY_NODE, Stale_Node]}},
status {{IN_SERVICE(no expiry)-HEALTHY}}, the new pipeline is closed.
A passing test means the defect is present. With the patch both tests fail at
the last assertion because nothing is closed.
h3. Suggested fix
Firing the health returned by the operational state update in
{{setNodeOperationalState}} is not enough on its own: if the update lands just
before the restore, STALE_NODE is still fired for a datanode that is HEALTHY
right after, and it can still be handled late. The check belongs in the
handler, which covers both triggers.
h3. Patch
[^CR-3-skip-stale-node-event-for-healthy-node.patch], against
6752db7978761a749a7d42a72a85311818e3d870. It also applies to master at
72f2d1486f3.
{{StaleNodeHandler.onMessage}} reads the current status of the datanode after
reading its pipelines and returns if the datanode is HEALTHY. Reading it after
the pipelines means a pipeline placed on the recovered datanode is closed only
if the datanode is stale again. STALE and DEAD datanodes are handled as before.
A datanode that is HEALTHY again before its event is handled now also keeps the
pipelines it had before it went stale; if their Ratis group is broken, the
datanodes report it to SCM through pipeline actions as usual. Every transition
into STALE fires its own event, so a datanode that goes STALE again still has
its pipelines closed. {{ReconStaleNodeHandler}} inherits the check.
Covered by {{testStaleNodeHandlerSkipsNodeThatIsNoLongerStale}} in the existing
{{TestDeadNodeHandler}}, next to the matching {{DeadNodeHandler}} test: no
close for a HEALTHY datanode, a close for a STALE one. Without the change it
fails because {{closePipeline}} is called for the HEALTHY datanode. With it
{{TestDeadNodeHandler}}, {{TestSCMNodeManager}}, {{TestNodeStateManager}},
{{TestDatanodeAdminMonitor}}, {{TestNodeDecommissionManager}} and
{{TestContainerPlacement}} (125 tests) pass and checkstyle is clean. Mini
cluster suites were not run.
Found by code review of the SCM datanode health checker and node state paths,
as part of the TLA+ verification effort under HDDS-15926, on commit
6752db7978761a749a7d42a72a85311818e3d870. Checked against HDDS issues and
apache/ozone pull requests for duplicates before filing (HDDS-17313 is a
different event on the same {{setNodeOperationalState}} code and is not fixed
by this patch). The attached patch is a proposal for review. Generated with
Specula (Claude Opus 5.5).
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
