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

mrunalp · 2016-03-29T23:46:25Z

This helps in supporting read-only containers.
For e.g. if one specifies a tmpfs at /etc then the contents of /etc are copied up into the tmpfs and discarded on container exit.

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

crosbymichael · 2016-03-29T23:50:16Z

Is there a way to make this an option so that there is a way to disable it?

mrunalp · 2016-03-30T00:13:24Z

Yeah, I think we can make this a mount option or a runtime flag. I think mount option might be less intrusive.

crosbymichael · 2016-03-30T00:17:33Z

Either work for me

rhatdan · 2016-03-30T17:53:04Z

I would prefer it enabled by default.

/tmp:clean
/tmp:empty

mrunalp · 2016-03-30T21:46:21Z

Updated to introduce a copyup extension in mount options. PTAL.

cyphar · 2016-03-31T00:23:55Z

libcontainer/configs/mount.go

 	Relabel string `json:"relabel"`

+	// Extensions are additional flags that are specific to runc.
+	Extensions int `json:"extensions"`


While I love the use of bits for this, I feel like it's more Go-like to use a slice of integers (which is parsed from a slice of strings) or something like that (so it makes sense when you look at the JSON). Or we could use bits internally but make the JSON a slice of strings.

This is internal. The option is passed as copyup in mount options in the config json.

crosbymichael · 2016-03-31T00:28:45Z

I thought we wanted to get away from deps in docker but now we are adding almost 10k lines. We were so close!!!!!

cyphar · 2016-03-31T00:29:10Z

libcontainer/rootfs_linux.go

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


I'm not sure this should be an else, because the point of the if is to create the mountpoint? Or am I misunderstanding what you're doing here?

mrunalp · 2016-03-31T00:31:31Z

@crosbymichael Yeah, I was also surprised at what all had to be pulled in. I can look into alternative libraries for handling this. (The docker pkg/archive package seemed super easy to use :) )

cyphar · 2016-03-31T00:33:29Z

@crosbymichael Yeah, I'm quite disappointed about all of the dependencies on Docker. IMO we should work on separating out a lot of the pkg/s in Docker to separate repositories so we can import them without importing Docker. Also, I get the feeling that most of the stuff in this PR shouldn't need Docker-specific packages (but I might be mistaken).

@mrunalp It'd be great if there's nicer libraries for handling this. I really want to limit how much we pull in from Docker.

cyphar · 2016-03-31T00:38:09Z

@mrunalp Overall, I like this. But as I mentioned there's a few things I'd like to see fixed (and a lot of dependencies nixed). I'm not entirely sure about using tar archives to implement copy-up -- but aside from using overlay (bad) or writing our own filesystem-style-magic using FUSE (bad) there's not that much else we can do here.

So yeah, design LGTM.

mrunalp · 2016-03-31T00:40:37Z

@cyphar Yeah, will address your comments. I think using tar (or some equivalent) is the simplest option for what we are doing here for now. Thanks!

crosbymichael · 2016-03-31T00:45:07Z

libcontainer/rootfs_linux.go

+				if err != nil {
+					return fmt.Errorf("failed to tar archive: %v", err)
+				}
+				tmpFile, err := ioutil.TempFile("/run", "runctmpfile")


This is using /run on the host right?

Yes, it is.

Sent from my iPhone

On Mar 30, 2016, at 5:45 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 }

} else {

if copyUp {

tarStream, err = archive.Tar(dest, archive.Uncompressed)

if err != nil {

return fmt.Errorf("failed to tar archive: %v", err)

}

tmpFile, err := ioutil.TempFile("/run", "runctmpfile")
This is using /run on the host right?

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

Definitely use /run not /tmp, since /tmp could be attacked by non priv users.

mrunalp · 2016-03-31T19:05:35Z

@crosbymichael WDYT about a smaller version of docker pkg/archive to a different repo that doesn't have as many dependencies. I have that working with just a docker dependency on pkg/pools. I can clean this new repo up as docker packages are moved out into first class packages.

mrunalp · 2016-03-31T19:18:22Z

@crosbymichael Something like this https://github.com/mrunalp/archive

crosbymichael · 2016-03-31T19:58:41Z

@mrunalp we can get a docker/archive package created if you want to help moving the code but we should preserve the history by that whole subtree magic.

mrunalp · 2016-03-31T20:07:53Z

@crosbymichael Yeah, moving it out will be best but it has dependencies on 6 other packages.

mrunalp · 2016-03-31T20:09:00Z

I think it makes sense to move all of them out if you'll are okay with it :)

crosbymichael · 2016-03-31T20:15:27Z

We should open an issue for it

mrunalp · 2016-03-31T20:18:03Z

@crosbymichael I have opened moby/moby#21700. Thanks!

rhatdan · 2016-04-25T20:38:04Z

Any movement on this?

mrunalp · 2016-04-25T21:15:46Z

@crosbymichael Any thoughts? I don't think there was any conclusion on moby/moby#21700

rhatdan · 2016-04-26T13:06:02Z

mrunalp is this possible to do this just using the gotar? Or would you end up having to build archive all over again?

mrunalp · 2016-04-26T15:19:05Z

@rhatdan It ends up being most of archive. That's what I did here https://github.com/mrunalp/simpletar.

cyphar · 2016-04-26T15:55:08Z

@mrunalp I'd recommend using the path-related tools in Docker because you can get some pretty hairy filesystem breakouts. I'd just prefer we stuck with the Docker implementation, just because it's more tested and is very careful about the edgecases caused by scoping the root.

mrunalp · 2016-04-26T16:37:02Z

@cyphar I agree and that's what I started this PR with ;) The issue is we didn't want to pull in so many dependencies from the docker repo.

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

Also, defines a EXT_COPYUP flag for supporting tmpfs copyup operation Signed-off-by: Mrunal Patel <[email protected]>

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

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]>

crosbymichael · 2016-05-26T20:48:07Z

Closing because this was replaced by another PR

Anchor tags

I expect the lifecycle information was removed accidentally in be59415 (Split create and start, 2016-04-01, opencontainers#384), because for a time it seemed like that PR would also be removing hooks. Putting the lifecycle information back in, I made some tweaks to adjust to the new environment, for example: * Put the pre-start hooks after the 'start' call, but before the meat of the start call (the container-process exec trigger). Folks who want a post-create hook can add one with that name. I'd like to have renamed poststop to post-delete to avoid confusion like [1]. But the motivation for keeping hooks was backwards compatibility [2] so I've left the name alone. * Put each "...command is invoked..." lifecycle entry in its own list entry, to match the 'create' list entry. * Move the rules about what happens on hook failure into the lifecycle. This matches pre-split entries like: If any prestart hook fails, then the container MUST be stopped and the lifecycle continues at step 7. and avoids respecifying that information in a second location (config.md). * I added the warning section to try and follow post-split's generic "generates an error" approach while respecting the pre-split desire to see what failed (we had "then an error including the exit code and the stderr is returned to the caller" and "then an error is logged"). * I left the state 'id' context out, since Michael didn't want it [3]. * Make runtime.md references to "generate an error" and "log a warning" links, so readers have an easier time finding more detail on that wording. Where I reference a section, I'm still using the auto-generated anchor for that header and not the anchors which were added in 41839d7 (Merge pull request opencontainers#707 from mrunalp/anchor_tags, 2017-03-03) and similar. Mrunal suggested that the manually-added anchors were mainly intended for the validation tooling [4]. [1]: opencontainers/runtime-spec#395 Subject: Run post-stop hooks before the container sandbox is deleted. [2]: opencontainers/runtime-spec#483 (comment) Subject: *: Remove hooks [3]: opencontainers/runtime-spec#532 (comment) Subject: Restore hook language removed by create/start split [4]: http://ircbot.wl.linuxfoundation.org/eavesdrop/%23opencontainers/%23opencontainers.2017-03-03.log.html#t2017-03-03T18:02:12 Signed-off-by: W. Trevor King <[email protected]>

GordonTheTurtle added the status/0-triage label Mar 29, 2016

mrunalp force-pushed the tar_tmpfs branch 2 times, most recently from e115580 to 1e50aa0 Compare March 30, 2016 21:41

cyphar reviewed Mar 31, 2016
View reviewed changes

crosbymichael reviewed Mar 31, 2016
View reviewed changes

mrunalp force-pushed the tar_tmpfs branch from 1e50aa0 to 64e79ac Compare March 31, 2016 20:02

mrunalp mentioned this pull request Mar 31, 2016

Move pkg/archive and dependencies out into their own repositories moby/moby#21700

Closed

teohhanhui mentioned this pull request Apr 25, 2016

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

Closed

mrunalp added 4 commits April 26, 2016 15:23

Update godep dependencies for supporting copyup

c7b4b8a

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

config: Add new Extensions flag to support custom mount options in runc

c4a1fb7

Also, defines a EXT_COPYUP flag for supporting tmpfs copyup operation Signed-off-by: Mrunal Patel <[email protected]>

Parse the new extension flags

f471e0b

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

Support copyup extension for tmpfs

3216e59

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]>

mrunalp force-pushed the tar_tmpfs branch from 64e79ac to 3216e59 Compare April 26, 2016 22:28

mrunalp mentioned this pull request May 26, 2016

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

Merged

crosbymichael closed this May 26, 2016

stefanberger pushed a commit to stefanberger/runc that referenced this pull request Sep 8, 2017

Merge pull request opencontainers#707 from mrunalp/anchor_tags

41839d7

Anchor tags

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

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

Uh oh!

Conversation

mrunalp commented Mar 29, 2016

Uh oh!

crosbymichael commented Mar 29, 2016

Uh oh!

mrunalp commented Mar 30, 2016

Uh oh!

crosbymichael commented Mar 30, 2016

Uh oh!

rhatdan commented Mar 30, 2016

Uh oh!

mrunalp commented Mar 30, 2016

Uh oh!

cyphar Mar 31, 2016

Choose a reason for hiding this comment

Uh oh!

mrunalp Mar 31, 2016

Choose a reason for hiding this comment

Uh oh!

crosbymichael commented Mar 31, 2016

Uh oh!

cyphar Mar 31, 2016

Choose a reason for hiding this comment

Uh oh!

mrunalp commented Mar 31, 2016

Uh oh!

cyphar commented Mar 31, 2016

Uh oh!

cyphar commented Mar 31, 2016

Uh oh!

mrunalp commented Mar 31, 2016

Uh oh!

crosbymichael Mar 31, 2016

Choose a reason for hiding this comment

Uh oh!

mrunalp Mar 31, 2016

Choose a reason for hiding this comment

Uh oh!

rhatdan Mar 31, 2016

Choose a reason for hiding this comment

Uh oh!

mrunalp commented Mar 31, 2016

Uh oh!

mrunalp commented Mar 31, 2016

Uh oh!

crosbymichael commented Mar 31, 2016

Uh oh!

mrunalp commented Mar 31, 2016

Uh oh!

mrunalp commented Mar 31, 2016

Uh oh!

crosbymichael commented Mar 31, 2016

Uh oh!

mrunalp commented Mar 31, 2016

Uh oh!

rhatdan commented Apr 25, 2016

Uh oh!

mrunalp commented Apr 25, 2016

Uh oh!

rhatdan commented Apr 26, 2016

Uh oh!

mrunalp commented Apr 26, 2016

Uh oh!

cyphar commented Apr 26, 2016

Uh oh!

mrunalp commented Apr 26, 2016

Uh oh!

crosbymichael commented May 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants