[
https://issues.apache.org/jira/browse/HDDS-17605?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17605:
------------------------------
Attachment: TestBugCR3StaleDirDeepCleanFlagAfterPurge.java
> Deleting services can mark the wrong snapshot deep cleaned after a snapshot
> rename, or undo the deep clean reset of a snapshot purge
> ------------------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17605
> URL: https://issues.apache.org/jira/browse/HDDS-17605
> Project: Apache Ozone
> Issue Type: Bug
> Affects Versions: 2.0.0, 2.1.0, 2.2.0, 2.1.1, 2.1.2, 2.2.1
> Reporter: Siyao Meng
> Priority: Minor
> Attachments:
> MC-3-recheck-snapshot-before-setting-deep-clean-flag.patch,
> TestBugCR3StaleDirDeepCleanFlagAfterPurge.java,
> TestBugMC3RenamedSnapshotDeepCleanFlag.java
>
>
> h3. Mechanism
> {{KeyDeletingService}} and {{DirectoryDeletingService}} deep clean one
> snapshot per task. When a task has nothing left to purge, it sends
> {{SetSnapshotProperty}} with {{deepCleanedDeletedKey=true}} or
> {{deepCleanedDeletedDir=true}}, and every later task skips a snapshot that
> carries the flag. The flag request names the snapshot only by the table key
> ({{/volume/bucket/name}}) that the task read when it started, and carries
> nothing about the previous snapshot the scan ran against.
> {{OMSnapshotSetPropertyRequest.validateAndUpdateCache}} sets the flag on
> whatever row has that key when it is applied. Unlike {{PurgeKeys}} and
> {{PurgeDirectories}}, which call {{SnapshotUtils.validatePreviousSnapshotId}}
> at apply time, nothing checks that the snapshot is still the one that was
> scanned, against the same previous snapshot.
> Two cases follow.
> * *Rename.* {{OMSnapshotRenameRequest}} takes the bucket lock and the
> snapshot name locks, not the {{SNAPSHOT_GC_LOCK}} the deleting services use,
> so a rename of the scanned snapshot followed by a new snapshot with the old
> name can be applied while the task runs. The flag then lands on the new
> snapshot, which was never scanned. The renamed snapshot stays unmarked and is
> scanned again (harmless).
> * *Purge of the previous snapshot.* When a deleted snapshot is purged,
> {{OMSnapshotPurgeRequest}} sets {{deepClean}} and {{deepCleanedDeletedDir}}
> back to false on the next two snapshots so they are cleaned again against
> their new previous snapshot. {{SnapshotDeletingService}} submits the purge
> after it releases its lock, the purge apply takes no lock, and
> {{DirectoryDeletingService}} submits the directory flag after its workers
> have released their GC locks. A flag built against the old previous snapshot
> can therefore be applied after the purge and overwrite the reset.
> h3. Trigger
> * *Rename:* from ozone-2.2.0 it needs
> {{ozone.om.snapshot.rename.allowed=true}} (default false). ozone-2.0.0 to
> ozone-2.1.2 have no rename switch, but there snapshot deep cleaning is off by
> default ({{ozone.snapshot.deep.cleaning.enabled=false}}; HDDS-15187 turned it
> on by default in 2.2.0), so they are affected only with it enabled. A user
> renames snapshot S and creates a new snapshot named S in the same bucket
> while a deleting service task on S is running. For a
> {{DirectoryDeletingService}} task, which sends the flag only when it purged
> nothing, and for a {{KeyDeletingService}} task that purges nothing, the
> window is the whole task, from the start of the scan to the apply of the
> flag. For a {{KeyDeletingService}} task that purges, it starts at the apply
> of its last {{PurgeKeys}}: a {{PurgeKeys}} applied after the rename goes to
> the old name, fails its previous snapshot check, and no flag follows.
> * *Purge:* no special config from ozone-2.2.0; on ozone-2.1.x only with
> {{ozone.snapshot.deep.cleaning.enabled=true}}. Snapshot S1 is deleted while
> {{DirectoryDeletingService}} deep cleans the next snapshot S2 against S1 and
> finds nothing it can purge, then {{SnapshotDeletingService}} purges S1 (it
> has nothing to move) and the purge is applied before the directory flag. The
> directory workers hold GC read locks on S1 and S2 while they scan, so
> {{SnapshotDeletingService}} can only evaluate S1 after they release them; its
> purge then races the coordinator's flag request through Ratis. This is argued
> from the code; the reproductions control the order and do not measure how
> often the race is won in a running cluster.
> h3. Impact
> The wrong snapshot carries a deep clean flag until a later purge in its chain
> resets it or the snapshot itself is deleted.
> * Rename, key flag: the new snapshot is skipped by every
> {{KeyDeletingService}} run, so its deleted keys that are reclaimable against
> the renamed snapshot are not reclaimed. Rename, directory flag: the same for
> the deleted directories of the new snapshot (shown at flag level only).
> * Purge, directory flag: a deleted directory that became reclaimable against
> the new previous snapshot stays in the snapshot's {{deletedDirTable}},
> together with whatever it holds.
> No data is lost and nothing is reclaimed early; the effect is space that is
> not reclaimed. The same purge mechanism for the key flag is HDDS-16464. At
> this commit that case is already narrow: {{KeyDeletingService}} keeps its GC
> read locks on the snapshot and its previous snapshots until its
> {{SetSnapshotProperty}} is applied, and its filter refuses a previous
> snapshot that is not active, so {{SnapshotDeletingService}} cannot purge the
> previous snapshot of a run that read any entry before the key flag is
> applied. The {{DirectoryDeletingService}} coordinator submits its flag after
> the workers have released those locks, which is why the directory flag is the
> purge case reported here.
> h3. Reproduction
> Two self contained classes for {{hadoop-ozone/ozone-manager}} (target path
> and Maven command in each class comment) run one real OM through
> {{OmTestManagers}}, real {{KeyDeletingTask}} / {{DirDeletingTask}} runs, and
> requests sent through {{OzoneManagerProtocol}} or Ratis. No state is
> injected; the only aid is a hook that runs the rename or purge at a chosen
> point of the task.
> * [^TestBugMC3RenamedSnapshotDeepCleanFlag.java]: with rename allowed,
> snapshot snapa (id1) is renamed to snapb and a new snapa (id2) holding one
> reclaimable deleted key is created after the task's {{PurgeKeys}} request.
> id2 gets {{deepCleanedDeletedKey=true}}, id1 stays false, and a later task on
> id2 leaves the key in id2's {{deletedTable}}.
> * [^TestBugCR3StaleDirDeepCleanFlagAfterPurge.java]: snap0, snap1, snap2 with
> a directory that only snap1 references in snap2's {{deletedDirTable}}. snap1
> is deleted during the {{DirDeletingTask}} on snap2 and purged with the
> request {{SnapshotDeletingService}} sends, applied before the flag. snap2's
> previous becomes snap0 with {{deepCleanedDeletedDir=true}} in RocksDB, and a
> second task on snap2 skips it and leaves the directory in place.
> Both classes also run a case where the change happens after the flag request
> has been built; see Patch.
> h3. Suggested fix
> The strict fix is for {{SetSnapshotPropertyRequest}} to carry the snapshot ID
> and the expected previous snapshot ID, checked at apply time like the purge
> requests. That is a new request field, so it needs a new layout feature to
> keep OMs of two versions applying the same log in agreement (HDDS-16464
> proposes the previous snapshot ID field for the key flag only, without a
> layout feature). The attached patch stays on the submitting side.
> h3. Patch
> [^MC-3-recheck-snapshot-before-setting-deep-clean-flag.patch], against
> 284fad307bfd129fca3f4966d854ac45ea3b0d11. It also applies to master at
> 6135e81a09ac6d618dc0ad4d220dd8cf254586b6.
> A new helper {{SnapshotUtils.getTableKeyToMarkDeepCleaned}}, used by both
> services right before they build the flag request, looks the snapshot's table
> key up again by snapshot ID and returns nothing, so no flag is sent, when the
> snapshot is gone, its previous snapshot is not the one the scan used, or that
> previous snapshot is no longer active. The last condition covers a purge
> already on its way: {{SnapshotDeletingService}} only purges deleted
> snapshots. A skipped flag only means the snapshot is scanned again in the
> next run; exclusive size updates in the same request are sent as before. No
> request, config or apply path changes. Because the helper is shared,
> {{KeyDeletingService}} gets the same check for the key flag, which also
> narrows what is left of the HDDS-16464 case (a run that read no entry and so
> took no GC lock) on the submitting side.
> This narrows the windows, it does not close them: a rename plus a create, or
> a delete, its flush and a {{SnapshotDeletingService}} run, all landing
> between the check and the apply of the flag request (one Ratis round trip)
> still mark the wrong snapshot. Each reproduction class has a case for that
> order, and it still shows the bug with the patch. A rename plus a create in
> the few lines between a task reading the snapshot by ID and opening its DB by
> name also remains: the task then scans the new snapshot while the flag goes
> to the renamed one, and any purge in that run is rejected.
> New tests in existing suites:
> {{TestDirectoryDeletingService#testDeepCleanFlagAfterSnapshotChangeDuringRun}}
> (no change, previous purged during the run, previous deleted with its purge
> applied just before the flag, rename and reuse of the name) ,
> {{TestKeyDeletingService.SnapshotDbHandleLifecycle#testDeepCleanFlagFollowsRenamedSnapshot}}
> and {{TestSnapshotUtils#testTableKeyToMarkDeepCleanedWhenPreviousIsPurged}}
> (the previous snapshot purged between the helper's two lookups counts as not
> active instead of failing the whole request). Without the change the three
> change cases and the rename case fail; with it they pass, as do
> {{TestKeyDeletingService}}, {{TestDirectoryDeletingService}},
> {{TestSnapshotDeletingService}},
> {{TestOMSnapshotSetPropertyRequestAndResponse}},
> {{TestOMSnapshotPurgeRequestAndResponse}}, {{TestSnapshotUtils}} and the
> {{TestReclaimable*}} filter suites (578 tests), and checkstyle is clean.
> Found by TLA+ model checking and code review of the OM snapshot purge, set
> property and double buffer apply paths under HDDS-15926, on commit
> 284fad307bfd129fca3f4966d854ac45ea3b0d11. Checked against HDDS issues and
> apache/ozone pull requests for duplicates before filing. Related but not a
> duplicate: HDDS-16464 (the purge case for the key flag, with a proposed apply
> side check of the previous snapshot ID; it does not cover the directory flag,
> and its text does not discuss renames, though its proposed check would reject
> most of them for the key flag because the new snapshot's previous is the
> renamed one). The attached patch is a proposal for review. Generated with
> Specula (Claude Opus 5.5).
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
