Skip to content

Introduce v1beta2 VolumeGroupSnapshot API #1312

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

leonardoce
Copy link
Contributor

@leonardoce leonardoce commented Jun 25, 2025

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:

This commit introduces the new VolumeGroupSnapshot v1beta2 API, updates the CRD, the generated code, and introduce a conversion webhook allowing idempotent conversion between v1beta1 and v1beta2.

Both v1beta1 and v1beta2 APIs are served, with v1beta2 being the stored version.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

NA

Does this PR introduce a user-facing change?:

Introduce the `v1beta2` VolumeGroupSnapshot API as described by [KEP 5013](https://github.com/kubernetes/enhancements/pull/5013)

@k8s-ci-robot k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 25, 2025
@k8s-ci-robot k8s-ci-robot requested review from pohly and RaunakShah June 25, 2025 16:00
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 25, 2025
@Madhu-1
Copy link
Contributor

Madhu-1 commented Jun 26, 2025

/lgtm
/assign @xing-yang

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 26, 2025
@xing-yang
Copy link
Collaborator

leonardoce and others added 5 commits July 28, 2025 17:38
This commit introduces the new VolumeGroupSnapshot v1beta2 API, updates
the CRD and the generated code.

Both v1beta1 and v1beta2 APIs are served, with v1beta1 being the stored
version.
This patch configures the conversion strategy on the
VolumeGroupSnapshot, VolumeGroupSnapshotContent and
VolumeGroupSnapshotClass CRDs.

Since v1beta2 is marked as the stored version, client using v1beta1 will
now require the conversion webhook to operate propertly.
This commit has just the minimal changes to make the code use the new
API. The new fields are not yet set.
This patch improves the sidecar to set the fields introduced by the
VolumeGroupSnapshotContent v1beta2 API when dynamically provisioning a
VolumeGroupSnapshot.

This information is then used by the common snapshot controller to set
the status of the member VolumeSnapshotContent objects, allowing the
relative fields to be set even when the CSI driver does not implement
the ListSnapshots RPC call.
This patch implements a reversible conversion webhook from/to
VolumeGroupSnapshots v1beta1 to/from VolumeGroupSnapshots v1beta2.

The extra fields included in the new API are tracked in an annotation
that is automatically set on v1beta1 objects.
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: leonardoce
Once this PR has been reviewed and has the lgtm label, please ask for approval from xing-yang. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

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

Copy link
Contributor

@jsafrane jsafrane left a comment

Choose a reason for hiding this comment

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

Checked the API + webhook. The controllers will follow.

Comment on lines 282 to +284
### Upgrade from v1beta1 to v1

Validation webhook should be installed before upgrading to v1. Potential impacts of not installing the validation webhook before upgrading to v1 include being unable to delete invalid snapshot objects. See the section on Validation Webhook for details.
Copy link
Contributor

@jsafrane jsafrane Aug 4, 2025

Choose a reason for hiding this comment

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

This should be more like v1beta1 to v1beta2 upgrade. IMHO we need a new chapter. Or remove v1beta1 to v1 completely, it happened a long time ago.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, we can remove the v1beta1 to v1 section for VolumeSnapshot API now.
You can add a new section for VolumeGroupSnapshot.

Comment on lines 282 to +284
### Upgrade from v1beta1 to v1

Validation webhook should be installed before upgrading to v1. Potential impacts of not installing the validation webhook before upgrading to v1 include being unable to delete invalid snapshot objects. See the section on Validation Webhook for details.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, we can remove the v1beta1 to v1 section for VolumeSnapshot API now.
You can add a new section for VolumeGroupSnapshot.

@leonardoce
Copy link
Contributor Author

I realized I didn't had a look at the documentation. I'm looking at it now.

Comment on lines 20 to 28
conversion:
strategy: Webhook
webhook:
conversionReviewVersions: ["v1","v1beta1"]
clientConfig:
service:
namespace: default
name: snapshot-webhook-service
path: /convert
Copy link
Contributor

Choose a reason for hiding this comment

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

If someone deploys the CRD as it is, it will require the conversion webhook to run.
That complicates my local tests - I just want v1beta2 and I want to run it quickly, without generating TLS keys and whatnot. Would it be possible to make the conversion opt-in (or opt-out) via kustomize? And is it worth the effort?

shift
done

[ -z ${service} ] && service=admission-webhook-example-svc
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[ -z ${service} ] && service=admission-webhook-example-svc
service=${service:-admission-webhook-example-svc}

This patch makes several corrections to the conversion webhook:

- fixes several occurrences of the old validation webhook codebase
- simplify the webhook command and drops the direct cobra dependency
- align the README with what the code really does
- removes the unneeded structure in the unit tests
- removes unneeded code and the unneeded dependencies
- drop support for converting VolumeSnapshots and VolumeSnapshotClasses as they
  are structurally identical

Signed-off-by: Leonardo Cecchi <[email protected]>
@@ -0,0 +1,15 @@
#!/bin/bash
# File originally from https://github.com/banzaicloud/admission-webhook-example/blob/blog/deployment/webhook-patch-ca-bundle.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/banzaicloud/admission-webhook-example does not exist and thus I can't check what license the script had :-(

Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is not addressed yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. But I'm not so sure on what we should do about it.
This comes from the old validation webhook, even if it has been heavily changed.
I'm removing it for now. Do you have a better idea?

Copy link
Contributor

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

sample yamls files in client/hack/cel-tests need to be updated and the example yamls for the volumesnapshotgroup need to be updated as well.

kind: Deployment
metadata:
name: snapshot-conversion-webhook-deployment
namespace: default # NOTE: change the namespace
Copy link
Contributor

Choose a reason for hiding this comment

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

use kube-system which is the namespace where we have the controller running, to keep it parity?

Copy link
Contributor Author

@leonardoce leonardoce Aug 8, 2025

Choose a reason for hiding this comment

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

I'm not sure of the rationale of it. This comes from the old validating webhook.
Maybe we want to the user to not forget to change the namespace.

Signed-off-by: Leonardo Cecchi <[email protected]>
Signed-off-by: Leonardo Cecchi <[email protected]>
@leonardoce
Copy link
Contributor Author

leonardoce commented Aug 8, 2025

sample yamls files in client/hack/cel-tests need to be updated and the example yamls for the volumesnapshotgroup need to be updated as well.

Done!
I also removed a rule that was deprecated by #1184

@@ -0,0 +1,15 @@
#!/bin/bash
# File originally from https://github.com/banzaicloud/admission-webhook-example/blob/blog/deployment/webhook-patch-ca-bundle.sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is not addressed yet.

spec:
containers:
- name: snapshot-conversion-webhook
image: registry.k8s.io/sig-storage/snapshot-conversion-webhook:v8.0.1 # change the image if you wish to use your own custom conversion server image
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't have this image tag for the snapshot-conversion-webhook yet: v8.0.1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Still I believe we should provide an image name for testing purposes.
Should we use the tag of the version where we'll release this? What v9.0.0 be fine?

@xing-yang
Copy link
Collaborator

Can you add what tests you have run?

@leonardoce
Copy link
Contributor Author

Can you add what tests you have run?

I run my own set of E2e tests that use the v1beta1 (old) API, covering:

  • Volume Group Snapshots
    • Dynamic provisioning
      • DeletionPolicy = Delete
      • DeletionPolicy = Retain
    • Static provisioning
      • DeletionPolicy = Delete
      • DeletionPolicy = Retain
  • Volume Snapshots
    • Dynamic provisioning
      • DeletionPolicy = Delete
      • DeletionPolicy = Retain
    • Static provisioning
      • DeletionPolicy = Delete
      • DeletionPolicy = Retain

I run them with both the ListSnapshots RPC call enabled and disabled.

This is an example of a dynamic provisioned VolumeGroupSnaphot:

apiVersion: groupsnapshot.storage.k8s.io/v1beta2
kind: VolumeGroupSnapshot
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"groupsnapshot.storage.k8s.io/v1beta2","kind":"VolumeGroupSnapshot","metadata":{"annotations":{},"name":"new-groupsnapshot-demo","namespace":"default"},"spec":{"source":{"selector":{"matchLabels":{"cnpg.io/instanceName":"cluster-example-1"}}},"volumeGroupSnapshotClassName":"csi-hostpath-groupsnapclass"}}
  creationTimestamp: "2025-08-05T12:08:44Z"
  finalizers:
  - groupsnapshot.storage.kubernetes.io/volumegroupsnapshot-bound-protection
  generation: 1
  name: new-groupsnapshot-demo
  namespace: default
  resourceVersion: "4913"
  uid: 0b61d9aa-c3c3-4d40-ab12-8090f39ead9c
