[jira] [Updated] (HDDS-16732) Open key reaper enqueues a live key's inherited blocks for deletion on a versioned bucket

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

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

Siyao Meng updated HDDS-16732:
------------------------------
    Attachment: MC-1-open-key-reaper-retains-live-blocks.patch

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