rbd: replace O(n) trash list scan with O(1) ID-based trash removal by Rakshith-R · Pull Request #6472 · ceph/ceph-csi

Rakshith-R · 2026-08-14T10:01:49Z

Describe what this PR does

Replace ensureImageCleanup() - which scans the entire RBD trash list
to find an image by name - with direct O(1) removal by image ID using
the new removeImageFromTrash() helper.

The image ID is always available: modern volumes store it in OMAP at
creation time, and legacy volumes get it backfilled by storeImageID()
during the first generateVolumeFromVolumeID() call.

Commit 1 - rbd: capture parent image ID in getImageInfo

Add ParentImageID field to rbdImage and populate it in
getImageInfo() from GetParent().Image.ImageID. Valid even when the
parent is in trash.

Commit 2 - rbd: make trash remove idempotent and add removeImageFromTrash

Add removeImageFromTrash() as an O(1) helper that removes a trashed
image by its known ID. Make trashRemoveImage() treat ENOENT as success
on both the ceph mgr and direct paths (image already purged).

Commit 3 - rbd: avoid trash ls in DeleteVolume/DeleteSnapshot retry paths

In checkErrAndUndoReserve() and cleanUpImageAndSnapReservation(),
switch from ensureImageCleanup() to removeImageFromTrash(). Also fix
a pre-existing ErrorLog format string mismatch (2 format verbs, 3 args).

Commit 4 - rbd: remove ensureImageCleanup and use ID-based trash removal

In DeleteTempImage(), use the volume's ParentImageID to identify a
trashed temp clone instead of scanning the trash list. Remove
ensureImageCleanup() entirely - no callers remain.

Is there anything that requires special attention

DeleteTempImage() relies on being called before rbdVol.Delete() so
that GetParent() data is still valid. This ordering already exists in
cleanupRBDImage() and is now documented with a comment.
When parent info is unavailable in DeleteTempImage() (temp clone not
in trash), it returns success - same behavior as the old code when the
trash scan found nothing.

The change is backward compatible. No API or OMAP schema changes.

Future concerns

GetTrashList is still used in waitForCleanTrash() (e2e test helper).
That is unrelated to production code paths.

Checklist:

Commit Message Formatting: Commit titles and messages follow
guidelines in the developer
guide.
Reviewed the developer guide on Submitting a Pull
Request
Pending release
notes
updated with breaking and/or notable changes for the next major release.
N/A - internal optimization, no user-facing changes.
Documentation has been updated, if necessary.
N/A - no doc changes needed, internal refactor only.
Unit tests have been added, if necessary.
N/A - functions require a live Ceph cluster (librbd calls), not unit-testable.
Existing e2e delete/snapshot tests exercise these code paths.
Integration tests have been added, if necessary.
N/A - existing e2e tests for DeleteVolume, DeleteSnapshot, and volume
cloning already cover the trash cleanup paths exercised by this change.

Show available bot commands

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

/retest ci/centos/<job-name>: retest the <job-name> after unrelated
failure (please report the failure too!)

Rakshith-R · 2026-08-14T10:02:35Z

/test ci/centos/mini-e2e-operator/k8s-1.36

Copilot

Pull request overview

This PR refactors RBD deletion/trash-cleanup paths to avoid O(n) trash-list scans by switching to ID-based trash removal, and propagates parent image IDs to enable faster cleanup of temporary clone images during volume clone workflows.

Changes:

Add ParentImageID to rbdImage and populate it via getImageInfo() to enable identifying trashed parents by ID.
Make trash removal idempotent on ENOENT and introduce removeImageFromTrash() as a helper for ID-based trash purge.
Update delete retry/cleanup paths (DeleteVolume/DeleteSnapshot and temp-clone cleanup) to use ID-based trash removal and fix a logging format mismatch.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
internal/rbd/rbd_util.go	Adds `ParentImageID`, introduces `removeImageFromTrash()`, updates `DeleteTempImage()` to remove temp clones from trash by ID, and makes trash removal idempotent.
internal/rbd/controllerserver.go	Switches retry/cleanup paths to `removeImageFromTrash()` and updates a log line to fix formatting.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

YiteGu · 2026-08-14T10:30:04Z

@Rakshith-R I've been thinking about how to solve this problem lately. This PR is very very nice.

nixpanic

Thanks!

nixpanic · 2026-08-17T09:46:26Z

/queue

ceph-csi-bot · 2026-08-17T09:46:48Z

@Mergifyio rebase

mergify · 2026-08-17T09:47:07Z

rebase

🛑 The pull request rule doesn't match anymore

Details

This action has been cancelled.

Store the parent image's ID (from GetParent().Image.ImageID) in a new ParentImageID field on rbdImage. This ID is valid even when the parent is in trash, enabling O(1) trash removal without scanning the entire trash list. Reset ParentImageID to empty when GetParent returns ErrNotFound (no parent or parent link severed by flatten). Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: Rakshith R <rar@redhat.com>

Make trashRemoveImage() treat ENOENT as success on both the ceph mgr (AddTrashRemove) and direct (TrashRemove) paths - the image was already purged from trash. The mgr path checks rados.ErrNotFound before isCephMgrSupported to avoid misclassifying ENOENT as a mgr failure. Add removeImageFromTrash() as an O(1) helper that removes a trashed image by its known ID. Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: Rakshith R <rar@redhat.com>

In checkErrAndUndoReserve() and cleanUpImageAndSnapReservation(), use removeImageFromTrash() to remove the trashed image directly by its known ID from the OMAP journal instead of scanning the entire trash list with ensureImageCleanup(). The image ID is always available in OMAP: modern volumes store it at creation time, and legacy volumes get it backfilled by storeImageID() during the first (successful) generateVolumeFromVolumeID() call while the image is still alive. Also fix pre-existing ErrorLog format string mismatch in cleanUpImageAndSnapReservation (2 format verbs but 3 arguments). Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: Rakshith R <rar@redhat.com>

In DeleteTempImage(), use the volume's parent info (ParentImageID) to identify a trashed temp clone by ID instead of scanning the trash list. This works because cleanupRBDImage() calls DeleteTempImage() before rbdVol.Delete(), so the child image still exists and GetParent() data is valid. When parent info is unavailable (temp clone not in trash), treat it as already removed and return success. Remove ensureImageCleanup() entirely - the O(n) GetTrashList scan is no longer needed. All volume/snapshot image IDs are stored in OMAP at creation time and backfilled by storeImageID() on first access, so ImageID is always available for direct O(1) trash removal. Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: Rakshith R <rar@redhat.com>

ceph-csi-bot · 2026-08-17T09:47:36Z

/test ci/centos/k8s-e2e-external-storage/1.34

ceph-csi-bot · 2026-08-17T09:47:37Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2026-08-17T09:47:37Z

/test ci/centos/k8s-e2e-external-storage/1.36

ceph-csi-bot · 2026-08-17T09:47:37Z

/test ci/centos/mini-e2e-helm/k8s-1.34

ceph-csi-bot · 2026-08-17T09:47:37Z

/test ci/centos/k8s-e2e-external-storage/1.35

ceph-csi-bot · 2026-08-17T09:47:37Z

/test ci/centos/upgrade-tests-rbd

ceph-csi-bot · 2026-08-17T09:47:38Z

/test ci/centos/mini-e2e-helm/k8s-1.36

ceph-csi-bot · 2026-08-17T09:47:38Z

/test ci/centos/mini-e2e/k8s-1.34

ceph-csi-bot · 2026-08-17T09:47:38Z

/test ci/centos/mini-e2e-helm/k8s-1.35

ceph-csi-bot · 2026-08-17T09:47:39Z

/test ci/centos/mini-e2e/k8s-1.35

ceph-csi-bot · 2026-08-17T09:47:39Z

/test ci/centos/mini-e2e/k8s-1.36

YiteGu · 2026-08-17T17:48:37Z

 		if vol.ParentName != "" {
 			depth++
 		}
 		vol.RbdImageName = vol.ParentName


Now that we have ParentImageID, we need to add an assignment here.
vol.ImageID = vol.ParentImageID

Now that we have ParentImageID, we need to add an assignment here. vol.ImageID = vol.ParentImageID

it's not used.

YiteGu · 2026-08-17T18:05:28Z

 		return nil, err
 	}

 	image, err := librbd.OpenImage(ri.ioctx, ri.RbdImageName, librbd.NoSnapshot)


If the imageID comes from the parentImageID, and the parentImage is in the trash, we should use librbd.OpenImageById(ri.ioctx, ri.ImageID, librbd.NoSnapshot).

If the imageID comes from the parentImageID, and the parentImage is in the trash, we should use librbd.OpenImageById(ri.ioctx, ri.ImageID, librbd.NoSnapshot).

There's no use case right now. We can add it when it is required.

One scenario I'm considering is that when obtaining clone depth, the presence of the parent image in the trash might cause inaccurate clone depth calculations.

if err != nil { // if the parent image is moved to trash the name will be present // in rbd image info but the image will be in trash, in that case // return the found depth if errors.Is(err, rbderrors.ErrImageNotFound) { return depth, nil }

There's a comment in code regarding this.
Including trashed image in clonedepth calculation will lead to other complexities since trashed image cannot be flattened. Selective flattening logic in cephcsi is already complex. I would prefer it stay as is until there's a issue found relating to the scenario.

iPraveenParihar · 2026-08-18T04:07:21Z

/retest ci/centos/mini-e2e-helm/k8s-1.36

mergify · 2026-08-18T06:51:21Z

Deprecation notice: This pull request comes from a fork and was queued with update_method=rebase and update_bot_account impersonation. This capability will be removed on July 1, 2026. After this date, the merge queue will no longer be able to rebase fork pull requests with this configuration. To avoid disruption, switch to update_method=merge in your queue rule.

mergify · 2026-08-18T06:51:44Z

Merge Queue Status

✅ Entered queue — 2026-08-18 06:51 UTC · Rule: default · triggered by merge protections
✅ Checks skipped · PR is already up-to-date
✅ Merged — 2026-08-18 06:52 UTC · at 0cf959b9f1bcd3b0548f66dbaacd6775ee1f6bb7 · rebase

This pull request spent 48 seconds in the queue, including 6 seconds running CI.

Required conditions to merge

github-review-approved [🛡 GitHub branch protection]
- rbd: replace O(n) trash list scan with O(1) ID-based trash removal #6472

Rakshith-R requested review from a team as code owners August 14, 2026 10:01

Copilot AI lite review requested due to automatic review settings August 14, 2026 10:01

Copilot started reviewing on behalf of Rakshith-R August 14, 2026 10:02 View session

mergify Bot added the component/rbd Issues related to RBD label Aug 14, 2026

Copilot AI reviewed Aug 14, 2026

View reviewed changes

Comment thread internal/rbd/rbd_util.go

Comment thread internal/rbd/rbd_util.go

Rakshith-R force-pushed the trash-cleanup-by-id branch from 6c6efd6 to e0e1fe0 Compare August 14, 2026 10:35

iPraveenParihar approved these changes Aug 17, 2026

View reviewed changes

nixpanic approved these changes Aug 17, 2026

View reviewed changes

ceph-csi-bot added the queued/rebase label Aug 17, 2026

nixpanic mentioned this pull request Aug 17, 2026

rbd: avoid GetTrashList in ensureImageCleanup by using ImageID #6468

Closed

Rakshith-R added 4 commits August 17, 2026 09:47

ceph-csi-bot force-pushed the trash-cleanup-by-id branch from e0e1fe0 to 0cf959b Compare August 17, 2026 09:47

ceph-csi-bot added ok-to-test Label to trigger E2E tests and removed queued/rebase labels Aug 17, 2026

ceph-csi-bot added ci/in-progress/e2e This label acts like a guard and prevents Mergify from adding the `ok-to-test` label again. and removed ok-to-test Label to trigger E2E tests labels Aug 17, 2026

mergify Bot removed the ci/in-progress/e2e This label acts like a guard and prevents Mergify from adding the `ok-to-test` label again. label Aug 17, 2026

YiteGu reviewed Aug 17, 2026

View reviewed changes

mergify Bot merged commit 0c9084c into ceph:devel Aug 18, 2026
44 checks passed

Rakshith-R mentioned this pull request Aug 18, 2026

DFBUGS-10065: rbd: replace O(n) trash list scan with O(1) ID-based trash removal red-hat-storage/ceph-csi#852

Merged

2 tasks

nixpanic added the backport-to-release-v3.17 Label to backport from devel to release-v3.17 branch label Aug 18, 2026

mergify Bot mentioned this pull request Aug 18, 2026

rbd: replace O(n) trash list scan with O(1) ID-based trash removal (backport #6472) #6484

Draft

6 tasks

Uh oh!

Conversation

Rakshith-R commented Aug 14, 2026

Describe what this PR does

Is there anything that requires special attention

Future concerns

Uh oh!

Rakshith-R commented Aug 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

YiteGu commented Aug 14, 2026

Uh oh!

nixpanic left a comment

Choose a reason for hiding this comment

Uh oh!

nixpanic commented Aug 17, 2026

Uh oh!

ceph-csi-bot commented Aug 17, 2026

Uh oh!

mergify Bot commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🛑 The pull request rule doesn't match anymore

Uh oh!

ceph-csi-bot commented Aug 17, 2026

Uh oh!

ceph-csi-bot commented Aug 17, 2026

Uh oh!

ceph-csi-bot commented Aug 17, 2026

Uh oh!

ceph-csi-bot commented Aug 17, 2026

Uh oh!

ceph-csi-bot commented Aug 17, 2026

Uh oh!

ceph-csi-bot commented Aug 17, 2026

Uh oh!

ceph-csi-bot commented Aug 17, 2026

Uh oh!

ceph-csi-bot commented Aug 17, 2026

Uh oh!

ceph-csi-bot commented Aug 17, 2026

Uh oh!

ceph-csi-bot commented Aug 17, 2026

Uh oh!

ceph-csi-bot commented Aug 17, 2026

Uh oh!

YiteGu Aug 17, 2026

Choose a reason for hiding this comment

Uh oh!

Rakshith-R Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

YiteGu Aug 17, 2026

Choose a reason for hiding this comment

Uh oh!

Rakshith-R Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

YiteGu Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

Rakshith-R Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

iPraveenParihar commented Aug 18, 2026

Uh oh!

mergify Bot commented Aug 18, 2026

Uh oh!

mergify Bot commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Uh oh!

Uh oh!

Reviewers

Assignees

mergify Bot commented Aug 17, 2026 •

edited

Loading

mergify Bot commented Aug 18, 2026 •

edited

Loading