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

Update KEP to reflect volume cloning beta in 1.16 #1147

Merged
merged 1 commit into from
Jul 30, 2019

Conversation

j-griffith
Copy link
Contributor

Update KEP to note graduation from Alpha to Beta for sig-storage cloning feature in 1.16. Also add a note regarding the storageClass restrictions on cloning.

issue: #989

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 17, 2019
@k8s-ci-robot k8s-ci-robot requested review from childsb and saad-ali July 17, 2019 04:35
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/storage Categorizes an issue or PR as relevant to SIG Storage. labels Jul 17, 2019
@j-griffith
Copy link
Contributor Author

/assign @msau42

@@ -130,6 +132,9 @@ Given that the only implementation changes in Kuberenetes is to enable the featu

## Implementation History

Update to beta status for 1.16 release
Copy link
Member

Choose a reason for hiding this comment

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

Since we haven't landed beta yet, let's just update this to say that 1.15 is alpha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I thought we needed to update the KEP to indicate our intent to go to beta for 1.16. I'll replace that, and then when 1.16 lands I'll update the Implementation history then.

@@ -114,6 +114,8 @@ Currently the CSI provisioner already accepts the DataSource field in new provis

To emphasize above, this feature will ONLY be available for CSI. This feature wil NOT be added to In-tree plugins or Flex drivers, this is strictly a CSI only feature.

It's important to note that we intentionally require that a source PVC be in the same StorageClass as the PVC being created. This is currently required because the StorageClass determintes characteristics for a volume like `fsType`. Performing a clone from an xfs volume to an ext4 volume for example would not be acceptable, given that a storageClass can have unique information, cloning across storage classes is not something we're able to try and determine safely at this time.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this would be more visible under non-goals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a note in "non-goals" with a link, seems like a good fit, let me know what you think.

@j-griffith j-griffith force-pushed the update_cloning_kep branch 3 times, most recently from d6250bd to 9e315e2 Compare July 17, 2019 18:28
@msau42
Copy link
Member

msau42 commented Jul 17, 2019

Thanks, can you also update the last-updated entry in the top?

@j-griffith j-griffith force-pushed the update_cloning_kep branch from 9e315e2 to a4e8b18 Compare July 17, 2019 19:05
@j-griffith
Copy link
Contributor Author

Thanks, can you also update the last-updated entry in the top?

Updated, and added the status update there as well, thanks!

last-updated: 2019-04-29
status: implementable
last-updated: 2019-07-17
status: Alpha
Copy link
Member

Choose a reason for hiding this comment

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

don't think alpha is a valid value for status: https://github.com/kubernetes/enhancements/blob/master/keps/YYYYMMDD-kep-template.md

I think leaving it implementable is fine. I'm unsure if we move it to implemented once it's GA?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, interesting... sorry, that seemed like a "natural" thing. I put it back to implementable

@j-griffith j-griffith force-pushed the update_cloning_kep branch from a4e8b18 to 1d7b8a7 Compare July 17, 2019 20:24
@msau42
Copy link
Member

msau42 commented Jul 17, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 17, 2019
Copy link
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.

/lgtm
/approve
For move to beta in 1.16

@@ -114,6 +115,8 @@ Currently the CSI provisioner already accepts the DataSource field in new provis

To emphasize above, this feature will ONLY be available for CSI. This feature wil NOT be added to In-tree plugins or Flex drivers, this is strictly a CSI only feature.

It's important to note that we intentionally require that a source PVC be in the same StorageClass as the PVC being created. This is currently required because the StorageClass determintes characteristics for a volume like `fsType`. Performing a clone from an xfs volume to an ext4 volume for example would not be acceptable, given that a storageClass can have unique information, cloning across storage classes is not something we're able to try and determine safely at this time.
Copy link
Member

Choose a reason for hiding this comment

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

It's fine for beta, but we should reassess this requirement for GA and consider loosening it: Instead of adding this limitation/validation to k8s, maybe we say it is up to the storage system to decide if it can support the clone. If someone tries to clone and old/new StorageClass don't match, we can leave it up to the storage system to make that decision and fail the CreateVolume call if it wants to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@saad-ali Working on update to GA status and was going to move forward with rewording/lifting the StorageClass restriction. This seems reasonable to me (still), just want to make sure no new thoughts here since July on your side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@msau42 ^^

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we want to lift the StorageClass restriction. We need to very carefully consider the implications given kubernetes/kubernetes#85233

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: j-griffith, msau42, 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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 30, 2019
@k8s-ci-robot k8s-ci-robot merged commit 22ac1b8 into kubernetes:master Jul 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants