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

jsafrane · 2017-05-12T11:34:26Z

This is part of ~~kubernetes/community#589~~ kubernetes/community#659

We'd like kubelet to be able to consume mounts from containers in the future, therefore kubelet should make sure that /var/lib/kubelet has shared mount propagation to be able to see these mounts.

On most distros, root directory is already mounted with shared mount propagation and this code will not do anything. On older distros such as Debian Wheezy, this code detects that /var/lib/kubelet is a directory on / which has private mount propagation and kubelet bind-mounts /var/lib/kubelet as rshared.

Both "regular" linux mounter and NsenterMounter are updated here.

@kubernetes/sig-storage-pr-reviews @kubernetes/sig-node-pr-reviews
@vishh

Release note:

Kubelet re-binds /var/lib/kubelet directory with rshared mount propagation during startup if it is not shared yet.

jsafrane · 2017-05-12T11:37:56Z

cmd/kubelet/app/server.go

@@ -861,6 +861,12 @@ func RunKubelet(kubeFlags *options.KubeletFlags, kubeCfg *componentconfig.Kubele
 		}
 	}

+	// set up /var/lib/kubelet sharing
+	err = kubeDeps.Mounter.MakeShared(kubeCfg.RootDirectory)


I hope RunKubelet is the right place to do this startup initialization. Feel free to suggest a better one.

Answering to myself, setupDataDirs looks like a better place.

jsafrane · 2017-05-12T14:04:30Z

tested with KUBE_NODE_OS_DISTRIBUTION=gci go run hack/e2e.go -v --up and KUBE_NODE_OS_DISTRIBUTION=debian. The patch does not do anything on GCI and it bind-mounts /var/lib/kubelet on Debian and makes it rshared.

Surprisingly, something already bind-mounts /var/lib/kubelet as shared on GCI even though it's not necessary, / is already shared.

jsafrane · 2017-05-20T12:49:30Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

vishh

Just a few nits.

vishh · 2017-05-20T12:58:43Z

pkg/util/mount/mount.go

@@ -55,6 +55,9 @@ type Interface interface {
 	// GetDeviceNameFromMount finds the device name by checking the mount path
 	// to get the global mount path which matches its plugin directory
 	GetDeviceNameFromMount(mountPath, pluginDir string) (string, error)
+	// MakeShared checks that given path is on a mount with 'shared' mount
+	// propagation. If not, it bind-mounts the path as shared.
+	MakeShared(path string) error


nit: Call it MakeRShared to reflect the fact that it shares recursively?

vishh · 2017-05-20T13:00:26Z

pkg/util/mount/mount_linux.go

+	if err != nil {
+		return []mountInfo{}, err
+	}
+	for {


I wish we could make this a generic utility for reading many kernel APIs that return data more than a page in size.

Done (with some refactoring of listProcMounts to use the same function).

vishh · 2017-05-20T13:01:14Z

pkg/util/mount/mount_linux.go

+	}
+
+	glog.V(2).Infof("Bind-mounting %q with shared mount propagation", path)
+	// mount --bind /var/lib/kubelet /var/lib/kubelet


Why not use the syscall package? Why exec?

huh? syscall.Exec will replace /usr/bin/kulebet in current process with /bin/mount and it won't ever return. We need fork()+exec() and that's exactly what os.Exec does.

https://golang.org/pkg/syscall/#Mount

I see. Reworked to use syscall.Mount.

vishh · 2017-05-20T13:01:31Z

pkg/util/mount/mount_linux.go

+// doMakeShared is common implementation of MakeShared on Linux. It checks if
+// path is shared and bind-mounts it as shared if needed. mountCmd and mountArgs
+// are expected to contain mount-like command, doMakeShared will add '--bind
+// <path> <path>' and '--make-shared <path>' to mountArgs.


nit: make-rshared

vishh · 2017-05-20T13:03:08Z

pkg/util/mount/mount_linux_test.go

+func TestIsSharedSuccess(t *testing.T) {
+	successMountInfo :=
+		`62 0 253:0 / / rw,relatime shared:1 - ext4 /dev/mapper/ssd-root rw,seclabel,data=ordered
+76 62 8:1 / /boot rw,relatime shared:29 - ext4 /dev/sda1 rw,seclabel,data=ordered


Can you add a test case with /var/lib/kubelet explicitly marked as shared?

added a test with /var/lib/foo that is a mount point and is shared.

jsafrane · 2017-05-22T16:25:51Z

@vishh, thanks for the review, I'll update the PR tomorrow.

I played with this PR and with #41683 to make kubelet mount stuff from containers and it looks like /var/lib/kubelet is not enough and https://github.com/kubernetes/kubernetes/pulls/jsafrane is still needed on machines without systemd. See kubernetes/community#648 for details.

ncdc · 2017-05-22T17:51:31Z

/unassign
/assign @derekwaynecarr @sjenning

sjenning · 2017-05-22T19:10:26Z

Doesn't seem like this would work for nsenter mounter i.e. containerized kubelet. The container itself will have already bind mounted /var/lib/kubelet as private. nsentering the host and changing the mount propagation after container start isn't going to change the fact that the containerized kubelet has a private /var/lib/kubelet right?

sjenning · 2017-05-22T19:23:17Z

pkg/util/mount/mount_linux.go

+}
+
+type mountInfo struct {
+	root, mountPoint string


not seeing where root is ever used

sjenning · 2017-05-22T19:34:14Z

I'm also not seeing the code for "if / is already rshared, do nothing". Really the rule should be:

for dir in /var/lib/kubelet /var/lib /var /; do
  if dir exists in mountpoints
    if dir is private; then
      remount /var/lib/kubelet as rshared
    fi
    return
  fi
done

If you get to the end without a remount, it means the /var/lib/kubelet is under / and is reshared already. You only need to remount if /var/lib/kubelet or one of its parents directories is 1) is a mountpoint and 2) is private. I think 😄

jsafrane · 2017-08-31T14:01:04Z

/retest
#51692

vishh · 2017-08-31T14:49:53Z

pkg/util/mount/mount_linux.go

+	}
+
+	// mount --make-rshared /var/lib/kubelet
+	if err := syscall.Mount(path, path, "" /*fstype*/, syscall.MS_SHARED|syscall.MS_REC, "" /*data*/); err != nil {


Is a remount flag (MS_REMOUNT) required here?

No, remount is not required. mount --make-rshared uses only the fags above as implied by the man page and checked with strace.

vishh · 2017-08-31T14:51:47Z

I'd like to see a node e2e test that verifies this feature. This may prevent regressions. e2es can land even after code freeze I think.

jsafrane · 2017-08-31T19:11:56Z

I have e2e test for whole propagation in my queue, waiting for #46444 to merge. And if it runs on wheezy we can be quite sure that this PR works as expected.

vishh · 2017-08-31T22:29:11Z

/lgtm
/approve

jsafrane · 2017-09-01T08:27:36Z

/assign @smarterclayton @thockin
for approval. PTAL.

jsafrane · 2017-09-01T19:51:37Z

/retest

smarterclayton · 2017-09-01T20:01:59Z

/approve

jsafrane · 2017-09-01T20:34:38Z

/approve no-issue
(just checking... :-)

smarterclayton · 2017-09-01T20:40:09Z

/approve no-issue

k8s-github-robot · 2017-09-01T20:40:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, sjenning, smarterclayton, vishh

Associated issue requirement bypassed by: smarterclayton

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/OWNERS~~ [smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

fejta-bot · 2017-09-02T07:15:51Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

fejta-bot · 2017-09-02T17:24:51Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

k8s-github-robot · 2017-09-02T18:12:48Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-09-02T19:00:30Z

Automatic merge from submit-queue

@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

k8s-ci-robot · 2017-09-02T19:31:22Z

@jsafrane: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	`d950010`	link	`/test pull-kubernetes-e2e-kops-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

…lement mount.Interface again. This was broken in #45724

@jsafrane

Automatic merge from submit-queue Make *fakeMountInterface in container_manager_unsupported_test.go implement mount.Interface again. This was broken in #45724 **Release note**: ```release-note NONE ``` /sig storage /sig node /cc @jsafrane, @vishh

k8s-ci-robot 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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 12, 2017

k8s-github-robot assigned yifan-gu and ncdc May 12, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 12, 2017

jsafrane force-pushed the mount-propagation2 branch from 40bcd14 to 19b6d67 Compare May 12, 2017 11:36

jsafrane commented May 12, 2017

View reviewed changes

jsafrane force-pushed the mount-propagation2 branch 5 times, most recently from defadd4 to 27389e0 Compare May 12, 2017 14:01

jsafrane force-pushed the mount-propagation2 branch from 27389e0 to cc20410 Compare May 12, 2017 14:36

yifan-gu assigned vishh and yujuhong and unassigned yifan-gu May 17, 2017

jsafrane force-pushed the mount-propagation2 branch from cc20410 to bf2a9d0 Compare May 20, 2017 08:05

vishh suggested changes May 20, 2017

View reviewed changes

k8s-ci-robot assigned derekwaynecarr and sjenning and unassigned ncdc May 22, 2017

sjenning reviewed May 22, 2017

View reviewed changes

vishh reviewed Aug 31, 2017

View reviewed changes

vishh approved these changes Aug 31, 2017

View reviewed changes

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

k8s-ci-robot assigned smarterclayton and thockin Sep 1, 2017

jsafrane mentioned this pull request Sep 1, 2017

Configure mount propagation on systems without systemd #44389

Closed

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2017

abgworrall modified the milestone: v1.8 Sep 2, 2017

k8s-github-robot merged commit 917f9f0 into kubernetes:master Sep 2, 2017

rohitagarwal003 mentioned this pull request Sep 5, 2017

Make *fakeMountInterface in container_manager_unsupported_test.go implement mount.Interface again. #51928

Merged

k8s-github-robot pushed a commit that referenced this pull request Sep 6, 2017

Make *fakeMountInterface in container_manager_unsupported_test.go imp…

08ea02b

…lement mount.Interface again. This was broken in #45724

jsafrane mentioned this pull request Nov 14, 2017

Add e2e test for mount propagation #51905

Merged

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

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

Conversation

jsafrane commented May 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsafrane commented May 12, 2017

jsafrane commented May 20, 2017

vishh 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

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

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

jsafrane commented May 22, 2017

ncdc commented May 22, 2017

sjenning commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjenning commented May 22, 2017

jsafrane commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishh commented Aug 31, 2017

jsafrane commented Aug 31, 2017

vishh commented Aug 31, 2017

jsafrane commented Sep 1, 2017

jsafrane commented Sep 1, 2017

smarterclayton commented Sep 1, 2017

jsafrane commented Sep 1, 2017

smarterclayton commented Sep 1, 2017

k8s-github-robot commented Sep 1, 2017

fejta-bot commented Sep 2, 2017

fejta-bot commented Sep 2, 2017

k8s-github-robot commented Sep 2, 2017

k8s-github-robot commented Sep 2, 2017

k8s-ci-robot commented Sep 2, 2017

jsafrane commented May 12, 2017 •

edited

Loading