[
https://issues.apache.org/jira/browse/HDDS-17431?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17431:
------------------------------
Affects Version/s: 2.3.0
> With PutBlock without Raft, a failed data stream close is reported as
> success, so OM commits a block that no replica or only one replica has
> --------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17431
> URL: https://issues.apache.org/jira/browse/HDDS-17431
> Project: Apache Ozone
> Issue Type: Bug
> Affects Versions: 2.3.0
> Reporter: Siyao Meng
> Priority: Major
> Attachments: MC-1-fail-stream-close-on-rejected-putblock.patch,
> TestBugCR5StreamCloseFailureIgnored.java,
> TestBugMC1ClosedContainerStreamClose.java
>
>
> h3. Mechanism
> With {{ozone.client.datastream.putblock.without.raft.enabled=true}} (client)
> and {{hdds.container.ratis.datastream.enabled=true}} (datanode), both false
> by default, the final PutBlock of a streaming write is appended to the data
> stream and committed by each datanode when the stream closes
> ({{StreamInitWithPutBlock}}). The close reply from Ratis is the only signal
> that this commit happened, and two places lose it:
> * Datanode: {{ContainerStateMachine.streamPutBlock}} ignores the response of
> {{dispatchCommand}}. When the dispatcher rejects the PutBlock (for example
> {{CLOSED_CONTAINER_IO}}, "Requested operation not allowed as ContainerState
> is CLOSED"), {{KeyValueStreamDataChannel.closeWithStreamPutBlock}} still
> calls {{setLinked()}}, the channel close succeeds and Ratis replies success
> to the close. The mid stream PutBlock
> ({{ContainerStateMachine.streamCommand}}) already throws on a result other
> than SUCCESS; the close path does not.
> * Client: {{BlockDataStreamOutput.close()}} waits for the close reply but
> never checks {{isSuccess()}}. A close that fails on the pipeline (Ratis
> replies success=false when the local close throws or a remote close fails) is
> handled only by a {{whenComplete}} callback in {{executePutBlock}} that calls
> {{executePutBlock(true, false)}} again. That call cannot send anything:
> {{buffersForPutBlock}} is already null (NullPointerException) or the stream
> is already closed (IOException), and the exception is thrown inside a future
> nobody reads. So {{close()}} returns, {{KeyDataStreamOutput}} never retries,
> and OM commits the key on the original block.
> The Raft path (the default) is not affected in the same way, because there
> the separate Raft PutBlock is the commit and its failure is reported.
> h3. Trigger
> Datanode streaming enabled, a client that writes through the streaming API
> with PutBlock without Raft enabled, and all datanodes of the pipeline at the
> version that supports it. Then either:
> * the container is closed while the stream is open (container full, pipeline
> close, or an admin close), so every datanode rejects the close PutBlock, or
> * the followers of the stream pipeline fail after the last data packet was
> acknowledged and before the close, so the close fails on the pipeline while
> the primary has already committed the PutBlock locally.
> h3. Impact
> Reproduced on unmodified source with a 5 datanode mini cluster through the
> public client API:
> * *Container closed before the stream close, nothing committed yet.* 50 bytes
> written, container closed through SCM, then {{key.close()}}. The close
> returned normally, OM committed 50 bytes on the original block, the replicas
> logged {{op=PUT_BLOCK ... ret=FAILURE}} and have no such block, and reading
> the key fails with "Unable to find the block". With PutBlock through Raft the
> client rewrites the data on a new block instead.
> [^TestBugMC1ClosedContainerStreamClose.java]
> * *Container closed before the stream close, part of the block committed.*
> 500 bytes written with a PutBlock in the middle of the stream at 400 bytes,
> which all replicas committed. After the container close and {{key.close()}},
> OM has 500 bytes on the original block, every replica has 400, and reading
> the key fails with "Inconsistent read".
> [^TestBugMC1ClosedContainerStreamClose.java]
> * *Followers lost before the stream close.* 100 bytes written and
> acknowledged, both followers shut down, then {{key.close()}}. The client
> logged {{Failed executePutBlockClose, reply=...,FAILED}}, {{close()}}
> returned normally, OM committed the original block, the primary has it with
> length 100 and both followers still lack it after restart. The key is
> readable only while the primary lives.
> [^TestBugCR5StreamCloseFailureIgnored.java]
> In all cases the writer is told the data is stored. Nothing repairs it
> automatically: SCM replication does not see a difference between the
> replicas. Admin triggered container reconciliation was not tried.
> PutBlock without Raft is part of HDDS-15463 (Streaming Write Pipeline without
> Raft), is off by default and is not in any release yet. It came with the
> commits "Streaming Write also commit PutBlock at the time of closing"
> (HDDS-15792), "Commit PutBlock without Raft in Client" (HDDS-15758) and
> "PutBlocks from Flushes also go without Raft" (HDDS-16008). The callback that
> cannot retry dates from HDDS-6500 and is in released versions, but there it
> is harmless because the Raft PutBlock is the commit.
> h3. Reproduction
> The two attached classes are self contained JUnit tests for
> {{hadoop-ozone/integration-test}} (target path and Maven command in each
> class comment). Each starts its own mini cluster and uses only the public
> client API, {{OzoneTestHelper.waitForContainerClose}}, datanode shutdown and
> restart, and reads of the datanode's block metadata, with no injected state
> and no sleeps. They pass on unmodified source when the bug is present and
> fail with the patch.
> h3. Patch
> [^MC-1-fail-stream-close-on-rejected-putblock.patch], against
> 7212465c0bb19615708c144238317db51c2a696d. It also applies to master at
> ae71e2dd50c.
> * {{ContainerStateMachine.streamPutBlock}} throws a
> {{StorageContainerException}} when the result is not SUCCESS, as
> {{streamCommand}} does. {{closeWithStreamPutBlock}} then does not mark the
> channel linked, the close fails, Ratis replies success=false and the client
> does not send the link request.
> * With PutBlock without Raft, {{BlockDataStreamOutput}} handles the close
> reply like the reply to a mid stream PutBlock command: on success=false it
> fails the stream, so {{KeyDataStreamOutput}} excludes the pipeline and writes
> the unacknowledged bytes again on a new block; on success it registers the
> buffers the close PutBlock covered, so that they are released like those of
> the Raft PutBlock. {{executePutBlock}} no longer registers the callback that
> cannot retry on this path.
> The release is needed for the retry to be correct. All blocks of a key share
> one buffer list, and before this patch a successful close without Raft never
> released its buffers. A retry on a later block then wrote the previous
> block's tail into the new block as well, and the key commit failed with an
> {{IllegalArgumentException}} ({{BlockDataStreamOutputEntryPool.commitKey}},
> offset does not match the block lengths). That already happened before this
> patch when a PutBlock in the middle of a later block failed; with the close
> failures now surfaced it would also hit a key whose last close fails,
> including a key whose data was all hsynced before its container closed. With
> the patch these keys commit and read back correctly (checked on a mini
> cluster with 1000 byte blocks: a failed close of the second block, a failed
> mid stream PutBlock of the second block, and a hsynced second block whose
> container closes before the close).
> Both datanode and client changes are needed for the container close case:
> with only the client change the close reply is still success, with only the
> datanode change the client ignores the failed reply (both checked with the
> new integration test). The followers case needs only the client change; there
> the rewrite, like the one after a failed Raft PutBlock, succeeds or fails
> loudly depending on whether SCM still hands out pipelines with a stopped
> datanode, and while it does the client can keep retrying for minutes (about
> two minutes in one run); that retry behaviour predates this patch. Nothing
> changes when PutBlock goes through Raft, and there is no protocol or
> configuration change; a client and its datanodes both need the patch for the
> container close case.
> Tests:
> *
> {{TestBlockDataStreamOutput#closeFailsIfStreamCloseCommittingPutBlockFails}}
> and {{#closeReleasesBuffersIfStreamCloseCommittingPutBlockSucceeds}} (mocked
> pipeline, close replied with success=false or success).
> *
> {{TestKeyDataStreamOutput#failedStreamCloseOfSecondBlockRewritesOnlyItsData}}
> (mocked pipelines, two block key whose second close fails).
> * {{ContainerStateMachineTests#testStreamPutBlockFailsIfDispatchFails}} and
> {{#testStreamPutBlockSucceedsIfDispatchSucceeds}}.
> *
> {{TestContainerStateMachineStream#testContainerClosedBeforeStreamCloseWithPutBlockWithoutRaft}}
> (shared non HA cluster: write, close the container, close the key, read it
> back).
> All of them except {{#testStreamPutBlockSucceedsIfDispatchSucceeds}} fail
> without the change; that one guards the success path. With the change they
> pass, as do {{TestBlockDataStreamOutput}}, {{TestClientThreadShutdown}},
> {{TestContainerStateMachineLeader}}, {{TestContainerStateMachineFollower}},
> {{TestKeyValueStreamDataChannel}}, {{TestKeyDataStreamOutput}},
> {{TestContainerStateMachineStream}}, the integration
> {{TestBlockDataStreamOutput}} and {{TestOzoneFileSystemWithStreaming}}, and
> checkstyle is clean.
> 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. Related but not
> duplicates: HDDS-16265 (the same callback, described as an unbounded retry
> whose failure is non critical because the Raft PutBlock does the commit;
> without Raft the close PutBlock is the commit, and on either path the
> re-entry fails before sending anything, so after this patch HDDS-16265
> concerns only the Raft path), HDDS-16828 (a stream PutBlock accepted on a
> finalized block), HDDS-16822 (hsync with PutBlock through Raft) and PR #11384
> (HDDS-16191, refactors the same method). This should be fixed before PutBlock
> without Raft (HDDS-15463) is enabled. 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]
