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

Documentation for volume scheduling alpha feature #6392

Merged
merged 1 commit into from
Dec 2, 2017

Conversation

msau42
Copy link
Member

@msau42 msau42 commented Nov 21, 2017

Need to document:

  • volume scheduling concept
  • new StorageClass VolumeBindingMode
  • update local volume documentation

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 21, 2017
@k8sio-netlify-preview-bot
Copy link
Collaborator

k8sio-netlify-preview-bot commented Nov 21, 2017

Deploy preview ready!

Built with commit 809604a

https://deploy-preview-6392--kubernetes-io-vnext-staging.netlify.com

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 22, 2017
@msau42
Copy link
Member Author

msau42 commented Nov 22, 2017

@tengqm @zacharysarah I noticed that StorageClasses got split out into its own page on the master branch. Are there plans to also cherry pick those changes to the 1.9 branch?

/assign @saad-ali @jsafrane @thockin

@zacharysarah
Copy link
Contributor

@msau42 Yes; they'll get picked up the next time I merge master into release-1.9. In fact, I'll go do that right now.

@msau42 msau42 force-pushed the release-1.9 branch 2 times, most recently from b6ba54a to 937424f Compare November 22, 2017 23:42
@msau42 msau42 changed the title WIP Documentation for volume scheduling alpha feature Documentation for volume scheduling alpha feature Nov 23, 2017
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 23, 2017
@zacharysarah zacharysarah added this to the 1.9 milestone Nov 27, 2017
@msau42 msau42 force-pushed the release-1.9 branch 3 times, most recently from 99ad3d2 to 3b83d07 Compare November 27, 2017 23:18
This volume type is alpha in 1.7.
{% assign for_k8s_version="v1.7" %}{% include feature-state-alpha.md %}

This alpha feature requires the `PersistentLocalVolumes` feature gate to be
Copy link
Contributor

Choose a reason for hiding this comment

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

PersistentLocalVolumes became a thing since v1.9, how come the 'local' volume type is available since v1.7?

Copy link
Member Author

Choose a reason for hiding this comment

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

PersistentLocalVolumes was a new feature added in 1.7.

VolumeScheduling is a new feature in 1.9.

Copy link
Member Author

Choose a reason for hiding this comment

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

They are considered two separate features because the VolumeScheduling feature can theoretically work for more than just local volumes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm this is even messier though. Starting in 1.9, the VolumeScheduling feature gate is required if you also need the PersistentLocalVolumes feature.

Copy link
Contributor

@tengqm tengqm Nov 30, 2017

Choose a reason for hiding this comment

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

Okay, line 442 is clarifying this requirement.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry can you clarify? I added a note stating that starting in 1.9, the VolumeScheduling feature gate is also required

Copy link
Contributor

Choose a reason for hiding this comment

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

Just did a search over the source code. It looks to me that the VolumeScheuling feature gate has to be enabled on the following components:

  • apiserver: for validation
  • kcm: for pv controller
  • scheduler: for scheduler behavior changes

For PersistentLocalVolumes, it has to be enabled on the following components:

  • apiserver: for validation
  • kubelet: for mounter
  • scheduler: for node affinity checking?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes that's correct. But like I mentioned in the other PR, should we just recommend that that the feature gates be set on all components? It would be a pain for an administrator to keep track of which feature gates are enabled where for each feature.

@msau42 msau42 force-pushed the release-1.9 branch 2 times, most recently from 1ff6131 to 57ace35 Compare November 29, 2017 00:48
### Volume Binding Mode
{% assign for_k8s_version="v1.9" %}{% include feature-state-alpha.md %}

This field requires the `VolumeScheduling` feature gate to be enabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

Link for how to enable?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we have a page describing how to enable feature gates, but if you search the site, it will quickly point to some generated docs on each of the Kubernetes components and the flags that they take.

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be beneficial to provide a link here so users don't have to search for it themselves.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the challenge is there isn't just one link that describes the whole sequence. It's just every single kubernetes component has a help page describing their flags that you can pass.

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -481,6 +486,10 @@ spec:
**Note:** The local PersistentVolume cleanup and deletion requires manual intervention without the external provisioner.
{: .note}

Starting in 1.9, local volume binding can be delayed by creating a StorageClass
Copy link
Contributor

Choose a reason for hiding this comment

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

"can be delayed [until scheduling time]" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

done


A new field, `volumeBindingMode`, controls if volume binding should happen
immediately or wait until pod scheduling. Valid values are `Immediate` and
`WaitForFirstConsumer`. The default mode is `Immediate`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider clarifying that Immediate is equivalent to the behavior prior to 1.9.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

This requires the `VolumeScheduling` feature gate to be enabled.

Normally, volume binding occurs immediately upon PVC creation.
However, for PVs that specify NodeAffinity, it may be beneficial to delay
Copy link
Contributor

Choose a reason for hiding this comment

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

What is "PVs that specify NodeAffinity"?
From PersistentVolumeSpec or PersistentVolumeClaimSpec, I cannot find a hint where to specify node affinity.

Trying to make a guess ... it is about "PVs which require a node affinity be specified on the consuming Pod"?
Not accurate though. "PV types which are meant to be consumed on some specific nodes, where the nodes can be either specified using node affinity or can be determined after the pod is scheduled. In the latter case, a support to late binding is necessary."
Also, this seems only affecting dynamically provisioned volumes ...
please consider make this clear.

controlled through the `VolumeBindingMode` parameter in the
[StorageClass](storage-classes.md#thestorageclassresource).

**Note:** Dynamic provisioning is not supported yet with this alpha feature.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is very confusing. If dynamic provisioning is not supported, what are the use cases of setting 'volumeBindingMode` property?

that the volume binding decision will also be evaluated with any other node
constraints the pod may have, such as node resource requirements, node
selectors, pod affinity, and pod anti-affinity. This new mode of binding is
controlled through the `VolumeBindingMode` parameter in the
Copy link
Contributor

Choose a reason for hiding this comment

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

from user's perspective, this is better described as "the volumeBindingMode field" for better matching the property name users see.


**Note:** The `WaitForFirstConsumer` volume binding mode does not support
dynamic provisioning yet. Also, it currently is only useful for the
[local](volumes.md#local) volume type. Additional volume types will be
Copy link
Contributor

Choose a reason for hiding this comment

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

ah ... I see. This is the only use case. Makes sense now .

I'd suggest we avoid document "predictions" about future, so the last sentence can be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since this feature is currently only useful for local volumes right now, I could move all of this under the local volume section instead. And when we support more volume types in the future, then I can move it out.

@msau42
Copy link
Member Author

msau42 commented Dec 1, 2017

Updated to only document the new changes under the local volume section, since it's currently restricted to only local volumes in the code.

@msau42
Copy link
Member Author

msau42 commented Dec 1, 2017

PTAL @jsafrane @saad-ali

Copy link
Contributor

@zacharysarah zacharysarah left a comment

Choose a reason for hiding this comment

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

Good work. ✨ Minor formatting edits only:

  • Only one space after periods
  • Add note formatting to line 442

@@ -60,7 +60,7 @@ please check [kube-apiserver](/docs/admin/kube-apiserver/) documentation.

### Binding

A user creates, or has already created in the case of dynamic provisioning, a `PersistentVolumeClaim` with a specific amount of storage requested and with certain access modes. A control loop in the master watches for new PVCs, finds a matching PV (if possible), and binds them together. If a PV was dynamically provisioned for a new PVC, the loop will always bind that PV to the PVC. Otherwise, the user will always get at least what they asked for, but the volume may be in excess of what was requested. Once bound, `PersistentVolumeClaim` binds are exclusive, regardless of the mode used to bind them.
A user creates, or has already created in the case of dynamic provisioning, a `PersistentVolumeClaim` with a specific amount of storage requested and with certain access modes. A control loop in the master watches for new PVCs, finds a matching PV (if possible), and binds them together. If a PV was dynamically provisioned for a new PVC, the loop will always bind that PV to the PVC. Otherwise, the user will always get at least what they asked for, but the volume may be in excess of what was requested. Once bound, `PersistentVolumeClaim` binds are exclusive, regardless of how they were bound. A PVC to PV binding is a one-to-one mapping.
Copy link
Contributor

Choose a reason for hiding this comment

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

Only one space after periods, please.

Copy link
Member Author

Choose a reason for hiding this comment

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

done. I also fixed a bunch of other places nearby, so this change just got a little bigger.

This alpha feature requires the `PersistentLocalVolumes` feature gate to be
enabled.

Starting in 1.9, the `VolumeScheduling` feature gate must also be enabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems worth a note:

**Note:** Starting in 1.9, the `VolumeScheduling` feature gate must also be enabled.
{: .note}

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@zacharysarah
Copy link
Contributor

Docs LGTM pending tech review 👍

@jsafrane
Copy link
Member

jsafrane commented Dec 2, 2017

lgtm from tech side

@tengqm tengqm merged commit 39908ed into kubernetes:release-1.9 Dec 2, 2017
@msau42 msau42 deleted the release-1.9 branch February 9, 2018 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.