[jira] [Updated] (HDDS-16827) Lease recovery commit closes the file of a writer that overwrote the path during the recovery and drops its hsynced data

["Siyao Meng (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/HDDS-16827?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Siyao Meng updated HDDS-16827:
------------------------------
    Attachment: MC-1-reject-recovery-commit-with-block-not-in-open-key.patch

> 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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org