Implement CSI SnapshotMetadata service #569

PrasadG193 · 2024-10-15T12:10:59Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR implements SnapshotMetadata service from the CSI specs. This service in combination with external-snapshot-metadata will be used to compare and query changed block metadata between two snapshots.
Right now, the service will be disabled by default till external-snapshot-metadata alpha is release.

The changes in deploy script to add external-snapshot-metadata sidecar will be added after alpha release.

NOTE

This is a sample implementation of SnapshotMetadata intended to be used for demo and CI testing purpose only.
This should not be used in production or as an example about how to write a real driver.

Test plan

Complete test plan is documented at - https://gist.github.com/PrasadG193/1fab082f03bbdb7db25a41ed17b7e7c1

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add sample implementation of CSI SnapshotMetadata service

Signed-off-by: Prasad Ghangal <[email protected]>

k8s-ci-robot · 2024-10-15T12:11:02Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Implement GetMetadataAllocated RPC handler Signed-off-by: Prasad Ghangal <[email protected]>

Signed-off-by: Prasad Ghangal <[email protected]>

xing-yang · 2024-10-25T02:11:16Z

/assign
/assign @bswartz

Signed-off-by: Prasad Ghangal <[email protected]>

cmd/hostpathplugin/main.go

pkg/hostpath/hostpath.go

pkg/hostpath/snapshotmetadata.go

pkg/hostpath/snapshotmetadataserver.go

xing-yang · 2024-10-31T15:03:00Z

This is a new feature. Please add a release note.
Can you add a note in the PR description that this is a sample implementation, and not for production use.

cmd/hostpathplugin/main.go

pkg/hostpath/hostpath.go

pkg/hostpath/snapshotmetadata.go

Signed-off-by: Prasad Ghangal <[email protected]>

xing-yang · 2024-12-02T17:20:01Z

Can you add your testing steps?

Rakshith-R · 2024-12-04T16:13:16Z

Can you add your testing steps?

hey @xing-yang ,

@PrasadG193 is on vacation and will be back thursday.

He had developed a client to test the new RPCs https://github.com/PrasadG193/external-snapshot-metadata-client
which has been adopted as a tool in this pr kubernetes-csi/external-snapshot-metadata#64

We are creating a gh action that adds simple e2e
kubernetes-csi/external-snapshot-metadata#67

Add a github action that:
- checkout repository
- sets up minikube
- builds latest sidecar image
- deploys csi-driver-hostpath driver resources
- runs simple e2e tests to create pvc, pod and snapshots and to write data.
- run client to verify working of both RPCs.

Sample run https://github.com/Rakshith-R/external-snapshot-metadata/actions/runs/12163453092/job/33922692908
(with the fix @iPraveenParihar has suggested above)

pkg/hostpath/snapshotmetadataserver.go

Signed-off-by: Prasad Ghangal <[email protected]>

PrasadG193 · 2024-12-09T12:56:39Z

@xing-yang I have documented complete test plan along with steps to deploy csi driver with external-snapshot-metadata here - https://gist.github.com/PrasadG193/1fab082f03bbdb7db25a41ed17b7e7c1

iPraveenParihar · 2024-12-10T12:35:41Z

LGTM! @PrasadG193

bswartz

Overall this looks pretty good. There are some inefficient bits of code but for a proof-of-concept it seems fine. I found one bug with error handling.

pkg/hostpath/snapshotmetadata.go

Signed-off-by: Prasad Ghangal <[email protected]>

bswartz

/lgtm

Signed-off-by: Prasad Ghangal <[email protected]>

pnhoang75 · 2024-12-17T17:33:05Z

cmd/hostpathplugin/main.go

+	// validate snapshot-metadata-type arg block type
+	bt, ok := csi.BlockMetadataType_value[*snapshotMetadataBlockType]
+	if !ok {
+		fmt.Printf("invalid snapshot-metadata-block-type passed, please pass one of the - FIXED_LENGTH, VARIABLE_LENGTH")


nit: use upper case for beginning of the error/exit message to conform with other error messages in the file. And probably rephase "please pass one of the -..." to "valid types are FIXED_LENGTH, VARIABLE_LENGTH".

bswartz

/lgtm

xing-yang · 2024-12-18T17:58:29Z

/approve

k8s-ci-robot · 2024-12-18T17:58:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bswartz, PrasadG193, xing-yang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [xing-yang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

CSI SnapshotMetadata service has been implemented for csi-driver-host-path PR: kubernetes-csi/csi-driver-host-path#569 Signed-off-by: Praveen M <[email protected]>

Implement SnapshotMetadataService

57eff9b

Signed-off-by: Prasad Ghangal <[email protected]>

k8s-ci-robot requested review from chrishenzie and humblec October 15, 2024 12:11

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 15, 2024

Add unit tests

3b2b102

Implement GetMetadataAllocated RPC handler Signed-off-by: Prasad Ghangal <[email protected]>

PrasadG193 force-pushed the snapshot-metadata-service branch from 2835d54 to 3b2b102 Compare October 15, 2024 12:21

PrasadG193 added 2 commits October 16, 2024 17:48

Do not enable snapshotmetadata service by default

dd68929

Signed-off-by: Prasad Ghangal <[email protected]>

Add support for variable length metadata

91c42e3

Signed-off-by: Prasad Ghangal <[email protected]>

PrasadG193 marked this pull request as ready for review October 24, 2024 17:45

k8s-ci-robot requested review from carlory and mauriciopoppe October 24, 2024 17:45

PrasadG193 changed the title ~~[WIP] Implement CSI SnapshotMetadata service~~ Implement CSI SnapshotMetadata service Oct 24, 2024

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 24, 2024

k8s-ci-robot assigned bswartz and xing-yang Oct 25, 2024

Remove unreachable code

9821fd8

Signed-off-by: Prasad Ghangal <[email protected]>

carlbraganza reviewed Oct 28, 2024

View reviewed changes

cmd/hostpathplugin/main.go Outdated Show resolved Hide resolved

pkg/hostpath/hostpath.go Outdated Show resolved Hide resolved

pkg/hostpath/snapshotmetadata.go Outdated Show resolved Hide resolved

carlbraganza reviewed Oct 28, 2024

View reviewed changes

pkg/hostpath/snapshotmetadataserver.go Outdated Show resolved Hide resolved

carlbraganza reviewed Oct 28, 2024

View reviewed changes

pkg/hostpath/snapshotmetadataserver.go Outdated Show resolved Hide resolved

xing-yang reviewed Oct 31, 2024

View reviewed changes

cmd/hostpathplugin/main.go Outdated Show resolved Hide resolved

pkg/hostpath/hostpath.go Outdated Show resolved Hide resolved

pkg/hostpath/snapshotmetadata.go Show resolved Hide resolved

Remove TODO comments

d49ecb5

Signed-off-by: Prasad Ghangal <[email protected]>

PrasadG193 mentioned this pull request Nov 7, 2024

Add example of csi hostpath driver kubernetes-csi/external-snapshot-metadata#62

Closed

Find changed blocks in sync call

a06f79b

Signed-off-by: Prasad Ghangal <[email protected]>

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Nov 7, 2024

carlbraganza reviewed Dec 4, 2024

View reviewed changes

pkg/hostpath/snapshotmetadataserver.go Show resolved Hide resolved

PrasadG193 mentioned this pull request Dec 6, 2024

Update deploy script to deploy snapshot-metadata service PrasadG193/csi-driver-host-path#1

Draft

PrasadG193 added 2 commits December 9, 2024 17:30

Correct error checks

9144687

Signed-off-by: Prasad Ghangal <[email protected]>

Correct MetadataType in the response

cdcb8cd

Signed-off-by: Prasad Ghangal <[email protected]>

bswartz suggested changes Dec 11, 2024

View reviewed changes

pkg/hostpath/snapshotmetadata.go Show resolved Hide resolved

pkg/hostpath/snapshotmetadata.go Outdated Show resolved Hide resolved

Correct error handling

23f800d

Signed-off-by: Prasad Ghangal <[email protected]>

PrasadG193 requested review from bswartz and iPraveenParihar December 16, 2024 17:40

bswartz approved these changes Dec 16, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 16, 2024

Set default maxResults if not set by client

60a9768

Signed-off-by: Prasad Ghangal <[email protected]>

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 17, 2024

PrasadG193 mentioned this pull request Dec 17, 2024

Add documentation to deploy and test SnapshotMetadata support #575

Merged

pnhoang75 reviewed Dec 17, 2024

View reviewed changes

bswartz approved these changes Dec 18, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 18, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 18, 2024

k8s-ci-robot merged commit 245a8ee into kubernetes-csi:master Dec 18, 2024
10 checks passed

Rakshith-R mentioned this pull request Jan 22, 2025

fix: consider only non-zero blocks for GetMetadataAllocated #579

Merged

Rakshith-R mentioned this pull request Mar 4, 2025

CSI Differential Snapshot for Block Volumes kubernetes/enhancements#3314

Open

4 tasks

Implement CSI SnapshotMetadata service #569

Implement CSI SnapshotMetadata service #569

Conversation

PrasadG193 commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

NOTE

Test plan

Uh oh!

k8s-ci-robot commented Oct 15, 2024

Uh oh!

xing-yang commented Oct 25, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xing-yang commented Oct 31, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xing-yang commented Dec 2, 2024

Uh oh!

Rakshith-R commented Dec 4, 2024

Uh oh!

Uh oh!

PrasadG193 commented Dec 9, 2024

Uh oh!

iPraveenParihar commented Dec 10, 2024

Uh oh!

bswartz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bswartz left a comment

Choose a reason for hiding this comment

Uh oh!

pnhoang75 Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bswartz left a comment

Choose a reason for hiding this comment

Uh oh!

xing-yang commented Dec 18, 2024

Uh oh!

k8s-ci-robot commented Dec 18, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

PrasadG193 commented Oct 15, 2024 •

edited

Loading

pnhoang75 Dec 17, 2024 •

edited

Loading