Update VolumeSnapshot CRD version to v1beta #139

xing-yang · 2019-07-11T19:35:16Z

What type of PR is this?
/kind api-change

What this PR does / why we need it:
This PR changes VolumeSnapshot CRD version from v1alpha1 to v1beta1.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Changes VolumeSnapshot CRD version from v1alpha1 to v1beta1.

msau42 · 2019-07-11T20:31:43Z

/assign @msau42 @saad-ali

xing-yang · 2019-07-24T18:09:12Z

/retest

msau42 · 2019-07-31T19:41:58Z

/label api-review

johnbelamaric · 2019-08-07T22:35:20Z

See https://github.com/kubernetes-sigs/controller-tools for CRD generation from types.go

saad-ali

First round of reviews with @thockin

cmd/csi-snapshotter/create_crd.go

pkg/apis/volumesnapshot/v1beta1/types.go

saad-ali

Round 2 of feedback

pkg/apis/volumesnapshot/v1beta1/types.go

msau42

I'm not done yet, will need to finish the rest on Monday

config/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml

config/crd/snapshot.storage.k8s.io_volumesnapshotcontents.yaml

config/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml

config/crd/snapshot.storage.k8s.io_volumesnapshots.yaml

msau42 · 2019-08-23T19:26:04Z

General comment, I believe in the description, when we refer to field names, we should follow the casing used in the open schema.

pkg/apis/volumesnapshot/v1beta1/types.go

config/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml

config/crd/snapshot.storage.k8s.io_volumesnapshotcontents.yaml

config/crd/snapshot.storage.k8s.io_volumesnapshots.yaml

yuxiangqian

making another round of comment refinement based on feedback

config/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml

config/crd/snapshot.storage.k8s.io_volumesnapshots.yaml

config/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml

config/crd/snapshot.storage.k8s.io_volumesnapshotcontents.yaml

config/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml

config/crd/snapshot.storage.k8s.io_volumesnapshots.yaml

yuxiangqian · 2019-08-27T06:37:52Z

config/crd/snapshot.storage.k8s.io_volumesnapshots.yaml

+                  type: string
+              type: object
+            readyToUse:
+              description: 'readyToUse is a status/informational flag which provides


Actually this flag means more than underlying storage snapshot is available. It also means the binding has been successfully done.
for the implementation detail piece, it was requested during the review. I think it is beneficial to keep it for developer references.

config/crd/snapshot.storage.k8s.io_volumesnapshots.yaml

msau42 · 2019-10-23T21:51:55Z

pkg/apis/volumesnapshot/v1beta1/types.go

+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+
+// VolumeSnapshot is a user's request for taking a point-in-time snapshot of a PersistentVolumeClaim.
+// Upon successful creation of a snapshot by the underlying storage system, it is bound to a


This comment is still outstanding

msau42 · 2019-10-23T21:53:09Z

pkg/apis/volumesnapshot/v1beta1/types.go

+	// snapshot was successfully cut on the underlying storage system.
+	// In dynamic snapshot creation case, it will be filled in upon snapshot creation
+	// with the "creation_time" field returned from CSI "CreateSnapshot" gRPC call.
+	// For a pre-existing snapshot, it will be set to the "size_byte" value returned


size_bytes. thnx

msau42 · 2019-10-23T21:54:04Z

pkg/apis/volumesnapshot/v1beta1/types.go

+	// In dynamic snapshot creation case, it will be filled in upon snapshot creation
+	// with the "creation_time" field returned from CSI "CreateSnapshot" gRPC call.
+	// For a pre-existing snapshot, it will be set to the "size_byte" value returned
+	// from CSI "ListSnapshots" gRPC call if there exists a matching CSI driver that


"from the CSI "ListSnapshots" gRPC call if the driver supports it."

Ditto everywhere

msau42 · 2019-10-23T21:54:52Z

pkg/apis/volumesnapshot/v1beta1/types.go

+	// In dynamic snapshot creation case, readyToUse will be set to true after underlying storage
+	// system has successfully finished all out-of-bound procedures to make a snapshot ready to
+	// be used to restore a volume.
+	// For a pre-existing snapshot, readyToUse will be set to the "read_to_use" field


"ready_to_use"

msau42 · 2019-10-23T21:56:28Z

pkg/apis/volumesnapshot/v1beta1/types.go

+// TODO(xiangqian): Add a webhook to ensure that VolumeSnapshotContentSource members
+// will not be updated once specified.
+type VolumeSnapshotContentSource struct {
+	// volumeHandle specifies the CSI name of the volume from which a snapshot


don't see an update?

xing-yang · 2019-10-24T01:46:04Z

In VolumeSnapshotSpec, we have "VolumeSnapshotClassName".
In VolumeSnapshotContentSpec, we have "SnapshotClassName".
Can we be consistent and name both "VolumeSnapshotClassName"?

yuxiangqian · 2019-10-24T03:45:12Z

In VolumeSnapshotSpec, we have "VolumeSnapshotClassName".
In VolumeSnapshotContentSpec, we have "SnapshotClassName".
Can we be consistent and name both "VolumeSnapshotClassName"?

good catch, updated to use "VolumeSnapshotClassName" everywhere.

msau42 · 2019-10-24T16:48:43Z

pkg/apis/volumesnapshot/v1beta1/types.go

+	// In dynamic snapshot creation case, it will be filled in upon snapshot creation
+	// with the "creation_time" field returned from CSI "CreateSnapshot" gRPC call.
+	// For a pre-existing snapshot, it will be set to the "size_bytes" value returned
+	// from the CSI "ListSnapshots" gRPC call if the driver exists and supports it


Can you follow the same wording as in the VolumeSnapshotContent object for these fields?

Having the same wording in both places may introduce other confusions.
i.e., CreationTime field in content status is an int64 which represents the epoch time of the snapshot cutting, in Snapshot status, its a datatime object for user readability.
RestoreSize in content status is an int64, where in Snapshot status its a resource.Quantity
another thing would be "binding" condition, for content status, it will be updated as long as driver supports, but for VolumeSnapshot status, it will only get updated after binding happened.

chatted offline with @msau42 , she meant only the CSI pieces not the entire block. will update.

msau42 · 2019-10-24T16:51:38Z

config/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml

+    listKind: VolumeSnapshotClassList
+    plural: volumesnapshotclasses
+    singular: volumesnapshotclass
+  scope: Cluster


I mean to test that this v1beta1 schema is compatible with v1. Can we temporarily convert this to v1 and try it on a 1.16 cluster?

msau42 · 2019-10-24T16:53:36Z

Can you also write up a quick README on how to make API changes? I assume we need to invoke some tools after modifying types.go.

msau42 · 2019-10-24T16:57:16Z

Structurally, I would like to move the api and generated clients outside of "pkg". We can do that as a separate pr: #186

msau42

Just one nit from me

msau42 · 2019-10-24T21:18:23Z

config/crd/snapshot.storage.k8s.io_volumesnapshotcontents.yaml

            source:
              description: source specifies from where a snapshot will be created.
                This field is immutable after creation. Required.
              properties:
                snapshotHandle:
-                  description: snapshotHandle specifies the CSI name of a pre-existing
+                  description: snapshotHandle specifies the "snapshot_id" of a pre-existing


Should we specify this is the CSI snapshot_id? ditto everywhere

I think the context should provide enough information?
I can add if that sound more clear. LMK

yeah let's just quickly clarify that this is the CSI field since all the other fields we say which csi call it's from

msau42 · 2019-10-24T23:23:54Z

config/crd/snapshot.storage.k8s.io_volumesnapshotcontents.yaml

                  type: string
                volumeHandle:
-                  description: volumeHandle specifies the CSI name of the volume from
-                    which a snapshot should be dynamically taken from. This field
+                  description: volumeHandle specifies the "volume_id" of the volume


sorry, also specify "CSI" here too (and everywhere else snapshot_id, or volume_id) is used

msau42 · 2019-10-24T23:26:14Z

also please squash commits

preserveUnknownField set to false, comments updates, adding pull request annotation more comment updates VolumeSnapshot comments rename to VolumeSnapshotClassName adding license

yuxiangqian · 2019-10-24T23:38:33Z

updated and squashed. Anything more before I do a final push? @msau42

msau42 · 2019-10-24T23:50:45Z

/lgtm
/approve
Thanks so much for all the hard work!

k8s-ci-robot · 2019-10-24T23:50:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, 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

yuxiangqian · 2019-10-24T23:52:09Z

thanks @msau42! now let's move to splitting controllers.

xing-yang · 2019-10-25T00:20:43Z

Awesome! Thanks @msau42!

a1e1127 Merge pull request kubernetes-csi#139 from pohly/kind-for-kubernetes-latest 1c0fb09 prow.sh: use KinD main for latest Kubernetes 1d77cfc Merge pull request kubernetes-csi#138 from pohly/kind-update-0.10 bff2fb7 prow.sh: KinD 0.10.0 git-subtree-dir: release-tools git-subtree-split: a1e1127

bc0504a Merge pull request kubernetes-csi#140 from jsafrane/remove-unused-k8s-libs 5b1de1a go-get-kubernetes.sh: remove unused k8s libs 49b4269 Merge pull request kubernetes-csi#120 from pohly/add-kubernetes-release a1e1127 Merge pull request kubernetes-csi#139 from pohly/kind-for-kubernetes-latest 1c0fb09 prow.sh: use KinD main for latest Kubernetes 1d77cfc Merge pull request kubernetes-csi#138 from pohly/kind-update-0.10 bff2fb7 prow.sh: KinD 0.10.0 95eac33 Merge pull request kubernetes-csi#137 from pohly/fix-go-version-check 437e431 verify-go-version.sh: fix check after removal of travis.yml 1748b16 Merge pull request kubernetes-csi#136 from pohly/go-1.16 ec844ea remove travis.yml, Go 1.16 df76aba Merge pull request kubernetes-csi#134 from andyzhangx/add-build-arg e314a56 add build-arg ARCH for building multi-arch images, e.g. ARG ARCH FROM k8s.gcr.io/build-image/debian-base-${ARCH}:v2.1.3 7bc70e5 Merge pull request kubernetes-csi#129 from pohly/squash-documentation e0b02e7 README.md: document usage of --squash 316cb95 Merge pull request kubernetes-csi#132 from yiyang5055/bugfix/boilerplate 26e2ab1 fix: default boilerplate path 1add8c1 Merge pull request kubernetes-csi#133 from pohly/kubernetes-1.20-tag 3e811d6 prow.sh: fix "on-master" prow jobs 1d60e77 Merge pull request kubernetes-csi#131 from pohly/kubernetes-1.20-tag 9f10459 prow.sh: support building Kubernetes for a specific version f7e7ee4 docs: steps for adding testing against new Kubernetes release fe1f284 Merge pull request kubernetes-csi#121 from kvaps/namespace-check 8fdf0f7 Merge pull request kubernetes-csi#128 from fengzixu/master 1c94220 fix: fix a bug of csi-sanity a4c41e6 Merge pull request kubernetes-csi#127 from pohly/fix-boilerplate ece0f50 check namespace for snapshot-controller dbd8967 verify-boilerplate.sh: fix path to script 9289fd1 Merge pull request kubernetes-csi#125 from sachinkumarsingh092/optional-spelling-boilerplate-checks ad29307 Make the spelling and boilerplate checks optional 5f06d02 Merge pull request kubernetes-csi#124 from sachinkumarsingh092/fix-spellcheck-boilerplate-tests 48186eb Fix spelling and boilerplate errors 71690af Merge pull request kubernetes-csi#122 from sachinkumarsingh092/include-spellcheck-boilerplate-tests 981be3f Adding spelling and boilerplate checks. 2bb7525 Merge pull request kubernetes-csi#117 from fengzixu/master 4ab8b15 use the tag to replace commit of csi-test 5d74e45 change the csi-test import path to v4 7dcd0a9 upgrade csi-test to v4.0.2 git-subtree-dir: release-tools git-subtree-split: bc0504a

…latest prow.sh: use KinD main for latest Kubernetes

STOR-1700: Rebase `external-snapshotter` to v7.0.0 to get VolumeGroupSnapshot

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Jul 11, 2019

k8s-ci-robot requested review from jingxu97 and saad-ali July 11, 2019 19:35

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 11, 2019

xing-yang removed the request for review from saad-ali July 11, 2019 19:35

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 11, 2019

k8s-ci-robot assigned msau42 and saad-ali Jul 11, 2019

k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label Jul 31, 2019

saad-ali requested changes Aug 7, 2019

View reviewed changes

saad-ali requested changes Aug 9, 2019

View reviewed changes

xing-yang force-pushed the beta branch from 86234ff to 038d92c Compare August 11, 2019 17:26

xing-yang mentioned this pull request Aug 19, 2019

Snapshot / Restore Volume Support for Kubernetes (CRD + External Controller) kubernetes/enhancements#177

Closed

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 23, 2019

yuxiangqian force-pushed the beta branch from 04497ae to 9b86d7c Compare August 23, 2019 06:15

msau42 reviewed Aug 23, 2019

View reviewed changes

xing-yang commented Aug 26, 2019

View reviewed changes

pkg/apis/volumesnapshot/v1beta1/types.go Outdated Show resolved Hide resolved

yuxiangqian force-pushed the beta branch 2 times, most recently from 0db4142 to 280d275 Compare August 27, 2019 00:38

msau42 reviewed Aug 27, 2019

View reviewed changes

yuxiangqian reviewed Aug 27, 2019

View reviewed changes

yuxiangqian force-pushed the beta branch from 280d275 to 060446d Compare August 27, 2019 06:54

msau42 reviewed Oct 23, 2019

View reviewed changes

yuxiangqian force-pushed the beta branch from daeb301 to cb69191 Compare October 24, 2019 00:45

yuxiangqian force-pushed the beta branch from 0e10048 to 693a0a7 Compare October 24, 2019 03:44

msau42 reviewed Oct 24, 2019

View reviewed changes

yuxiangqian force-pushed the beta branch from 693a0a7 to be76d8c Compare October 24, 2019 21:08

msau42 reviewed Oct 24, 2019

View reviewed changes

yuxiangqian force-pushed the beta branch from be76d8c to 9d16ba6 Compare October 24, 2019 21:47

msau42 reviewed Oct 24, 2019

View reviewed changes

Update VolumeSnapshot CRD version to v1beta

0c1faac

preserveUnknownField set to false, comments updates, adding pull request annotation more comment updates VolumeSnapshot comments rename to VolumeSnapshotClassName adding license

yuxiangqian force-pushed the beta branch from 9d16ba6 to 0c1faac Compare October 24, 2019 23:43

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

k8s-ci-robot merged commit bc6e42d into kubernetes-csi:master Oct 24, 2019

xing-yang mentioned this pull request Nov 11, 2019

Update Snapshot CRD version to v1beta1 #141

Closed

xing-yang mentioned this pull request Sep 6, 2020

Update volume snapshot CRDs to v1 version #367

Merged

pohly mentioned this pull request Mar 24, 2021

master: update release-tools #486

Merged

ggriffiths mentioned this pull request Apr 16, 2021

Add latest release-tools to release-4.0 branch #498

Merged

xing-yang pushed a commit to xing-yang/external-snapshotter that referenced this pull request Jul 26, 2021

Merge pull request kubernetes-csi#139 from pohly/kind-for-kubernetes-…

a1e1127

…latest prow.sh: use KinD main for latest Kubernetes

dobsonj pushed a commit to dobsonj/external-snapshotter that referenced this pull request Mar 8, 2024

Merge pull request kubernetes-csi#139 from mpatlasov/rebase-v7.0.0

196496e

STOR-1700: Rebase `external-snapshotter` to v7.0.0 to get VolumeGroupSnapshot

Update VolumeSnapshot CRD version to v1beta #139

Update VolumeSnapshot CRD version to v1beta #139

Conversation

xing-yang commented Jul 11, 2019

msau42 commented Jul 11, 2019

xing-yang commented Jul 24, 2019

msau42 commented Jul 31, 2019

johnbelamaric commented Aug 7, 2019

saad-ali left a comment

Choose a reason for hiding this comment

saad-ali left a comment

Choose a reason for hiding this comment

msau42 left a comment

Choose a reason for hiding this comment

msau42 commented Aug 23, 2019

yuxiangqian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xing-yang commented Oct 24, 2019

yuxiangqian commented Oct 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Oct 24, 2019

msau42 commented Oct 24, 2019

msau42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Oct 24, 2019

yuxiangqian commented Oct 24, 2019 • edited Loading

msau42 commented Oct 24, 2019

k8s-ci-robot commented Oct 24, 2019

yuxiangqian commented Oct 24, 2019

xing-yang commented Oct 25, 2019

yuxiangqian commented Oct 24, 2019 •

edited

Loading