rbd: use ListChildrenAttributes() instead of ListChildren() #5206

Rakshith-R · 2025-03-10T10:24:10Z

Describe what this PR does

This commit modifies listSnapAndChildren() to make use of ListChildrenAttributes() instead of ListChildren() which allows us to filter out images in trash.
This commit also order the alive images so that temp clone images are followed by images backing volume snapshots so that temp clone images are flattened first.

Is the change backward compatible?
Yes

Are there concerns around backward compatibility?
No

Related issues

Fixes: #5173

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.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

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 · 2025-03-10T10:24:35Z

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

Rakshith-R · 2025-03-11T06:48:41Z

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

nixpanic · 2025-03-11T08:38:19Z

internal/rbd/rbd_util.go


-	// ListChildren() returns pools, images, err.
-	_, children, err := image.ListChildren()
+	// ListChildrenAttributes() returns child ImageSpec list, err.


you can drop this comment, I think the function is rather clear

nixpanic · 2025-03-11T08:53:31Z

internal/rbd/rbd_util.go

+		if child.Trash {
+			continue
+		}
+		if strings.HasSuffix(child.ImageName, "-temp") {


Maybe consider adding a isTempImage(child) function?

nixpanic · 2025-03-11T08:55:28Z

internal/rbd/rbd_util.go

+		}
+	}
+	// order the temp clone images first followed by the volume snapshots
+	// so that the temp clones are flattened first.


This is the 2nd comment about temp clones are flattened first. I don't get why that is the case. If that is a requirement of the returned list, mention something about the order. Or wherever the list is consumed, use some other way of inspecting the list. Might be cleaner to return two lists, so that the distinction is very clear.

This is the 2nd comment about temp clones are flattened first. I don't get why that is the case. If that is a requirement of the returned list, mention something about the order. Or wherever the list is consumed, use some other way of inspecting the list. Might be cleaner to return two lists, so that the distinction is very clear.

I've removed it from the function definition, since we still are returning full list of child images which are not in trash.

I've expanded the comment in this section with reasons why such ordering is done so it can be understood in the future.

Thanks, that makes things a little bit clearer. I don't understand (yet) if, and why callers need to care about the order in the list that is returned. If callers need to care, or benefit from it, why not return separate lists? Or, a struct like { snaps, tempChildren, volSnapChildren } so that callers of the function are very aware of the differences, and can decide how to handle them?

Thanks, that makes things a little bit clearer. I don't understand (yet) if, and why callers need to care about the order in the list that is returned. If callers need to care, or benefit from it, why not return separate lists? Or, a struct like { snaps, tempChildren, volSnapChildren } so that callers of the function are very aware of the differences, and can decide how to handle them?

I've moved the logic to order images into the caller and added a struct for returned parameters.
PTAL

nixpanic

Thanks, makes it much cleaner. Just a nit that needs addressing.

nixpanic · 2025-03-11T12:10:20Z

internal/rbd/rbd_util.go


-// listSnapAndChildren returns list of names of snapshots and child images.
-func (ri *rbdImage) listSnapAndChildren() ([]librbd.SnapInfo, []string, error) {
+type SnapAndChildrenInfo struct {


No need to export this, use snapAndChildrenInfo instead.

nixpanic · 2025-03-11T12:35:59Z

internal/rbd/controllerserver.go

+	// changed block tracking(rbd snap diff).
+	// - favor scalability since multiple PVCs can be restored from same
+	// volumesnapshot versus just one PVC-PVC clone.
+	children := append(snapAndChildrenInfo.TempCloneChildren, snapAndChildrenInfo.VolumeSnapshotChildren...)


golangci-lint complains about this. append() may modify the 1st parameter (snapAndChildrenInfo), which could cause confusion later on in case it is used again (docs).

Instead, you could do it like this:

children := make([]string, len(snapAndChildrenInfo.TempCloneChildren) + len(snapAndChildrenInfo.VolumeSnapshotChildren)) children = append(children, snapAndChildrenInfo.TempCloneChildren...) children = append(children, snapAndChildrenInfo.VolumeSnapshotChildren...)

This commit modifies listSnapAndChildren() to make use of ListChildrenAttributes() instead of ListChildren() which allows us to filter out images in trash. This commit also order the alive images so that temp clone images are followed by images backing volume snapshots so that temp clone images are flattened first. Signed-off-by: Rakshith R <[email protected]>

Pull request has been modified.

Rakshith-R · 2025-03-11T12:55:26Z

golangci-lint complains about this. append() may modify the 1st parameter (snapAndChildrenInfo), which could cause confusion later on in case it is used again (docs).

Instead, you could do it like this:
children := make([]string, len(snapAndChildrenInfo.TempCloneChildren) + len(snapAndChildrenInfo.VolumeSnapshotChildren))
children = append(children, snapAndChildrenInfo.TempCloneChildren...)
children = append(children, snapAndChildrenInfo.VolumeSnapshotChildren...)

It needs to be initialized with zero length :(
ci passes now
ptal

Rakshith-R · 2025-03-12T05:47:20Z

@Mergifyio queue

mergify · 2025-03-12T05:47:27Z

queue

🛑 The pull request has been removed from the queue `default`

The merge conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

ceph-csi-bot · 2025-03-12T05:47:39Z

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

ceph-csi-bot · 2025-03-12T05:47:40Z

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

ceph-csi-bot · 2025-03-12T05:47:40Z

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

ceph-csi-bot · 2025-03-12T05:47:41Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2025-03-12T05:47:41Z

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

ceph-csi-bot · 2025-03-12T05:47:41Z

/test ci/centos/upgrade-tests-rbd

ceph-csi-bot · 2025-03-12T05:47:41Z

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

ceph-csi-bot · 2025-03-12T05:47:42Z

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

ceph-csi-bot · 2025-03-12T05:47:43Z

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

ceph-csi-bot · 2025-03-12T05:47:43Z

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

ceph-csi-bot · 2025-03-12T05:47:44Z

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

mergify · 2025-03-12T05:49:05Z

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You may have to fix your CI before adding the pull request to the queue again.

If you want to requeue this pull request, you can post a @mergifyio requeue comment.

ceph-csi-bot · 2025-03-12T06:00:10Z

/retest ci/centos/upgrade-tests-rbd

ceph-csi-bot · 2025-03-12T06:00:11Z

@Rakshith-R "ci/centos/upgrade-tests-rbd" test failed. Logs are available at location for debugging

ceph-csi-bot · 2025-03-12T06:00:11Z

/retest ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2025-03-12T06:00:12Z

@Rakshith-R "ci/centos/upgrade-tests-cephfs" test failed. Logs are available at location for debugging

ceph-csi-bot · 2025-03-12T06:00:12Z

@Mergifyio requeue

mergify · 2025-03-12T06:00:18Z

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

ceph-csi-bot · 2025-03-12T06:41:12Z

/retest ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2025-03-12T06:41:12Z

@Rakshith-R "ci/centos/upgrade-tests-cephfs" test failed. Logs are available at location for debugging

ceph-csi-bot · 2025-03-12T06:41:13Z

/retest ci/centos/upgrade-tests-rbd

ceph-csi-bot · 2025-03-12T06:41:14Z

@Rakshith-R "ci/centos/upgrade-tests-rbd" test failed. Logs are available at location for debugging

ceph-csi-bot · 2025-03-12T06:41:14Z

@Mergifyio requeue

mergify · 2025-03-12T06:41:20Z

requeue

☑️ This pull request is already queued

Rakshith-R · 2025-03-12T07:21:20Z

Upgrade tests are failing due to
#5211

Rakshith-R · 2025-03-12T08:32:48Z

/retest ci/centos/upgrade-tests-rbd

Rakshith-R · 2025-03-12T08:33:04Z

/retest ci/centos/upgrade-tests-cephfs

Rakshith-R · 2025-03-12T08:33:21Z

@Mergifyio requeue

mergify · 2025-03-12T08:33:30Z

requeue

☑️ This pull request is already queued

mergify bot added the component/rbd Issues related to RBD label Mar 10, 2025

nixpanic reviewed Mar 11, 2025

View reviewed changes

Rakshith-R force-pushed the list-child-images-with-attributes branch from 72596fb to a3dd6d2 Compare March 11, 2025 09:17

Rakshith-R requested review from a team and nixpanic March 11, 2025 09:17

Rakshith-R force-pushed the list-child-images-with-attributes branch 2 times, most recently from ee0e32a to db3dd12 Compare March 11, 2025 10:18

nixpanic reviewed Mar 11, 2025

View reviewed changes

Rakshith-R force-pushed the list-child-images-with-attributes branch 2 times, most recently from 83e17f9 to a4ab561 Compare March 11, 2025 12:40

nixpanic previously approved these changes Mar 11, 2025

View reviewed changes

Rakshith-R force-pushed the list-child-images-with-attributes branch from a4ab561 to 0a35f96 Compare March 11, 2025 12:50

Rakshith-R requested a review from nixpanic March 11, 2025 12:55

nixpanic approved these changes Mar 11, 2025

View reviewed changes

nixpanic requested a review from a team March 11, 2025 15:02

yati1998 approved these changes Mar 12, 2025

View reviewed changes

mergify bot added the ok-to-test Label to trigger E2E tests label Mar 12, 2025

ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Mar 12, 2025

Rakshith-R added the ci/retry/e2e Label to retry e2e retesting on approved PR's label Mar 12, 2025

Rakshith-R removed the ci/retry/e2e Label to retry e2e retesting on approved PR's label Mar 12, 2025

mergify bot merged commit 796e6b6 into ceph:devel Mar 12, 2025
40 checks passed

Uh oh!

rbd: use ListChildrenAttributes() instead of ListChildren() #5206

rbd: use ListChildrenAttributes() instead of ListChildren() #5206

Uh oh!

Conversation

Rakshith-R commented Mar 10, 2025

Describe what this PR does

Related issues

Uh oh!

Rakshith-R commented Mar 10, 2025

Uh oh!

Rakshith-R commented Mar 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nixpanic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rakshith-R commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rakshith-R commented Mar 12, 2025

Uh oh!

mergify bot commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🛑 The pull request has been removed from the queue default

Uh oh!

ceph-csi-bot commented Mar 12, 2025

Uh oh!

ceph-csi-bot commented Mar 12, 2025

Uh oh!

ceph-csi-bot commented Mar 12, 2025

Uh oh!

ceph-csi-bot commented Mar 12, 2025

Uh oh!

ceph-csi-bot commented Mar 12, 2025

Uh oh!

ceph-csi-bot commented Mar 12, 2025

Uh oh!

ceph-csi-bot commented Mar 12, 2025

Uh oh!

ceph-csi-bot commented Mar 12, 2025

Uh oh!

ceph-csi-bot commented Mar 12, 2025

Uh oh!

ceph-csi-bot commented Mar 12, 2025

Uh oh!

ceph-csi-bot commented Mar 12, 2025

Uh oh!

mergify bot commented Mar 12, 2025

Uh oh!

ceph-csi-bot commented Mar 12, 2025

Uh oh!

ceph-csi-bot commented Mar 12, 2025

Uh oh!

ceph-csi-bot commented Mar 12, 2025

Uh oh!

ceph-csi-bot commented Mar 12, 2025

Uh oh!

ceph-csi-bot commented Mar 12, 2025

Uh oh!

mergify bot commented Mar 12, 2025

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

Uh oh!

ceph-csi-bot commented Mar 12, 2025

Uh oh!

Rakshith-R commented Mar 11, 2025 •

edited

Loading

mergify bot commented Mar 12, 2025 •

edited

Loading

🛑 The pull request has been removed from the queue `default`