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

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

Closed
euank opened this issue Aug 24, 2016 · 13 comments
Closed
Labels
area/api Indicates an issue on api area. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage.

Comments

@euank
Copy link
Contributor

euank commented Aug 24, 2016

Note: I'm happy to write a more formal proposal out, I just wanted to quickly float the idea and see if it makes any sense to have.

Introduction

A hostPath volume source is probably the simplest one to define, needing only a single path. However, that simplicity comes with many assumptions and caveats.

Right now, the largest caveat with a hostPath volume under Docker is that the given target will be created as an empty directory owned by root if it does not already exist. This is rarely what the user actually wants because hostPath volumes are typically used to express a dependency on an existing external file or directory.

To copy from a different comment: hostPath volumes have the following uses:

Hostpath use-cases

  1. Communicating with an external system that produced data, is producing data, or is listening on a unix socket, such as a user who produced a configuration directory for the application, or X and the X11 socket.
  2. Communicating with external systems that will consume data created by the application; e.g. a container that produces logs at /var/log/fooApp and wishes to bindmount it to the host at /var/log/fooApp as well because the operator knows an external logging process will scan through that directory and collect them.
  3. Communicating between instances of yourself: e.g. a container that bindmounts in /var/cache/appName/thumbnails for each run and populates it with any thumbnails not present, but prefers to re-use existing ones between invocations simply to avoid unneeded processing.

Proposed API change

I propose that the v1.HostPathVolumeSource be changed to include the following field:

  1. type - optional string of auto|exists|file|device|socket|directory - If not set, defaults to auto, whereby any existing path will be accepted, and non-existent paths be created as new directories If set to a given value, something of the given type must be present at that path. If there is nothing present, or it is not of the specified type, the pod will fail to run.

Note: For the default case, the presence or absence of the given path is resolved at container-runtime for any container that references the given path. For the other cases, it is resolved when the pod is placed prior to any containers being run.

Reason for change

In the first use-case above, creating the nonexistent thing is not a desirable action and can end up breaking things (aside, docker once had a bug where it created the docker.sock after starting old containers, so any restart=always containers bindmounting the docker.sock would create a directory there and crashloop the daemon).

In the second and third use-cases, you do want the directory created. I think the first use-case is, however, the most common one.

Because Kubernetes runs across many machines and should value correctness, I propose that the hostPath volume should allow a user to specify additional expectations about what already exists, or does not, at that path such that it can fail quickly if those assumptions are not met, rather than mutate the host filesystem.

This ensures that a host that is in an odd state (e.g. with some socket not existing that should) does not end up in a worse state, and also allows a pod to more clearly express the dependency it has such that an operator who accidentally runs it on a system it is not suited for can see quickly and clearly what went wrong.

Other considerations

the subPath feature is also relevant because it is a way in which one can reference directories which might-or-might not exist within a volume as well. One possible way to resolve both hostPath and subPath at the same time would be making my above proposed api-changes attributes of the reference within the container, not the volume source outside of it. I think that having the attribute of the volume on the host attached to the source is neater and makes more logical sense however, so I'd prefer keeping it there. Other solutions are to simply assume subPaths should always be created, or to duplicate these options for subPath as well. Opinions welcome!

The examples folder contains many examples of pods where this feature would be useful. For example:

  1. sysdig-cloud example -- If /boot does not exist or /var/run/docker.sock is at an alternate location, this should fail to run and the operator should edit the pod, not clobber the host
  2. storage/vitess example -- If /etc/ssl/certs is not present (e.g. on /etc/pki/ distros) it should not be created, it should be an error
  3. newrelic example -- Same as sysdig, a docker.sock reference
  4. ... many others in the wild I'm sure

Opinions on whether this increase in expressiveness is worth the increased API surface area. I think it is, but second opinions would be much appreciated.

cc @kubernetes/sig-storage @thockin @jonboulle

@euank
Copy link
Contributor Author

euank commented Aug 24, 2016

Related: #16888

@thockin
Copy link
Member

thockin commented Aug 24, 2016

SGTM overall - the subPath point is interesting. Would like to see more
pro/con analysis.

On Wed, Aug 24, 2016 at 3:49 PM, Euan Kemp [email protected] wrote:

Related: #16888 #16888


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#31384 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVL8XD77ZTqkZ0hLqSSb-R5Opwv75ks5qjMp5gaJpZM4Jsh0I
.

@yujuhong yujuhong added sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/node Categorizes an issue or PR as relevant to SIG Node. and removed area/kubectl labels Aug 25, 2016
@k8s-github-robot k8s-github-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. and removed sig/node Categorizes an issue or PR as relevant to SIG Node. labels Aug 25, 2016
@yifan-gu
Copy link
Contributor

cc @bgrant0607

@yujuhong yujuhong added the area/api Indicates an issue on api area. label Aug 25, 2016
@euank
Copy link
Contributor Author

euank commented Aug 26, 2016

I think what makes it confusing is that subPath is really an attribute of the volume source (it modifies the volume source path), not the VolumeMount. As such, it being attached to the volume mount ends up with the API being icky in this case.

I think it would have been cleaner to implement it as its own type of volume for consistency... and it also would make this change easier to model for it.

What I wish subPath were is the following:

spec:
  containers:
  - name: example
    volumeMounts:
    - name: dbus
      mountPath: /var/run/dbus/
    - name: dockersock
      mountPath: /var/run/docker.sock
  volumes:
  - name: var-run
    hostPath:
      path: /var/run
      type: directory
  - name: dbus
    subPath:
      parent: var-run
      subPath: dbus # or maybe just 'path'
      type: directory
  - name: dockersock
    subPath:
      parent: var-run
      subPath: docker.sock
      type: socket

