Add support for copying up directories into tmpfs when a tmpfs is mounted over them #845

mrunalp · 2016-05-26T19:38:50Z

This replaces #707 by simply copying over a directory instead of using tar.

crosbymichael · 2016-05-26T20:52:24Z

libcontainer/specconv/spec_linux.go

+				extFlags |= f.flag
+			}
 		} else {
 			data = append(data, o)


Are we sure there are no mount data or fstypes that take a copyup option?

I found one here http://linux.die.net/man/1/funionfs.
WDYT about calling it runc_copyup?

is there something shorter? Maybe runccopyup rcopyup rccopyup tmpcopyup

I like tmpcopyup. I'll push updated code.

There is no implementation of -o delete and -o copyup for now.

😉

But I think we should have a nicer way of doing this than adding to a flag list that might eventually clash with the kernel. Maybe we should add an extension field to mounts? Or would that be overkill?

@cyphar I think we can revisit when we have more extensions 😉

mrunalp · 2016-05-26T22:40:57Z

Pushed update to parse it as tmpcopyup

crosbymichael · 2016-05-27T00:07:57Z

libcontainer/rootfs_linux.go

 			}
 		}
+		if copyUp {
+			tmpDir, err = ioutil.TempDir("/run", "runctmpdir")


Why are you using /run here? Shouldn't this be under the machines configured temp dir and not fillup space on /run?

I'll move it to /tmp

cyphar · 2016-05-27T11:53:14Z

@mrunalp

Why don't we do it like this:

Create a tmpfs somewhere (/tmp/runc-copyup.XXXXX).
Copy the contents of the directory into that tmpfs.
Bind-mount the tmpfs over the directory.
Lazy unmount and remove the initial temporary directory.

That way, we don't have to do any double-copying. In fact, we could just use mount(..., MS_MOVE) and completely skip step 4.

cyphar · 2016-05-27T12:11:46Z

@mrunalp Also (and I know this sounds like a trivial thing), but please put your fileutils project under a free software license (currently it is technically proprietary).

rhatdan · 2016-05-27T13:20:23Z

The tmpdir should probably look for environment variables Something like checking for the existence of
$XDG_RUNTIME_DIR, and then fall back to /tmp. I always worry about creating directories in /tmp since other users could potentially read/write the content in this directory. But since we want runc to be able to run without being UID=0, we don't know for sure if it can write to /run.

cyphar · 2016-05-27T13:30:32Z

@rhatdan

I always worry about creating directories in /tmp since other users could potentially read/write the content in this directory.

This would no longer be true if we decide to change the mode to something like 0700 (which I would actually recommend).

rhatdan · 2016-05-27T13:42:11Z

Yes, also if this is in a different mount namespace it could be hidden from other users.

mrunalp · 2016-05-27T15:20:58Z

@cyphar With your suggested approach, I suspect that tmpfs won't be charged to cgroup of the container if bind mounted (I'll take a look anyway). Though the double copy take more time, it doesn't change the semantics of how --tmpfs works today.

cyphar · 2016-05-27T15:26:07Z

@mrunalp We could use MS_MOVE instead if you want. But since the tmpfs was created after cgroup join, I'm fairly sure kmemcg will track it properly (if not, that's a kernel bug).

mrunalp · 2016-05-27T16:15:29Z

@cyphar Yeah, MS_MOVE should work. I'll try that.

mrunalp · 2016-05-27T18:11:59Z

