Siyao Meng created HDDS-16732:
---------------------------------
Summary: Open key reaper enqueues a live key's inherited blocks
for deletion on a versioned bucket
Key: HDDS-16732
URL: https://issues.apache.org/jira/browse/HDDS-16732
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments: MC-1-open-key-reaper-retains-live-blocks.patch
h3. Mechanism
With bucket versioning enabled, {{OMKeyRequest.prepareFileInfo}} calls
{{OmKeyInfo.addNewVersion(locations, false,
omBucketInfo.getIsVersionEnabled())}} ({{OMKeyRequest.java:1017}}), and
{{addNewVersion}} clears {{keyLocationVersions}} only when {{keepOldVersions}}
is false ({{OmKeyInfo.java:417}}). An open key created over an existing key
therefore keeps the live key's block version groups, and that whole record is
what {{OMKeyCreateRequest}} writes to {{openKeyTable}}.
If that open key is abandoned, {{OpenKeyCleanupService}} submits a
{{DeleteOpenKeysRequest}}. {{OMOpenKeysDeleteRequest.updateOpenKeyTableCache}}
({{OMOpenKeysDeleteRequest.java:186}}) checks only that the key is still in
{{openKeyTable}}, never {{keyTable}}, and
{{OMOpenKeysDeleteResponse.addToDBBatch}}
({{OMOpenKeysDeleteResponse.java:74}}) hands the entire record to
{{AbstractOMKeyDeleteResponse.addDeletionToBatch}}, which puts every version
group into {{deletedTable}} with no reference check.
{{filterOutBlocksStillInUse}} ({{OMKeyRequest.java:1248}}), the guard written
for exactly this hazard, is wired into {{OMKeyCommitRequest.java:364}},
{{OMKeyCommitRequestWithFSO.java:277}} and
{{S3MultipartUploadCompleteRequest.java:354}}, but not into the reaper path.
{{ReclaimableKeyFilter.isReclaimable}} ({{ReclaimableKeyFilter.java:87}})
decides purely from previous snapshots and returns true immediately when none
exist, so nothing downstream catches it either.
h3. Trigger
No conditional write, no fault injection, no timing race.
# Bucket has versioning enabled.
# Client A writes key {{k}}; {{keyTable[k]}} references block {{b1}}.
# Client B calls {{createKey(k)}} to overwrite. The {{openKeyTable}} entry now
holds two version groups, {{[{b1}, {b2}]}}.
# Client B never commits (process killed, partition, abandoned upload). The key
is not hsync'd, so {{getExpiredOpenKeys}} routes it to the delete branch.
# After {{ozone.om.open.key.expire.threshold}} (default 5 min),
{{OpenKeyCleanupService}} submits {{DeleteOpenKeysRequest}}.
# {{deletedTable}} now holds an entry whose blocks include {{b1}}, while
{{keyTable[k]}} still references {{b1}}.
# {{KeyDeletingService}} ships {{b1}} to SCM
({{KeyDeletingService.java:171-172}}). Client A's committed key is destroyed.
h3. Impact
A committed key's blocks are enqueued for physical deletion while the key is
still live and readable. The state is durable in RocksDB, nothing re-checks it,
and {{deletedTable}}'s only consumer acts on it. Default configuration, no
cluster faults required.
h3. Developer intent
{{git log -S filterOutBlocksStillInUse}} returns exactly two commits, both
fixing this hazard class at other sites. The first introduced the helper for
key commit and states the intent plainly: "Remove any block from oldVerKeyInfo
that share the same container ID and local ID with omKeyInfo blocks'.
Otherwise, it causes data loss once those shared blocks are added to
deletedTable and processed by KeyDeletingService for deletion." The second
extended it to S3 MPU complete three weeks before this head. Neither touches
the open key reaper. {{TestOMOpenKeysDeleteRequest}} asserts only table
membership, metrics and audit, never the blocks that reach {{deletedTable}}, so
nothing encodes the current behaviour as intended.
h3. Reproduction
PASS at escalation level 0, pure black box. Every step is a real OM write
request driven through {{preExecute}} plus {{validateAndUpdateCache}} plus
{{addToDBBatch}} with the RocksDB batch committed, exactly as the Ratis state
machine drives them. No state injection, no product code patch, no fault
injection.
{{repro/test_bugMC-1_run.sh}}, which runs {{mvn -B -o -pl
hadoop-ozone/ozone-manager test
-Dtest=TestSpeculaBugMC1OpenKeyReaperBlockLoss}}.
{noformat}
[repro] live keyTable blocks = [conID: 5001 locID: 1]
[repro] abandoned openKey blocks = [conID: 5001 locID: 1, conID: 5002 locID:
1] (versionGroups=2)
[repro] deletedTable blocks = [conID: 5001 locID: 1, conID: 5002 locID: 1]
[repro] blocks KeyDeletingService would send to SCM = [conID: 5001 locID: 1,
conID: 5002 locID: 1]
[repro] LIVE block conID: 5001 locID: 1 in deletedTable? true ; handed to SCM?
true
[control-commit] deletedTable = []
[control-nover] deletedTable = [conID: 5002 locID: 1]
{noformat}
Block 5001 is the live key's committed block. After the reaper runs it is in
{{deletedTable}} and in the list {{keyManager.getPendingDeletionKeys(...)}}
returns, which is what {{KeyDeletingService.java:171-172}} submits to
{{ScmBlockLocationProtocol.deleteKeyBlocks}}. Correct behaviour would have put
only 5002 there.
Two negative controls pin the mechanism. {{[control-commit]}}: same scenario
but client B commits, which enqueues nothing, so the defect is the reaper path
rather than the inheritance itself. {{[control-nover]}}: same reaper scenario
on a non versioned bucket, where {{keyLocationVersions.clear()}} runs and only
5002 reaches {{deletedTable}}, pinning the trigger to {{getIsVersionEnabled()}}.
Stated limitation: the final DataNode level block removal was not executed,
since a request level unit test has no SCM or DataNode. What is demonstrated is
that OM durably enqueues a live key's block and hands it to the SCM delete path.
h3. Suggested fix
Call {{filterOutBlocksStillInUse}} on the open key reaper path as the commit
and MPU complete paths already do: in
{{OMOpenKeysDeleteRequest.updateOpenKeyTableCache}}, look the key up in
{{keyTable}} under the bucket lock already held, strip from the expiring record
every block the live key still references, and skip the {{deletedTable}} put
entirely when nothing is left.
A root cause alternative is to stop the {{openKeyTable}} record carrying the
live key's version groups at all, since the open key only needs its own newly
allocated blocks and the old versions are re-read from {{keyTable}} at commit.
That removes the hazard from every reaper like consumer at once but touches
versioning semantics, so the targeted filter is the safer first patch.
Either way, add the missing negative assertion to
{{TestOMOpenKeysDeleteRequest}}: on a versioned bucket with a pre-existing live
key, the reaped open key must contribute no block that {{keyTable}} still
references. The reproduction is written against {{OMKeyRequestTests}}, the
shared base for that package, so it can be folded in as a test method rather
than a new class.
h3. Patch
[^MC-1-open-key-reaper-retains-live-blocks.patch], against
20e6a1c0f6039248373efd3eb1d7f2afcf7f1535.
Routes the record the reaper enqueues through a new {{withLiveBlocksRetained}},
which resolves the live key from the open key name (dropping the trailing
client id segment inverts both {{getOpenKey}} and {{getOpenFileName}}, so it is
layout agnostic) and runs the existing {{filterOutBlocksStillInUse}} over a
copy of the expiring record. A record left with no blocks is still removed from
openKeyTable and simply contributes nothing to deletedTable, because
{{AbstractOMKeyDeleteResponse.addDeletionToBatch}} already guards that put with
{{isKeyEmpty}}, so no response class changes.
Covered by {{testReapRetainsBlocksTheCommittedKeyStillReferences}} in the
existing {{TestOMOpenKeysDeleteRequest}}, parameterized over both bucket
layouts. It asserts both directions: the inherited key must not reach
deletedTable, and a key whose block no committed key holds must still be
reclaimed. Verified to fail without the change and pass with it on OBS and FSO.
Checkstyle clean. The 14 tests in that class pass.
Found by TLA+ model checking and code review of the OM conditional create,
overwrite and delete paths under HDDS-15926, on commit
20e6a1c0f6039248373efd3eb1d7f2afcf7f1535. Checked against open HDDS issues for
duplicates before filing, in addition to the repository's merged git history.
Generated with Specula (Claude Opus 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]
