[
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: [email protected]
For additional commands, e-mail: [email protected]
