Skip to content

Conversation

@lvlv
Copy link
Contributor

@lvlv lvlv commented Dec 7, 2016

Move from kubernetes/kubernetes#37276

What this PR does / why we need it:
Add a design doc about HostPath volume propagation

Special notes for your reviewer:
As discussed in kubernetes/kubernetes#31504, I created a document about our use cases and our alternative implementations.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 7, 2016
@lvlv lvlv requested review from pmorie and thockin December 8, 2016 14:20
Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

lgtm

@thockin thockin merged commit ab32d77 into kubernetes:master Dec 9, 2016
@euank
Copy link
Contributor

euank commented Dec 9, 2016

I feel as if several of my comments weren't properly addressed.

Most notably, I feel like we needed discussion about the implications of "silent failure" on a misconfigured node (per my comment kubernetes/kubernetes#37276 (comment)).

return ""
}
if container.SecurityContext.Privileged {
return "rshared"
Copy link

@lucab lucab Dec 9, 2016

Choose a reason for hiding this comment

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

This is not consistent with the proposal itself and the decision below, as rshared is not the same as shared.

@lucab
Copy link

lucab commented Dec 9, 2016

From a runtime/node point of view, I'm concerned by this proposal because it completely skips discussing complex topics such as:

  • how to clean such pod resources (as mounts are now crossing pod boundaries, thus they can be kept busy indefinitely by processes outside of the pod)
  • side-effects on restarts (possibly piling up layers of full-propagation mounts)
  • how does this interacts with other mount features (nested volumeMounts may or may not propagate back to the host, depending of ordering of mount operations)
  • limitations this imposes on runtimes (RO-remounting may now affects the host, is it on purpose or a dangerous side-effect?)
  • interaction with pod semantics (docker doesn't have a pod concept, but pod-aware runtimes may perform additional moves/remounts while preparing a pod)

(most of those have been highlighted by @euank before)

rkt/rkt#3465 (and references therein) may be a good starting point for reading about real-world issues related to (some) of the above topics.

@philips
Copy link
Contributor

philips commented Dec 9, 2016

@lucab @euank I think we should file an issue against the community repo on this because I think these are legit issues. OR submit a PR noting that all of these are known un-answered issues with the proposal.

@lvlv
Copy link
Contributor Author

lvlv commented Dec 10, 2016

@lucab @euank These concerns are valid, that's why we explicitly restricted this proposal to HostPath. In the scene of HostPath, I think the side-effects you mentioned is just the design.

In HostPath volume, any runtime should NOT perform any additional action (such as clean up) other than the bind mount.
Say, we don't clean-up the content in HostPath volume, we don't clean-up mounts here, which is consistent. Users may writes to host path, while users may change mounts in host path, all these affect directly to the host path (on host), and will not be automatically cleaned up or removed by runtime/k8s.

@philips
Copy link
Contributor

philips commented Dec 16, 2016

@lvlv can you file a follow-up PR that adds these notes?

@thockin
Copy link
Member

thockin commented Dec 16, 2016

@lvlv I need the comments addressed. I hit the merge too fast since comments were lost during the repo move. My fault.

shyamjvs pushed a commit to shyamjvs/community that referenced this pull request Sep 22, 2017
Adds release note for DNS Horizontal Autoscaling
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants