Skip to content
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

Promote VolumePVCDataSource to beta for 1.16 #81792

Merged
merged 1 commit into from Aug 29, 2019

Conversation

@j-griffith
Copy link
Contributor

commented Aug 22, 2019

What type of PR is this?

/kind feature

**What this PR does

Promotes the VolumePVCDataSource feature (cloning) to beta for the 1.16
release.

Since alpha release in 1.15 there have been a number of minor bug fixes
in the CSI Hospath Provisioner and the CSI provisioner sidecar. We've
also added e2e tests using the Hostpath provisioner.
s PR does / why we need it**:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Website docs update: kubernetes/website#16013
Does this PR introduce a user-facing change?:

Promotes VolumePVCDataSource (Cloning) feature to beta for 1.16 release 
@j-griffith

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

/sig storage
/assign @msau42

@msau42

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

Can you also remove the "Feature" tag from the e2e test so it can run by default?

Promotes the VolumePVCDataSource feature (cloning) to beta for the 1.16
release.

Since alpha release in 1.15 there have been a number of minor bug fixes
in the CSI Hospath Provisioner and the CSI provisioner sidecar.  We've
also added e2e tests using the Hostpath provisioner.
@j-griffith j-griffith force-pushed the j-griffith:move_cloning_to_beta branch from a68f5f6 to 280b331 Aug 22, 2019
@j-griffith

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

Can you also remove the "Feature" tag from the e2e test so it can run by default?

Done

@saad-ali

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

/assign

@saad-ali

This comment has been minimized.

Copy link
Member

commented Aug 24, 2019

The only thing holding me back from approving this is a conversation I had about maybe changing DataSource from inline to a pointer.

The benefit of making it a pointer is for things like snapshots and backups it would allow the DataSource to be defined dynamically (so you could have, for example, a controller that would be able to automatically set the DataSource to "latest snapshot). This can be done via admission controller if DataSource was inline, but the problem would be how do you configure the admission controller? If config was a config map or CRD, then you have a order of creation issue (config map or CRD indicating that DataSource should be dynamically be created has to be created before PVC which goes against declarative API principles.

Discussing this offline with folks. If we decide not to do that, we are good here.

@j-griffith

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2019

The only thing holding me back from approving this is a conversation I had about maybe changing DataSource from inline to a pointer.

could be that I just don't remember that conversation.. do you by chance have links in the KEP, a GH Issue or Review?

The benefit of making it a pointer is for things like snapshots and backups it would allow the DataSource to be defined dynamically (so you could have, for example, a controller that would be able to automatically set the DataSource to "latest snapshot).

Interesting idea.. but given the requirements for enforcement of types I'm unclear how you'd represent that? I'm assuming you must be involved in recent discussions on wants from the Snapshot team? I'm also not sure I'd agree that this would be best place to represent that sort of operation. Rather than bake that into the K8s API, it would seem to me that if you followed the pattern we've been promoting/following that that sort of feature should be provided by an external controller, and not be a special case for the API (remember populators?).

This can be done via admission controller if DataSource was inline, but the problem would be how do you configure the admission controller? If config was a config map or CRD, then you have a order of creation issue (config map or CRD indicating that DataSource should be dynamically be created has to be created before PVC which goes against declarative API principles.

Discussing this offline with folks. If we decide not to do that, we are good here.

I'm curious why is this being discussed offline? I've been active on this feature for PVC's for close to 9 months now, and participated in the snapshot working group for more than a year; what "offline" mode of communication is being utilized here?

I wouldn't have any problem with not graduating to beta, however we discussed this multiple times as a working group during 1.16 planning and agreed to target this for beta. Now at the last minute there have been some "other" conversations and now we want to revisit the implementation?

I'm great with that actually, but I'd like to revisit why we even enforce these checks on the dataSource in the first place. If we opened that up for example it would make it very easy to just add your use case of some "latest snapshot".

@saad-ali

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

could be that I just don't remember that conversation.. do you by chance have links in the KEP, a GH Issue or Review?

I haven't had a chance to write it up yet. It came up as I was reviewing the snapshots API to beta.

Interesting idea.. but given the requirements for enforcement of types I'm unclear how you'd represent that? I'm assuming you must be involved in recent discussions on wants from the Snapshot team? I'm also not sure I'd agree that this would be best place to represent that sort of operation. Rather than bake that into the K8s API, it would seem to me that if you followed the pattern we've been promoting/following that that sort of feature should be provided by an external controller, and not be a special case for the API (remember populators?).

Yes, this functionality would be provided by an external controller. The problem with inline DataSource is it doesn't provide a clean way to do this. Imagine a controller that takes a CRD as input (e.g. snapshot policy, like latest) and spits out DataSource objects (the typical controller pattern). Now imagine if the DataSource is inline -- manipulating DataSource becomes much more challenging. We could get close by using an admission controller, but that means you have to make sure to "setup" the admission controller before you create your PVC, which isn't pretty.

I'm curious why is this being discussed offline? I've been active on this feature for PVC's for close to 9 months now, and participated in the snapshot working group for more than a year; what "offline" mode of communication is being utilized here?

This came up as I was reviewing the snapshots API this past week. The intention is to bring it up to the broader community ASAP, hence the comment here (and with the snapshot folks), and hopefully a follow up meeting with the broader community soon.

I wouldn't have any problem with not graduating to beta, however we discussed this multiple times as a working group during 1.16 planning and agreed to target this for beta. Now at the last minute there have been some "other" conversations and now we want to revisit the implementation?

I appreciate the hard work here, and the intention is not to torpedo anything last minute. As we promote both clones and snapshots to beta, I want to make sure we're setting ourselves up for success for both of these features as well as for generic populators, so please bear with me.

I'm great with that actually, but I'd like to revisit why we even enforce these checks on the dataSource in the first place. If we opened that up for example it would make it very easy to just add your use case of some "latest snapshot".

I'd like to set up a quick SIG call Monday 1:30 PM - 2:00 PM PT to discuss this if that works for you?

@j-griffith

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2019

@saad-ali

Yes, this functionality would be provided by an external controller. The problem with inline DataSource is it doesn't provide a clean way to do this. Imagine a controller that takes a CRD as input (e.g. snapshot policy, like latest) and spits out DataSource objects (the typical controller pattern). Now imagine if the DataSource is inline -- manipulating DataSource becomes much more challenging. We could get close by using an admission controller, but that means you have to make sure to "setup" the admission controller before you create your PVC, which isn't pretty.

I'd thought we would do something like have the controller (in this case the snapshot controller, or snapshot policy controller) do the work of querying the API for the "latest" and then requesting the create of the new claim. In other words incremental building block approach. You could use the snapshot controller to create the claim from data-source/snapshot on your behalf. In other words create-from-latest-snapshot is a snapshot API call, but I might be missing a nuance here.

That being said I'm certainly more interested in getting it "right", so if there's a better approach we need to be taking here I'm definitely interested in working through it. Getting it right is better than getting it a release sooner.

I'd like to set up a quick SIG call Monday 1:30 PM - 2:00 PM PT to discuss this if that works for you?

I'm on vacation, but may have connectivity so that would be great. If nothing else I trust the SIG to review what's needed and make the best decision here with or without my input.

@saad-ali

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

I'm on vacation, but may have connectivity so that would be great. If nothing else I trust the SIG to review what's needed and make the best decision here with or without my input.

Sounds good. I sent out the invite to SIG Storage.

I'd thought we would do something like have the controller (in this case the snapshot controller, or snapshot policy controller) do the work of querying the API for the "latest" and then requesting the create of the new claim. In other words incremental building block approach. You could use the snapshot controller to create the claim from data-source/snapshot on your behalf. In other words create-from-latest-snapshot is a snapshot API call, but I might be missing a nuance here.

We definitely want an incremental building block approach. But it doesn't quite make sense to have only the PVC object control both volume provisioning and the data source to use. For example, PVCs are used by StatefulSets, which means that the StatefulSet controller has to become data source aware to handle injection of data source. If data source were a seperate object then a seperate controller could handle the creation of the data source object and leave the stateful set controller simple.

That being said I'm certainly more interested in getting it "right", so if there's a better approach we need to be taking here I'm definitely interested in working through it. Getting it right is better than getting it a release sooner.

Agreed and agreed! Thanks again for all your hard work and thought leadership in this space.

@nrb nrb referenced this pull request Aug 27, 2019
5 of 5 tasks complete
@pure-yesmat

This comment has been minimized.

Copy link

commented Aug 28, 2019

Hi @j-griffith, do you have a link to the populators design doc/code?

@saad-ali

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

Based on the SIG Storage meetings this week, plan is that we will not block the move of Volume Clone and Volume Snapshot to beta on DataSource. Instead we will continue to work over that design in conjunction with the design for data populators and larger plan for StatefulSet/application snapshot restore workflows.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 28, 2019
@msau42

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

/priority important-soon

@saad-ali

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: j-griffith, saad-ali

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:

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

@k8s-ci-robot k8s-ci-robot merged commit b362655 into kubernetes:master Aug 29, 2019
23 checks passed
23 checks passed
cla/linuxfoundation j-griffith authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.