Skip to content

Enable VolumeSnapshotDataSource Feature Gate and update e2e tests for VolumeSnapshot CRD v1beta1#80058

Merged
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
xing-yang:snapshot_beta
Nov 12, 2019
Merged

Enable VolumeSnapshotDataSource Feature Gate and update e2e tests for VolumeSnapshot CRD v1beta1#80058
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
xing-yang:snapshot_beta

Conversation

@xing-yang
Copy link
Copy Markdown
Contributor

@xing-yang xing-yang commented Jul 11, 2019 •
edited
Loading

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR enables VolumeSnapshotDataSource feature gate and updates e2e tests.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Enables VolumeSnapshotDataSource feature gate and promotes volume snapshot APIs to beta.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 11, 2019
@k8s-ci-robot k8s-ci-robot requested review from copejon and verult July 11, 2019 20:14
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 11, 2019
@xing-yang
Copy link
Copy Markdown
Contributor Author

xing-yang commented Jul 11, 2019

/assign @msau42

@xing-yang
Copy link
Copy Markdown
Contributor Author

xing-yang commented Jul 11, 2019

/retest

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2019
@xing-yang
Copy link
Copy Markdown
Contributor Author

xing-yang commented Jul 29, 2019

/retest

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 14, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 17, 2019
@xing-yang xing-yang mentioned this pull request Aug 19, 2019
Closed
@xing-yang
Copy link
Copy Markdown
Contributor Author

xing-yang commented Aug 26, 2019

/retest

@xing-yang
Copy link
Copy Markdown
Contributor Author

xing-yang commented Aug 26, 2019

/test pull-kubernetes-conformance-kind-ipv6

@xing-yang
Copy link
Copy Markdown
Contributor Author

xing-yang commented Nov 3, 2019

/test pull-kubernetes-e2e-gce-alpha-features

@xing-yang
Copy link
Copy Markdown
Contributor Author

xing-yang commented Nov 3, 2019

/test pull-kubernetes-e2e-gce-storage-slow

1 similar comment
@xing-yang
Copy link
Copy Markdown
Contributor Author

xing-yang commented Nov 3, 2019

/test pull-kubernetes-e2e-gce-storage-slow

pohly
pohly reviewed Nov 6, 2019
View reviewed changes
cluster/addons/volumesnapshots/rbac/rbac-volume-snapshot-controller.yaml Outdated
Copy link
Copy Markdown
Contributor

@pohly pohly Nov 6, 2019

Choose a reason for hiding this comment

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

Should this be "kube-system", as in volume-snapshot-controller-deployment.yaml?

cluster/addons/volumesnapshots/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml Outdated
Copy link
Copy Markdown
Contributor

@pohly pohly Nov 6, 2019

Choose a reason for hiding this comment

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

Yes, see my comment about updating the RBAC file in this PR... personally I would prefer an "upstream" first approach: always make changes in the individual repo first (because that is the canonical definition of the RBAC files!), then copy into k/k.

With the CRDs it's going to be worse if they aren't even 1:1 copies from upstream. All I can think of is making changes upstream, overwrite the file in k/k, then use "git add -i" to commit only those changes that were made upstream since the last import.

cluster/gce/config-default.sh Outdated
Copy link
Copy Markdown
Contributor

@pohly pohly Nov 6, 2019

Choose a reason for hiding this comment

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

I'm not familiar with how "cluster/addons" is used. Looking at this change makes it sound like those addons explicitly have to be enabled?

Can that be done when using hack/local-up-cluster.sh? I ran it from your branch for this PR, but the resulting cluster didn't have the snapshot CRDs.

Copy link
Copy Markdown
Contributor Author

@xing-yang xing-yang Nov 6, 2019

Choose a reason for hiding this comment

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

That needs a separate PR. I'll work on that.

@alculquicondor alculquicondor mentioned this pull request Nov 7, 2019
Closed
@xing-yang
Copy link
Copy Markdown
Contributor Author

xing-yang commented Nov 7, 2019

/test pull-kubernetes-e2e-kind

saad-ali
saad-ali approved these changes Nov 7, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@saad-ali saad-ali left a comment

Choose a reason for hiding this comment

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

I remember we had a large discussion about CSI CRDs and how, when, and by who they should be installed. We ended up moving them to core types because of the tight dependency with core controllers. In this case, all our controllers are external, so that is not a concern here.

For the CSI CRDs we initially had the controller install CRDs, but that was frowned upon. So overall this seems reasonable to me.

/approve

Will let @yuxiangqian review and LGTM

@xing-yang
Copy link
Copy Markdown
Contributor Author

xing-yang commented Nov 7, 2019

/test pull-kubernetes-e2e-gce-alpha-features

@yuxiangqian
Copy link
Copy Markdown
Contributor

yuxiangqian commented Nov 7, 2019

@msau42 will LGTM on this one. Thanks Saad!

msau42
msau42 reviewed Nov 7, 2019
View reviewed changes
cluster/addons/volumesnapshots/rbac/rbac-volume-snapshot-controller.yaml Outdated
Copy link
Copy Markdown
Member

@msau42 msau42 Nov 7, 2019

Choose a reason for hiding this comment

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

I would put the rbac in the same folder as snapshot controller manifest

Copy link
Copy Markdown
Contributor Author

@xing-yang xing-yang Nov 8, 2019

Choose a reason for hiding this comment

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

ok

cluster/gce/gci/configure-helper.sh Outdated
Copy link
Copy Markdown
Member

@msau42 msau42 Nov 7, 2019

Choose a reason for hiding this comment

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

Looking at wait-for-apiserver-and-update-fluentd above, it seems to be waiting for some objects to be initialized before applying some modification. I'm not sure we need to do the same for snapshots. Especially because this function is run in the background, I'm not really sure how useful it is. We can probably remove it and not wait for anything.

Copy link
Copy Markdown
Contributor Author

@xing-yang xing-yang Nov 8, 2019

Choose a reason for hiding this comment

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

Sure, I'll give a try.

Copy link
Copy Markdown
Contributor Author

@xing-yang xing-yang Nov 8, 2019

Choose a reason for hiding this comment

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

Removed it but then everything is failing. Nothing from cluster addon manager gets installed. I don't even see logs from cluster addon. So I have to add it back.

test/e2e/testing-manifests/storage-csi/hostpath/hostpath/csi-hostpath-provisioner.yaml Outdated
Copy link
Copy Markdown
Member

@msau42 msau42 Nov 7, 2019

Choose a reason for hiding this comment

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

Does provisioner rbac need to be updated to read the new beta snapshot objects?

Copy link
Copy Markdown
Contributor Author

@xing-yang xing-yang Nov 8, 2019

Choose a reason for hiding this comment

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

No changes are needed. It's the same API group and resource names.

@xing-yang
Copy link
Copy Markdown
Contributor Author

xing-yang commented Nov 8, 2019

/retest

@xing-yang
Copy link
Copy Markdown
Contributor Author

xing-yang commented Nov 8, 2019

/test pull-kubernetes-e2e-gce

msau42
msau42 reviewed Nov 9, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@msau42 msau42 left a comment

Choose a reason for hiding this comment

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

LGTM, just one nit

cluster/gce/gci/configure-helper.sh Outdated
Copy link
Copy Markdown
Member

@msau42 msau42 Nov 9, 2019

Choose a reason for hiding this comment

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

Can you update the comment and the function name here, we're not actually updating anything, only waiting

Copy link
Copy Markdown
Contributor Author

@xing-yang xing-yang Nov 9, 2019

Choose a reason for hiding this comment

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

Done

@msau42
Copy link
Copy Markdown
Member

msau42 commented Nov 9, 2019

/assign @zmerlynn
for cluster changes

@xing-yang
Copy link
Copy Markdown
Contributor Author

xing-yang commented Nov 9, 2019

@msau42 addressed your comments. PTAL.

@xing-yang
Copy link
Copy Markdown
Contributor Author

xing-yang commented Nov 9, 2019

/test pull-kubernetes-node-e2e-containerd

@xing-yang
Copy link
Copy Markdown
Contributor Author

xing-yang commented Nov 9, 2019

/test pull-kubernetes-integration

@xing-yang
Copy link
Copy Markdown
Contributor Author

xing-yang commented Nov 9, 2019

/test pull-kubernetes-node-e2e-containerd

1 similar comment
@xing-yang
Copy link
Copy Markdown
Contributor Author

xing-yang commented Nov 9, 2019

/test pull-kubernetes-node-e2e-containerd

@xing-yang
Copy link
Copy Markdown
Contributor Author

xing-yang commented Nov 11, 2019

/test pull-kubernetes-conformance-kind-ipv6

@msau42
Copy link
Copy Markdown
Member

msau42 commented Nov 11, 2019

/lgtm
/approve

@xing-yang
Copy link
Copy Markdown
Contributor Author

xing-yang commented Nov 11, 2019

/assign @dims

@zmerlynn
Copy link
Copy Markdown
Member

zmerlynn commented Nov 11, 2019

/approve

@msau42
Copy link
Copy Markdown
Member

msau42 commented Nov 11, 2019

/priority important-soon

@dims
Copy link
Copy Markdown
Member

dims commented Nov 12, 2019

/approve
/lgtm

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Nov 12, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, msau42, saad-ali, xing-yang, zmerlynn

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:

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

@xing-yang
Copy link
Copy Markdown
Contributor Author

xing-yang commented Nov 12, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

@xing-yang xing-yang mentioned this pull request Mar 12, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@copejon copejon Awaiting requested review from copejon

@verult verult Awaiting requested review from verult

4 more reviewers

@pohly pohly pohly left review comments

@yuxiangqian yuxiangqian yuxiangqian left review comments

@msau42 msau42 msau42 left review comments

@saad-ali saad-ali saad-ali approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@dims dims

@zmerlynn zmerlynn

@msau42 msau42

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

v1.17

Development

Successfully merging this pull request may close these issues.

8 participants

@xing-yang @yuxiangqian @msau42 @zmerlynn @dims @k8s-ci-robot @pohly @saad-ali