[
https://issues.apache.org/jira/browse/HDDS-16828?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-16828:
------------------------------
Attachment: TestBugMC4HsyncAckedBeyondRecoveredLength.java
> [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
> Priority: Major
> 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]
