[
https://issues.apache.org/jira/browse/HDDS-17518?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17518:
------------------------------
Attachment: TestBugCR3StaleNodeEventClosesHealthyNodePipeline.java
> 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
> Affects Versions: 0.4.0, 0.4.1, 0.5.0, 1.0.0, 1.1.0, 1.2.0, 1.3.0, 1.2.1,
> 1.4.0, 2.0.0, 1.4.1, 2.1.0, 2.2.0, 2.1.1, 2.1.2, 2.2.1
> Reporter: Siyao Meng
> Priority: Minor
> 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]
