-
Notifications
You must be signed in to change notification settings - Fork 397
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
base: master
Are you sure you want to change the base?
Conversation
/lgtm |
Do you need to update README to generate files? |
client/clientset/versioned/typed/volumegroupsnapshot/v1beta1/doc.go
Outdated
Show resolved
Hide resolved
client/config/crd/groupsnapshot.storage.k8s.io_volumegroupsnapshotclasses.yaml
Outdated
Show resolved
Hide resolved
client/config/crd/groupsnapshot.storage.k8s.io_volumegroupsnapshotclasses.yaml
Outdated
Show resolved
Hide resolved
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.
New changes are detected. LGTM label has been removed. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: leonardoce 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 |
There was a problem hiding this 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.
### 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
client/config/crd/groupsnapshot.storage.k8s.io_volumegroupsnapshotclasses.yaml
Outdated
Show resolved
Hide resolved
client/config/crd/groupsnapshot.storage.k8s.io_volumegroupsnapshots.yaml
Outdated
Show resolved
Hide resolved
pkg/webhook/testdata/v1beta1_to_v1beta2/annotation_no_status_v1beta1.yaml
Outdated
Show resolved
Hide resolved
pkg/webhook/testdata/v1beta1_to_v1beta2/annotation_no_status_v1beta2.yaml
Outdated
Show resolved
Hide resolved
### 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. |
There was a problem hiding this comment.
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.
I realized I didn't had a look at the documentation. I'm looking at it now. |
Signed-off-by: Leonardo Cecchi <[email protected]>
conversion: | ||
strategy: Webhook | ||
webhook: | ||
conversionReviewVersions: ["v1","v1beta1"] | ||
clientConfig: | ||
service: | ||
namespace: default | ||
name: snapshot-webhook-service | ||
path: /convert |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[ -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 |
There was a problem hiding this comment.
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 :-(
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
…e conversion webhook Signed-off-by: Leonardo Cecchi <[email protected]>
Signed-off-by: Leonardo Cecchi <[email protected]>
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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]>
Done! |
@@ -0,0 +1,15 @@ | |||
#!/bin/bash | |||
# File originally from https://github.com/banzaicloud/admission-webhook-example/blob/blog/deployment/webhook-patch-ca-bundle.sh |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
Can you add what tests you have run? |
Signed-off-by: Leonardo Cecchi <[email protected]>
I run my own set of E2e tests that use the v1beta1 (old) API, covering:
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 |
Can you show an example of a VolumeSnapshot and VolumeSnapshotContent that are part of a group snapshot? |
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 |
What type of PR is this?
/kind api-change
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?: