rbd: add support for changed block tracking RPCs #5347

Rakshith-R · 2025-05-30T10:02:39Z

Describe what this PR does

rbd: add support for changed block tracking RPCs

This commit adds support for GetMetadataAllocated
and GetMetadataDelta RPCs.
CSI Snapshot Metadata Service RPCs

Updates: #5346

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!)

internal/rbd/controllerserver.go

internal/rbd/snap_diff.go

Madhu-1 · 2025-06-02T07:01:51Z

internal/rbd/controllerserver.go

+
+	baseRBDSnap, err := genSnapFromSnapID(ctx, baseSnapshotID, cr, req.GetSecrets())
+	if err != nil {
+		return status.Error(codes.Internal, err.Error())


Need to handle NotFound?

internal/rbd/controllerserver.go

internal/rbd/identityserver.go

internal/rbd/snap_diff.go

Rakshith-R · 2025-06-02T12:29:19Z

Thanks for the quick detailed review

The new main modifications are:

moved the new calls to snap_diff.go file
debug logs for feature support check errors
refactored to have a single flow with sendStreamResponse handling which response to send allocated or delta
modified return statement and ensured they are formatted and have correct error codes
added validation for req parameters
added check for snapshot's existence + returns correct error code now
added additional checks for startingOffset (add out of range error code as specified in the spec)
additional checks for maxResults and internal value to limit memory usage ([spec](spec allows us to send lower number of blocks as well))

failing golang lintci is being handled in #5351

Please take a look again

mergify · 2025-06-02T15:19:20Z

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

internal/rbd/snap_diff.go

nixpanic

1st pass going through this. Needs cleanup and clear splitting of internal RBD things and ControllerServer Snapshot usage. Extend the types.Snapshot interface with required functions, so that ControllerServer does not need to use rbdSnap types directly.

internal/rbd/features/features.go

nixpanic · 2025-06-03T07:50:10Z

internal/rbd/snap_diff.go

+
+	// streamDoneErrCode is a custom error code used to indicate that the
+	// stream was closed by the client.
+	streamDoneErrCode = -1


Is there a reason to not use google.golang.org/grpc/codes constants? Or, maybe base it on return values of read(), like EOF and so on?

internal/rbd/snap_diff.go

internal/rbd/snapshot.go

internal/rbd/snap_diff.go

PendingReleaseNotes.md

nixpanic · 2025-06-05T12:52:57Z

internal/rbd/errors/errors.go

 )
+
+type ErrorCode interface {
+	// Code returns the error code.


Comment Code -> ErrorCode.

Can you also add a comment for the interface itself, with a small explanation how (which kind of errors) it can be used?

nixpanic · 2025-06-05T13:00:09Z

internal/rbd/snap_diff.go

+// StreamMetadata streams the block metadata for a snapshot.
+// If baseRBDSnap is provided, it calculates the delta between the
+// current snapshot and the base snapshot.
+func (rbdSnap *rbdSnapshot) StreamMetadata(


I have the feeling this function does too much. The rbdSnapshot struct should probably not send anything, but instead return []*csi.BlockMetadata to the caller which can then send it out? Would that not make it a cleaner implementation?

I have the feeling this function does too much.

Broke down this function to include multiple helpers

The rbdSnapshot struct should probably not send anything, but instead return []*csi.BlockMetadata to the caller which can then send it out? Would that not make it a cleaner implementation?

It's a streaming API, so we would be sending more than once.
Used this as inspiration to instead send down a func that only takes []*csi.BlockMetadata as input and send stream response internally.

Looks easier to understand already, thanks.

I understand that passing sendResponse makes things easier. It ties CSI+rbd functionality very closely together, which isn't very nice. But librbd.DiffIterateByIDConfig{} needs the callback, so I'm also not sure how to split the CSI/rbd things more. This is fine for now, maybe we can come up with an alternative approach later.

internal/rbd/snapshot.go

internal/rbd/types/snapshot.go

internal/rbd/snap_diff.go

internal/rbd/types/snapshot.go

Madhu-1 · 2025-07-08T11:00:53Z

@Mergifyio queue

mergify · 2025-07-08T11:01:18Z

queue

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

The pull request can't be updated.

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

mergify · 2025-07-08T11:01:28Z

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

The pull request can't be updated.

You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

Madhu-1 · 2025-07-08T11:05:02Z

@Mergifyio rebase

Signed-off-by: Rakshith R <rar@redhat.com>

This commit adds support for GetMetadataAllocated and GetMetadataDelta RPCs. Signed-off-by: Rakshith R <rar@redhat.com>

mergify · 2025-07-08T11:05:17Z

rebase

✅ Branch has been successfully rebased

Madhu-1 · 2025-07-08T11:05:38Z

@Mergifyio queue

mergify · 2025-07-08T11:05:46Z

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 about the failure.

ceph-csi-bot · 2025-07-08T11:06:00Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2025-07-08T11:06:01Z

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

ceph-csi-bot · 2025-07-08T11:06:01Z

/test ci/centos/upgrade-tests-rbd

ceph-csi-bot · 2025-07-08T11:06:01Z

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

ceph-csi-bot · 2025-07-08T11:06:02Z

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

ceph-csi-bot · 2025-07-08T11:06:03Z

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

ceph-csi-bot · 2025-07-08T11:06:04Z

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

ceph-csi-bot · 2025-07-08T11:06:05Z

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

ceph-csi-bot · 2025-07-08T11:06:05Z

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

ceph-csi-bot · 2025-07-08T11:06:06Z

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

ceph-csi-bot · 2025-07-08T11:06:07Z

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

mergify · 2025-07-08T11:39:44Z

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:

❌ multi-arch-build

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

Rakshith-R · 2025-07-08T11:53:50Z

@Mergifyio requeue

mergify · 2025-07-08T11:54:17Z

requeue

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

mergify bot added the component/rbd Issues related to RBD label May 30, 2025

Rakshith-R mentioned this pull request May 30, 2025

Add support for Changed Block Tracking feature #5346

Open

4 tasks

Rakshith-R requested review from a team May 30, 2025 10:04

Nikhil-Ladha requested changes Jun 2, 2025

View reviewed changes

internal/rbd/controllerserver.go Outdated Show resolved Hide resolved

internal/rbd/controllerserver.go Outdated Show resolved Hide resolved

internal/rbd/controllerserver.go Outdated Show resolved Hide resolved

Rakshith-R force-pushed the add-snapshot-diff branch from b60a6f5 to 3892027 Compare June 2, 2025 07:06

Madhu-1 requested changes Jun 2, 2025

View reviewed changes

Rakshith-R force-pushed the add-snapshot-diff branch 2 times, most recently from 85a53a1 to 288a354 Compare June 2, 2025 10:43

Rakshith-R mentioned this pull request Jun 2, 2025

util: pass Servers by reference to Start() #5351

Merged

6 tasks

Rakshith-R force-pushed the add-snapshot-diff branch 2 times, most recently from 56db368 to 6a3d211 Compare June 2, 2025 12:18

Rakshith-R requested review from Madhu-1 and Nikhil-Ladha June 2, 2025 12:29

iPraveenParihar reviewed Jun 3, 2025

View reviewed changes

internal/rbd/snap_diff.go Outdated Show resolved Hide resolved

nixpanic reviewed Jun 3, 2025

View reviewed changes

Rakshith-R force-pushed the add-snapshot-diff branch 3 times, most recently from 72390e5 to 4ff0a22 Compare June 5, 2025 12:26

nixpanic reviewed Jun 5, 2025

View reviewed changes

internal/rbd/snap_diff.go Outdated Show resolved Hide resolved

internal/rbd/types/snapshot.go Outdated Show resolved Hide resolved

Rakshith-R force-pushed the add-snapshot-diff branch 7 times, most recently from 9c9afb4 to 755348e Compare June 20, 2025 11:25

Nikhil-Ladha approved these changes Jul 8, 2025

View reviewed changes

Rakshith-R added 3 commits July 8, 2025 11:05

doc: add release note for changed block tracking feature

dde7f55

Signed-off-by: Rakshith R <rar@redhat.com>

doc: add documentation for CBT feature

f771409

Signed-off-by: Rakshith R <rar@redhat.com>

rbd: add support for changed block tracking RPCs

9d66e6c

This commit adds support for GetMetadataAllocated and GetMetadataDelta RPCs. Signed-off-by: Rakshith R <rar@redhat.com>

Madhu-1 force-pushed the add-snapshot-diff branch from 9982594 to 9d66e6c Compare July 8, 2025 11:05

mergify bot added the ok-to-test Label to trigger E2E tests label Jul 8, 2025

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

mergify bot merged commit 66b3e23 into ceph:devel Jul 8, 2025
37 of 38 checks passed

iPraveenParihar mentioned this pull request Jul 18, 2025

Alpha feature: RBD snapshot metadata sidecar ceph/ceph-csi-operator#274

Merged

6 tasks

rbd: add support for changed block tracking RPCs #5347

rbd: add support for changed block tracking RPCs #5347

Uh oh!

Conversation

Rakshith-R commented May 30, 2025

Describe what this PR does

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Madhu-1 Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

mergify bot commented Jun 2, 2025

Uh oh!

Uh oh!

nixpanic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nixpanic Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nixpanic Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

nixpanic Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

Rakshith-R Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

nixpanic Jul 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Madhu-1 commented Jul 8, 2025

Uh oh!

mergify bot commented Jul 8, 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!

mergify bot commented Jul 8, 2025

Uh oh!

Madhu-1 commented Jul 8, 2025

Uh oh!

mergify bot commented Jul 8, 2025

✅ Branch has been successfully rebased

Uh oh!

Madhu-1 commented Jul 8, 2025

Uh oh!

mergify bot commented Jul 8, 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 Jul 8, 2025

Uh oh!

Rakshith-R commented Jun 2, 2025 •

edited

Loading

mergify bot commented Jul 8, 2025 •

edited

Loading

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

mergify bot commented Jul 8, 2025 •

edited

Loading

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