[
https://issues.apache.org/jira/browse/HDDS-17062?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17062:
------------------------------
Attachment: TestBugMC1StalePutBackIcr.java
> ICRs put back after a failed datanode heartbeat are sent after the newer full
> container report that replaced them
> -----------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17062
> URL: https://issues.apache.org/jira/browse/HDDS-17062
> Project: Apache Ozone
> Issue Type: Bug
> Reporter: Siyao Meng
> Priority: Minor
> Attachments:
> MC-1-drop-put-back-icrs-covered-by-newer-full-report.patch,
> TestBugMC1StalePutBackIcr.java
>
>
> h3. Mechanism
> {{StateContext.getFullContainerReportDiscardPendingICR}} drops the ICRs still
> queued for each endpoint before it builds a new full container report (FCR),
> so that no ICR older than the FCR is sent after it (HDDS-5267). It cannot see
> the ICRs that a heartbeat in flight has already taken from the queue. If that
> heartbeat fails with an {{IOException}},
> {{HeartbeatEndpointTask.putBackIncrementalReports}} returns those ICRs to the
> head of the queue through {{StateContext.putBackReports}}. The next heartbeat
> then carries the new FCR followed by the older ICRs, and Recon and SCM apply
> the ICRs on top of the FCR.
> h3. Trigger
> # A container changes state on the datanode and its ICR is queued (for
> example the container is marked UNHEALTHY, or deleted).
> # A heartbeat takes the ICR and its RPC is in flight.
> # While the RPC is in flight, the datanode builds a new FCR (the periodic
> container report, or a volume failure) from a container state that already
> includes later changes (the container was then deleted, or recreated by
> replication).
> # The RPC fails (an RPC timeout or a short network failure) and the ICR is
> put back.
> # The next heartbeat sends the FCR, then the stale ICR.
> At Recon, an outage of 3 heartbeat intervals or more (180s by default) makes
> Recon ask the datanode to re-register, and the register request carries a
> fresh container report, which repairs the state seconds later. So at Recon
> the trigger is a single failed heartbeat. SCM has no such gap check (read,
> not run).
> h3. Impact
> * A stale UNHEALTHY or CLOSED ICR re-adds a replica that the FCR has just
> removed (the datanode no longer has the container), and a stale DELETED ICR
> removes a replica that the FCR has just reported (the datanode has it again).
> Both were run against Recon.
> * The wrong replica stays until the next FCR from that datanode, which comes
> 60 to 120 minutes after the previous one ({{hdds.container.report.interval}}
> plus a random delay of up to the same interval).
> * SCM processes the same reports with the same handler code, so the same
> phantom or missing replica appears there for the same time. That can hold a
> DELETING container in DELETING, or skew the replica counts used by
> ReplicationManager (argued from the code, not run). I found no realistic
> sequence in which this makes SCM move a DELETING or DELETED container back:
> SCM is in DELETING only after it took this datanode's replica as empty or
> gone, which only the FCR in the same heartbeat can tell it, so
> ReplicationManager would have to run between the FCR and the stale ICR
> handlers (argued from the code, not run).
> h3. Reproduction
> PASS, deterministic, unmodified source. [^TestBugMC1StalePutBackIcr.java]
> ({{hadoop-ozone/recon}}) runs the real datanode {{StateContext}},
> {{HeartbeatEndpointTask}} and {{ContainerSet}} against a stub RPC proxy that
> feeds the real {{SCMDatanodeHeartbeatDispatcher}},
> {{ReconContainerReportQueue}} and Recon FCR and ICR handlers. The stub fails
> one heartbeat with a {{SocketTimeoutException}}, and runs the concurrent
> datanode work (container state change and FCR build) on a separate thread
> while that RPC is in flight. The first test shows Recon keeping an UNHEALTHY
> replica of a container the datanode no longer has, across further heartbeats,
> until the next periodic FCR. The second shows Recon dropping a CLOSED replica
> that the datanode still holds. A passing test means the defect is present.
> h3. Patch
> [^MC-1-drop-put-back-icrs-covered-by-newer-full-report.patch], against
> 916aa1d9949cbb10f2750a40c8704340893131a2. It also applies to master at
> 62d982ff2d9. The open pull request for HDDS-16723 (apache/ozone#11423) also
> changes {{StateContext}}; the main code of this patch applies cleanly on its
> head commit.
> {{StateContext}} remembers, under the lock of the incremental report queue,
> the endpoints whose ICRs taken by the heartbeat in flight are older than the
> latest FCR: {{getFullContainerReportDiscardPendingICR}} marks every endpoint
> when it drops the queued ICRs, {{getIncrementalReports}} clears the mark when
> a heartbeat takes the endpoint's ICRs, and {{putBackReports}} leaves out ICRs
> (but still puts back command status reports) while the endpoint is marked.
> Heartbeats to one endpoint are serialized, so the mark means that an FCR was
> built after the failed heartbeat took its ICRs, and that FCR already covers
> them. Nothing changes when no FCR is built while a heartbeat fails. Checking
> the FCR ready flag instead would be racy, because {{refreshFullReport}} sets
> it after the drop, outside that lock.
> Covered by
> {{testFailedHeartbeatDropsIncrementalReportsCoveredByNewerFullReport}} in the
> existing {{TestHeartbeatEndpointTask}}: two failed heartbeats, the second
> with an FCR rebuild and a newer ICR during the RPC; the third heartbeat must
> carry the FCR, only the newer ICR, and the command status report. Without the
> change it fails because the older ICR is also sent. With it
> {{TestHeartbeatEndpointTask}}, {{TestStateContext}}, {{TestContainerSet}},
> {{TestReportPublisher}}, {{TestVolumeSetDiskChecks}}, {{TestOzoneContainer}},
> {{TestDatanodeStateMachine}}, {{TestRunningDatanodeState}},
> {{TestHddsDispatcher}} and {{TestKeyValueHandler}} (165 tests) and
> {{TestEndPoint}} in server-scm (18 tests) pass, checkstyle is clean. With the
> patch both tests of the reproduction fail, because the stale ICR is no longer
> sent.
> 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. 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]
