Use base snapshot CSI handle instead of name in GetMetadataRequest #180

carlbraganza · 2025-09-17T01:25:19Z

Replaced base_snapshot_name with base_snapshot_id in the GetMetadataRequest Kubernetes SnapshotMetadata API.
The iterator package now accepts both the previous snapshot name or CSI handle with preference given to the handle.

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change

/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
There was an API change requested in Issue 165, and approved by the SIG-Storage DP WG.

Which issue(s) this PR fixes:

Fixes #165

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

action required
The signature of the Kubernetes GetMetadataRequest RPC call has changed.

Rakshith-R · 2025-09-17T15:23:08Z

examples/snapshot-metadata-lister/main.go


 	stringFlag(&args.Namespace, "namespace", "n", "", "The Namespace containing the VolumeSnapshot objects.")
 	stringFlag(&args.SnapshotName, "snapshot", "s", "", "The name of the VolumeSnapshot for which metadata is to be displayed.")
+	stringFlag(&args.PrevSnapshotID, "previous-snapshot-id", "P", "", "The CSI handle of an earlier VolumeSnapshot against which changed block metadata is to be displayed.")


Just capital "P" maybe a bit confusion, how about p-id or something else?

The P is the short flag and was chosen because the short flag for the name is p. The long flag is -previous-snapshot-id.

Rakshith-R · 2025-09-23T09:08:32Z

@carlbraganza

We're building test tag here

external-snapshot-metadata/.github/workflows/integration-test.yaml

Line 30 in b4b23f3

    
                     # minikube image build -f ./cmd/csi-snapshot-metadata/Dockerfile -t gcr.io/k8s-staging-sig-storage/csi-snapshot-metadata:test .

but testing with main tag 🤦 .

external-snapshot-metadata/.github/workflows/integration-test.yaml

Line 49 in b4b23f3

    
                     CSI_SNAPSHOT_METADATA_REGISTRY="gcr.io/k8s-staging-sig-storage" UPDATE_RBAC_RULES="false" CSI_SNAPSHOT_METADATA_TAG="main" SNAPSHOT_METADATA_TESTS=true HOSTPATHPLUGIN_REGISTRY="gcr.io/k8s-staging-sig-storage" HOSTPATHPLUGIN_TAG="canary" ~/csi-driver-host-path/deploy/kubernetes-latest/deploy.sh

Can you please fix CSI_SNAPSHOT_METADATA_TAG="test" in Line 49 ?
then it should work as expected.

Rakshith-R · 2025-09-23T09:10:05Z

Can you please take a look at adding tests for previous snapID usage too in the following ?

external-snapshot-metadata/.github/workflows/integration-test.yaml

Line 66 in b4b23f3

- name: Execute tests

carlbraganza · 2025-09-23T21:44:38Z

Can you please take a look at adding tests for previous snapID usage too in the following ?

external-snapshot-metadata/.github/workflows/integration-test.yaml

Line 66 in b4b23f3

- name: Execute tests

I will try, as soon as I make some progress in understanding the test and making it work.

I presume I can use something like this to extract the snapshot handle:

VSC_NAME=`kubectl get volumesnapshot snap1 -o jsonpath="{.status.boundVolumeSnapshotContentName}"`
SNAP_HANDLE=`kubectl get volumesnapshotcontent $VSC_NAME -o jsonpath="{.status.snapshotHandle}"`

and then repeat the diff with the handle as a parameter instead of snap1.

I will also have to modify the verifier command to take the new flag.

GetMetadataRequest Kubernetes SnapshotMetadata API. The iterator package now accepts both the previous snapshot name or CSI handle with preference given to the handle.

Rakshith-R

Looks good to me,
I'll let @PrasadG193 review too.

xing-yang · 2025-10-13T02:18:17Z

Looks good to me as well.
@PrasadG193 can you take a look?

PrasadG193 · 2025-10-14T06:38:44Z

proto/schema.proto

+  // request is made, and take that into consideration in its snapshot retention policy.
  // This field is REQUIRED.
-  string base_snapshot_name = 3;
+  string base_snapshot_id = 3;


Just to confirm, are we not supporting volume ID in GetMetadataAllocatedRequest?

Does it make sense to also change target_snapshot_name to target_snapshot_id to make it uniform? Using ID for one and name for other could be confusing for users. Was this discussed and decided in the community meeting?

Just to confirm, are we not supporting volume ID in GetMetadataAllocatedRequest?

You meant snapshot ID in GetMetadataAllocatedRequest? That is not required because this API only needs one snapshot which is just created and it does not need to be retained for a long time.
The main reason for changing the base_snapshot_name to base_snapshot_id is that so we don't require a storage system to retain a snapshot for a long time which may result in performance degradation on some storage systems, defeating the purpose of using CBT.

Does it make sense to also change target_snapshot_name to target_snapshot_id to make it uniform? Using ID for one and name for other could be confusing for users. Was this discussed and decided in the community meeting?

That's a good point. If we do that, it means the backup vendor will create a VolumeSnapshot first and then retrieve the snapshot_id instead of letting the External Snapshot Metadata sidecar do that.
I'm fine either way.

Discussed this at DP WG meeting today. @bswartz mentioned that if we change target_snapshot_name to target_snapshot_id, that will affect the rbac control.

Ok, that makes sense. We don't have any other info to validate the auth otherwise

prasadg-veeam

/lgtm
/approve

PrasadG193

/lgtm
/approve

k8s-ci-robot · 2025-10-15T16:19:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carlbraganza, prasadg-veeam, PrasadG193, Rakshith-R

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~~ [PrasadG193,Rakshith-R,carlbraganza]

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

k8s-ci-robot requested review from Rakshith-R and hairyhum September 17, 2025 01:25

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 17, 2025

carlbraganza force-pushed the issue-165 branch from bda429d to fa1d7c5 Compare September 17, 2025 01:31

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2025

carlbraganza requested review from PrasadG193 and removed request for hairyhum September 17, 2025 01:32

Rakshith-R reviewed Sep 17, 2025

View reviewed changes

carlbraganza force-pushed the issue-165 branch from fa1d7c5 to 0ec0c58 Compare September 23, 2025 21:43

carlbraganza force-pushed the issue-165 branch 3 times, most recently from d873aef to fdc6de4 Compare September 23, 2025 22:54

Replaced base_snapshot_name with base_snapshot_id in the

aa1a285

GetMetadataRequest Kubernetes SnapshotMetadata API. The iterator package now accepts both the previous snapshot name or CSI handle with preference given to the handle.

carlbraganza force-pushed the issue-165 branch from fdc6de4 to aa1a285 Compare September 23, 2025 23:26

carlbraganza requested a review from Rakshith-R September 23, 2025 23:33

Rakshith-R approved these changes Sep 25, 2025

View reviewed changes

PrasadG193 reviewed Oct 14, 2025

View reviewed changes

prasadg-veeam approved these changes Oct 15, 2025

View reviewed changes

kubernetes-csi deleted a comment from prasadg-veeam Oct 15, 2025

kubernetes-csi deleted a comment from k8s-ci-robot Oct 15, 2025

PrasadG193 approved these changes Oct 15, 2025

View reviewed changes

k8s-ci-robot assigned PrasadG193 Oct 15, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 15, 2025

k8s-ci-robot merged commit b44a413 into kubernetes-csi:main Oct 15, 2025
9 checks passed

Use base snapshot CSI handle instead of name in GetMetadataRequest #180

Use base snapshot CSI handle instead of name in GetMetadataRequest #180

Conversation

carlbraganza commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rakshith-R Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

carlbraganza Sep 23, 2025 • 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!

Rakshith-R commented Sep 23, 2025

Uh oh!

Rakshith-R commented Sep 23, 2025

Uh oh!

carlbraganza commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rakshith-R left a comment

Choose a reason for hiding this comment

Uh oh!

xing-yang commented Oct 13, 2025

Uh oh!

PrasadG193 Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

xing-yang Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

xing-yang Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

PrasadG193 Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

prasadg-veeam left a comment

Choose a reason for hiding this comment

Uh oh!

PrasadG193 left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Oct 15, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

carlbraganza commented Sep 17, 2025 •

edited

Loading

carlbraganza Sep 23, 2025 •

edited

Loading

carlbraganza commented Sep 23, 2025 •

edited

Loading