spec:
  source:
    selector:
      matchLabels:
        cnpg.io/instanceName: cluster-example-1
  volumeGroupSnapshotClassName: csi-hostpath-groupsnapclass
status:
  boundVolumeGroupSnapshotContentName: groupsnapcontent-0b61d9aa-c3c3-4d40-ab12-8090f39ead9c
  creationTime: "2025-08-05T12:08:44Z"
  readyToUse: true

And this is the relative VolumeGroupSnapshotContent as per the v1beta2 API.

apiVersion: groupsnapshot.storage.k8s.io/v1beta2
kind: VolumeGroupSnapshotContent
metadata:
  creationTimestamp: "2025-08-05T12:08:44Z"
  finalizers:
  - groupsnapshot.storage.kubernetes.io/volumegroupsnapshotcontent-bound-protection
  generation: 1
  name: groupsnapcontent-0b61d9aa-c3c3-4d40-ab12-8090f39ead9c
  resourceVersion: "4880"
  uid: 3113c30e-59b1-4e01-bd26-0a458e0d0e90
spec:
  deletionPolicy: Delete
  driver: hostpath.csi.k8s.io
  source:
    volumeHandles:
    - f49f1cb2-71f2-11f0-b3d2-5258f6f0f310
    - f49f025c-71f2-11f0-b3d2-5258f6f0f310
  volumeGroupSnapshotClassName: csi-hostpath-groupsnapclass
  volumeGroupSnapshotRef:
    apiVersion: groupsnapshot.storage.k8s.io/v1beta2
    kind: VolumeGroupSnapshot
    name: new-groupsnapshot-demo
    namespace: default
    resourceVersion: "4870"
    uid: 0b61d9aa-c3c3-4d40-ab12-8090f39ead9c
