Redesign mount propagation #659

jsafrane · 2017-05-24T10:51:40Z

The proposal won't work as it was merged, it makes too many directories as shared (see #648).

A different approach is needed, I've chosen 'Add an option in VolumeMount API', but I would be fine also with 'Add an option in HostPathVolumeSource', there is only very small difference to me.

The proposal also describes how it will be implemented, especially during alpha phase.

Fixes #648

@kubernetes/sig-node-proposals @kubernetes/sig-storage-proposals

k8s-ci-robot · 2017-05-24T10:51:44Z

@jsafrane: These labels do not exist in this repository: sig/node, sig/storage.

In response to this:

The proposal won't work as it was merged, it makes too many directories as shared (see #648).

A different approach is needed, I've chosen 'Add an option in VolumeMount API', but I would be fine also with 'Add an option in HostPathVolumeSource', there is only very small difference to me.

The proposal also describes how it will be implemented, especially during alpha phase.

Fixes #648

@kubernetes/sig-node-proposals @kubernetes/sig-storage-proposals

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. I understand the commands that are listed here.

jsafrane · 2017-05-24T10:53:11Z

For alpha annotation, I know there is attempt to have real fields in alpha in https://docs.google.com/document/d/1wuoSqHkeT51mQQ7dIFhUKrdi3-1wbKrNWeIL4cKb9zU/edit#, but it seems it has not been implemented yet. Alpha annotation is still described as preferred way to introduce new fields in https://github.com/kubernetes/community/blob/master/contributors/devel/api_changes.md

yujuhong · 2017-05-26T00:27:47Z

Adding a couple of folks who were on the original proposal.

/cc @vishh @euank

dchen1107 · 2017-05-30T18:41:22Z

We discussed at sig-node meeting, and agreed with the redesign the proposal scope. But this feature is not targeted for 1.7 release.

inc0 · 2017-05-30T19:19:31Z

This is required change for openstack-on-kubernetes too. Could we please add sig-openstack to this review?

kfox1111 · 2017-05-30T19:32:28Z

+1 for sig-openstack.

I would also greatly benefit from it for some high energy physics work I do.

If the patch set is ready for 1.7 will it go in?

Is there anything I can do to help get it ready faster?

ddysher · 2017-06-06T08:43:02Z

This is also required for local volume provisioner. The provisioner is running as a daemonset to monitor local volume and post PV to kubernetes. To correctly detect volume capacity, the daemonset pod must mount hostpath (path where volume is detected) as rshared.

Are we targeting alpha in 1.8 now?

jsafrane · 2017-06-06T08:51:21Z

yes, we missed 1.7

ddysher · 2017-06-06T08:55:10Z

Thanks for confirm. I'd be happy to test it out when implementation is out.

jsafrane · 2017-06-08T14:23:35Z

I updated the PR with the latest development:

added links to Kubernetes PRs (waiting for review)
added note that the default propagation should be rslave. This is what was agreed in the previous proposal, now it's more explicit.
I tested my PRs with various distros and I stumbled upon Google COS (former GCI). It runs Docker in its own mount namespace with slave mount propagation. I.e. it can't ever run a pod with shared mount propagation. There should be a check in kubelet that tests if it can run a pod with shared mount propagation, at least on /var/lib/kubelet, and refuse to start if it can't.
- This test is quite tricky, the best would be to run some dummy container (gcr.io/*/pause?) with docker run -v /var/lib/kubelet:/tmp:rshared and check if it failed.
- Note that COS-60 fixes this issue, however the test might be useful if someone runs another old or ~~weird~~ unsupported distro.

msau42 · 2017-06-09T18:31:51Z

contributors/design-proposals/propagation.md

+  it will be marked as deprecated from the beginning.
+  Developers / testers can enable it in their clusters manually.
+  Mount propagation may be redesigned or even removed in any future release.
+* The default mount propagation will be `rslave`, which is different to current


Earlier, it says the default is private, to not break backwards compatiblity. Why is it rslave here?

thanks for spotting this, the default should be rslave, which should not break backward compatibility (testing needed!) and enables this use case kubernetes/kubernetes#46444 (comment)

kfox1111 · 2017-07-14T15:43:12Z

Ping. What is the status of getting this into 1.8? I don't see it marked for 1.8.

I'm stuck on k8s 1.4 currently, as I need this kind of functionality and docker 1.9 is the last version to default mounts to shared and k8s 1.4 is the last version to support docker 1.9.

I would be happy if this was merged but hidden behind an experimental flag or something, just so those of us that need to make forward progress can do so.

kfox1111 · 2017-07-18T20:04:54Z

ping

luxas · 2017-07-20T16:22:33Z

@jsafrane It would indeed be nice to have this as alpha in v1.8

jsafrane · 2017-07-20T16:25:38Z

@luxas, all that's missing is review of #46444

saad-ali · 2017-07-28T02:07:42Z

/lgtm
/approve

We discussed this design in a meeting on Monday July 24. I am fine with this.

One thing I just remembered is that there is a pending design to add "HostPathType" to hostpath volumesource: kubernetes/kubernetes#46597
Let's make sure that these two options (propagation and hostPathType play well together)

CC @timstclair @dixudx

dixudx · 2017-07-28T02:28:37Z

@saad-ali propagation will not conflict with HostPathType.

jsafrane · 2017-07-28T08:14:35Z

@timstclair was there any discussion about feature gate vs --exeprimental cmdline argument? I am happy with both, I just need a decision.

jsafrane · 2017-08-09T16:55:02Z

I would like to understand what the pros/cons of a new volume type would be.

So, let there be LinuxVolumeSource:

type LinuxVolume struct {
    MountPropagation string
    WrappedVolume *VolumeSource
}

type VolumeSource struct {
   ... aws, gce, hostPath, emptyDir, git, ...
    LinuxVolume *LinuxVolumeSource
}

It's recursive. Recursive structures in API are IMO ugly.
Does not help with application portability at all. If an application needed mount propagation it won't work on macOS / Windows at all, regardless if LinuxVolume or HostPath was used.
I do not see any added value other than increased complexity and confusion.

In my world, VolumeSource describes where to get the data from. I still personally think that it should not include things like readOnly or mountPropagation. It should be in VolumeMount struct which says how the data should be made available. It's much easier to validate it, it goes well along with VolumeMount.readOnly. I can accept that HostPathVolumeSource and EmptyDirVolumeSource is the right place, it will work well for all use cases that I know about. I don't see any reason why to invent a new volume type.

bgrant0607 · 2017-08-09T17:36:32Z

For reference:
https://lwn.net/Articles/689856/
https://www.kernel.org/doc/Documentation/filesystems/sharedsubtree.txt

kfox1111 · 2017-08-09T17:45:26Z

I agree. Properties on how something gets mounted is a totally different thing than what the thing storing the data is. hostPath, emptyDir, etc will need the flag. not, recreate volume types for rsharedHostPath and rsharedEmptyDir. Once namedEmptyDir becomes a thing, it will want it too.

kfox1111 · 2017-08-09T18:49:41Z

@thockin yeah, I get the desire for a correct api. Totally a good thing to get right. Just saying, for an alpha, getting it perfect is not so critical. I'm totally fine accepting the risk of writing software against an api that is expected to drastically change in future releases. That part is easy. Living without this feature is really really hard. I'm facing a 1.4 -> 1.8 migration path already, with broken steps in between. :/ Several of these releases are already unsupported.

As for openstack on k8s, it can't really be a complete thing until this feature merges in some form or another. Neutron really needs it.

bgrant0607 · 2017-08-09T23:44:08Z

contributors/design-proposals/propagation.md

+* Node conformance suite will check that mount propagation in /var/lib/kubelet
+  works.
+* During alpha, all the behavior above must be explicitly enabled by
+  `kubelet --feature-gates=MountPropagation=true`


ICYMI

Field gate proposal: #869

bgrant0607 · 2017-08-09T23:46:19Z

contributors/design-proposals/propagation.md

@@ -73,13 +79,18 @@ type VolumeMount struct {
 	// Required.
 	MountPath string `json:"mountPath"`
 	// Optional.
-	Propagation string `json:"propagation"`
+	Propagation PropagationMode `json:"propagation,omitempty"`


This field name is very vague. So is the file name, by the way. Propagation of what? In what direction?

This field name is very vague... Propagation of what? In what direction?

Renamed to MountPropagation with more documentation around. Still, I don't want to put whole kernel documentation there.

So is the file name

What file name?

The proposal filename.

thockin · 2017-08-11T00:06:59Z

This change is

jsafrane · 2017-08-15T08:13:29Z

We had a productive meeting yesterday with @thockin and @saad-ali and we're back at "Add an option in VolumeMount API".

While we do not see any use case for any other volume than HostPath or EmptyDir to be rshared, we will not block it, at least not in alpha. It must be explicitly enabled and having rshared GCE volume is not more harmful than HostPath.
Renamed (r)shared and (r)slave in the API to Bidirectional and HostToContainer.
Removed private as we do not see any use case for it.
We use enum instead of simple bool to be able to add private and non-recursive shared and slave if we need so. We can't extend bool so easily.

I did more edits describing what exactly will be in alpha, see the last commit.

@bgrant0607, can you please look at the API again? PR with Kubernetes changes will follow very shortly to catch 1.8

saad-ali · 2017-08-15T17:25:47Z

Revised proposal:

/lgtm
/approve

Thanks @jsafrane

saad-ali · 2017-08-22T18:01:12Z

/lgtm

k8s-github-robot · 2017-08-22T18:05:48Z

Automatic merge from submit-queue

bgrant0607 · 2017-08-23T05:54:27Z

Github notifications are useless :-(

@derekwaynecarr

Automatic merge from submit-queue (batch tested with PRs 45724, 48051, 46444, 51056, 51605) Mount propagation in kubelet Together with #45724 it implements mount propagation as proposed in kubernetes/community#659 There is: - New alpha annotation that allows user to explicitly set propagation mode for each `VolumeMount` in pod containers (to be replaced with real `VolumeMount.Propagation` field during beta) + validation + tests. "Private" is the default one (= no change to existing pods). I know about proposal for real API fields for alpha feature in https://docs.google.com/document/d/1wuoSqHkeT51mQQ7dIFhUKrdi3-1wbKrNWeIL4cKb9zU/edit, but it seems it's not implemented yet. It would save me quite lot of code and ugly annotation. - Updated CRI API to transport chosen propagation to Docker. - New `kubelet --experimental-mount-propagation` option to enable the previous bullet without modifying types.go (worked around with changing `KubeletDeps`... not nice, but it's better than adding a parameter to `NewMainKubelet` and removing it in the next release...) ```release-note kubelet has alpha support for mount propagation. It is disabled by default and it is there for testing only. This feature may be redesigned or even removed in a future release. ``` @derekwaynecarr @dchen1107 @kubernetes/sig-node-pr-reviews

Automatic merge from submit-queue (batch tested with PRs 60476, 62462, 61391, 62535, 62394). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Private mount propagation This PR changes the default mount propagation from "rslave" (newly added in 1.10) to "private" (default in 1.9 and before). "rslave" as default causes regressions, see below. Value `"None"` has to be added to `MountPropagationMode` enum in API ("I don't want any propagation at all"), which translates to "private" on Linux. [We did not have use cases for it](kubernetes/community#659 (comment)), but we have them now. **Which issue(s) this PR fixes** Fixes #62397, fixes #62396 **Special notes for your reviewer**: CRI already has an option for private mount propagation in volumes, however it's called "PRIVATE", while Kubernetes API value is "None". I did not change PRIVATE to NONE to keep the interface stable. See `kubelet_pods.go`. **Release note**: ```release-note Default mount propagation has changed from "HostToContainer" ("rslave" in Linux terminology) to "None" ("private") to match the behavior in 1.9 and earlier releases. "HostToContainer" as a default caused regressions in some pods. ``` /sig storage /sig node

Automatic merge from submit-queue (batch tested with PRs 60476, 62462, 61391, 62535, 62394). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Private mount propagation This PR changes the default mount propagation from "rslave" (newly added in 1.10) to "private" (default in 1.9 and before). "rslave" as default causes regressions, see below. Value `"None"` has to be added to `MountPropagationMode` enum in API ("I don't want any propagation at all"), which translates to "private" on Linux. [We did not have use cases for it](kubernetes/community#659 (comment)), but we have them now. **Which issue(s) this PR fixes** Fixes #62397, fixes #62396 **Special notes for your reviewer**: CRI already has an option for private mount propagation in volumes, however it's called "PRIVATE", while Kubernetes API value is "None". I did not change PRIVATE to NONE to keep the interface stable. See `kubelet_pods.go`. **Release note**: ```release-note Default mount propagation has changed from "HostToContainer" ("rslave" in Linux terminology) to "None" ("private") to match the behavior in 1.9 and earlier releases. "HostToContainer" as a default caused regressions in some pods. ``` /sig storage /sig node Kubernetes-commit: 92065407b783a6de56ce181967c2a7778aa6521a

Automatic merge from submit-queue Redesign mount propagation The proposal won't work as it was merged, it makes too many directories as shared (see kubernetes#648). A different approach is needed, I've chosen 'Add an option in VolumeMount API', but I would be fine also with 'Add an option in HostPathVolumeSource', there is only very small difference to me. The proposal also describes how it will be implemented, especially during alpha phase. Fixes kubernetes#648 @kubernetes/sig-node-proposals @kubernetes/sig-storage-proposals

Signed-off-by: Zou Nengren <zouyee1989@gmail.com>

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 24, 2017

jsafrane mentioned this pull request May 25, 2017

Mount propagation in kubelet kubernetes/kubernetes#46444

Merged

k8s-ci-robot requested review from euank and vishh May 26, 2017 00:27

ddysher mentioned this pull request Jun 7, 2017

Detect local volume capacity kubernetes-retired/external-storage#135

Merged

jsafrane force-pushed the fix-shared-propagation branch from 8536f25 to cd1a7dd Compare June 8, 2017 14:23

msau42 reviewed Jun 9, 2017

View reviewed changes

jsafrane force-pushed the fix-shared-propagation branch from cd1a7dd to 041526e Compare June 13, 2017 10:08

dchen1107 assigned saad-ali and dchen1107 Jul 15, 2017

jsafrane mentioned this pull request Jul 20, 2017

Containerized mounts kubernetes/enhancements#278

Closed

jsafrane force-pushed the fix-shared-propagation branch from 041526e to 60a34d4 Compare July 24, 2017 14:59

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 28, 2017

bgrant0607 reviewed Aug 9, 2017

View reviewed changes

Rename fields and enhance their documentation

99c31f9

Final version

7a2b2f7

jsafrane force-pushed the fix-shared-propagation branch from f67efc3 to 272c49b Compare August 15, 2017 08:09

jsafrane force-pushed the fix-shared-propagation branch from 272c49b to 7a2b2f7 Compare August 15, 2017 08:37

jsafrane mentioned this pull request Aug 15, 2017

Proposal: containerized mount utilities in pods #589

Merged

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 15, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2017

k8s-github-robot merged commit c9f14de into kubernetes:master Aug 22, 2017

jsafrane mentioned this pull request Aug 30, 2017

Make /var/lib/kubelet as shared during startup kubernetes/kubernetes#45724

Merged

saad-ali mentioned this pull request Sep 18, 2017

Mount namespace propagation kubernetes/enhancements#432

Closed

fabiand mentioned this pull request Oct 30, 2017

Proposal: File-system PV based backing store for virtual disks kubevirt/kubevirt#371

Closed

jsafrane mentioned this pull request Apr 12, 2018

Private mount propagation kubernetes/kubernetes#62462

Merged

danehans pushed a commit to danehans/community that referenced this pull request Jul 18, 2023

add zouyee as a member (kubernetes#659)

77222aa

Signed-off-by: Zou Nengren <zouyee1989@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redesign mount propagation #659

Redesign mount propagation #659

jsafrane commented May 24, 2017

k8s-ci-robot commented May 24, 2017

jsafrane commented May 24, 2017

yujuhong commented May 26, 2017

dchen1107 commented May 30, 2017

inc0 commented May 30, 2017

kfox1111 commented May 30, 2017

ddysher commented Jun 6, 2017

jsafrane commented Jun 6, 2017

ddysher commented Jun 6, 2017

jsafrane commented Jun 8, 2017

msau42 Jun 9, 2017

jsafrane Jun 13, 2017

kfox1111 commented Jul 14, 2017

kfox1111 commented Jul 18, 2017

luxas commented Jul 20, 2017

jsafrane commented Jul 20, 2017

saad-ali commented Jul 28, 2017

dixudx commented Jul 28, 2017

jsafrane commented Jul 28, 2017

jsafrane commented Aug 9, 2017

bgrant0607 commented Aug 9, 2017

kfox1111 commented Aug 9, 2017

kfox1111 commented Aug 9, 2017

bgrant0607 Aug 9, 2017

jsafrane Aug 10, 2017

bgrant0607 Aug 9, 2017

jsafrane Aug 10, 2017

bgrant0607 Aug 23, 2017

thockin commented Aug 11, 2017

jsafrane commented Aug 15, 2017

saad-ali commented Aug 15, 2017

saad-ali commented Aug 22, 2017

k8s-github-robot commented Aug 22, 2017

bgrant0607 commented Aug 23, 2017

Redesign mount propagation #659

Redesign mount propagation #659

Conversation

jsafrane commented May 24, 2017

k8s-ci-robot commented May 24, 2017

jsafrane commented May 24, 2017

yujuhong commented May 26, 2017

dchen1107 commented May 30, 2017

inc0 commented May 30, 2017

kfox1111 commented May 30, 2017

ddysher commented Jun 6, 2017

jsafrane commented Jun 6, 2017

ddysher commented Jun 6, 2017

jsafrane commented Jun 8, 2017

msau42 Jun 9, 2017

Choose a reason for hiding this comment

jsafrane Jun 13, 2017

Choose a reason for hiding this comment

kfox1111 commented Jul 14, 2017

kfox1111 commented Jul 18, 2017

luxas commented Jul 20, 2017

jsafrane commented Jul 20, 2017

saad-ali commented Jul 28, 2017

dixudx commented Jul 28, 2017

jsafrane commented Jul 28, 2017

jsafrane commented Aug 9, 2017

bgrant0607 commented Aug 9, 2017

kfox1111 commented Aug 9, 2017

kfox1111 commented Aug 9, 2017

bgrant0607 Aug 9, 2017

Choose a reason for hiding this comment

jsafrane Aug 10, 2017

Choose a reason for hiding this comment

bgrant0607 Aug 9, 2017

Choose a reason for hiding this comment

jsafrane Aug 10, 2017

Choose a reason for hiding this comment

bgrant0607 Aug 23, 2017

Choose a reason for hiding this comment

thockin commented Aug 11, 2017

jsafrane commented Aug 15, 2017

saad-ali commented Aug 15, 2017

saad-ali commented Aug 22, 2017

k8s-github-robot commented Aug 22, 2017

bgrant0607 commented Aug 23, 2017