rbd: fix DiffIterateByID Length calculation and add callback tests by kaovilai · Pull Request #6200 · ceph/ceph-csi

kaovilai · 2026-03-24T22:44:20Z

Summary

Fix: DiffIterateByIDConfig.Length was set to VolSize (total volume size), but rbd_diff_iterate3 treats len as bytes to scan from ofs. When startingOffset > 0, this scanned [startingOffset, startingOffset + VolSize) instead of [startingOffset, VolSize). Fixed by using VolSize - startingOffset.
Tests: Add 7 unit tests for createDiffIterateByIDCB covering block collection, LUKS padding adjustment, batching, error propagation, context cancellation, and edge cases.

Note

This project requires CGo (Ceph C libraries librbd.h, librados.h) to compile. The tests cannot be validated on a standard dev machine without these libraries — relying on CI to verify.

Test plan

CI passes go test ./internal/rbd/ -run Test_createDiffIterateByIDCB
CI passes go test ./internal/rbd/ -run Test_handleDiffIterateError
Existing tests unaffected

Ref: kaovilai/cephcsi-cbt-e2e#2

🤖 Generated with Claude Code
via Happy

kaovilai · 2026-03-24T22:52:58Z

Note

Responses generated with Claude

Local Test Results

All tests pass when run in the project's test container:

podman run --rm -v "$(pwd):/go/src/github.com/ceph/ceph-csi" quay.io/cephcsi/cephcsi:test \
  go test -tags='ceph_preview' -mod=vendor -v \
  -run 'Test_createDiffIterateByIDCB|Test_handleDiffIterateError' ./internal/rbd/

--- PASS: Test_createDiffIterateByIDCB (0.00s)
    --- PASS: Test_createDiffIterateByIDCB/collects_single_block_without_LUKS_padding (0.00s)
    --- PASS: Test_createDiffIterateByIDCB/returns_Unknown_code_on_sendResponse_error (0.00s)
    --- PASS: Test_createDiffIterateByIDCB/batches_at_maxResults_and_resets_slice (0.00s)
    --- PASS: Test_createDiffIterateByIDCB/returns_Canceled_on_context_cancellation (0.00s)
    --- PASS: Test_createDiffIterateByIDCB/adjusts_offset_for_LUKS_header_padding (0.00s)
    --- PASS: Test_createDiffIterateByIDCB/zero_blocks_produces_no_batches (0.00s)
    --- PASS: Test_createDiffIterateByIDCB/multiple_batches_with_remainder (0.00s)
--- PASS: Test_handleDiffIterateError (0.00s)
    --- PASS: Test_handleDiffIterateError/nil_error (0.00s)
    --- PASS: Test_handleDiffIterateError/response_send_failure (0.00s)
    --- PASS: Test_handleDiffIterateError/unrecognized_error_code (0.00s)
    --- PASS: Test_handleDiffIterateError/ok_error_code (0.00s)
    --- PASS: Test_handleDiffIterateError/non-ErrorCode_error (0.00s)
    --- PASS: Test_handleDiffIterateError/stream_closed (0.00s)
PASS
ok  	github.com/ceph/ceph-csi/internal/rbd	0.054s

Note: Building the test container requires GOARCH=arm64 on Apple Silicon Macs — the Makefile defaults to GOARCH from go env which may not be set if Go isn't installed locally.

…fset tests The Offset and Length fields in DiffIterateConfig and DiffIterateByIDConfig lacked documentation explaining that Length is the number of bytes to scan from Offset (range [Offset, Offset+Length)), not an absolute end position. This ambiguity contributed to a bug in ceph-csi (PR #6200) where Length was set to the full volume size instead of (volumeSize - offset), causing zero blocks to be returned when using a non-zero offset (e.g. LUKS header skip). Add doc comments clarifying the semantics and add tests exercising non-zero Offset values for both DiffIterate and DiffIterateByID, verifying that only extents within the [Offset, Offset+Length) range are reported. See: ceph/ceph-csi#6200 See: kaovilai/cephcsi-cbt-e2e#2 Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>

Copilot

Pull request overview

Fixes snapshot diff iteration to scan the correct byte range when a non-zero starting offset is used, and adds unit tests around the diff-iterate callback behavior used for metadata streaming.

Changes:

Fix DiffIterateByIDConfig.Length to be VolSize - startingOffset (bytes remaining from the starting offset), avoiding scanning beyond the volume end.
Add unit tests for createDiffIterateByIDCB covering batching, LUKS padding adjustment, cancellation, and error propagation.

Reviewed changes

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

File	Description
`internal/rbd/snap_diff.go`	Corrects diff-iterate scan length to reflect remaining bytes from the requested offset.
`internal/rbd/snap_diff_test.go`	Adds callback-focused unit tests for batching/offset adjustment/cancellation/error paths.

Copilot · 2026-03-24T23:04:47Z

 	diffIterateByIDConfig := librbd.DiffIterateByIDConfig{
 		Offset:   uint64(startingOffset),
-		Length:   uint64(rbdSnap.VolSize),
+		Length:   uint64(rbdSnap.VolSize) - uint64(startingOffset),
 		Callback: cb,


The new Length calculation fixes the scan range, but the behavior isn’t covered by tests: Test_createDiffIterateByIDCB only validates the callback, and Test_handleDiffIterateError only validates error mapping. Consider adding a small unit test that asserts the computed (Offset, Length) range for a non-zero startingOffset (and with/without LUKS padding). If mocking image.DiffIterateByID is hard, extracting the range computation into a helper would make this directly testable.

nixpanic · 2026-03-26T16:03:16Z

Looks reasonable to me. Please correct the commit message formatting a little, just use Assisted-by with the name+email of the AI agent. References to issues should be to issues in a project repository, not issues in a personal fork of the repository.

nixpanic · 2026-03-27T12:13:44Z

golangci-lint fails on internal/rbd/snap_diff_test.go, it would be good to get that corrected. Thanks!

kaovilai · 2026-03-27T12:14:45Z

noted

kaovilai · 2026-03-27T12:29:01Z

lint fixed

kaovilai · 2026-03-27T12:34:42Z

Created #6208 to assist

…fset tests The Offset and Length fields in DiffIterateConfig and DiffIterateByIDConfig lacked documentation explaining that Length is the number of bytes to scan from Offset (range [Offset, Offset+Length)), not an absolute end position. This ambiguity contributed to a bug in ceph-csi (PR #6200) where Length was set to the full volume size instead of (volumeSize - offset), causing zero blocks to be returned when using a non-zero offset (e.g. LUKS header skip). Add doc comments clarifying the semantics and add tests exercising non-zero Offset values for both DiffIterate and DiffIterateByID, verifying that only extents within the [Offset, Offset+Length) range are reported. See: ceph/ceph-csi#6200 See: kaovilai/cephcsi-cbt-e2e#2 Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering> Signed-off-by: Tiger Kaovilai <passawit.kaovilai@gmail.com>

nixpanic

@Rakshith-R , backport this to the current release?

Rakshith-R · 2026-03-30T10:06:13Z

@Rakshith-R , backport this to the current release?

The feature is still in alpha in current release, I think we skip back-porting this fix.

@kaovilai Can you please check why your commit shows as un-verified on github ?
The changes look good to me otherwise.

kaovilai · 2026-03-30T13:05:41Z

Because I didn't use the same commit email as the one I have gpg sign setup probably

kaovilai · 2026-03-31T02:09:29Z

I can make it verified by fixing committee email to match gpg key. Never found any repos requiring gpg signing before but I guess unverified badge can be spooky. It only happens if it does gpg signing but not using matching committer email.

nixpanic · 2026-04-01T08:43:27Z

I can make it verified by fixing committee email to match gpg key. Never found any repos requiring gpg signing before but I guess unverified badge can be spooky. It only happens if it does gpg signing but not using matching committer email.

I do not think it is required to address for this PR. It probably is something you want to fix for future contributions.

mergify · 2026-04-01T09:32:40Z

Merge Queue Status

✅ Entered queue — 2026-04-01 09:32 UTC · Rule: default
✅ Checks passed · in-place
✅ Merged — 2026-04-01 10:09 UTC · at 6717dbdf70bc28ed76904688222a88d39cfbed09

This pull request spent 37 minutes 15 seconds in the queue, including 36 minutes 54 seconds running CI.

Required conditions to merge

#approved-reviews-by >= 2 [🛡 GitHub branch protection]
- rbd: fix DiffIterateByID Length calculation and add callback tests #6200
#changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
- rbd: fix DiffIterateByID Length calculation and add callback tests #6200

Fix Length calculation in DiffIterateByID to exclude startingOffset, and add unit tests for createDiffIterateByIDCB and handleDiffIterateError. Assisted-by: Claude (Anthropic) <noreply@anthropic.com> Signed-off-by: Tiger Kaovilai <passawit.kaovilai@gmail.com>

Madhu-1 · 2026-04-01T09:32:56Z

@Mergifyio rebase

mergify · 2026-04-01T09:33:13Z

rebase

☑️ Nothing to do, the required conditions are not met

Details

queue-position = -1 [📌 rebase requirement]
any of:
- #commits-behind > 0 [📌 rebase requirement]
- -linear-history [📌 rebase requirement]
-closed [📌 rebase requirement]
-conflict [📌 rebase requirement]

kaovilai · 2026-04-01T17:57:05Z

@nixpanic fixed now. it's honestly just a server side UI thing.

kaovilai · 2026-04-01T17:57:19Z

See that prior committed commits now show verified.

…fset tests The Offset and Length fields in DiffIterateConfig and DiffIterateByIDConfig lacked documentation explaining that Length is the number of bytes to scan from Offset (range [Offset, Offset+Length)), not an absolute end position. This ambiguity contributed to a bug in ceph-csi (PR #6200) where Length was set to the full volume size instead of (volumeSize - offset), causing zero blocks to be returned when using a non-zero offset (e.g. LUKS header skip). Add doc comments clarifying the semantics and add tests exercising non-zero Offset values for both DiffIterate and DiffIterateByID, verifying that only extents within the [Offset, Offset+Length) range are reported. See: ceph/ceph-csi#6200 See: kaovilai/cephcsi-cbt-e2e#2 Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Signed-off-by: Tiger Kaovilai <passawit.kaovilai@gmail.com>

kaovilai force-pushed the test-snap-diff-callback-coverage branch from a491036 to 41d995b Compare March 24, 2026 22:44

mergify Bot added component/rbd Issues related to RBD bug Something isn't working labels Mar 24, 2026

kaovilai mentioned this pull request Mar 24, 2026

rbd: document DiffIterate Offset/Length semantics and add non-zero offset tests ceph/go-ceph#1242

Merged

3 tasks

kaovilai marked this pull request as ready for review March 24, 2026 22:59

Copilot AI review requested due to automatic review settings March 24, 2026 22:59

Copilot started reviewing on behalf of kaovilai March 24, 2026 23:00 View session

Copilot AI reviewed Mar 24, 2026

View reviewed changes

kaovilai mentioned this pull request Mar 24, 2026

CephCSI GetMetadataAllocated returns empty stream (0 blocks) despite rbd diff showing data kaovilai/cephcsi-cbt-e2e#2

Closed

4 tasks

nixpanic requested a review from Rakshith-R March 26, 2026 16:03

nixpanic mentioned this pull request Mar 27, 2026

rbd: add debug logging to ProcessMetadata for CBT diagnostics #6203

Open

kaovilai force-pushed the test-snap-diff-callback-coverage branch 2 times, most recently from f307ff5 to 6bb479a Compare March 27, 2026 12:28

kaovilai force-pushed the test-snap-diff-callback-coverage branch 2 times, most recently from 4b8a3a7 to 5f86f08 Compare March 27, 2026 13:15

nixpanic approved these changes Mar 30, 2026

View reviewed changes

Rakshith-R approved these changes Apr 1, 2026

View reviewed changes

Madhu-1 approved these changes Apr 1, 2026

View reviewed changes

Madhu-1 added the ci/skip/e2e skip running e2e CI jobs label Apr 1, 2026

ceph-csi-bot force-pushed the test-snap-diff-callback-coverage branch from 5f86f08 to 6717dbd Compare April 1, 2026 09:32

mergify Bot added the queued label Apr 1, 2026

mergify Bot merged commit 2b92dc1 into ceph:devel Apr 1, 2026
19 checks passed

mergify Bot removed the queued label Apr 1, 2026

Conversation

kaovilai commented Mar 24, 2026

Summary

Note

Test plan

Uh oh!

kaovilai commented Mar 24, 2026

Local Test Results

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

nixpanic commented Mar 26, 2026

Uh oh!

nixpanic commented Mar 27, 2026

Uh oh!

kaovilai commented Mar 27, 2026

Uh oh!

kaovilai commented Mar 27, 2026

Uh oh!

kaovilai commented Mar 27, 2026

Uh oh!

nixpanic left a comment

Choose a reason for hiding this comment

Uh oh!

Rakshith-R commented Mar 30, 2026

Uh oh!

kaovilai commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaovilai commented Mar 31, 2026

Uh oh!

nixpanic commented Apr 1, 2026

Uh oh!

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

Uh oh!

Merge Queue Status

Uh oh!

Madhu-1 commented Apr 1, 2026

Uh oh!

mergify Bot commented Apr 1, 2026

☑️ Nothing to do, the required conditions are not met

Uh oh!

Uh oh!

kaovilai commented Apr 1, 2026

Uh oh!

kaovilai commented Apr 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kaovilai commented Mar 30, 2026 •

edited

Loading

mergify Bot commented Apr 1, 2026 •

edited

Loading