Move snapshot APIs from v1beta1 to v1 by xing-yang · Pull Request #389 · kubernetes-csi/external-snapshotter

xing-yang · 2020-10-05T18:57:49Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR moves Snapshot APIs from v1beta1 to v1.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Move Snapshot APIs to V1.

k8s-ci-robot · 2020-10-05T18:57:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xing-yang

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

The pull request process is described here

Details

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

xing-yang · 2020-10-05T18:58:15Z

/assign @yuxiangqian

xing-yang · 2020-10-05T19:00:21Z

Note: Changes to tighten the validation is not done yet.

yuxiangqian · 2020-10-07T05:24:40Z

per offline discussion, this PR will need further refinement.
In order to move to snapshot API to V1, a transition period is needed, more details can be found in this KEP. In short:
phase 1: introducing validation webhook service, no API change (this has been done)
phase 2: CRD have both v1beta1 and v1 schema, v1beta1 stored in API server, serving both v1 and v1beta1
phase 3: v1 stored (potentially might still need to serve both v1 & v1beta1)

xing-yang · 2020-10-08T14:09:20Z

per offline discussion, this PR will need further refinement.
In order to move to snapshot API to V1, a transition period is needed, more details can be found in this KEP. In short:
phase 1: introducing validation webhook service, no API change (this has been done)
phase 2: CRD have both v1beta1 and v1 schema, v1beta1 stored in API server, serving both v1 and v1beta1
phase 3: v1 stored (potentially might still need to serve both v1 & v1beta1)

@yuxiangqian I have addressed your comments and made changes for phase 2: CRDs now have both v1beta1 and v1 schema, v1beta1 stored in API server, serving both v1 and v1beta1.

vendor/github.com/kubernetes-csi/external-snapshotter/client/v3/apis/volumesnapshot/v1/types.go

saad-ali · 2020-10-21T07:49:48Z

vendor/github.com/kubernetes-csi/external-snapshotter/client/v3/apis/volumesnapshot/v1/types.go

+
+	// name of the VolumeSnapshotClass to which this snapshot belongs.
+	// +optional
+	VolumeSnapshotClassName *string `json:"volumeSnapshotClassName,omitempty" protobuf:"bytes,4,opt,name=volumeSnapshotClassName"`


Should we create a Parameters field and copy the SnapshotClass.parameters field here. A snapshot must be able to operate independently of the SnapshotClass after provisioning. We have DeletionPolicy here already, which is good, but do we need parameters too?

@msau42 reminded me that PV/PVC actually handles this in a better way. Instead of copying all parameters over from the Class object (which may include lots of unnecessary information), the CSI CreateVolumeResponse contains a set of volume_context -- basically letting the driver decide what additional important info should be maintained about the volume. We should do something similar here. See https://github.com/container-storage-interface/spec/blob/master/csi.proto#L488

We'll take a look.

client/hack/update-generated-code.sh

pkg/validation-webhook/snapshot.go

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

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

yuxiangqian

some nits, overall looks good! thanks Xing

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

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

...l-snapshotter/client/v3/clientset/versioned/typed/volumesnapshot/v1/volumesnapshotcontent.go

client/hack/README.md

xing-yang · 2020-11-02T01:45:03Z

@saad-ali @msau42 @yuxiangqian addressed your comments. PTAL.

k8s-ci-robot · 2020-11-02T23:09:24Z

@xing-yang: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-csi-external-snapshotter-unit	`674e840`	link	`/test pull-kubernetes-csi-external-snapshotter-unit`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

xing-yang · 2020-11-03T00:59:29Z

Note: unit test failed because I manually modified release-tools to install the V1 snapshot CRDs.

xing-yang · 2020-11-03T00:59:54Z

/hold

k8s-ci-robot · 2020-11-12T17:51:01Z

@xing-yang: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

xing-yang · 2020-11-12T19:34:09Z

This is addressed by different PRs. Closing this one.

k8s-ci-robot requested review from jingxu97 and saad-ali October 5, 2020 18:57

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 5, 2020

xing-yang force-pushed the api_v1 branch 2 times, most recently from 9da3a7b to 5875e1d Compare October 7, 2020 04:00

xing-yang force-pushed the api_v1 branch from 5875e1d to 54e7e29 Compare October 8, 2020 14:05

xing-yang changed the title ~~WIP: Move snapshot APIs from v1beta1 to v1~~ Move snapshot APIs from v1beta1 to v1 Oct 8, 2020

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 8, 2020

xing-yang force-pushed the api_v1 branch from 54e7e29 to 70e850e Compare October 13, 2020 18:55

saad-ali reviewed Oct 21, 2020

View reviewed changes

xing-yang force-pushed the api_v1 branch from 70e850e to d417acf Compare October 21, 2020 17:52

msau42 reviewed Oct 21, 2020

View reviewed changes

client/hack/update-generated-code.sh Outdated Show resolved Hide resolved

msau42 reviewed Oct 21, 2020

View reviewed changes

pkg/validation-webhook/snapshot.go Outdated Show resolved Hide resolved

client/config/crd/snapshot.storage.k8s.io_volumesnapshots.yaml Outdated Show resolved Hide resolved

client/config/crd/snapshot.storage.k8s.io_volumesnapshotcontents.yaml Outdated Show resolved Hide resolved

xing-yang force-pushed the api_v1 branch 5 times, most recently from 5657e86 to 3a2029d Compare October 24, 2020 02:21

yuxiangqian reviewed Oct 29, 2020

View reviewed changes

xing-yang force-pushed the api_v1 branch from 3a2029d to 8325efd Compare October 31, 2020 03:27

xing-yang force-pushed the api_v1 branch 2 times, most recently from 072421f to f2621c6 Compare November 1, 2020 03:17

xing-yang mentioned this pull request Nov 1, 2020

WIP: Update snapshot v1 dependency kubernetes-csi/external-provisioner#522

Closed

Move snapshot APIs from v1beta1 to v1

226fd7b

xing-yang force-pushed the api_v1 branch from f2621c6 to 226fd7b Compare November 2, 2020 01:44

Temporarily change prow.sh to install v1 CRDs

674e840

xing-yang force-pushed the api_v1 branch from 2d60947 to 674e840 Compare November 2, 2020 22:55

xing-yang mentioned this pull request Nov 2, 2020

Move snapshot APIs and client to v1 (no controller changes) #419

Merged

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 3, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 12, 2020

xing-yang closed this Nov 12, 2020

Conversation

xing-yang commented Oct 5, 2020

Uh oh!

k8s-ci-robot commented Oct 5, 2020

Uh oh!

xing-yang commented Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xing-yang commented Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yuxiangqian commented Oct 7, 2020

Uh oh!

xing-yang commented Oct 8, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

saad-ali Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

saad-ali Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

xing-yang Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yuxiangqian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xing-yang commented Nov 2, 2020

Uh oh!

k8s-ci-robot commented Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xing-yang commented Nov 3, 2020

Uh oh!

xing-yang commented Nov 3, 2020

Uh oh!

k8s-ci-robot commented Nov 12, 2020

Uh oh!

xing-yang commented Nov 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xing-yang commented Oct 5, 2020 •

edited

Loading

xing-yang commented Oct 5, 2020 •

edited

Loading

k8s-ci-robot commented Nov 2, 2020 •

edited

Loading