[
https://issues.apache.org/jira/browse/HDDS-16733?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-16733:
------------------------------
Attachment: MC-2-versioned-overwrite-orphans-previous-blocks.patch
> Concurrent open of an absent key on a versioned bucket drops the first
> committed version and leaks its blocks
> -------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-16733
> URL: https://issues.apache.org/jira/browse/HDDS-16733
> Project: Apache Ozone
> Issue Type: Bug
> Reporter: Siyao Meng
> Priority: Critical
> Attachments: MC-2-versioned-overwrite-orphans-previous-blocks.patch
>
>
> h3. Mechanism
> Bucket versioning preserves an overwritten version only through block
> inheritance done at OPEN time ({{OMKeyRequest.prepareFileInfo}}, the
> {{dbKeyInfo != null}} branch at {{OMKeyRequest.java:1013}}), not through
> anything commit does. When two writers open the same not-yet-existing key,
> neither open key inherits anything.
> The first commit publishes its record. The second commit finds {{keyToDelete
> != null}} but, versioning being enabled, skips the overwrite cleanup branch
> at {{OMKeyCommitRequest.java:348}} and falls into the {{else}} at {{:391}},
> which only does quota accounting: no old version retention, no deletion
> enqueue, no reference tracking. The committed record is built from the second
> writer's open key, which never contained the first writer's blocks.
> The first version is therefore silently discarded despite versioning being
> on, and its blocks are referenced by nothing and queued for deletion by
> nothing. The same branch also calls {{incrUsedNamespace(1L)}} for what is
> really an overwrite.
> h3. Trigger
> Bucket {{v/b}} with versioning enabled (settable through the public native
> client: {{OzoneBucket.setVersioning(true)}} to {{RpcClient.java:1224}} to
> {{OMBucketSetPropertyRequest.java:175-181}}, no gate), key {{k}} absent:
> # Writer A sends {{CreateKey v/b/k}}. {{dbKeyInfo == null}}, so A's open key
> holds only A's blocks.
> # Writer B sends {{CreateKey v/b/k}} before A commits. Still {{dbKeyInfo ==
> null}}, so B's open key holds only B's blocks.
> # A sends {{CommitKey}}. {{keyToDelete == null}}; key table gets A's record,
> version 0, blocks {{{bA}}}.
> # B sends {{CommitKey}}. {{keyToDelete}} is A's record, versioning on, so
> control goes to the {{else}} at {{:391}}. Key table gets B's record, version
> 0, blocks {{{bB}}}. A's version is gone and {{{bA}}} was never enqueued.
> Both are plain unconditional PUTs, so {{validateAtomicRewrite}}
> ({{OMKeyRequest.java:1367}}) is a no-op on this path. Four ordinary requests,
> no injected fault.
> h3. Impact
> The first committed version is unrecoverable and its blocks are leaked
> permanently: nothing in the tree scans containers for unreferenced blocks,
> and a block absent from {{deletedTable}} is never reclaimed. The bucket's
> {{usedNamespace}} and {{usedBytes}} served to users are also inflated, which
> {{ozone repair om quota}} can recompute later but which does not reclaim the
> blocks.
> h3. Developer intent
> {{TestOmBlockVersioning}} enables versioning ({{:78}}) and asserts versions
> accumulate 0, 1, 2 through the public client ({{:99-135}}), so version
> retention on the native path is a promised property and losing a version is
> incorrect. Blame on the {{else}} block shows only quota work; nothing
> discusses old block ownership on the versioned path. The "versioning is not
> supported" notes in the tree are about the S3 API and the MPU path, which is
> awareness that versioning is incomplete rather than a report of this defect,
> and they do not disable the native flag the commit path honours.
> h3. Reproduction
> PASS at escalation level 1.
> {{repro/test_bugMC-2_versioned_concurrent_open_orphans_blocks.java}}, run as
> {{TestSpeculaMC2VersionedConcurrentOpen extends OMKeyRequestTests}} in the
> existing {{request/key}} suite, no new cluster.
> Versioning ON, the bug lines are the last four:
> {noformat}
> [MC-2] bucket versioning enabled = true
> [MC-2] after A commit, keyTable versions=[version:0 conID 1001 locID 101 || ]
> [MC-2] after B commit, keyTable versions=[version:0 conID 1002 locID 102 || ]
> [MC-2] blocks referenced by keyTable: [{blockID={conID: 1002 locID: 102}}]
> [MC-2] blocks enqueued in deletedTable: []
> [MC-2] bucket usedNamespace=2 usedBytes=2000
> [MC-2] writer A block conID: 1001 locID: 101 referenced=false
> queuedForDelete=false
> {noformat}
> Negative control, identical interleaving with versioning OFF, which passes:
> {noformat}
> [MC-2] bucket versioning enabled = false
> [MC-2] blocks enqueued in deletedTable: [{blockID={conID: 1001 locID: 101}}]
> [MC-2] bucket usedNamespace=1 usedBytes=1000
> [MC-2] writer A block conID: 1001 locID: 101 referenced=false
> queuedForDelete=true
> {noformat}
> Expected correct behaviour is the control's: A's blocks either retained as
> version 0 of the versioned key, or enqueued in {{deletedTable}}. Flipping
> only {{isVersionEnabled}} moves control from {{:348}} (enqueues, quota
> correct at 1 key and 1000 bytes) to {{:391}} (enqueues nothing, quota
> inflated to 2 and 2000), so the cause is isolated and a harness artifact is
> ruled out.
> The OM apply path is single threaded by construction, so the interleaving is
> chosen rather than raced. The only harness adjustment of note is that the
> shared base class's {{allocateBlock}} stub returns one fixed block ID for
> every call, which real SCM never does and which would have hidden the leak
> for a mock only reason.
> h3. Suggested fix
> On the versioned commit path, treat {{keyToDelete != null}} as an overwrite
> that must preserve or release the previous version's blocks, rather than
> falling through to quota accounting only. Either retain the previous record's
> version group in the committed record (which is what versioning promises) or
> enqueue its blocks in {{deletedTable}} as the non versioned branch does. The
> quota call on that branch should also stop counting an overwrite as a new
> namespace entry.
> Add coverage to the existing {{request/key}} suite: two writers opening the
> same absent key on a versioned bucket, asserting after both commits that
> every block from the first commit is either referenced by {{keyTable}} or
> present in {{deletedTable}}, and that {{usedNamespace}} is 1.
> h3. Patch
> [^MC-2-versioned-overwrite-orphans-previous-blocks.patch], against
> 20e6a1c0f6039248373efd3eb1d7f2afcf7f1535.
> Widens the overwrite cleanup branch in {{OMKeyCommitRequest}} and
> {{OMKeyCommitRequestWithFSO}} from {{keyToDelete != null &&
> !getIsVersionEnabled()}} to also cover the versioned case where the committed
> record carries over none of the previous record's blocks, which is exactly
> the concurrent open shape. A new shared {{hasBlocksNotCarriedOver}} in
> {{OMKeyRequest}} is the predicate. An ordinary versioned overwrite, where the
> open key did inherit the previous version, still takes the untouched {{else}}
> branch, so version retention and its quota arithmetic are unchanged. On the
> orphaning path, {{filterOutBlocksStillInUse}} keeps any block the new record
> does share, and the branch's existing {{decrUsedNamespace}} plus
> {{incrUsedNamespace}} arithmetic nets the inflated namespace count back to 1.
> Scope note, for reviewers to decide: this makes the first version's blocks
> reclaimable and fixes the namespace count. It does not add true version
> retention, which would mean renumbering the committing writer's version group
> and prepending the previous one, a change to version numbering semantics that
> belongs in its own issue. After this patch the first version is still lost,
> as it is today, but its blocks are no longer leaked.
> True version retention is also in scope for the active S3 object versioning
> work (HDDS-15728 and its T1 to T7 subtasks), so that half may belong there
> rather than in a separate fix.
> Covered by {{testVersionedConcurrentOpenDoesNotOrphanTheFirstVersionsBlocks}}
> in the existing {{TestOMKeyCommitRequest}}, inherited by the FSO subclass.
> Two writers commit the same absent key on a versioned bucket; the test
> asserts the first writer's block is enqueued for deletion and that
> {{usedNamespace}} is 1. Verified to fail without the change and pass with it
> on OBS and FSO. Checkstyle clean. The 23 tests in each of those two classes
> 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]
