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

CSI inline volume size #1409

Open
wants to merge 3 commits into
base: master
from
Open

Conversation

@pohly
Copy link
Contributor

pohly commented Dec 13, 2019

The proposal is to introduce a standardized parameter for the size of ephemeral inline volumes. This will help users (consistent API for different drivers) and enable the implementation of new functionality in Kubernetes (capacity-aware pod scheduling).

Original this was part of the Storage Capacity Constraints for Pod Scheduling KEP, but as it also makes sense by itself it was split out.

Enhancement issue: #1472

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 13, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pohly
To complete the pull request process, please assign saad-ali
You can assign the PR to them by writing /assign @saad-ali in a comment when ready.

The full list of commands accepted by this bot can be found 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

@pohly

This comment has been minimized.

Copy link
Contributor Author

pohly commented Dec 13, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 13, 2019

@pohly: GitHub didn't allow me to request PR reviews from the following users: satoru-takeuchi.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @satoru-takeuchi @msau42

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.

@pohly pohly force-pushed the pohly:csi-inline-volume-size branch from b4e3be6 to 4bd8edc Dec 13, 2019
pohly added a commit to pohly/enhancements that referenced this pull request Dec 13, 2019
mount](https://kubernetes-csi.github.io/docs/pod-info.html) feature:
if (and only if) the driver enables that, then a new
`csi.storage.k8s.io/size` entry in
`NodePublishVolumeRequest.publish_context` is set to the string

This comment has been minimized.

Copy link
@satoru-takeuchi

satoru-takeuchi Dec 18, 2019

It's not publish_context, but volume_context. For more information, please refer to the following PR.

kubernetes-csi/docs#249

As a side note, I found this documentation error when reviewing this KEP, thanks!

This comment has been minimized.

Copy link
@pohly

pohly Dec 20, 2019

Author Contributor

Good catch and thanks for taking care of it also in the docs from which I copied this. I wonder how many other developers have skipped over this without noticing the mistake...

Fixed.

string.

This has to be optional because CSI drivers written for 1.16 might do
strict validation of the `publish_context` content and reject volumes

This comment has been minimized.

Copy link
@satoru-takeuchi

This comment has been minimized.

Copy link
@pohly

pohly Dec 20, 2019

Author Contributor

Fixed.

@satoru-takeuchi

This comment has been minimized.

Copy link

satoru-takeuchi commented Dec 18, 2019

I left some comments, others are LGTM.

This was found by Satoru Takeuchi and also fixed by him in
kubernetes-csi/docs#249.
Copy link

satoru-takeuchi left a comment

LGTM

@ymmt2005 ymmt2005 mentioned this pull request Jan 9, 2020
4 of 4 tasks complete
@pohly pohly force-pushed the pohly:csi-inline-volume-size branch 2 times, most recently from ea36596 to a75f17c Jan 17, 2020
@pohly

This comment has been minimized.

Copy link
Contributor Author

pohly commented Jan 17, 2020

I added a75f17c since @satoru-takeuchi reviewed this PR, with only one minor change to the proposal: the size string passed to the CSI driver is now explicitly documented as value in plain decimal to simplify parsing.

The main change is the addition of the parts that are needed for "implementable".

/assign @saad-ali

For final review and approval.

This adds the missing details (test plan, implementation history).

Also clarifies what exactly the string representation of the size is
when passed to the CSI driver.
@pohly pohly force-pushed the pohly:csi-inline-volume-size branch from a75f17c to 1a557c6 Jan 21, 2020
vendor-specific parameter, it's not available to the Kubernetes
scheduler.

### fsSize field

This comment has been minimized.

Copy link
@msau42

msau42 Jan 22, 2020

Member

Dose the scheduler have to look at this field and compare it with the storage pool capacity reported in #1353?

This comment has been minimized.

Copy link
@pohly

pohly Jan 23, 2020

Author Contributor

Yes, that was the original motivation for adding this field.

@pohly

This comment has been minimized.

Copy link
Contributor Author

pohly commented Jan 28, 2020

/hold

With #1353 on hold there is no urgent need to get this merged. More importantly, I'm starting to suspect that adding more features to CSI inline ephemeral volumes is just going to make the code base too complicated.

To me it seems more promising to map inline volumes to the normal provisioning and then let the normal code paths deal with provisioning, failure recovery, etc. One additional bonus would be that CSI drivers do not need to be modified to work with ephemeral inline volumes.

Here's a rough outline of that idea (from https://kubernetes.slack.com/archives/C8EJ01Z46/p1580043788038400?thread_ts=1579890578.025900&cid=C8EJ01Z46):

  • inline we allow a volume to be specified with a complete PVC (not the name, the actual spec of a PVC)
  • when the scheduler sees such a pod, it creates a new PVC in the namespace of the pod with a generated name (details to be decided...)
  • this PVC has selected-node set and then is treated like a normal PVC with late binding
    it also has the pod as owner ref, so when the pod gets deleted, so does the PVC ("ephemeral")

One concern about this approach is that these generated PVCs will be visible to users and users might do "bad things" (TM) with them because they typically have permission to do so. But the same is true for pods created by a stateful set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.