However, keeping a happy backwards compatible api is more important, and I wouldn't want two different ways to do subPaths so the above isn't useful for this discussion other than framing why this feels so odd in the first place.

Without further ado: The pros for putting them as an attribute of HostPathVolumeSource vs including them as an attribute of the container's VolumeMount.

Volume source vs Volume mount

Important things are starred

Pros (of HostPathVolumeSource attribute)

  • This is consistent with the fact that the attributes refer to the host side of the bindmount. It has no effect on the container side of it ⭐
  • In practically all cases, multiple containers using the same mount would want the same choice (e.g. two containers in a pod both want a docker.sock, which will have the same type/create constraints)
  • Slightly shorter pod yamls
  • Clear from the API that the option only applies to hostPath really, not to e.g. nfs type volumes ⭐ ⭐

Cons (vs VolumeMount attribute)

  • subPath specs in VolumeMounts will either have to have a separate / duplicate pair of specifiers, or simply always be created if-not-exist; they cannot be captured by HostPathVolumeSource adequately
  • Collocates the concept of "this path should be this" with the consumer of that path, providing better context on why the failure scenario makes sense ⭐

These lists are pretty short, but I think the pros are important. Especially the ability to explicitly attach these as part of HostPathVolumeSource vs attach them to VolumeMount and have them be essentially meaningless for every other type of volume

specify subPath too vs no option

However, that leaves the question of what to do for subPath.

I propose that we add one additional subPath option, subPathType:, which has the exact same possible arguments as HostPathVolumeSource.type and behaves similarly.

I think the other sane option is to simply not allow subPaths to tweak this option at all.

Pros (of allowing specification)

  • For the same reason it is valuable to express your dependency on the presence/absence of data in hostPath volumes, it's important to here ⭐
  • subPath volumes are expected to effectively be 'hostPath' directories within a parent volume mount; it's consistent with hostPath

Cons (vs not allowing)

  • It adds another item to the API that might not be used much or ever since the existing behavior isn't too bad ⭐
  • subPath directories are contained within another volume and thus quite likely to not have as significant impact on the host if created
  • Having both subPath and subPathType (which is only valid with subPath set) is slightly messy api design.

split shouldCreate + type, or just type

One final pro-con split. In the original comment, I specified createPath and pathType as both being options. I think it's clear that it should be just create and type at least to avoid stuttering with hostPath, but I think that it's also worth questioning whether it should have both type and create, or just type.

I'm leaning towards having just type, and have updated my original proposal to reflect the behavior I would want there.

Pros (of having both)

  • Makes it possible to specify that a type, other than directory, can be created.
  • Might be slightly more clear whether something will be created or not

Cons (vs just type)

  • Defaulting to the existing for type being empty doesn't lose us any expressivity over now
  • It's possible to create files or sockets or what have you with init containers, so not being able to specify a file should be created can be worked around with a simple init container that creates it (though the mount might not be able to specify the type due to when types are resolved)

Conclusion

I'll toss in an opinion that we should toss on type under HostPathVolumeSource, and also toss on subPathType with identical behavior, other than only being valid when subPath is specified, under VolumeMount

@bgrant0607
Copy link
Member

bgrant0607 commented Aug 26, 2016

@jonboulle
Copy link
Contributor

Thanks for writing this up; basically +1 on your conclusion on both sides.

@pmorie
Copy link
Member

pmorie commented Aug 30, 2016

I won't have time to review this for a couple of days, but the write up looks very thorough, thanks for the attention to detail.

@euank
Copy link
Contributor Author

euank commented Sep 6, 2016

A gentle ping @pmorie.

I'd like to know what the right next step should be for this. Perhaps distill this into a proper .md proposal?
I'd also be happy to drop by a sig-storage meeting if that would be the quickest way forward.

@pmorie
Copy link
Member

pmorie commented Sep 6, 2016

@euank I think a proposal PR is definitely the right way to go. I'm having a little trouble parsing your conclusion - can you clarify?

@thockin
Copy link
Member

thockin commented Sep 7, 2016

I'm SGTM on the HostPathVolumeSource change, and the single field model
(need to enumerate sufficiently, maybe allow "Any" which implies "don't
care as long as it exists".

I'm ambivalent about the subPath part. Is it a real pain point or
hypothetical?

On Tue, Sep 6, 2016 at 11:12 AM, Paul Morie [email protected]
wrote:

@euank https://github.com/euank I think a proposal PR is definitely the
right way to go. I'm having a little trouble parsing your conclusion - can
you clarify?


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

@euank
Copy link
Contributor Author

euank commented Sep 10, 2016

I didn't get around to making a proposal for this and I'm heading off on two weeks vacation, so if anyone else wants to pick it up, please feel free!

If no one does, I'll turn it into a real proposal when I get back.

@euank
Copy link
Contributor Author

euank commented Oct 5, 2016

Discussion should happen in #34058

@euank euank closed this as completed Oct 5, 2016
k8s-github-robot pushed a commit that referenced this issue Oct 22, 2016
Automatic merge from submit-queue

proposals: Add Volume Hostpath "type" proposal

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?
xingzhou pushed a commit to xingzhou/kubernetes that referenced this issue Dec 15, 2016
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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Indicates an issue on api area. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage.
Projects
None yet
Development

No branches or pull requests

9 participants