Skip to content

Add CBT interfaces for block data mover#9716

Merged
Lyndon-Li merged 5 commits intovelero-io:mainfrom
Lyndon-Li:cbt-interfaces
Apr 21, 2026
Merged

Add CBT interfaces for block data mover#9716
Lyndon-Li merged 5 commits intovelero-io:mainfrom
Lyndon-Li:cbt-interfaces

Conversation

@Lyndon-Li
Copy link
Copy Markdown
Contributor

Fix issue #9709, add interfaces for CBT service and CBT bitmap

@Lyndon-Li Lyndon-Li marked this pull request as ready for review April 13, 2026 08:56
@github-actions github-actions Bot requested review from sseago and ywk253100 April 13, 2026 09:04
@Lyndon-Li Lyndon-Li force-pushed the cbt-interfaces branch 5 times, most recently from e91e12c to afcb864 Compare April 13, 2026 09:58
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 13, 2026

Codecov Report

❌ Patch coverage is 0% with 13 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pkg/uploader/cbt/set.go 0.00% 13 Missing ⚠️

📢 Thoughts on this report? Let us know!

Signed-off-by: Lyndon-Li <lyonghui@vmware.com>
Comment thread changelogs/unreleased/9716-Lyndon-Li
Comment thread pkg/cbtservice/service.go
@Lyndon-Li Lyndon-Li requested a review from kaovilai April 14, 2026 07:42
@Lyndon-Li Lyndon-Li force-pushed the cbt-interfaces branch 3 times, most recently from bad491a to cbcd437 Compare April 16, 2026 09:32
@Lyndon-Li
Copy link
Copy Markdown
Contributor Author

@kaovilai Please help to take another look, I made some adjustments.

kaovilai
kaovilai previously approved these changes Apr 16, 2026
Copy link
Copy Markdown
Collaborator

@kaovilai kaovilai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

edit: lgtm.

Comment thread pkg/cbtservice/service.go
GetAllocatedBlocks(ctx context.Context, snapshot string, record func([]Range) error) error

// GetChangedBlocks enumerates the changed blocks of the snapshot since PIT of changeID and call the record callback
GetChangedBlocks(ctx context.Context, snapshot string, changeID string, record func([]Range) error) error
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To maintain consistency with the SnapshotMetadataService specification, should we align the input parameters of GetChangedBlocks with GetMetadataDeltaRequest, which contain "base_snapshot_id", "target_snapshot_name", "starting_offset", etc?

Same for the method "GetAllocatedBlocks".

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was covered in the design perhaps to be abstract just in case.

ChangeId

ChangeId identifies the base that CBT is generated from, it must strictly map to the parent snapshot in the repository. Otherwise, there will be data corruption in the incremental backup.
Therefore, ChangeId is saved together with the repository snapshot.
The data mover always queries parent snapshot from Unified Repo together with the ChangeId. In this way, no mismatch would happen.
Inside the uploader, the upper layer (DataUpload controller) could also provide the ChangeId as a mechanism of double confirmation. The received ChangeId would be re-evaluated against the one in the provided snapshot.

For Kubernetes API, changeId is represented by BaseSnapshotId.
changeId retrieval is storage specific, generally, it is retrieved from the SnapshotHandle of the VolumeSnapshotContent object; however, storages may also refer to other places to retrieve the changeId.
That is, SnapshotHandle and changeId may be two different values, in this case, the both values need to be preserved.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an abstraction provided by Velero, as the design shared by @kaovilai, this abstraction is for the below purpose:

  1. Support both Kubernetes native CBT service (SnapshotMetadataService) and the possible customized CBT service
  2. It is used by the uploader to translate the entire allocated/changed blocks into the CBT bitmap

Because of 1, the abstraction just purposefully avoids to use the primatives from Kubernetes upstream, e.g., in the upstream target_snapshot_name means the name of the VolumeSnapshot, however, for customized CBT service, it could not be the name of VolumeSnapshot or even not a name (e.g., a snapshot ID instead).
Because of 2, it means it is a special/compacted version when comparing to the upstream primitives, e.g., starting_offset is not required since the uploader always expects to translates the entire CBT, from the begining to end.

Comment thread pkg/uploader/cbt/set.go
kaovilai
kaovilai previously approved these changes Apr 17, 2026
Copy link
Copy Markdown

@EunbiZhang EunbiZhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me as far as I can tell. Thanks for working on the change Lyndon!

Signed-off-by: Lyndon-Li <lyonghui@vmware.com>
@Lyndon-Li
Copy link
Copy Markdown
Contributor Author

Lyndon-Li commented Apr 17, 2026

@kaovilai @EunbiZhang
Please take another look. Added one more field VolumeID into SourceInfo of CBTService to address below statement in the design:

SnapshotHandle and changeId may be two different values, in this case, the both values need to be preserved

For the same PVC, the underlying volumes may be changed, but the CBT associates to the a specific volume only. So the VolumeID uniquely identifies the volume, and do necessary checks to detect the volume change cases.

@Lyndon-Li Lyndon-Li requested a review from kaovilai April 17, 2026 07:45
@Lyndon-Li Lyndon-Li merged commit 1160ae2 into velero-io:main Apr 21, 2026
56 of 57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants