Siyao Meng created HDDS-17431:
---------------------------------
Summary: 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
Reporter: Siyao Meng
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]
