Propose KEP to extend allowed DataSource entries to include PVC #642
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
size/L
|
@jingxu97 and @saad-ali showed me some of this last week. I think cloning a disk is legitimate and self-evident use-case. Focus on that. The idea of software initializers is interesting, but can mostly be solved with sidecars, right? Sidecars have the distinct advantage of being active - they can resync if you need, which these initializers can never do. I'm not super inclined to go out of our way to support initializers that could just be sidecars. If they happen to fall out of the clone design, I won't go out of my way to break them, but I'd need to be convinced to lift a finger to support them. |
|
@thockin thanks for the feedback. Fore sure, the initializer portion could be a side-car; the appealing thing about the DataSource in this case for me was simply a way to formally/consistently express that, and follow the pattern that the Snapshot Controller uses (and well, because it was there). I like the side car idea, I'll have to research a bit to understand how I would express that though. Regardless, I'll focus on Cloning for now. |
j-griffith
force-pushed
the
extend_volume_source_kep
branch
from
fb0ad4f to
7a2a6b8
Compare
j-griffith
changed the title
|
@thockin I updated the PR to remove the populator component and just focus on the cloning use case. I'll follow up with a single proposal for the init/populator use cases to keep things focused. |
| reviewers: | ||
| - TBD | ||
| approvers: | ||
| - TBD |
There was a problem hiding this comment.
Can we please have approvers listed? I think this would be the SIG Storage chairs/tech leads but may review this guidance later.
|
|
||
| The external risk to adding PVC as a valid DataSource is minimal, there are also minimal risks or impacts associated with the addition of the new allowed type. The bulk of the needed changes needed are in the Provisioner and of course the implementation via the CSI plugins themeselves. | ||
|
|
||
| ## Graduation Criteria |
There was a problem hiding this comment.
The template for graduation criteria notes:
How will we know that this has succeeded? Gathering user feedback is crucial for building high quality experiences and SIGs have the important responsibility of setting milestones for stability and completeness. Hopefully the content previously contained in umbrella issues will be tracked in the Graduation Criteria section.
Aside from the basic requirements which are things being implemented, how can we know this GA quality vs beta quality?
There was a problem hiding this comment.
Added additional wording, thanks!
Yes, we've discussed this at length in sig-storage |
j-griffith
force-pushed
the
extend_volume_source_kep
branch
from
7a2a6b8 to
5ca8f61
Compare
|
@j-griffith @saad-ali @childsb What was the output of the conversation in SIG Storage? |
There was a problem hiding this comment.
Please remove any references to NEXT_KEP_NUMBER and rename the KEP to just be the draft date and KEP title.
KEP numbers will be obsolete once #703 merges.
j-griffith
force-pushed
the
extend_volume_source_kep
branch
from
5ca8f61 to
aa0e1f8
Compare
k8s-ci-robot
added
the
do-not-merge/blocked-paths
j-griffith
force-pushed
the
extend_volume_source_kep
branch
from
aa0e1f8 to
f7e14e8
Compare
k8s-ci-robot
removed
the
do-not-merge/blocked-paths
|
/uncc |
j-griffith
force-pushed
the
extend_volume_source_kep
branch
from
f7e14e8 to
8ba2a21
Compare
|
@thockin I've reworked this a bit based on discussions in sig-storage yesterday and as per recommendations from @saad-ali The proposal removes any reference to "populators", and instead just proposes removing API validation checks against the PVC DataSource 1 (which was apparently the original intent of the field). Currently works with old style provisioners requires 2 to work with CSI provisioners |
j-griffith
force-pushed
the
extend_volume_source_kep
branch
from
8ba2a21 to
f3f1944
Compare
j-griffith
force-pushed
the
extend_volume_source_kep
branch
from
f3f1944 to
85d3354
Compare
|
We should finalize on the end to end plan for how generic populators will work before relaxing the validation. Specifically we should finalize on how PVCs will be safely mounted to populators (but no other workloads) until the populator is done. |
WRT to this KEP I disagree, I specifically removed any mention of Populators, I just want to enable PVC's for cloning at this point. Additionally; why wouldn't we use the same taint mechanism we've agreed upon for snapshots? I've somewhat given up on Populators (at least for the short term) but I still think that we should get Clones working. After all it's in the CSI Spec, It doesn't introduce any risk that I can think of, and it's a pretty common back end storage feature. I'd just file a bug and add only PVC, but I can't do that because of the API validation checks without a KEP. |
|
LGTM |
Propose extending DataSource field in PVCs to allow existing in namespace PVCs (Clones).
j-griffith
force-pushed
the
extend_volume_source_kep
branch
from
85d3354 to
a36f891
Compare
|
|
||
| ## Motivation | ||
|
|
||
| Features like Cloning are common in most storage devices, not only is the capability available on most devices, it's also frequently used in various use cases whether it be for duplicating data or to use as a disaster recovery method. |
There was a problem hiding this comment.
Cloning is a common feature on most storage devices. It is used for duplicating data and, in some cases, as a disaster recovery method.
| #### Story 4 | ||
| As a cluster admin or user, I want to be able to provide the equivalent of data templates to users in the Cluster to ensure consistent and secure data sets | ||
|
|
||
| ### Implementation Details/Notes/Constraints [optional] |
There was a problem hiding this comment.
Let's specify that this functionality will only be added for CSI. In-tree volume plugins and Flex drivers will not get this functionality.
There was a problem hiding this comment.
This could be added in non-goals or constraints.
There was a problem hiding this comment.
I went ahead and added it non-goals, and also put a note in the Implementation Details/Notes section to just stress the point. Thanks
|
|
||
| ### Risks and Mitigations | ||
|
|
||
| The primary risk of this feature is a Plugin that doesn't handle Cloning in a safe way for running applications. It's assumed that the responsibility for reporting Clone Capabilities in this case is up to the Plugin, and if a Plugin is reporting Clone support that implies that they can in fact Clone Volumes without disrupting or corrupting users that may be actively using the specified source volume. |
There was a problem hiding this comment.
cool, added csi driver and csi plugin so there shouldn't be any question going forward
| * API changes allowing specification of a PVC as a valid DataSource in a new PVC spec | ||
| * Implementation of the PVC DataSource in the CSI external provisioner | ||
|
|
||
| The API can be immediately promoted to Beta, as it's just an extension of the existing DataSource field. There are no implementations or changes needed in Kubernetes other than accepting Object Types in the DataSource field. This should be promoted to GA after a release assuming no major issues or changes were needed/required during the Beta stage. |
There was a problem hiding this comment.
Currently the PVC Datasource field is gated by the VolumeSnapshotDataSource alpha feature gate.
Going forward it makes sense to add additional feature gates for each type of datasource, e.g. cloning.
But I'm not sure that cloning should get to skip alpha?
There was a problem hiding this comment.
Sure, I can change that. It just seemed that since all we were doing was adding a feature gate for PVC to be specified in DataSource that it shouldn't take a full alpha and beta to be comfortable with it. I'll remove the "skip to beta" verbage and we can go from there. If we feel more comfortable with an alpha then beta release that's great.
|
|
||
| ## Alternatives [optional] | ||
|
|
||
| Snapshots and Clones are very closely related, in fact some back ends my implement cloning via snapshots (take a snapshot, create a volume from that snapshot). Users can do this currently with Kubernetes, and it's good, however some back ends have specific clone functionality that is much more efficient, and even for those that don't, this proposal provides a simple one-step process for a user to request a Clone of a volume. |
There was a problem hiding this comment.
Should we recommend against storage systems implementing it this way? I.e. do we want to provide storage systems constraints on how and when to implement clone?
There was a problem hiding this comment.
I'm not sure honeslty; I used this as an alternative example because it came up :) The only reason I could see any storage provider using this as a clone implementation is because they don't offer cloning on their backend; in which case it's not ideal, but "ok"; they still eliminate the multi step process for the end user.
I think honestly I should've reworded this completely and left out the implementation part; and just pointed out that it's a possible alternative to get the clone behavior without this KEP.
There was a problem hiding this comment.
IMO, the clone api should be a common/single interface exposed to the user and storage provider/driver can do it by their choice in the backend. ie from snapshot or directly by their clone api.
|
@saad-ali Now that we're heading into a new release and I've removed all wording regarding populators and just discuss Clone only; any chance we can get this approved/merged so I can get the implementations done here and in the provisioner hopefully for this next release? |
|
@justaugustus I squashed your request for changes so can't really fix things up; any chance I could get you to take a look and either update or remove your review change request? |
|
/uncc |
k8s-ci-robot
added
the
lgtm
|
[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 |
k8s-ci-robot
added
the
approved
Propose extending DataSource field in PVCs to allow existing in
namespace PVCs (Clones) and external data population CRDs (Populators).