Siyao Meng created HDDS-16827:
---------------------------------
Summary: 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
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]