status:
  creationTime: "2025-08-05T12:08:44Z"
  readyToUse: true
  volumeGroupSnapshotHandle: ef337d91-71f4-11f0-b3d2-5258f6f0f310
  volumeSnapshotInfoList:
  - creationTime: 1754395724757339958
    readyToUse: true
    restoreSize: 1073741824
    snapshotHandle: ef337da2-71f4-11f0-b3d2-5258f6f0f310
    volumeHandle: f49f1cb2-71f2-11f0-b3d2-5258f6f0f310
  - creationTime: 1754395724757339958
    readyToUse: true
    restoreSize: 1073741824
    snapshotHandle: ef6c9521-71f4-11f0-b3d2-5258f6f0f310
    volumeHandle: f49f025c-71f2-11f0-b3d2-5258f6f0f310

This object, when converted to the v1beta1 API, looks like this:

% kubectl get volumegroupsnapshotcontent.v1beta1.groupsnapshot.storage.k8s.io groupsnapcontent-0b61d9aa-c3c3-4d40-ab12-8090f39ead9c -o yaml
apiVersion: groupsnapshot.storage.k8s.io/v1beta1
kind: VolumeGroupSnapshotContent
metadata:
  annotations:
    groupsnapshot.storage.kubernetes.io/volume-snapshot-info-list: '[{"creationTime":1754395724757339958,"readyToUse":true,"restoreSize":1073741824,"snapshotHandle":"ef337da2-71f4-11f0-b3d2-5258f6f0f310","volumeHandle":"f49f1cb2-71f2-11f0-b3d2-5258f6f0f310"},{"creationTime":1754395724757339958,"readyToUse":true,"restoreSize":1073741824,"snapshotHandle":"ef6c9521-71f4-11f0-b3d2-5258f6f0f310","volumeHandle":"f49f025c-71f2-11f0-b3d2-5258f6f0f310"}]'
  creationTimestamp: "2025-08-05T12:08:44Z"
  finalizers:
  - groupsnapshot.storage.kubernetes.io/volumegroupsnapshotcontent-bound-protection
  generation: 1
  name: groupsnapcontent-0b61d9aa-c3c3-4d40-ab12-8090f39ead9c
  resourceVersion: "4880"
  uid: 3113c30e-59b1-4e01-bd26-0a458e0d0e90
spec:
  deletionPolicy: Delete
  driver: hostpath.csi.k8s.io
  source:
    volumeHandles:
    - f49f1cb2-71f2-11f0-b3d2-5258f6f0f310
    - f49f025c-71f2-11f0-b3d2-5258f6f0f310
  volumeGroupSnapshotClassName: csi-hostpath-groupsnapclass
  volumeGroupSnapshotRef:
    apiVersion: groupsnapshot.storage.k8s.io/v1beta2
    kind: VolumeGroupSnapshot
    name: new-groupsnapshot-demo
    namespace: default
    resourceVersion: "4870"
    uid: 0b61d9aa-c3c3-4d40-ab12-8090f39ead9c
status:
  creationTime: "2025-08-05T12:08:44Z"
  readyToUse: true
  volumeGroupSnapshotHandle: ef337d91-71f4-11f0-b3d2-5258f6f0f310
  volumeSnapshotHandlePairList:
  - snapshotHandle: ef337da2-71f4-11f0-b3d2-5258f6f0f310
    volumeHandle: f49f1cb2-71f2-11f0-b3d2-5258f6f0f310
  - snapshotHandle: ef6c9521-71f4-11f0-b3d2-5258f6f0f310
    volumeHandle: f49f025c-71f2-11f0-b3d2-5258f6f0f310

@xing-yang
Copy link
Collaborator

Can you show an example of a VolumeSnapshot and VolumeSnapshotContent that are part of a group snapshot?

@leonardoce
Copy link
Contributor Author

Sure!

% kubectl get volumesnapshot snapshot-67917c951a1b246f664709d3f3b5ac1d5d11b0371047835ae91ae9b78378c44f -o yaml
apiVersion: snapshot.storage.k8s.io/v1
kind: VolumeSnapshot
metadata:
  creationTimestamp: "2025-08-05T12:08:45Z"
  finalizers:
  - snapshot.storage.kubernetes.io/volumesnapshot-in-group-protection
  - snapshot.storage.kubernetes.io/volumesnapshot-as-source-protection
  generation: 1
  name: snapshot-67917c951a1b246f664709d3f3b5ac1d5d11b0371047835ae91ae9b78378c44f
  namespace: default
  ownerReferences:
  - apiVersion: groupsnapshot.storage.k8s.io/v1beta2
    kind: VolumeGroupSnapshot
    name: new-groupsnapshot-demo
    uid: 0b61d9aa-c3c3-4d40-ab12-8090f39ead9c
  resourceVersion: "4898"
  uid: 16d90945-465a-4d16-99e1-bbe2e0d99836
spec:
  source:
    persistentVolumeClaimName: cluster-example-1
status:
  boundVolumeSnapshotContentName: snapcontent-67917c951a1b246f664709d3f3b5ac1d5d11b0371047835ae91ae9b78378c44f
  creationTime: "2025-08-05T12:08:44Z"
  readyToUse: true
  restoreSize: 1Gi
  volumeGroupSnapshotName: new-groupsnapshot-demo
apiVersion: snapshot.storage.k8s.io/v1
kind: VolumeSnapshotContent
metadata:
  annotations:
    groupsnapshot.storage.k8s.io/volumeGroupSnapshotHandle: ef337d91-71f4-11f0-b3d2-5258f6f0f310
  creationTimestamp: "2025-08-05T12:08:45Z"
  finalizers:
  - snapshot.storage.kubernetes.io/volumesnapshotcontent-bound-protection
  generation: 2
  name: snapcontent-67917c951a1b246f664709d3f3b5ac1d5d11b0371047835ae91ae9b78378c44f
  resourceVersion: "4887"
  uid: 7346342b-8635-4794-b566-2d245af6ac47
spec:
  deletionPolicy: Delete
  driver: hostpath.csi.k8s.io
  source:
    volumeHandle: f49f1cb2-71f2-11f0-b3d2-5258f6f0f310
  sourceVolumeMode: Filesystem
  volumeSnapshotRef:
    kind: VolumeSnapshot
    name: snapshot-67917c951a1b246f664709d3f3b5ac1d5d11b0371047835ae91ae9b78378c44f
    namespace: default
    uid: 16d90945-465a-4d16-99e1-bbe2e0d99836
status:
  creationTime: 1754395724757339958
  readyToUse: true
  restoreSize: 1073741824
  snapshotHandle: ef337da2-71f4-11f0-b3d2-5258f6f0f310
  volumeGroupSnapshotHandle: ef337d91-71f4-11f0-b3d2-5258f6f0f310

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants