[
https://issues.apache.org/jira/browse/HDDS-17432?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17432:
------------------------------
Attachment: TestBugMC2StreamPutBlockCommittedAfterClose.java
> Stream PutBlock that passed the open check can commit into a container after
> it is closed and its checksum and close report were taken
> --------------------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17432
> URL: https://issues.apache.org/jira/browse/HDDS-17432
> Project: Apache Ozone
> Issue Type: Bug
> Affects Versions: 2.3.0
> Reporter: Siyao Meng
> Priority: Minor
> Attachments:
> MC-2-recheck-container-state-under-lock-for-stream-putblock.patch,
> TestBugMC2StreamPutBlockCommittedAfterClose.java
>
>
> h3. Mechanism
> With {{ozone.client.datastream.putblock.without.raft.enabled}}, PutBlocks
> reach the datanode through the data stream instead of the Raft log: as a
> stream command in the middle of the stream
> ({{ContainerStateMachine.streamCommand}}) and when the stream closes
> ({{ContainerStateMachine.streamPutBlock}}). They are not ordered with the
> Raft-applied CloseContainer. {{KeyValueHandler.handlePutBlock}} calls
> {{checkContainerOpen}} (which accepts OPEN and CLOSING) and later commits the
> block through {{BlockManagerImpl.persistPutBlock}} without holding the
> container lock. {{KeyValueHandler.closeContainer}} holds the container write
> lock only while it syncs the DB and moves the replica to CLOSED; it then
> builds the data checksum from the block table and sends the close ICR.
> A stream PutBlock that passed the check before the close can therefore commit
> after all of that. The CLOSED replica gains a block row and its block count
> changes, while the close ICR and the stored data checksum describe the
> replica without it. For the mid stream PutBlock,
> {{KeyValueContainer.addToPendingPutBlockCache}} then fails because the close
> cleared the pending block set, and the handler returns
> {{CONTAINER_INTERNAL_ERROR}}. {{HddsDispatcher}} treats that as a replica
> failure on a container that is no longer open and throws
> {{IllegalStateException}}, so the stream command fails for a PutBlock that
> was committed. The stream close PutBlock gets {{SUCCESS}}.
> h3. Trigger
> # Datanodes with {{hdds.container.ratis.datastream.enabled=true}} and a
> client with {{ozone.client.datastream.putblock.without.raft.enabled=true}}
> (both default false) write through the data stream.
> # SCM closes the container while a stream PutBlock for it is between the open
> check and the commit on one replica.
> No fault is needed. The window is narrow (from the open check to the RocksDB
> batch commit). Without a timing hook a tight loop hit it rarely (1 in 3000
> attempts); the stale close report and checksum were only shown with a timing
> hook.
> h3. Impact
> * The close ICR reports a key count and data checksum that do not include the
> block, and the stale checksum is kept across a restart. The next full
> container report refreshes the key count and the data scanner rebuilds the
> checksum (every 7 days by default).
> * Mid stream PutBlock: if the other replicas rejected it, the late replica
> keeps a block row that no key references, because the client wrote the data
> again to a new block. Until the scanner runs the replicas report the same
> checksum; afterwards Recon reports a replica mismatch, and an admin reconcile
> copies the row to the peers. This is the same kind of leftover an abandoned
> write leaves through the Raft path. (Argued from the code, not run on a
> cluster.)
> * Stream close PutBlock: if the other replicas rejected it, the late replica
> is the only one that holds the block, and the client is told the write
> succeeded only because of HDDS-17431, which ignores the rejection on the
> other replicas.
> * I found no wrong result visible to a client or OM from this race alone.
> After the failed mid stream command the client excludes the pipeline and
> writes the data to a new block.
> * PutBlocks over gRPC (EC) can race a direct close from
> {{CloseContainerCommandHandler}} in the same way, with the same transient
> effect on the close ICR and checksum. This patch does not change that path.
> (Argued from the code, not run.)
> h3. Reproduction
> PASS, deterministic, unmodified source.
> [^TestBugMC2StreamPutBlockCommittedAfterClose.java]
> ({{hadoop-hdds/container-service}}) drives a real {{KeyValueHandler}} and
> container. The PutBlock enters {{handlePutBlock}} with the op
> {{ContainerStateMachine}} uses for each path, and the close goes through
> {{KeyValueHandler.closeContainer}}. The only hook is a Mockito spy on the
> real {{BlockManager}} that starts the close when the PutBlock reaches the
> commit and lets the commit run once the close has finished. For both paths it
> shows a CLOSED replica holding the block while its close ICR says keyCount 0
> and its stored checksum is that of an empty container. A passing test means
> the defect is present. It calls the handler directly, so for the mid stream
> PutBlock it sees {{CONTAINER_INTERNAL_ERROR}} rather than the
> {{IllegalStateException}} that {{HddsDispatcher}} turns it into.
> h3. Patch
> [^MC-2-recheck-container-state-under-lock-for-stream-putblock.patch], against
> 7212465c0bb19615708c144238317db51c2a696d. It also applies to master at
> ae71e2dd50c.
> For stream PutBlocks only, {{handlePutBlock}} repeats {{checkContainerOpen}}
> under the container read lock and commits while holding it. Close, quasi
> close and mark unhealthy change the state under the container write lock, so
> a stream PutBlock is now either committed before the close takes its checksum
> and ICR, or rejected with {{CLOSED_CONTAINER_IO}} without writing anything.
> Raft-applied PutBlocks and the gRPC path take no new lock; the block file is
> still closed outside the lock.
> The mid stream part does not depend on anything else: {{streamCommand}}
> already fails on a non success result, and the patch turns today's
> {{IllegalStateException}} and leftover row into a plain
> {{CLOSED_CONTAINER_IO}} with no row. The stream close part depends on
> HDDS-17431: {{ContainerStateMachine.streamPutBlock}} ignores the PutBlock
> result, so a late stream close PutBlock that this patch now rejects would
> leave the block off that replica while the client is told the write
> succeeded. Land this patch with or after the fix for HDDS-17431.
> HDDS-16828's proposed patch takes the container read lock at the same place
> in {{handlePutBlock}} for its finalized block check. Whichever lands second
> should put both checks in one locked section; with HDDS-16828 in, this fix
> reduces to repeating {{checkContainerOpen}} inside it.
> Covered by two tests in the existing {{TestKeyValueHandler}}, each for both
> stream paths. {{testStreamPutBlockRacingCloseIsNotCommittedAfterClose}} runs
> the close while the PutBlock is at the commit and asserts that the PutBlock
> succeeds and the checksum taken at close covers its block; without the change
> it fails for both paths.
> {{testStreamPutBlockRejectedWhenContainerClosesBeforeCommit}} closes the
> container after the first open check and before the lock is taken, and
> asserts {{CLOSED_CONTAINER_IO}} and no block; with the re-check under the
> lock removed it fails for both paths. With the change
> {{TestKeyValueHandler}}, {{TestKeyValueHandlerWithUnhealthyContainer}},
> {{TestKeyValueContainer}}, {{TestBlockManagerImpl}}, {{TestHddsDispatcher}},
> {{TestKeyValueStreamDataChannel}}, {{TestCloseContainerCommandHandler}} and
> the container state machine leader and follower tests pass,
> {{TestBlockDataStreamOutput}} and {{TestContainerStateMachineStream}} pass on
> a mini cluster (with and without the stream PutBlock), and checkstyle is
> clean. With the patch the reproduction fails, because the PutBlock is
> committed before the close and the close ICR counts it.
> Found by TLA+ model checking and code review of the datanode streaming
> PutBlock paths under HDDS-15926, on commit
> 7212465c0bb19615708c144238317db51c2a696d. 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
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
