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

Cleaned enable pvc datasource #76913

Merged

Conversation

@j-griffith
Copy link
Contributor

commented Apr 22, 2019

This enables the ability to specify and existing PVC as a DataSource in
a new PVC Spec (eg "clone" and existing volume).

What type of PR is this?

/kind feature

What this PR does / why we need it:

Currently the only valid input Kind for PVC DataSource is "VolumeSnapshot", this feature adds the ability to also specify a DataSource Kind of "PersistentVolumeClaim".

This results in the ability for a user to specify intent to clone and existing PVC, it does NOT implement cloning functionality in Kubernetes; that's left to the external csi provisioner and plugins. It does however provide a mechanism to request a clone operation to the csi-provisioner and ultimately the csi plugin that the original PVC is allocated from.

Currently (as of 1.14) a user is able to enter a DataSource with any information they wish, however if it's not of the type "VolumeSnapshot" and the "VolumeSnapshotDataSource" feature gate is not enabled, the input is nilled and silently ignored. This change follows the same pattern, via the "VolumeDataSource" feature gate.

Which issue(s) this PR fixes:

Implements enhancement/KEP 642
Issue kubernetes/enhancements#989

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

This change enables a user to specify a DataSource/Kind of type "PersistentVolumeClaim" in their PVC spec.  This can then be detected by the external csi-provisioner and plugins if capable.
@j-griffith

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

/assign @msau42 @jsafrane

@j-griffith

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

This supersedes #76913

@fejta-bot

This comment has been minimized.

Copy link

commented Apr 22, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@fejta-bot

This comment has been minimized.

Copy link

commented Apr 23, 2019

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/check-cla

@@ -504,6 +510,7 @@ var defaultKubernetesFeatureGates = map[utilfeature.Feature]utilfeature.FeatureS
NodeLease: {Default: true, PreRelease: utilfeature.Beta},
SCTPSupport: {Default: false, PreRelease: utilfeature.Alpha},
VolumeSnapshotDataSource: {Default: false, PreRelease: utilfeature.Alpha},
VolumePVCDataSource: {Default: false, PreRelease: utilfeature.Alpha},

This comment has been minimized.

Copy link
@xing-yang

xing-yang Apr 23, 2019

Contributor

On a second thought, I think "PVCDataSource" is better. We can't drop "Volume" from "VolumeSnapshotDataSource" because a snapshot could be for something else such as a database.

This comment has been minimized.

Copy link
@j-griffith

j-griffith Apr 23, 2019

Author Contributor

Yeah, that naming gets ugly 👍

@j-griffith j-griffith force-pushed the j-griffith:cleaned_enable_pvc_datasource branch from 2199b6f to 8f2b9a9 Apr 23, 2019

@xing-yang

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

My comment is addressed. LGTM

@jsafrane

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

LGTM from my side.

There is a small change in types.go, an already existing field is extended and its documentation is updated. Let's start formal review, it can be always shortened.

/label api-review
/priority important-soon

@msau42

This comment has been minimized.

Copy link
Member

commented May 3, 2019

/assign @saad-ali

@humblec

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

lgtm from my side . Thanks @j-griffith !

@jsafrane jsafrane added the api-review label May 9, 2019

@liggitt liggitt added this to 1.15, Unassigned in API Reviews May 11, 2019

@saad-ali

This comment has been minimized.

Copy link
Member

commented May 30, 2019

/assign @thockin
For API approval (no new API just loosening what the existing API validation to allow for volume cloning).

@thockin
Copy link
Member

left a comment

What bad things happen if we let users put whatever they want in this field and have the provisioners be responsible for checking the flag gate and eventing "not supported" ?

@@ -52,3 +52,20 @@ func volumeSnapshotDataSourceInUse(oldPVCSpec *core.PersistentVolumeClaimSpec) b
}
return false
}

func dataSourceIsEnabled(pvcSpec *core.PersistentVolumeClaimSpec) bool {
if pvcSpec.DataSource != nil {

This comment has been minimized.

Copy link
@thockin

thockin May 30, 2019

Member

Do we really need this level of checking? I mean, is it possible that external controllers could consume this with other types that we don't know about here?

This comment has been minimized.

Copy link
@j-griffith

j-griffith May 30, 2019

Author Contributor

IMO no, in fact I thought it seemed redundant and had initially just removed the restrictions on the datasource field with a new feature gate and called it done. Yes, my thought was this was designed for external controllers and I'd love to be able to use it as such. During conversations I was asked to add the checks back in.

I'm probably not helping myself out here.

// If the provisioner does not support VolumeSnapshot data source, volume will
// This field can be used to specify either:
// * An existing VolumeSnapshot object (snapshot.storage.k8s.io/VolumeSnapshot)
// * An existing PVC (""/PersistentVolumeClaim)

This comment has been minimized.

Copy link
@thockin

thockin May 30, 2019

Member

whats with teh quotes?

This comment has been minimized.

Copy link
@j-griffith

j-griffith May 30, 2019

Author Contributor

Oops, fixed.

@j-griffith

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

What bad things happen if we let users put whatever they want in this field and have the provisioners be responsible for checking the flag gate and eventing "not supported" ?

@thockin Not much really, either it does "nothing" because there isn't another controller looking for it or acting upon it, or said entry is valid and it's acted upon (or rejected) by the interested controller.

@thockin

This comment has been minimized.

Copy link
Member

commented May 30, 2019

I'm fine with this for alpha, but it seems like the final state is that the provisioner is responsible for evaluating this and deciding whether it supports the source or not (and in fact, already not all provisioners will support all sources or even support any source at all, right)?

I leave it to you and SIG (@saad-ali and @msau42 assigned here) to decide, but I will approve as-is.

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@saad-ali

This comment has been minimized.

Copy link
Member

commented May 30, 2019

What bad things happen if we let users put whatever they want in this field and have the provisioners be responsible for checking the flag gate and eventing "not supported" ?

The idea was to start off strict and loosen over time: only allow specific datasources initially, and loosen over time to eventually allow arbitrary datasources. In order to support aribitrary datasources we want to formalize the idea of "populators" independent of "provisioners". We haven't gotten that far yet, so for now we're just slightly loosening, instead of allowing anything.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

@j-griffith: you cannot LGTM your own PR.

In response to this:

/LGTM

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.

@saad-ali

This comment has been minimized.

Copy link
Member

commented May 31, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 31, 2019

j-griffith added some commits Apr 11, 2019

Enable PVC as DataSource for PVC creation
This enables the ability to specify and existing PVC as a DataSource in
a new PVC Spec (eg "clone" and existing volume).
Update unit tests and feature name
Update the unit tests to include checks for incorrect APIGroup type in
PVC DataSource and change the name of the feature gate to be more clear:
s/VolumeDataSource/VolumePVCDataSource/
Enable PVC as DataSource for PVC creation
This enables the ability to specify and existing PVC as a DataSource in
a new PVC Spec (eg "clone" and existing volume).

@j-griffith j-griffith force-pushed the j-griffith:cleaned_enable_pvc_datasource branch from f57bcbb to 62a4861 May 31, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label May 31, 2019

@j-griffith

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

@saad-ali I had to rebase; shouldn't be anything else added if you could reapply lgtm

@jsafrane

This comment has been minimized.

Copy link
Member

commented May 31, 2019

/lgtm
just restoring after rebase

@k8s-ci-robot k8s-ci-robot added the lgtm label May 31, 2019

@k8s-ci-robot k8s-ci-robot merged commit 82bfa66 into kubernetes:master May 31, 2019

21 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 Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
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

@janetkuo janetkuo added sig/storage and removed sig/apps labels Jun 6, 2019

@liggitt liggitt added this to the v1.15 milestone Jun 11, 2019

@liggitt liggitt moved this from 1.15, Unassigned to Completed, 1.15 in API Reviews Jun 11, 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.