proposals: Add Volume Hostpath "type" proposal #34058

euank · 2016-10-04T22:26:21Z

This is a continuation of #31384. It's related to #26816 as well.
The discussion in #31384 is worth reading and this proposal largely derives from my comments there.

cc @thockin @pmorie @saad-ali @kubernetes/sig-storage
cc @yujuhong since it talks briefly about kubelet doing more

cc @calebamiles I think we might need a "Feature" for this since it's an api change, though a minor one?

This change is

smarterclayton · 2016-10-04T23:10:34Z

docs/proposals/volume-hostpath-qualifiers.md

+| `exists` | If nothing exists at the given path, the pod will fail to run and provide an informative error message |
+| `file` | If a file does not exist at the given path, the pod will fail to run and provide an informative error message |
+| `device` | If a block or character device does not exist at the given path, the pod will fail to run and provide an informative error message |
+| `socket` | If a socket does not exist at the given path, the pod will fail to run and provide an informative error message |


If I want to share a socket between two containers in the same pod, would I want the same support here for that? Or are we saying emptyDir and intra-pod are less challenged than pod to host?

Within a pod, it's the responsibility of those two processes to synchronize on the socket themselves.

The root problem, of some random directory being created if the socket doesn't exist, doesn't happen in that instance because the containers presumably don't implement docker's logic of "exists || mkdir", and it's well understood that the emptyDir exists as a directory with nothing in it.

intrapod is also less challenging because it isn't implicitly relying on some external state/process existing.

smarterclayton · 2016-10-04T23:12:11Z

docs/proposals/volume-hostpath-qualifiers.md

+While I propose exposing the same set of possible values for consistency, in reality a `SubPath` is useful primarily under a network mount. Because of this, the `device` and `socket` values would likely not be useful.
+
+I do not have any examples of concrete examples from the Kubernetes project for this, in part because there are very few examples of `SubPath` to begin with.
+However, each of the use-cases described for Host Volumes also could conceivably occur as subdirectories of a network mount using this feature, and thus still offer insight into why it would be of value.


I'm aware of folks using it to share host NFS mounts in predictable ways across a cluster (I think)

Will double check.

smarterclayton · 2016-10-04T23:13:33Z

docs/proposals/volume-hostpath-qualifiers.md

+
+### Containerized Kubelet
+
+A containerized kubelet would have difficulty creating directories. The implementation will likely respect the `containerized` flag, or similar, allowing it to either break out or be "/rootfs/" aware and thus operate as desired.


Containerized kubelets effectively are required to have access to host rootfs in practice (that's the only variant we support in production at Red Hat), so seems ok.

euank · 2016-10-06T19:39:36Z

I added an alternative possible API that reduces it to adding a single MustExist bool. I think the gain in simplicity is likely worth the loss in specificity, but a second opinion would be appreciated.

@jonboulle, @pmorie, @thockin if any of you have a strong opinion on the above or cycles to review, it would be appreciated!

jonboulle · 2016-10-12T15:25:43Z

Overall, this LGTM!
One question: what permissions and ownership should/would Kubelet apply if it creates a directory?

euank · 2016-10-12T15:36:06Z

@jonboulle root:root 755 seems like the sane choice. We can add knobs in the future if there's a good reason to do so. There's some discussion related to this here and this change doesn't impact that assessment.

I'll edit it in

thockin

LGTM overall

thockin · 2016-10-13T06:30:48Z

docs/proposals/volume-hostpath-qualifiers.md

+conceivably occur as subdirectories of a network mount using this feature, and
+thus still offer insight into why it would be of value.
+
+<!-- uncomment if this comes up :) An argument against providing this feature for SubPaths is that it might not increase correctness as much; while adding such qualifications to Host volumes allows a pod to express a dependency that the node may not meet, adding it to subPath allows the pod to express an assumption about a given filesystem it's mounting. This assumption is not node specific and, when it fails, likely means the given volume was not pre-populated/provisioned appropriately. It's arguable whether this sort of failure is as likely or as important to handle well. -->


I'm inclined to agree - when in doubt, leave it out,

thockin · 2016-10-13T06:34:23Z

docs/proposals/volume-hostpath-qualifiers.md

+
+| Value | Behavior |
+|:------|:---------|
+| `auto ` | If nothing exists at the given path, an empty directory will be created there. Otherwise, behaves like `exists` |


You need to handle existing objects which will spontaneously grow this field, with a default value of "". Perhaps "" is how you spell "anything".

Given that, the field might be better named as "require" something, where "" naturally means "nothing" or "no requirements"

I'm not sure I understand. Regarding handling existing objects, above I wrote "If not set, defaults to auto" which was meant to cover that. auto is the existing behaviour, so it's backwards compatible for existing objects.
Is having nil == "" == "auto" a no-no for k8s api?

require seems clever for the one case of default, but I think I like type more overall. (hostPath.type = file vs hostPath.require = file seems more natural as type).

Mind clarifying what you meant?

thockin · 2016-10-13T18:13:33Z

What I meant: "If not set, defaults to auto" means that any object that
comes through the API will have this field set to "auto". Objects that
already exist do not get the default-setting hook called, so they exist
with a literal value of "". But upon reflection, I am wrong. When they are
read from etcd, they get defaults, which handles this case.

I still think it might be cleaner to have "" be the default value, rather
than a literal "auto" - that way it will simply be omitted when the user
doesn't care.

On Thu, Oct 13, 2016 at 10:37 AM, Euan Kemp notifications@github.com
wrote:

@euank commented on this pull request.

In docs/proposals/volume-hostpath-qualifiers.md
#34058:

+of these volumes into the Kubelet to the benefit of the rkt container runtime.
+
+
+## Proposed API Change
+
+### Host Volume
+
+I propose that the
+v1.HostPathVolumeSource
+object be changed to include the following additional field:
+
+Type - An optional string of auto|exists|file|device|socket|directory - If not set, defaults to auto. These values have the following behavior:
+
+| Value | Behavior |
+|:------|:---------|
+| auto | If nothing exists at the given path, an empty directory will be created there. Otherwise, behaves like exists |

I'm not sure I understand. Regarding handling existing objects, above I
wrote "If not set, defaults to auto" which was meant to cover that. auto
is the existing behaviour, so it's backwards compatible for existing
objects.
Is having nil == "" == "auto" a no-no for k8s api?

require seems clever for the one case of default, but I think I like type
more overall. (hostPath.type = file vs hostPath.require = file seems more
natural as type).

Mind clarifying what you meant?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34058, or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVG40zd5K0Eycvlx3ZSn6F4pG2Zgpks5qzmx2gaJpZM4KOOl9
.

euank · 2016-10-13T19:12:35Z

Updated to remove subpath and change the default to "" per @thockin's comments. Agreed that that's cleaner, and we can add subpathtype later if people want it.

I also added some words about selinux (cc @pmorie) and permissions at the bottom.

pmorie · 2016-10-14T12:55:36Z

This generally LGTM, but I do have one concern that we need to check -- what the SELinux context of artifacts on the host's FS is when containerized Kubelet creates them.

euank · 2016-10-14T18:09:58Z

Just squashed, no actual changes..

k8s-ci-robot · 2016-10-14T18:19:31Z

Jenkins verification failed for commit 75e4be59d7cc14b16ebc859c57f6b9ab918686fa. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

euank · 2016-10-18T22:28:49Z

sigh missed a munger lint, no actual changes.

k8s-github-robot · 2016-10-22T02:47:41Z

Automatic merge from submit-queue

@thockin

Automatic merge from submit-queue proposals: Add Volume Hostpath "type" proposal This is a continuation of kubernetes#31384. It's related to kubernetes#26816 as well. The discussion in kubernetes#31384 is worth reading and this proposal largely derives from my comments there. cc @thockin @pmorie @saad-ali @kubernetes/sig-storage cc @yujuhong since it talks briefly about kubelet doing more cc @calebamiles I think we might need a "Feature" for this since it's an api change, though a minor one?

prabhakar-pal · 2017-03-21T12:45:02Z

Could you please inform if the hostPath type attribute has been added to kubernetes? If so, in which version?

euank · 2017-03-21T17:35:47Z

@prabhakar-pal it has not been implemented yet.

thockin · 2017-05-26T04:48:56Z

Damn, I assumed that since we merged this, we must have gotten around to implementing, but I guess not.

This would be a GREAT contribution.

bboreham · 2017-05-30T15:10:50Z

docs/proposals/volume-hostpath-qualifiers.md

+
+|Value | Behavior | Reason for exclusion |
+|:-----|:---------|:---------------------|
+| `new-directory` | Like `auto`, but the given path must be a directory if it exists | `auto` mostly fills this use-case |


auto is not defined in this document

@thockin

Automatic merge from submit-queue (batch tested with PRs 51113, 46597, 50397, 51052, 51166) implement proposal 34058: hostPath volume type **What this PR does / why we need it**: implement proposal #34058 **Which issue this PR fixes** : fixes #46549 **Special notes for your reviewer**: cc @thockin @luxas @euank PTAL

googlebot added the cla: yes label Oct 4, 2016

k8s-github-robot assigned smarterclayton Oct 4, 2016

k8s-github-robot added kind/design Categorizes issue or PR as related to design. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Oct 4, 2016

smarterclayton reviewed Oct 4, 2016

View reviewed changes

calebamiles mentioned this pull request Oct 5, 2016

Umbrella issue for the Linux Foundation (CNCF) CLA system #27796

Closed

euank mentioned this pull request Oct 5, 2016

volumes/hostPath: Proposal - support for more explicitly specified hostPath volumes #31384

Closed

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 5, 2016

thockin reviewed Oct 13, 2016

View reviewed changes

euank force-pushed the volume-qualifications branch from 724e9c6 to 7b82846 Compare October 13, 2016 19:10

thockin added release-note-none Denotes a PR that doesn't merit a release note. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed release-note-label-needed labels Oct 13, 2016

euank force-pushed the volume-qualifications branch from 7b82846 to 75e4be5 Compare October 14, 2016 17:48

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 14, 2016

thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 14, 2016

proposals: Add Volume Hostpath "type" proposal

a0739bf

euank force-pushed the volume-qualifications branch from 75e4be5 to a0739bf Compare October 18, 2016 20:56

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 18, 2016

smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 22, 2016

k8s-github-robot merged commit 24144be into kubernetes:master Oct 22, 2016

euank deleted the volume-qualifications branch October 22, 2016 18:33

euank mentioned this pull request Oct 31, 2016

proposal: Add proposal for a chrooted kubelet #35328

Closed

thockin mentioned this pull request May 26, 2017

Add iptables lock-file mount to kube-proxy manifest #46259

Merged

dixudx mentioned this pull request May 29, 2017

implement proposal 34058: hostPath volume type #46597

Merged

dixudx added a commit to dixudx/kubernetes that referenced this pull request May 30, 2017

implement proposal kubernetes#34058: hostPath volume type

780f39e

bboreham reviewed May 30, 2017

View reviewed changes

dixudx added a commit to dixudx/kubernetes that referenced this pull request May 31, 2017

implement proposal kubernetes#34058: hostPath volume type

39901d1

saad-ali mentioned this pull request Aug 1, 2017

Add hostPath volume type kubernetes/enhancements#376

Closed

dixudx mentioned this pull request Mar 21, 2018

CRI: define the mount behavior when host path does not exist #61460

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proposals: Add Volume Hostpath "type" proposal #34058

proposals: Add Volume Hostpath "type" proposal #34058

euank commented Oct 4, 2016 •

edited by k8s-oncall

Loading

smarterclayton Oct 4, 2016

euank Oct 5, 2016

smarterclayton Oct 4, 2016

smarterclayton Oct 4, 2016

smarterclayton Oct 4, 2016

euank commented Oct 6, 2016

jonboulle commented Oct 12, 2016

euank commented Oct 12, 2016 •

edited

Loading

thockin left a comment

thockin Oct 13, 2016

thockin Oct 13, 2016

euank Oct 13, 2016

thockin commented Oct 13, 2016

@euank commented on this pull request.

euank commented Oct 13, 2016

pmorie commented Oct 14, 2016

euank commented Oct 14, 2016

k8s-ci-robot commented Oct 14, 2016

euank commented Oct 18, 2016

k8s-github-robot commented Oct 22, 2016

prabhakar-pal commented Mar 21, 2017

euank commented Mar 21, 2017

thockin commented May 26, 2017

bboreham May 30, 2017


		### Containerized Kubelet

		A containerized kubelet would have difficulty creating directories. The implementation will likely respect the `containerized` flag, or similar, allowing it to either break out or be "/rootfs/" aware and thus operate as desired.

proposals: Add Volume Hostpath "type" proposal #34058

proposals: Add Volume Hostpath "type" proposal #34058

Conversation

euank commented Oct 4, 2016 • edited by k8s-oncall Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

euank commented Oct 6, 2016

jonboulle commented Oct 12, 2016

euank commented Oct 12, 2016 • edited Loading

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Oct 13, 2016

@euank commented on this pull request.

euank commented Oct 13, 2016

pmorie commented Oct 14, 2016

euank commented Oct 14, 2016

k8s-ci-robot commented Oct 14, 2016

euank commented Oct 18, 2016

k8s-github-robot commented Oct 22, 2016

prabhakar-pal commented Mar 21, 2017

euank commented Mar 21, 2017

thockin commented May 26, 2017

Choose a reason for hiding this comment

euank commented Oct 4, 2016 •

edited by k8s-oncall

Loading

euank commented Oct 12, 2016 •

edited

Loading