Containerized mount utilities #53440

jsafrane · 2017-10-04T16:14:30Z

This is implementation of kubernetes/community#589

@tallclair @vishh @dchen1107 PTAL
@kubernetes/sig-node-pr-reviews

Release note:

Kubelet supports running mount utilities and final mount in a container instead running them on the host.

ianchakeres · 2017-10-05T01:23:44Z

/sig storage

cblecker · 2017-10-05T18:59:23Z

Needs a hack/update-bazel.sh:

I1004 16:23:28.818] Verifying hack/make-rules/../../hack/verify-bazel.sh
I1004 16:23:49.573] --- /go/src/k8s.io/kubernetes/pkg/kubelet/mountpod/BUILD	2017-10-04 16:17:57.518523051 +0000
I1004 16:23:49.574] +++ /tmp/BUILD245520658	2017-10-04 16:23:38.822525349 +0000
I1004 16:23:49.574] @@ -6,7 +6,6 @@
I1004 16:23:49.574]      visibility = ["//visibility:public"],
I1004 16:23:49.574]      deps = [
I1004 16:23:49.574]          "//pkg/kubelet/pod:go_default_library",
I1004 16:23:49.574] -        "//vendor/github.com/golang/glog:go_default_library",
I1004 16:23:49.574]          "//vendor/k8s.io/api/core/v1:go_default_library",
I1004 16:23:49.574]      ],
I1004 16:23:49.575]  )
I1004 16:23:49.575] 
I1004 16:23:49.575] 
I1004 16:23:49.575] Run ./hack/update-bazel.sh
I1004 16:23:49.615] FAILED   hack/make-rules/../../hack/verify-bazel.sh	21s

tallclair

No big issues.

tallclair · 2017-10-13T20:02:04Z

pkg/kubelet/mountpod/mount_pod.go

+	PodName       string
+	PodNamespace  string
+	PodUID        string
+	ContainerName string


nit: in the proposal, field names are lowercase in json. I think you need json tags to match.

Added json tags

tallclair · 2017-10-13T20:04:53Z

pkg/kubelet/mountpod/mount_pod.go

+	return path.Join(m.registrationDirectory, safePluginName)
+}
+
+func (m *basicManager) GetMountPod(pluginName string) (*v1.Pod, string, error) {


Name return values (i.e. what is the string?)

tallclair · 2017-10-13T20:06:45Z

pkg/kubelet/mountpod/mount_pod.go

+	if !ok {
+		return nil, "", fmt.Errorf("unable to process %s: pod %s/%s not found", regPath, reg.PodNamespace, reg.PodName)
+	}
+	if string(pod.UID) != reg.PodUID {


I think this will be OK, but double check that this works with static/mirror pods.

Static pod gets a different UID via dawnward API than it's in the API server :-(

Can we require that pods with mount utilities are not static?

Scratch the previous comment. Pod indeed gets different UID via downward API than it's in the API server, however kubelet sees the right ("downward") UID in this check and everything works.

tallclair · 2017-10-13T20:26:23Z

pkg/util/mount/exec_mount.go

+	output, err := m.exec.Run("mount", mountArgs...)
+	glog.V(5).Infof("Exec mounted %v: %v: %s", mountArgs, err, string(output))
+	if err != nil {
+		glog.Errorf("Mount failed: %v\nMounting command: %s\nMounting arguments: %s %s %s %v\nOutput: %s\n", err, "mount", source, target, fstype, options, string(output))


nit: errors shouldn't be logged if they're also propagated

log removed

tallclair · 2017-10-13T20:27:28Z

pkg/util/mount/exec_mount.go

+	outputBytes, err := m.exec.Run("umount", target)
+	glog.V(5).Infof("Exec unmounted %v: %s", err, string(outputBytes))
+	if len(outputBytes) != 0 {
+		glog.V(5).Infof("Output of unmounting %s: %v", target, string(outputBytes))


combine this with the above log message

reworked a bit to have higher log level on error

jsafrane · 2017-10-19T12:59:06Z

Polished the PR a bit, namely introduced DefaultKubeletPluginContainersDirName in pkg/kubelet/config/defaults.go to keep directory names together.

jsafrane · 2017-10-27T07:35:26Z

/assign @tallclair
is there anything missing?

@rootfs

Automatic merge from submit-queue (batch tested with PRs 54635, 54250, 54657, 54696, 54700). 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>. Don't cache exec and mounter in RBD volume plugin #51608 has broken containerized RBD mount utilities proposed in #53440. Volume plugin can get a different exec and mounter implementation with every call, it must not be cached. ```release-note NONE ``` /sig storage /assign @rootfs

jsafrane · 2017-11-02T16:20:05Z

Rebased
@tallclair, PTAL.

tallclair · 2017-11-09T18:27:01Z

Looks good, just a couple nits.

tallclair · 2017-11-09T18:19:53Z

pkg/kubelet/mountpod/mount_pod.go

+	for i := range pod.Spec.Containers {
+		if pod.Spec.Containers[i].Name == reg.ContainerName {
+			found = true
+			break


nit: just return here, and get rid of found

tallclair · 2017-11-09T18:20:59Z

pkg/kubelet/volume_host.go

-	return mount.NewOsExec()
+	exec, err := kvh.getMountExec(pluginName)
+	if err != nil {
+		glog.V(2).Info(err.Error())


nit: give this a little context

Added context:
Error finding mount pod for plugin %s: %s", pluginName, err.Error()

tallclair · 2017-11-09T18:25:20Z

pkg/kubelet/volume_host.go

-	return kvh.kubelet.mounter
+	exec, err := kvh.getMountExec(pluginName)
+	if err != nil {
+		glog.V(2).Info(err.Error())


nit: error context

Added context:
Error finding mount pod for plugin %s: %s", pluginName, err.Error()

jsafrane · 2017-11-10T12:27:24Z

rebased + fixed review remarks
@tallclair, PTAL

tallclair · 2017-11-10T22:44:56Z

/lgtm

jsafrane · 2017-11-13T15:23:54Z

/assign @thockin
for approval, thanks in advance!

thockin · 2017-11-13T20:41:59Z

/approve no-issue

k8s-github-robot · 2017-11-13T20:42:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, tallclair, thockin

Associated issue requirement bypassed by: thockin

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~~ [thockin]

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

k8s-github-robot · 2017-11-14T00:45:31Z

Automatic merge from submit-queue (batch tested with PRs 54005, 55127, 53850, 55486, 53440). If you want to cherry-pick this change to another branch, please follow the instructions here.

gnufied · 2017-11-15T02:08:31Z

pkg/util/mount/exec_mount.go

+	exec           Exec
+}
+
+func NewExecMounter(exec Exec, wrapped Interface) Interface {


Is it intentional that this does not touches SafeFormat portions of work?

SafeFormat is itself a wrapper over mount.Interface. It will work if it wraps ExecMounter instead of the generic Linux Mounter

kfox1111 · 2017-11-16T00:51:05Z

Question... how was flex volume support done? Is it broken up into one container per plugin, or one for all flexvolumes?

jsafrane · 2017-11-16T09:13:04Z

Flex is out of scope of this PR, at least in alpha. Flex has their own method of dynamic registration of drivers that could be used by flex driver running in a container. See https://github.com/kubernetes/community/blob/master/contributors/design-proposals/storage/flexvolume-deployment.md

kfox1111 · 2017-11-16T20:19:48Z

ok, thanks. I'll see if I can get that to work.

@jeffvance

Automatic merge from submit-queue. 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>. Fix test images These commits fix volume_io tests for iSCSI and Ceph RBD. Both server images were quite old (Fedora 22), so I updated them to ~~something more stable (CentOS 7) and to newer Ceph (Jewel, 10.2.7).~~ something newer (Fedora 26). The most important fix is that the test volumes have 120 MB so volume_io test can actually run - the tests put 100MB file to the volume to check its persistence. When mount containers in #53440 are merged I'll try to run the tests regularly with every PR (or merge) so we catch regressions quickly. ```release-note NONE ``` /sig testing /sig storage /assign @jeffvance Fixes: #56725

k8s-github-robot assigned mtaufen and pmorie Oct 4, 2017

jsafrane mentioned this pull request Oct 4, 2017

Proposal: containerized mount utilities in pods kubernetes/community#589

Merged

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Oct 5, 2017

mtaufen assigned saad-ali and jingxu97 and unassigned mtaufen Oct 9, 2017

tallclair reviewed Oct 13, 2017

View reviewed changes

jsafrane force-pushed the mount-container4-10-03 branch 2 times, most recently from 2d93066 to ff9dd61 Compare October 19, 2017 12:57

jsafrane force-pushed the mount-container4-10-03 branch from ff9dd61 to af13753 Compare October 19, 2017 13:01

jsafrane mentioned this pull request Oct 20, 2017

Fix test images #54300

Merged

k8s-ci-robot assigned tallclair Oct 27, 2017

jsafrane mentioned this pull request Oct 27, 2017

Don't cache exec and mounter in RBD volume plugin #54696

Merged

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 29, 2017

jsafrane force-pushed the mount-container4-10-03 branch from af13753 to 7981dd2 Compare November 2, 2017 16:19

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2017

tallclair reviewed Nov 9, 2017

View reviewed changes

Add alpha feature for mount containers

f177c1f

jsafrane added 2 commits November 10, 2017 13:14

Add ExecMounter

1dd32ce

Add mount containers to kubelet

1ddc6eb

jsafrane force-pushed the mount-container4-10-03 branch from 7981dd2 to 1ddc6eb Compare November 10, 2017 12:26

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

k8s-ci-robot assigned thockin Nov 13, 2017

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

k8s-github-robot merged commit 2d64ce5 into kubernetes:master Nov 14, 2017

This was referenced Nov 14, 2017

Use results of kube-controller-manager leader election in addon manager #55466

Merged

Revert "Containerized mount utilities" #55694

Closed

saad-ali mentioned this pull request Nov 14, 2017

pull-kubernetes-cross fails with undefined: "k8s.io/kubernetes/pkg/util/mount".NewExecMounter #55711

Closed

gnufied reviewed Nov 15, 2017

View reviewed changes

saad-ali mentioned this pull request Nov 28, 2017

Containerized mounts kubernetes/enhancements#278

Closed

Containerized mount utilities #53440

Containerized mount utilities #53440

Conversation

jsafrane commented Oct 4, 2017

ianchakeres commented Oct 5, 2017

cblecker commented Oct 5, 2017

tallclair 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

jsafrane commented Oct 19, 2017

jsafrane commented Oct 27, 2017

jsafrane commented Nov 2, 2017

tallclair commented Nov 9, 2017

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 Nov 10, 2017

tallclair commented Nov 10, 2017

jsafrane commented Nov 13, 2017

thockin commented Nov 13, 2017

k8s-github-robot commented Nov 13, 2017

k8s-github-robot commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfox1111 commented Nov 16, 2017

jsafrane commented Nov 16, 2017

kfox1111 commented Nov 16, 2017