I pushed a new commit (I'll clean up the PR after review) that uses MS_MOVE. Also, added code for using XDG_RUNTIME_DIR as prefix if it is set in the env. PTAL.

crosbymichael · 2016-05-27T18:30:04Z

@mrunalp do you need to rebase?

mrunalp · 2016-05-27T18:32:34Z

Rebased.

crosbymichael · 2016-05-27T22:15:30Z

libcontainer/rootfs_linux.go

-		if err := mountPropagate(m, rootfs, mountLabel); err != nil {
-			return err
+		if copyUp {
+			tmpDirPrefix := os.Getenv("XDG_RUNTIME_DIR")


I don't see how this would ever be set inside the container's init.

Yeah we can't leak it from outside with this code so was thinking whether setting in the config might be an acceptable compromise. Else we can special case leak it.

Sent from my iPhone

On May 27, 2016, at 3:15 PM, Michael Crosby [email protected] wrote:

In libcontainer/rootfs_linux.go:

stat, err := os.Stat(dest) if err != nil { if err := os.MkdirAll(dest, 0755); err != nil { return err } }

if err := mountPropagate(m, rootfs, mountLabel); err != nil {

return err

if copyUp {

tmpDirPrefix := os.Getenv("XDG_RUNTIME_DIR")
I don't see how this would ever be set inside the container's init.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Just use the container's /tmp dir or some hidden dir inside the rootfs to make this. Since its temporary and is only there to make the initial tmpfs destination it can be anywhere or even back in your container's /run.

Want to make this work with read only rootfs that we can't write to at all.

Sent from my iPhone

On May 27, 2016, at 3:36 PM, Michael Crosby [email protected] wrote:

In libcontainer/rootfs_linux.go:

stat, err := os.Stat(dest) if err != nil { if err := os.MkdirAll(dest, 0755); err != nil { return err } }

if err := mountPropagate(m, rootfs, mountLabel); err != nil {

return err

if copyUp {

tmpDirPrefix := os.Getenv("XDG_RUNTIME_DIR")
Just use the container's /tmp dir or some hidden dir inside the rootfs to make this. Since its temporary and is only there to make the initial tmpfs destination it can be anywhere or even back in your container's /run.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

We don't set it to readonly until after the mounts and after we pivot. Also you can just use /dev/ or /run to do this inside the container.

I meant read only even before we make it read only and I know we will need more changes to get there.

Could possibly use some other tmpfs mount if specified in the config before this mount.

Sent from my iPhone

On May 27, 2016, at 3:48 PM, Michael Crosby [email protected] wrote:

In libcontainer/rootfs_linux.go:

stat, err := os.Stat(dest) if err != nil { if err := os.MkdirAll(dest, 0755); err != nil { return err } }

if err := mountPropagate(m, rootfs, mountLabel); err != nil {

return err

if copyUp {

tmpDirPrefix := os.Getenv("XDG_RUNTIME_DIR")
We don't set it to readonly until after the mounts and after we pivot. Also you can just use /dev/ or /run to do this inside the container.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Probably safe enough to just use /tmp with 0700 and a random directory.

I removed XDG_RUNTIME_DIR lookup and switched to using /tmp which works well.

@mrunalp I don't quite understand where we are here? You wrote that you removed lookup, but it's still here.

mrunalp · 2016-06-09T17:18:30Z

@cyphar @crosbymichael PTAL

cyphar · 2016-06-10T08:56:01Z

@mrunalp There's a bug in fileutils, with the owner of directories (I've set /etc/ to be tmpcopyup and everything is owned by my user 1000:1000):

/ # ls /etc/ -la
total 72
drwxr-xr-x    6 root     root           480 Jun 10 08:44 .
drwxr-xr-x    1 1000     users          206 Jun 10 08:44 ..
-rw-r--r--    1 1000     users          466 Jun 10 08:44 fstab
-rw-r--r--    1 1000     users          298 Jun 10 08:44 group
-rw-r--r--    1 1000     users           10 Jun 10 08:44 hostname
-rw-r--r--    1 1000     users           40 Jun 10 08:44 hosts
drwxr-xr-x    2 root     root           140 Jun 10 08:44 init.d
-rw-r--r--    1 1000     users         1053 Jun 10 08:44 inittab
-rw-r--r--    1 1000     users         1180 Jun 10 08:44 inputrc
drwxr-xr-x    2 root     root           180 Jun 10 08:44 iproute2
-rw-r--r--    1 1000     users           21 Jun 10 08:44 issue
-rw-r--r--    1 1000     users            0 Jun 10 08:44 ld.so.conf
drwxr-xr-x    2 root     root            40 Jun 10 08:44 ld.so.conf.d
lrwxrwxrwx    1 1000     users           12 Jun 10 08:44 mtab -> /proc/mounts
drwxr-xr-x    8 root     root           180 Jun 10 08:44 network
-rw-r--r--    1 1000     users           95 Jun 10 08:44 os-release
-rw-r--r--    1 1000     users          371 Jun 10 08:44 passwd
-rw-r--r--    1 1000     users         1388 Jun 10 08:44 profile
-rw-r--r--    1 1000     users         2744 Jun 10 08:44 protocols
-rw-------    1 1000     users          512 Jun 10 08:44 random-seed
-rw-r--r--    1 1000     users          833 Jun 10 08:44 resolv.conf
-rw-r--r--    1 1000     users          386 Jun 10 08:44 securetty
-rw-r--r--    1 1000     users        10873 Jun 10 08:44 services
-rw-------    1 1000     users          239 Jun 10 08:44 shadow

The directories are owned by root because CopyDirectory doesn't Lchown directories. I'll open a PR against fileutils to fix this. We'll need to take the code from Docker that does MkdirAllAs.

I've opened this PR: mrunalp/fileutils#1.

Also, I would really like it if that repo had some reference to where the code came from as well as a maintainers file with @mrunalp as the sole maintainer.

cyphar · 2016-06-12T00:35:46Z

@mrunalp Since mrunalp/fileutils#1 was merged can you rebase with the vendor updated?

mrunalp · 2016-06-13T17:03:42Z

@cyphar Updated.

mrunalp · 2016-09-08T20:17:15Z

@opencontainers/runc-maintainers PTAL. All comments have been addressed.

rhatdan · 2016-09-08T20:23:16Z

I really want to start pushing the concept of readonly containers, and this patch is critical to this. Being able to mount tmpfs over certain directories allows us to preserve the layout of a container image and still run the container in read-only mode.

cyphar

I'm going to review this over the weekend. From a first look, this looks good.

cyphar · 2016-09-30T17:37:47Z

libcontainer/rootfs_linux.go

 		return nil
 	case "tmpfs":
+		copyUp := m.Extensions&configs.EXT_COPYUP == configs.EXT_COPYUP
+		tmpDir := ""


Well, you need it now. But you can use m.Destination in the second copyUp block. Doesn't really matter though it was just a thought.

cyphar · 2016-09-30T17:41:32Z

Dammit. GitHub doesn't let me retract the review with an "abstain". I'll LGTM this explicitly in the comments because clearly GitLab is still beating GitHub in the "reviewing code changes" space.

cyphar · 2016-10-01T12:38:55Z

@mrunalp This looks really nice. My main concern with this mode is the foot-gunning is quite easy. For example, if you put a mount for / with tmpcopyup after you've mounted /proc you get a lovely error like this:

container_linux.go:247: starting container process caused "process_linux.go:359: container init caused \"rootfs_linux.go:54: mounting \\\"tmpfs\\\" to rootfs \\\"/home/cyphar/src/runc/rootfs\\\" at \\\"/tmp/runctmpdir171994909\\\" caused \\\"failed to copy /home/cyphar/src/runc/rootfs to /tmp/runctmpdir171994909: read /home/cyphar/src/runc/rootfs/proc/1/attr/exec: invalid argument\\\"\""

Now, while it's true that you shouldn't be foot-gunning like this -- is there a nicer way we can handle these errors? If not, that's fine but it does make me a bit nervous wrt the bug reports we're going to get. I'm also currently testing user namespace interactions and will probably test with rootless containers.

cyphar · 2016-10-01T13:40:27Z

As expected, this doesn't play nicely with user namespace setups where there are unmapped UIDs in the thing you're copying over. Unfortunately, the only /nice/ way of handling this would be to do something exceptionally dodgy like: create all of the mounts until you hit a tmpcopyup then request the calling runC process to run a privileged process inside the mount namespace that does the copyup operation which is then MS_MOVEd. Since that is not going to pretty code by any stretch of the imagination, we should come up with some nice error message for things like this so that users don't spam us with "why doesn't this work". Here's an example error:

container_linux.go:247: starting container process caused "process_linux.go:359: container init caused \"rootfs_linux.go:54: mounting \\\"tmpfs\\\" to rootfs \\\"/home/cyphar/src/runc/rootfs\\\" at \\\"/tmp/runctmpdir618894030\\\" caused \\\"failed to copy /home/cyphar/src/runc/rootfs to /tmp/runctmpdir618894030: lchown /tmp/runctmpdir618894030/bin: invalid argument\\\"\""

This issue will be come quite pertinent with rootless containers.

cyphar

Adding an integration test would be a really good idea too, if the current framework (of using sed to modify config.json) is sufficient.

mrunalp · 2016-10-04T02:05:58Z

Added an integration test.

cyphar · 2016-10-04T06:56:02Z

+1 on the integration test. While I'm concerned about the fact that this would break in some user namespaced container setups, there's not much we can do without some pretty dodgy modify-the-mount-namespace-from-another-process-that-is-actually-root shennanigans. So I'm fine with the current limitations, but would like that the error message we get is slightly nicer -- maybe just use a genericError with a cause mentioning tmpcopyup?

If copyup is specified for a tmpfs mount, then the contents of the underlying directory are copied into the tmpfs mounted over it. Signed-off-by: Mrunal Patel <[email protected]>

Signed-off-by: Mrunal Patel <[email protected]>

mrunalp · 2016-10-04T18:36:45Z

@cyphar Updated error messages.

mrunalp · 2016-10-12T19:23:31Z

@opencontainers/runc-maintainers PTAL. All comments have been addressed.

cyphar · 2016-10-12T21:14:41Z

LGTM

LK4D4 · 2016-10-21T20:47:07Z

LGTM

rhatdan · 2016-10-21T21:01:51Z

WooHoo. Took a long time to finally get this functionality into Docker, but it is finally there.

bchallenor · 2018-02-14T10:32:52Z

Is this functionality accessible from Docker? @rhatdan had a Moby PR but it was not merged, and the comments lead here...

At the moment, docker --tmpfs seems to hide the underlying contents:

$ docker --version
Docker version 18.01.0-ce, build 03596f51b1

$ docker run --tmpfs /run debian:stable find /run
/run

$ docker run debian:stable find /run
/run
/run/lock
/run/mount
/run/mount/utab
/run/utmp

cyphar · 2018-02-14T10:35:03Z

@bchallenor I haven't tried this, but if you can specify tmpcopyup as one of the mount options with --tmpfs then it should trigger this functionality (as Docker just passes down the mount options to runc).

bchallenor · 2018-02-14T10:50:42Z

Any idea what the syntax would be for that? The only options that the Docker docs mention are tmpfs-size and tmpfs-mode.

rhatdan · 2018-02-14T10:58:35Z

I believe it should be the default. I am saddened to see that this behaviour does not work with k8s yet either. We are missing an opportunity to run containres as Read-Only mode. And this feature makes it easier. I believe the default for Kubernetes should be to run container in read-only mode. But this issue is containers tend to need to write to /run, /tmp, and /var/tmp. If we could setup container runtimes to mount tmpfs on these directories if the image is running in readonly mode by default, then most containers would run, except for the fact that sometimes the underlying directories contain content that is needed for the container. httpd for example usually ships with a /run/httpd directory and expects to write to this directory. If you just mount a tmpfs over /run and don't copy up, the httpd will fail, because it does not create the /run/httpd directory if it does not exist.

bchallenor · 2018-02-14T11:06:56Z

By "I believe it should be the default" do you mean that you think it should be the default, or that it already is? If the former, I agree, as I am also motivated by wanting a readonly rootfs with a few read/write holes punched in it.

You mentioned above "getting this functionality into Docker" - can it actually be used via the Docker UI, or is it confined to runc right now?

rhatdan · 2018-02-14T11:41:33Z

I am not sure if this ever got fully merged into upstream docker. Simple enough to check

docker run -ti --tmpfs /etc fedora ls /etc

cyphar · 2018-02-14T23:17:19Z

@bchallenor If you use the --mount flag you can specify the mount options explicitly. The docs you linked to explain how to use --mount instead of --tmpfs.

rhatdan · 2018-02-15T21:09:11Z

I believe it should be the default. I am saddened to see that this behaviour does not work with k8s yet either. We are missing an opportunity to run containres as Read-Only mode. And this feature makes it easier. I believe the default for Kubernetes should be to run container in read-only mode. But this issue is containers tend to need to write to /run, /tmp, and /var/tmp. If we could setup container runtimes to mount tmpfs on these directories if the image is running in readonly mode by default, then most containers would run, except for the fact that sometimes the underlying directories contain content that is needed for the container. httpd for example usually ships with a /run/httpd directory and expects to write to this directory. If you just mount a tmpfs over /run and don't copy up, the httpd will fail, because it does not create the /run/httpd directory if it does not exist.

josh1703658784 · 2022-04-03T23:43:42Z

So far I haven't been able to get this working. Does anyone currently have this working or know what options I need to give the mount declaration when using Docker?

== DETAILS ==

# docker info
Docker version 20.10.3, build b455053
Docker Compose version v2.3.3

# container / docker-compose info
  radarr:
    image: lscr.io/linuxserver/radarr:latest
    read_only: true
    cap_drop:
      - all
    cap_add:
      ...
    environment:
      - S6_READ_ONLY_ROOT=1
      ...
    volumes:
      ...
    tmpfs:
      ...
      - /app
      ...
    expose:
      - 7878

I tried each of the following options separately with no luck. I scraped these mount options from comments in the post and through review the code.

# docker-compose tmpfs modes
      tmpfs:
        - /app:tmpcopyup
        - /app:runctmpdir
        - /app:runccopyup 
        - /app:runc_copyup
        - /app:rcopyup 
        - /app:rccopyup

Regardless of which option I tried I receive the following error:

# error
./run: line 3: cd: /app/radarr/bin: No such file or directory

Docker documentation on volumes only includes mount options tmps-size and tmps-mode.

kolyshkin · 2022-04-04T17:36:21Z

@Joshuaks I suggest you ask Docker support about it.

GordonTheTurtle added the status/0-triage label May 26, 2016

mrunalp force-pushed the cp_tmpfs branch from 4df70ff to 00879dc Compare May 26, 2016 19:46

crosbymichael reviewed May 26, 2016
View reviewed changes

mrunalp force-pushed the cp_tmpfs branch from 00879dc to de88f60 Compare May 26, 2016 22:40

crosbymichael reviewed May 27, 2016
View reviewed changes

teohhanhui mentioned this pull request May 27, 2016

Tar up contents of --tmpfs dir ontop of tmpfs moby/moby#20501

Closed

mrunalp force-pushed the cp_tmpfs branch from 5df4813 to 4ceeae8 Compare May 27, 2016 18:32

crosbymichael reviewed May 27, 2016
View reviewed changes

mrunalp force-pushed the cp_tmpfs branch 2 times, most recently from 96507db to 7a8734d Compare May 31, 2016 16:25

cyphar mentioned this pull request Jun 10, 2016

fileutils: chown(2) directories when copying mrunalp/fileutils#1

Merged

mrunalp force-pushed the cp_tmpfs branch from 7a8734d to f0b779a Compare June 13, 2016 17:01

cyphar reviewed Sep 30, 2016

View reviewed changes

cyphar approved these changes Sep 30, 2016

View reviewed changes

cyphar requested changes Oct 1, 2016

View reviewed changes

mrunalp added 2 commits October 4, 2016 11:26

Support copyup mount extension for tmpfs mounts

c7406f7

If copyup is specified for a tmpfs mount, then the contents of the underlying directory are copied into the tmpfs mounted over it. Signed-off-by: Mrunal Patel <[email protected]>

Add an integration test for tmpfs copy up

c4e7f01

Signed-off-by: Mrunal Patel <[email protected]>

mrunalp force-pushed the cp_tmpfs branch from f2ed909 to c4e7f01 Compare October 4, 2016 18:27

cyphar approved these changes Oct 12, 2016 •

edited

Loading

View reviewed changes

rhatdan mentioned this pull request Oct 19, 2016

Share rootfs for read-only containers moby/moby#27364

Closed

LK4D4 merged commit 1ab9d5e into opencontainers:master Oct 21, 2016

wking mentioned this pull request Mar 26, 2018

Add read-only flag cri-o/cri-o#1434

Merged

Add support for copying up directories into tmpfs when a tmpfs is mounted over them #845

Add support for copying up directories into tmpfs when a tmpfs is mounted over them #845

Uh oh!

Conversation

mrunalp commented May 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crosbymichael May 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrunalp commented May 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyphar commented May 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyphar commented May 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhatdan commented May 27, 2016

Uh oh!

cyphar commented May 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhatdan commented May 27, 2016

Uh oh!

mrunalp commented May 27, 2016

Uh oh!

cyphar commented May 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrunalp commented May 27, 2016

Uh oh!

mrunalp commented May 27, 2016

Uh oh!

crosbymichael commented May 27, 2016

Uh oh!

mrunalp commented May 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrunalp commented Jun 9, 2016

Uh oh!

cyphar commented Jun 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyphar commented Jun 12, 2016

Uh oh!

mrunalp commented Jun 13, 2016

Uh oh!

mrunalp commented Sep 8, 2016

crosbymichael May 26, 2016 •

edited

Loading

cyphar commented May 27, 2016 •

edited

Loading

cyphar commented May 27, 2016 •

edited

Loading

cyphar commented May 27, 2016 •

edited

Loading

cyphar commented May 27, 2016 •

edited

Loading

cyphar commented Jun 10, 2016 •

edited

Loading

cyphar commented Oct 1, 2016 •

edited

Loading

cyphar commented Oct 1, 2016 •

edited

Loading

cyphar commented Oct 12, 2016 •

edited by caniszczyk

Loading

LK4D4 commented Oct 21, 2016 •

edited by caniszczyk

Loading