[
https://issues.apache.org/jira/browse/HDDS-16824?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-16824:
------------------------------
Attachment: TestBugMC5RecoveredLengthFromLaggingReplica.java
> Lease recovery takes the block length from the first replica it can ask, so a
> lagging replica truncates hsynced data
> --------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-16824
> URL: https://issues.apache.org/jira/browse/HDDS-16824
> Project: Apache Ozone
> Issue Type: Bug
> Reporter: Siyao Meng
> Priority: Major
> Attachments:
> MC-5-lease-recovery-use-largest-replica-block-length.patch,
> TestBugMC5RecoveredLengthFromLaggingReplica.java
>
>
> h3. Mechanism
> When {{finalizeBlock}} in {{BasicRootedOzoneClientAdapterImpl}} or
> {{BasicOzoneClientAdapterImpl}} cannot finalize a block through its Ratis
> pipeline (the pipeline is no longer open, or FinalizeBlock failed), it falls
> back to {{ContainerProtocolCalls.getCommittedBlockLength}} on a standalone
> pipeline made of the datanodes of the block. {{XceiverClientGrpc}} asks the
> datanodes one after another and returns the first successful answer, and
> {{LeaseRecoveryClientDNHandler}} commits the file with that length.
> Nothing on this path tells replicas apart. hsync commits the block to OM with
> the block ID as allocated, whose block commit sequence ID is 0, so
> {{BlockUtils.verifyBCSId}} on the datanode accepts every replica, and a
> replica that is still open answers {{GetCommittedBlockLength}} like a closed
> one. A replica that missed the last PutBlock of the block reports the shorter
> length, and the file is closed with it.
> With the default {{ozone.network.topology.aware.read=true}} the datanodes are
> asked in the order in which SCM returned them, otherwise in random order.
> h3. Trigger
> # A client writes a RATIS/THREE file and calls hsync.
> # One datanode of the pipeline stops. hsync keeps succeeding: after
> {{hdds.ratis.raft.client.rpc.watch.request.timeout}} (default 30s) the client
> stops waiting for all replicas and accepts a commit by the majority.
> # The writer dies while the datanode is down, before an hsync in a next block
> has reached OM. The container is closed and the pipeline removed before the
> datanode returns, so it cannot catch up and its replica keeps the shorter
> block.
> # {{recoverLease}} is called after the datanode is back and before
> ReplicationManager has replaced its replica, and that datanode is the first
> one asked.
> Steps 1, 2 and 4 are run as written. In step 3 the reproduction closes the
> container and removes the pipeline with SCM calls. That SCM does both on its
> own for a datanode that stays down longer than
> {{ozone.scm.stale.node.interval}} (default 5m), and that ReplicationManager
> needs minutes to replace the replica
> ({{hdds.scm.replication.thread.interval}} is 300s), is from reading the code,
> not run. Step 3 is one way to get there. What the fallback needs is any
> replica that is behind and reachable when it runs, for example a follower
> that is still replaying its log after a restart (argued, not run).
> h3. Impact
> The recovered file is shorter than what hsync acknowledged, although two of
> three replicas hold the data. In the reproduction the second hsync of 100
> bytes returned successfully and the file is closed with 100 bytes. For a
> write ahead log this is silent loss of acknowledged entries. The length is
> final once the file is closed, and the bytes beyond it on the two healthy
> replicas are no longer reachable through the file.
> h3. Reproduction
> PASS on unmodified source, nothing injected, recovery not forced.
> [^TestBugMC5RecoveredLengthFromLaggingReplica.java]
> ({{hadoop-ozone/integration-test}}) starts a mini cluster with three
> datanodes. The writer hsyncs 100 bytes, the datanode whose replica SCM lists
> first is stopped, and the writer hsyncs 100 more bytes. The container is
> closed and its pipeline removed, the datanode is started again and still has
> 100 bytes of the block, the other two have 200. {{recoverLease}} returns true
> and the file is closed with 100 bytes. The only non default settings are
> those that enable hsync, a lease soft limit of zero and
> {{hdds.ratis.raft.client.rpc.watch.request.timeout}} of 3s instead of 30s. A
> passing test means the defect is present.
> h3. Suggested fix
> The attached patch makes the fallback ask every replica and use the largest
> length. For writers whose PutBlock goes through the Raft log, which is the
> default, datanodes apply a PutBlock only after it is committed in Ratis, and
> the length of a block only grows with the applied log. The largest length
> among the replicas is then the length at the most advanced replica, which is
> the state ReplicationManager converges to (argued from the code).
> Not covered:
> * A data stream writer with
> {{ozone.client.datastream.putblock.without.raft.enabled}} (off by default)
> sends PutBlock outside the Raft log, so each datanode applies it on its own
> and the argument above does not hold. There the largest length can be one
> that only one replica has (argued, not run).
> * If only lagging replicas answer during recovery, the result is still too
> short, and a length below the one OM already holds for the block is still
> accepted. Failing recovery in that case unless it is forced belongs in
> {{LeaseRecoveryClientDNHandler}} and is not part of this patch. Requiring an
> answer from all or a majority of the replicas would close the rest, at the
> price of failing recovery whenever a datanode is down, which is the situation
> recovery exists for.
> * Until ReplicationManager has replaced the lagging replica, a reader that is
> served by it gets an error for the bytes it does not have (from reading the
> code, not run), because the recovered key carries block commit sequence ID 0
> and the read path cannot reject that replica. Returning the sequence ID from
> {{GetCommittedBlockLength}} and storing it at recovery would address that. It
> needs a protocol change.
> h3. Patch
> [^MC-5-lease-recovery-use-largest-replica-block-length.patch], against
> ea69b4a7d9abd040e5259dbcbb7c6ce52eb5d199. It also applies to master at
> 84f5594a6a2.
> {{ContainerProtocolCalls.getMaxCommittedBlockLength}} sends
> {{GetCommittedBlockLength}} to all datanodes of the pipeline with the
> existing {{XceiverClientSpi.sendCommandOnAllNodes}} and returns the largest
> length. Both adapters use it in the fallback. Replicas that fail or do not
> answer are skipped, and one warning is logged when a replica is skipped or
> the lengths differ. What changes for recoveries the bug does not affect:
> * It is one request per datanode of the pipeline of the block, sent in
> parallel, and all answers are awaited. For a container that is no longer open
> that is every replica SCM lists, in any state. A datanode that accepts the
> request and does not answer now always costs one
> {{ozone.client.read.timeout}} (30s), where before it cost that only if it was
> asked before one that answered. The worst case drops from one timeout per
> replica in a row to one.
> * If no replica returns a length, {{NO_SUCH_BLOCK}} or
> {{CONTAINER_NOT_FOUND}} is thrown only when every datanode that was asked
> answered with one of the two, so {{LeaseRecoveryClientDNHandler}} skips a
> block only then. Otherwise an {{IOException}} is thrown. Before, the error of
> the last datanode tried was thrown.
> * An {{UNAUTHENTICATED}} answer of a datanode no longer surfaces as
> {{SCMSecurityException}}. That replica is skipped like one that is
> unreachable (from the code, not run).
> * EC keys do not reach the fallback (hsync is not supported for EC and the
> fallback rejects non replicated keys).
> Covered by two unit tests in {{TestContainerProtocolCalls}} and by
> {{testBlockLengthOfLaggingReplicaIsNotUsed}} in the existing
> {{TestLeaseRecovery}}, which fails without the change with a file length of
> 100 instead of 200. Two assertions in {{TestLeaseRecovery}} that counted the
> datanodes tried one after another now count the requests that are sent. On
> unmodified source those two tests ({{testGetCommittedBlockLengthTimeout}},
> {{testGetCommittedBlockLengthWithException}}) fail intermittently, because
> their helper {{closeLatestContainer}} closes the last container SCM lists,
> which is not always the container of the file, and then the fallback is never
> reached. With the patch the reproduction fails at its length assertion,
> because the file is closed with 200 bytes.
> Found by TLA+ model checking and code review of the lease recovery paths for
> hsynced files under HDDS-15926, on commit
> ea69b4a7d9abd040e5259dbcbb7c6ce52eb5d199. 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
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
