Siyao Meng created HDDS-16828:
---------------------------------
Summary: [hsync] Datanode accepts a PutBlock on a finalized block
if it is logged after FinalizeBlock or sent through the data stream
Key: HDDS-16828
URL: https://issues.apache.org/jira/browse/HDDS-16828
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments: MC-4-reject-putblock-on-finalized-block-at-apply.patch,
TestBugCR6DatastreamHsyncAfterLeaseRecovery.java,
TestBugMC4HsyncAckedBeyondRecoveredLength.java
h3. Mechanism
Lease recovery finalizes the last block on the datanodes and closes the file at
the block length that the FinalizeBlock reply carries
({{KeyValueHandler.handleFinalizeBlock}} reads it when the request is applied,
{{BasicRootedOzoneClientAdapterImpl.finalizeBlock}} passes it on to OM). A
writer that hsyncs inside its current block does not call OM, so the only thing
that stops a writer whose lease was recovered is the datanode's finalized block
check ({{BLOCK_ALREADY_FINALIZED}}).
That check exists in one place, {{ContainerStateMachine.startTransaction}},
which runs on the Ratis leader for requests submitted through the Raft log.
Where a PutBlock is executed, {{KeyValueHandler.handlePutBlock}} and the
PutBlock carried by a WriteChunk in {{handleWriteChunk}} only check that the
container is open. Two kinds of PutBlock therefore get past the check:
* *Logged after FinalizeBlock.* Ratis calls {{startTransaction}} before it
appends the request to the log, and outside the server lock that orders the
append ({{RaftServerImpl.writeAsyncImpl}}, then {{appendTransaction}}, Ratis
3.3.1). A writer's PutBlock that passed the check can be appended, and applied,
after the FinalizeBlock of a concurrent lease recovery.
* *Sent through the data stream.* A PutBlock sent as a stream command
({{ContainerStateMachine.streamCommand}}, HDDS-16008) or at stream close
({{streamPutBlock}}, commit "HDDS-15757. Streaming Write also commit PutBlock
at the time of closing", pull request 10694) is dispatched to the handler on
each datanode directly. It never passes {{startTransaction}}.
In both cases the PutBlock extends the block after FinalizeBlock has reported
its length, and the writer gets a success reply.
h3. Trigger
hsync has to be enabled: {{ozone.fs.hsync.enabled=true}}, which is only
honoured with {{ozone.hbase.enhancements.allowed=true}} (both false by default).
* *Raft path, any client settings that enable hsync.* A writer's hsync races
with {{recoverLease}} from another client while the writer is still alive. The
window is on the datanode leader, between {{startTransaction}} returning and
the log append. It is short: the original investigation did not hit it in 160
free running attempts (not repeated here), and the attached reproduction holds
the writer's request in the window with a Ratis test hook. The check in
{{startTransaction}} (HDDS-10044) and FinalizeBlock (HDDS-9915) are in every
release tag from ozone-2.0.0 to ozone-2.2.1, so this window is in released code
(read from the tags, not run on a release).
* *Data stream path.* The writer has {{ozone.fs.datastream.enabled=true}} and
{{ozone.client.datastream.putblock.without.raft.enabled=true}} (both false by
default) and the datanodes have
{{hdds.container.ratis.datastream.enabled=true}} (false by default). Through
the FileSystem API the stream writer is selected only once more than
{{ozone.fs.datastream.auto.threshold}} (4MB by default) has been written before
the first hsync, hflush, flush or close. The reproduction lowers it to 1KB. No
timing is needed: every hsync after the recovery is accepted. With the data
stream enabled but PutBlock still sent through the Raft log (the default), the
writer is rejected by {{startTransaction}} as on the Raft path. The two stream
PutBlock paths are on master only, they are in no release tag up to ozone-2.2.1.
h3. Impact
The writer's {{hsync()}} returns success for bytes that are not in the closed
file. Reproduced on unmodified source:
* Raft path: the writer hsyncs 100 bytes, then writes 100 more and hsyncs while
{{recoverLease}} runs. The second hsync returns normally, the file is closed
with length 100 and 100 bytes are readable.
* Data stream path: the writer hsyncs 40960 bytes and {{recoverLease}} closes
the file at 40960. Two more hsyncs of 30720 bytes each return normally. The
committed block length is 102400 on all three datanodes, the file stays at
40960.
The writer only learns that it lost the file when it allocates the next block
or closes ({{KEY_NOT_FOUND}}). On the data stream path the loss is limited to
what the writer hsyncs inside the block that was current at recovery. On the
Raft path it is only the request caught in the window, the next hsync is
rejected on the leader as before. As a side effect the finalized block's
committed length on the datanodes is larger than the length of the key in OM.
h3. Reproduction
Two attached classes for {{hadoop-ozone/integration-test}}, each self contained
with its own three datanode mini cluster and driven through the FileSystem API.
Target path and Maven command are in each class comment. Both pass on
unmodified source when the bug is present.
* [^TestBugMC4HsyncAckedBeyondRecoveredLength.java]: the Raft path. The writer
uses incremental chunk list and PutBlock piggybacking (the settings of
{{TestHSync}}) so that one hsync is one Ratis request. The only timing aid is
Ratis' {{CodeInjectionForTesting}} at {{RaftServerImpl.appendTransaction}},
which holds the writer's request after {{startTransaction}} returned until
{{recoverLease}} has returned. No state is injected and FinalizeBlock is not
delayed, so the run shows that the window exists, not how often it is hit. The
test also asserts that the recovery took its length from the FinalizeBlock
reply and not from the fallback read.
* [^TestBugCR6DatastreamHsyncAfterLeaseRecovery.java]: the data stream path,
with the client settings above and no timing aid. It reads the committed block
length and the finalized state from the datanodes for its assertions.
h3. Patch
[^MC-4-reject-putblock-on-finalized-block-at-apply.patch], against
ea69b4a7d9abd040e5259dbcbb7c6ce52eb5d199. It also applies to master at
53179f8f8b6.
* {{KeyValueHandler}} checks the finalized block set where a PutBlock is
executed ({{handlePutBlock}} and the PutBlock carried by a WriteChunk), so the
order in the Raft log decides and the stream paths are covered by the same
check. The check and the PutBlock run under the container read lock and
{{handleFinalizeBlock}} under the write lock, which orders a stream PutBlock
(not in the Raft log) against FinalizeBlock on each datanode. A log entry that
is replayed after a restart and that {{BlockManagerImpl.persistPutBlock}} skips
by its BCSID is not checked, so replay behaves as before.
* {{BLOCK_ALREADY_FINALIZED}} is added to the results that
{{HddsDispatcher.canIgnoreException}} and
{{ContainerStateMachine.applyTransaction}} do not treat as a failure of the
replica. Without this the rejection would mark the container unhealthy and
close the pipeline on all three datanodes.
* {{startTransaction}} keeps its check as an early rejection, but no longer
adds the block to the leader's finalized set when it sees the FinalizeBlock
request. The set is now filled only when FinalizeBlock is applied (and from the
{{FINALIZE_BLOCKS}} table at restart), so leader and followers reach the same
decision for the same log.
The writer sees the same error as for a rejection in {{startTransaction}}
today: hsync throws an {{IOException}} ({{KEY_NOT_FOUND}} from OM when the
client retries on a new block). A plain WriteChunk logged after FinalizeBlock
is still written to the block file, as its data is written before the entry is
applied, but it does not change the committed length (by code reading).
Compatibility (by code reading, no mixed version run): the patch changes what a
datanode does when it applies a PutBlock that is behind the FinalizeBlock of
its block in the log, which is the window of this bug. For such an entry a
datanode without the patch updates the block metadata (chunk list, size, block
BCSID) and the container BCSID, a datanode with the patch does not. The block
file is the same on both, because the data is written before the entry is
applied. FinalizeBlock is ahead of the PutBlock in the log on every replica, so
the key length in OM is the length before that PutBlock everywhere: no replica
is shorter than the key and readers are not affected, and the replicas without
the patch are in the state all replicas are in today. What can be observed if
both versions apply such an entry: when it was the last PutBlock applied to the
container, the replicas report different BCSIDs at close, and the container
data checksums differ. No new request, field or persisted state is involved. A
layout feature would be the strict alternative.
Not covered, data stream path only (argued from the code, not run):
* A stream PutBlock that races with FinalizeBlock is decided on each datanode
separately. The writer is acknowledged only if all datanodes accepted it, and
each of them then completed it before its own FinalizeBlock read the length, so
an acknowledged hsync is always inside the recovered length. But if the
datanode whose FinalizeBlock reply the recovery uses accepted it and another
datanode rejected it, that replica's committed length stays below the length of
the key in OM, and a read that is served by that replica fails ("Inconsistent
read for blockID", the client does not fall back to another replica for this).
Without the patch every datanode accepts the PutBlock, so this state is new
with the patch. Closing it needs the stream PutBlock to be ordered with
FinalizeBlock through the Raft log.
* The close of a container clears its finalized blocks before it takes the
container lock, so a stream PutBlock that races with the close of the container
can still pass the check.
* {{streamPutBlock}} ignores the result of the PutBlock, so a rejected PutBlock
at stream close is not reported to the client (for a recovered writer the
commit to OM then fails).
Tests: {{TestLeaseRecovery.testHsyncAfterRecoveryIsRejected}} (Raft path as
control, data stream path) and {{testHsyncLoggedAfterFinalizeBlockIsRejected}}
(same Ratis hook as the reproduction),
{{TestHddsDispatcher.testPutBlockOnFinalizedBlockIsRejectedWithoutMarkingContainerUnhealthy}}
(rejection with and without a log index, replay, container stays healthy) and
{{ContainerStateMachineTests.testApplyTransactionOnFinalizedBlockKeepsContainerHealthy}}
for leader and follower. Without the change the data stream case fails with
"Expected java.io.IOException to be thrown, but nothing was thrown", the hook
case with a missing hsync failure, the dispatcher case with "expected:
BLOCK_ALREADY_FINALIZED but was: SUCCESS" and the state machine cases with a
{{StorageContainerException}}. With the change these pass, together with 383
container-service unit tests around the handler, dispatcher and state machine
and 77 integration tests in the hsync, FinalizeBlock, container state machine
and data stream suites (those ran before the replay condition was added to the
patch). Checkstyle is clean. The two attached reproductions no longer show the
bug: the hsync fails and every replica stays at the recovered length.
Found by TLA+ model checking and code review of the lease recovery paths for
hsynced files under HDDS-15926, on commit
ea69b4a7d9abd040e5259dbcbb7c6ce52eb5d199. The Raft path came from model
checking, the data stream path from code review. Checked against HDDS issues
and apache/ozone pull requests for duplicates before filing. Related but not
duplicates: HDDS-8439 (resolved, write after lease recovery does not fail, the
reason FinalizeBlock exists), HDDS-10563 (a writer rejected by the check, the
opposite symptom), HDDS-15723 (rejecting writes after a PutBlock with the end
of block flag, a different condition), HDDS-16730 (last chunk merge at
FinalizeBlock) and HDDS-16731 (scanner and reconciliation ignore the last chunk
rows). 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]
