Implement Mount interface using mount(8) and umount(8) #6400

ddysher · 2015-04-03T01:23:25Z

Forked from #6269

We have mount_linux.go which implements mount interface for Linux using syscall. But NFS implements another version of linux mouter based on exec('mount'). To make code cleaner, we should only have one implementation per platform, not for different FS.

To make syscall.Mount works on NFS, we need to pass data param, which is this PR does. We can also implement linux mounter using mount command, once for all.

Open for discussion, if it's hard to make it right, then just scratch.

@thockin @calfonso

googlebot · 2015-04-03T01:23:28Z

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

googlebot · 2015-04-03T01:23:47Z

CLAs look good, thanks!

thockin · 2015-04-06T15:11:17Z

I am OK with this, but there's more agreement on https://github.com/GoogleCloudPlatform/kubernetes/pull/5506/files#r27799955 that maybe the right answer is just to implement it in terms of exec, and be done with it. If we do that, we probably want to change flags into []string opts or similar.

@eparis @rootfs @markturansky @vishh I can't think of any good reason NOT to convert this to exec...

markturansky · 2015-04-06T15:16:01Z

IIRC, @calfonso uncovered compilation issues with syscall.Mount in the windows build. If so, that bolsters the generic "exec" argument.

rootfs · 2015-04-06T15:22:12Z

add @eparis
converting into syscall has limitations, the mount helpers (nfs, and upcoming gluster, etc) called by exec handle lots of tiny things before trapping into syscall.

thockin · 2015-04-06T15:26:23Z

No matter what, exec(mount) or syscall.mount() is not going to work on
windows. syscall.mount() has the advantage of failing to compile, rather
than just failing to work. But in both cases we need an unsupported
variant of mounter.

On Mon, Apr 6, 2015 at 8:16 AM, Mark Turansky notifications@github.com
wrote:

IIRC, @calfonso https://github.com/calfonso uncovered compilation
issues with syscall.Mount in the windows build. If so, that bolsters the
generic "exec" argument.

—
Reply to this email directly or view it on GitHub
#6400 (comment)
.

vishh · 2015-04-06T15:32:09Z

I would prefer not using exec mainly to avoid any possible additional
resource overhead in the future, especially when there is a lot of churn on
the node. As long as syscall.Mount works for all our use cases and we have
abstracted all its complexities into a package (which we have already), I
don't think its an issue.

On Mon, Apr 6, 2015 at 8:26 AM, Tim Hockin notifications@github.com wrote:

No matter what, exec(mount) or syscall.mount() is not going to work on
windows. syscall.mount() has the advantage of failing to compile, rather
than just failing to work. But in both cases we need an unsupported
variant of mounter.

On Mon, Apr 6, 2015 at 8:16 AM, Mark Turansky notifications@github.com
wrote:

IIRC, @calfonso https://github.com/calfonso uncovered compilation
issues with syscall.Mount in the windows build. If so, that bolsters the
generic "exec" argument.

—
Reply to this email directly or view it on GitHub
<
#6400 (comment)

.

—
Reply to this email directly or view it on GitHub
#6400 (comment)
.

rootfs · 2015-04-06T15:41:50Z

problem with syscall is that it loses user space validations and transformations. Take nfs as an example. The mount helper does lots param validation and tries to find the right nfs protocol version before trapping into syscall. This process is not abstracted so far in syscall.Mount or `util/mount``.

thockin · 2015-04-06T15:42:49Z

The proposal is to encapsulte the exec() into the package, keeping a
similar API, but implementing it in terms of the mount tool(s) which do
have a lot of accumulated knowledge above and beyond the raw syscall..

mounting is not a fast-path operation usually, so I am not too concerned
about resources - not yet anyway.

On Mon, Apr 6, 2015 at 8:32 AM, Vish Kannan notifications@github.com
wrote:

I would prefer not using exec mainly to avoid any possible additional
resource overhead in the future, especially when there is a lot of churn on
the node. As long as syscall.Mount works for all our use cases and we have
abstracted all its complexities into a package (which we have already), I
don't think its an issue.

On Mon, Apr 6, 2015 at 8:26 AM, Tim Hockin notifications@github.com
wrote:

No matter what, exec(mount) or syscall.mount() is not going to work on
windows. syscall.mount() has the advantage of failing to compile, rather
than just failing to work. But in both cases we need an unsupported
variant of mounter.

On Mon, Apr 6, 2015 at 8:16 AM, Mark Turansky notifications@github.com
wrote:

IIRC, @calfonso https://github.com/calfonso uncovered compilation
issues with syscall.Mount in the windows build. If so, that bolsters
the
generic "exec" argument.

—
Reply to this email directly or view it on GitHub
<

#6400 (comment)

.

—
Reply to this email directly or view it on GitHub
<
#6400 (comment)

.

—
Reply to this email directly or view it on GitHub
#6400 (comment)
.

rootfs · 2015-04-06T15:45:54Z

exec() encapsulation is good to me.

eparis · 2015-04-06T15:46:16Z

@vishh But it doesn't even work for everything today. We're are only specifying "nfs" as the fstype. If you were running on a kernel without nfsv4 which expected the binary nfs mount options it would fail. I don't buy the argument that performance is an issue here, the mount(8) util is basically little more than a call to mount(2) if there is no helper. And if there is a helper we would need to either just call exec or redo everything that the mount helpers do. So we are talking about an exec() syscall and a couple of stat/access syscalls on what is hopefully not tooo critical of a path, even on a quickly changing system.

I'd much rather see mount(8) used in the util/mount and it take a []string as arguments...

ddysher · 2015-04-09T23:24:33Z

Thanks for the feedback, I think mount(8) makes sense. To make the interface easier to use, let's do this:

Mount(source string, target string, options []string) error

I'll revisit the PR.

thockin · 2015-04-10T00:16:40Z

Don't you need a 'type' arg there?

mount -t $type -o strings.Join($opts, ",") $source $path

On Thu, Apr 9, 2015 at 4:24 PM, Deyuan Deng notifications@github.com
wrote:

Thanks for the feedback, I think mount(8) makes sense. To make the
interface easier to use, let's do this:

Mount(source string, target string, options []string) error

I'll revisit the PR.

—
Reply to this email directly or view it on GitHub
#6400 (comment)
.

ddysher · 2015-04-10T00:59:13Z

The latest change includes fstype.

When I posted the commend, I didn't look closely at our code. Based on mount command, the fstype is optional, kernel is able to infer the fs type for you. Hence I ignored fstype to provide more flexibility. But after reading the code, it seems we expect fstype to be explictly specified (especially in MountPoint structure, where we define FSType, and in gcepd, where we check if filesystem type is ext4). So I changed my mind :)

I've changed the interface and its implementation. Still need to convert Glusterfs.

One big problem is --bind with read-only. IIRC, this will mount the fs as read/write despite of the read-only flag. A quick search gives me this solution

mount --bind /vital_data /untrusted_container/vital_data
mount -o remount,ro /untrusted_container/vital_data

Anyone knows if this is still the case now? I may need to double check with that.

vishh · 2015-04-10T02:38:00Z

AFAIK bind mounts require a remount to be made read-only.

On Thu, Apr 9, 2015 at 5:59 PM, Deyuan Deng notifications@github.com
wrote:

The latest change includes fstype.

When I posted the commend, I didn't look closely at our code. Based on
mount command, the fstype is optional, kernel is able to infer the fs
type for you. Hence I ignored fstype to provide more flexibility. But after
reading the code, it seems we expect fstype to be explictly specified
(especially in MountPoint structure, where we define FSType, and in gcepd,
where we check if filesystem type is ext4). So I changed my mind :)

I've changed the interface and its implementation. Still need to convert
Glusterfs.

One big problem is --bind with read-only. IIRC, this will mount the fs as
read/write despite of the read-only flag. A quick search gives me this
solution

mount --bind /vital_data /untrusted_container/vital_data
mount -o remount,ro /untrusted_container/vital_data

Anyone knows if this is still the case now? I may need to double check
with that.

—
Reply to this email directly or view it on GitHub
#6400 (comment)
.

thockin · 2015-04-10T04:22:17Z

I think you should detect if both bind and ro are set and if so, do the two
step mount and remount.

Let me know when this is ready for review.

On Thu, Apr 9, 2015 at 5:59 PM, Deyuan Deng notifications@github.com
wrote:

The latest change includes fstype.

When I posted the commend, I didn't look closely at our code. Based on
mount command, the fstype is optional, kernel is able to infer the fs
type for you. Hence I ignored fstype to provide more flexibility. But after
reading the code, it seems we expect fstype to be explictly specified
(especially in MountPoint structure, where we define FSType, and in gcepd,
where we check if filesystem type is ext4). So I changed my mind :)

I've changed the interface and its implementation. Still need to convert
Glusterfs.

One big problem is --bind with read-only. IIRC, this will mount the fs as
read/write despite of the read-only flag. A quick search gives me this
solution

mount --bind /vital_data /untrusted_container/vital_data
mount -o remount,ro /untrusted_container/vital_data

Anyone knows if this is still the case now? I may need to double check
with that.

—
Reply to this email directly or view it on GitHub
#6400 (comment)
.

ddysher · 2015-04-10T15:53:05Z

@thockin Ready to review. Running e2e now.

ddysher · 2015-04-14T00:33:11Z

e2e passed

thockin · 2015-04-26T18:16:57Z

pkg/util/mount/mount_linux.go

+	if len(fstype) > 0 {
+		mountArgs = append(mountArgs, "-t", fstype)
+	}
+	if options != nil && len(options) > 0 {


I think you just want to check for len(options) != 0, which includes nil, but also catches a non-nil but empty slice.

pmorie · 2015-04-28T21:45:19Z

Excited for this PR 👏

eparis · 2015-04-28T22:33:22Z

LGTM. Although in full bikeshedding mode, I would call it "args" for unmount. As they are not the same as "options" for mount....

pmorie · 2015-04-29T00:39:19Z

@ddysher Would you like me to do a local e2e w/ selinux enforcing?

ddysher · 2015-04-29T01:59:41Z

@pmorie Yup, go ahead. I think it's ready, I was running a normal e2e test. It's faster if you can run a selinux e2e, I'll have to look into how it works in k8s. Glad this pr is uesful to you. :)

@eparis umount 'args' is ignored for now, feel free to rename it when it's supported.

pmorie · 2015-04-29T02:44:17Z

@ddysher the emptydir tmpfs e2e test exercises selinux when it's enforcing
on the host the pod is running on. Once the security context work and
service accounts stuff go in we'll need a more robust set of tests for this.
On Tue, Apr 28, 2015 at 9:59 PM Deyuan Deng notifications@github.com
wrote:

@pmorie https://github.com/pmorie Yup, go ahead. I think it's ready, I
was running a normal e2e test. It's faster if you can run a selinux e2e,
I'll have to look into how it works in k8s. Glad this pr is uesful to you.
:)

@eparis https://github.com/eparis umount 'args' is ignored for now,
feel free to rename it when it's supported.

—
Reply to this email directly or view it on GitHub
#6400 (comment)
.

thockin · 2015-04-29T04:01:57Z

pkg/util/mount/mount_linux.go

+// Unmount unmounts target with given options.
+func (mounter *Mounter) Unmount(target string, options []string) error {
+	glog.V(5).Infof("Unmounting %s %v", target, options)
+	if len(options) != 0 {


Let's just nuke the argument, then. Less is more.

thockin · 2015-04-29T04:02:36Z

LGTM except remove unmount options arg

pmorie · 2015-04-29T15:17:35Z

Pulling this code down for a spot check on tmpfs and selinux

pmorie · 2015-04-29T15:22:50Z

I got a clean e2e run locally of emptyDir and secrets E2Es with selinux enforcing.

ddysher · 2015-04-29T15:23:51Z

@thockin updated

@pmorie thanks. I got a trouble running local e2e using vagrant (bring up machines, enable selinux, then test).. 'isup' tells me e2e cluster is up, but run test tells me it's not up. :(

$ go run hack/e2e.go --isup
2015/04/29 11:21:00 Running: get status
2015/04/29 11:21:00 Cluster is UP

$ go run hack/e2e.go --test
Cluster validation succeeded
Done, listing cluster services:

current-context: "vagrant"
Running: /Users/deyuan/code/source/go-workspace/src/github.com/GoogleCloudPlatform/kubernetes/hack/e2e-internal/../../cluster/../cluster/vagrant/../../cluster/vagrant/../../cluster/../cluster/../cluster/vagrant/../../cluster/vagrant/../../cluster/../_output/dockerized/bin/darwin/amd64/kubectl cluster-info
Kubernetes master is running at https://10.245.1.2
kube-dns is running at https://10.245.1.2/api/v1beta3/proxy/namespaces/default/services/kube-dns

Vagrant test setup complete
2015/04/29 11:19:34 Running: get status
Vagrant doesn't need special preparations for e2e tests
current-context: "vagrant"
Running: /Users/deyuan/code/source/go-workspace/src/github.com/GoogleCloudPlatform/kubernetes/cluster/../cluster/vagrant/../../cluster/vagrant/../../cluster/../_output/dockerized/bin/darwin/amd64/kubectl --match-server-version version
Error: server version (version.Info{Major:"0", Minor:"14+", GitVersion:"v0.14.0-1717-g9d41025c441a92", GitCommit:"9d41025c441a926b03dbcabe53269c9058f3a19d", GitTreeState:"clean"}) differs from client version (version.Info{Major:"0", Minor:"16+", GitVersion:"v0.16.0-7-g6897095e56a5b8", GitCommit:"6897095e56a5b856863dcf6aae2d5ddbc66a3564", GitTreeState:"clean"})!

2015/04/29 11:19:34 Error running get status: exit status 1
2015/04/29 11:19:34 Testing requested, but e2e cluster not up!
exit status 1

pmorie · 2015-04-29T15:30:08Z

@ddysher I have some issues with that too -- I just ran the E2Es against a local cluster (ie, running directly on my host).

ddysher · 2015-04-29T15:46:47Z

@pmorie Ah, didn't know a local host would work for e2e.

@thockin Ready for review. gce e2e passed, and selinux passed based on @pmorie's comment.

pmorie · 2015-04-29T15:48:16Z

@ddysher Not everything works perfectly against a local cluster for e2e -- I've been adding skips for things that shouldn't run, etc under local because I dream of a day when you can get a nice clean green e2e run against local.

ddysher · 2015-04-29T15:52:20Z

Yeah, local e2e would be quite useful. Thanks for verifying the patch.

thockin · 2015-04-29T16:44:29Z

I am out of office. I just had the one request (drop options on unmount).

@vishh can you final-review this?
On Apr 29, 2015 8:47 AM, "Deyuan Deng" notifications@github.com wrote:

@pmorie https://github.com/pmorie Ah, didn't know a local host would
work for e2e.

@thockin https://github.com/thockin Ready for review. gce e2e passed,
and selinux passed based on @pmorie https://github.com/pmorie's comment.

—
Reply to this email directly or view it on GitHub
#6400 (comment)
.

pmorie · 2015-04-30T17:48:11Z

@vishh Do you anticipate getting to this today? I would really like to get this in so that #6936 can go in.

vishh · 2015-04-30T21:00:26Z

LGTM.

Implement Mount interface using mount(8) and umount(8)

pmorie · 2015-04-30T21:12:56Z

@ddysher Thanks for doing this, it has made my life a lot easier!

ddysher · 2015-04-30T22:10:07Z

@pmorie You are welcome!

googlebot added the cla: no label Apr 3, 2015

ddysher force-pushed the nfs-volume-mounter branch from 70ed977 to e8fc2fd Compare April 3, 2015 01:23

googlebot added cla: yes and removed cla: no labels Apr 3, 2015

yujuhong assigned thockin Apr 3, 2015

ddysher force-pushed the nfs-volume-mounter branch from e8fc2fd to 5b89c69 Compare April 10, 2015 00:44

ddysher force-pushed the nfs-volume-mounter branch from 5b89c69 to 8e1f560 Compare April 10, 2015 15:41

ddysher changed the title ~~Remove nfs custom mounter~~ Implement Mount inferface using mount(8) and umount(8) Apr 10, 2015

ddysher force-pushed the nfs-volume-mounter branch from 8e1f560 to f6293d9 Compare April 10, 2015 15:51

ddysher changed the title ~~Implement Mount inferface using mount(8) and umount(8)~~ Implement Mount interface using mount(8) and umount(8) Apr 10, 2015

ddysher force-pushed the nfs-volume-mounter branch 2 times, most recently from 2c7b0cd to a5872ab Compare April 13, 2015 23:15

thockin reviewed Apr 26, 2015
View reviewed changes

ddysher force-pushed the nfs-volume-mounter branch from ff74878 to 2519fea Compare April 28, 2015 21:36

ddysher force-pushed the nfs-volume-mounter branch from 2519fea to 9d41025 Compare April 29, 2015 01:59

thockin reviewed Apr 29, 2015
View reviewed changes

ddysher force-pushed the nfs-volume-mounter branch from 9d41025 to b96eb4d Compare April 29, 2015 13:41

Change mount.Interface.Mount to exec('mount'), instead of syscall

6897095

ddysher force-pushed the nfs-volume-mounter branch from b96eb4d to 6897095 Compare April 29, 2015 14:46

vishh added a commit that referenced this pull request Apr 30, 2015

Merge pull request #6400 from ddysher/nfs-volume-mounter

1dead9c

Implement Mount interface using mount(8) and umount(8)

vishh merged commit 1dead9c into kubernetes:master Apr 30, 2015

ddysher deleted the nfs-volume-mounter branch March 13, 2016 13:37

Implement Mount interface using mount(8) and umount(8) #6400

Implement Mount interface using mount(8) and umount(8) #6400

Conversation

ddysher commented Apr 3, 2015

googlebot commented Apr 3, 2015

googlebot commented Apr 3, 2015

thockin commented Apr 6, 2015

markturansky commented Apr 6, 2015

rootfs commented Apr 6, 2015

thockin commented Apr 6, 2015

vishh commented Apr 6, 2015

rootfs commented Apr 6, 2015

thockin commented Apr 6, 2015

rootfs commented Apr 6, 2015

eparis commented Apr 6, 2015

ddysher commented Apr 9, 2015

thockin commented Apr 10, 2015

ddysher commented Apr 10, 2015

vishh commented Apr 10, 2015

thockin commented Apr 10, 2015

ddysher commented Apr 10, 2015

ddysher commented Apr 14, 2015

thockin Apr 26, 2015

Choose a reason for hiding this comment

ddysher Apr 28, 2015

Choose a reason for hiding this comment

pmorie commented Apr 28, 2015

eparis commented Apr 28, 2015

pmorie commented Apr 29, 2015

ddysher commented Apr 29, 2015

pmorie commented Apr 29, 2015

thockin Apr 29, 2015

Choose a reason for hiding this comment

thockin commented Apr 29, 2015

pmorie commented Apr 29, 2015

pmorie commented Apr 29, 2015

ddysher commented Apr 29, 2015

pmorie commented Apr 29, 2015

ddysher commented Apr 29, 2015

pmorie commented Apr 29, 2015

ddysher commented Apr 29, 2015

thockin commented Apr 29, 2015

pmorie commented Apr 30, 2015

vishh commented Apr 30, 2015

pmorie commented Apr 30, 2015

ddysher commented Apr 30, 2015