[
https://issues.apache.org/jira/browse/HDDS-16827?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-16827:
------------------------------
Attachment: TestBugMC1RecoveryCommitClosesNewWriter.java
> Lease recovery commit closes the file of a writer that overwrote the path
> during the recovery and drops its hsynced data
> ------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-16827
> URL: https://issues.apache.org/jira/browse/HDDS-16827
> Project: Apache Ozone
> Issue Type: Bug
> Reporter: Siyao Meng
> Priority: Major
> Attachments:
> MC-1-reject-recovery-commit-with-block-not-in-open-key.patch,
> TestBugMC1RecoveryCommitClosesNewWriter.java
>
>
> h3. Mechanism
> {{recoverLease}} is two OM transactions with datanode RPCs in between:
> {{RecoverLease}}, which marks the writer's open key with {{LEASE_RECOVERY}}
> and returns the file's blocks, and a {{CommitKey}} with {{recovery=true}} and
> client id 0. For that commit {{OMKeyCommitRequestWithFSO.prepareCommit}}
> takes the writer from {{HSYNC_CLIENT_ID}} of the current {{fileTable}} entry
> and closes that writer's open key. Nothing ties the commit to the open key
> that {{RecoverLease}} worked on: the request does not name the writer, and a
> block in the request that the resolved open key does not have is dropped with
> a warning ("Unknown BlockLocation" in {{OmKeyInfo.updateLocationInfoList}})
> instead of failing the commit.
> Overwriting an hsynced file is allowed (HDDS-10770 made it a supported
> operation, and {{OMFileCreateRequestWithFSO}} has no hsync or recovery
> check). If another client overwrites the file and hsyncs between the two
> transactions, the recovery commit resolves the new writer's open key. The
> blocks named in the commit belong to the old writer, so they are dropped as
> unknown and the new writer's blocks are returned as uncommitted. Those go to
> {{deletedTable}}, and the file is closed with the old writer's length and no
> blocks.
> {{OMKeyCommitRequest}} has the same code. {{RecoverLease}} is refused for
> buckets that are not FSO, so the race was only run on FSO. The same writer
> resolution and block handling are in the ozone-2.0.0 and ozone-2.1.0 tags
> (read, not run).
> h3. Trigger
> hsync is enabled ({{ozone.hbase.enhancements.allowed}} and
> {{ozone.fs.hsync.enabled}}, both false by default), FSO bucket.
> # Writer 1 creates a file, writes, hsyncs and stalls.
> # A recoverer calls {{recoverLease}}. {{RecoverLease}} is applied and the
> recoverer talks to the datanodes ({{FinalizeBlock}} or
> {{GetCommittedBlockLength}}).
> # Writer 2 creates the same path with overwrite, writes and hsyncs.
> # The recoverer's commit arrives.
> No fault is needed. The window is the datanode phase of the recovery (not
> measured). Writer 2 has to create and hsync inside it. With a create alone
> the file table still names writer 1 and the recovery is correct.
> h3. Impact
> * The file is closed with writer 1's length (200 in the run) and an empty
> block list. A reader gets 0 bytes. The 60 bytes that writer 2 had hsynced,
> and that were readable just before, are gone.
> * Writer 2's block is in {{deletedTable}}. Deletion on the datanodes was not
> waited for.
> * {{recoverLease}} returns true to the recoverer.
> * Writer 2's {{close()}} fails with {{KEY_NOT_FOUND}} ("entry is not found in
> the OpenKey table").
> h3. Reproduction
> PASS, unmodified source. [^TestBugMC1RecoveryCommitClosesNewWriter.java]
> ({{hadoop-ozone/integration-test}}) uses a mini cluster with one OM and three
> datanodes and the FileSystem API only. {{ozone.om.lease.soft.limit}} is set
> to 0s (default 60s), as in {{TestLeaseRecovery}}, so that the recovery does
> not have to wait. The datanode fault injector that {{TestLeaseRecovery}}
> already uses holds the recovery inside {{FinalizeBlock}} until the second
> writer has overwritten the file and hsynced. It widens the window and changes
> no state. The test logs the key before and after the recovery commit and then
> asserts the wrong outcome, so a passing test means the defect is present.
> h3. Suggested fix
> The attached patch rejects a recovery commit that names a block the resolved
> open key does not have. One case is left: a stale recovery commit with an
> empty block list, that is writer 1 had hsynced before it wrote any data.
> There is nothing to compare, the commit is accepted and closes writer 2's
> file with no blocks (run as a temporary unit probe with the patch applied,
> not included). Such a recovery has no datanode phase, so its window is one
> client round trip (read from {{LeaseRecoveryClientDNHandler}}, the race was
> not run for this case).
> Two other checks were considered:
> * Reject a recovery commit when the resolved open key does not carry
> {{LEASE_RECOVERY}}. Not now: the soft limit check in {{RecoverLease}} uses
> the local clock at apply (HDDS-16767, related), so one OM can lack the marker
> that the others have, and that OM would reject a recovery commit the others
> apply and stay different from them. Once HDDS-16767 makes the marker the same
> on all OMs it is a useful additional guard. It narrows the empty list case,
> but does not close it, and on its own it accepts the stale commit when a
> third client has started a recovery of writer 2's file in between.
> * The exact fix: the client returns the writer id it got from
> {{RecoverLease}} in the recovery commit and the OM rejects the commit when
> the current writer is another one. This is the only one that closes the empty
> list case. It is a protocol change that needs a version gate for clients and
> OMs of different versions, so it is left for discussion.
> h3. Patch
> [^MC-1-reject-recovery-commit-with-block-not-in-open-key.patch], against
> ea69b4a7d9abd040e5259dbcbb7c6ce52eb5d199. It also applies to master at
> 53179f8f8b6.
> In {{OMKeyCommitRequestWithFSO.prepareCommit}} and
> {{OMKeyCommitRequest.validateAndUpdateCache}} a commit with the recovery flag
> is rejected with {{KEY_NOT_FOUND}} when it names a block that the resolved
> open key does not have. The check runs before anything is modified, so the
> second writer's file and open key stay as they are. The new
> {{OmKeyInfo.hasAllBlocks}} compares blocks the way {{updateLocationInfoList}}
> does (container id and local id, latest version) and the two share the code
> that collects the open key's blocks. A block that is listed twice in the
> request is not treated as unknown, because a recovering client can list its
> last block twice. Commits without the recovery flag are not changed.
> Why a recovery that is not raced is not affected: every block in a recovery
> commit comes from the {{RecoverLease}} response, which holds the file table
> blocks and the last block of the open key, and the file table blocks of an
> hsynced file are always blocks of its open key (read from the code, and the
> existing suites below pass). OMs that agree on the open key's blocks decide
> the same way, and the check does not read the {{LEASE_RECOVERY}} marker, so
> an OM whose open key lacks it decides like the others. An OM that already
> differs because of HDDS-16767 can lack a block the others have (it refused an
> {{AllocateBlock}} that they applied). It rejects a recovery commit naming
> that block, where without the patch it closes the file without the block
> (read, not run). A retried commit that was already applied still gets
> {{KEY_ALREADY_CLOSED}}, because that check comes first. The check also
> rejects the stale commit when a third client has started a recovery of writer
> 2's file in between. (That case needs the first recovery's commit to be still
> outstanding when the third client's {{RecoverLease}} is accepted, that is the
> third client uses {{force}}, or the new writer's open key has not been
> modified for the lease soft limit, 60s by default.)
> The recoverer now gets an {{OMException}} with {{KEY_NOT_FOUND}} from
> {{recoverLease}} (not a {{FileNotFoundException}}, only the {{RecoverLease}}
> step maps to that). A caller that calls {{recoverLease}} again starts a
> normal recovery of the second writer's file, subject to the soft limit.
> Compatibility: no new request, field or persisted state. An OM without the
> change accepts a recovery commit that names a block the open key does not
> have, and an OM with it rejects it, so OMs of two versions that apply the
> same entry (rolling upgrade, or replay after a restart into the new version)
> differ on that entry. This only affects an entry that hits this race, where
> the old result is the data loss. No version gate was added. Whether one is
> wanted is a question for review.
> Covered by {{testRecoveryCommitRejectedAfterOverwriteByAnotherWriter}} in the
> existing {{TestOMRecoverLeaseRequest}} (with and without a second recovery in
> between) and {{testRecoveryCommitChecksBlocksAgainstOpenKey}} in
> {{TestOMKeyCommitRequest}} (inherited by the FSO subclass, so both commit
> classes are covered; it also checks that a commit with only blocks of the
> open key, one of them listed twice, is still accepted). Without the change
> the four fail with "expected: <KEY_NOT_FOUND> but was: <OK>". With it the
> lease recovery, key commit, allocate block, multipart commit and open key
> cleanup unit suites and {{TestOmKeyInfo}} (161 tests) pass and checkstyle is
> clean. On a mini cluster {{TestHSync}} (42) passes. In {{TestLeaseRecovery}}
> (15) the only failures are in {{testGetCommittedBlockLengthTimeout}} and
> {{testGetCommittedBlockLengthWithException}}. Which of them fail varies
> between runs, they fail without the patch as well (the class is tagged flaky,
> HDDS-11323), and the new error does not occur in the suite's output. With the
> patch the reproduction fails at its first assertion, because {{recoverLease}}
> is rejected, the file keeps the second writer's 60 bytes and the second
> writer closes normally.
> 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]
