Change layout of mountpoints and mounts #136

LK4D4 · 2015-09-02T15:47:01Z

Added info about MountPoints to config.md.

Filled after discuss in opencontainers/runc#242
I noticed that MountPoints still has json tag mounts, should I rename it to mountPoints?

LK4D4 · 2015-09-02T15:52:26Z

ping @philips @mrunalp @crosbymichael

wking · 2015-09-02T16:24:51Z

runtime-config.md

Link with “… of [mount points from the portable config](config.md#mount-points)”?

The link is now on line 7 (“Only [mounts from …]”), so I'm fine leaving this line as it stands.

wking · 2015-09-02T16:34:24Z

On Wed, Sep 02, 2015 at 08:47:03AM -0700, Alexander Morozov wrote:

I noticed that MountPoints still has json tag mounts, should I
rename it to mountPoints?

I don't think so. We need different Go names because we're putting
all these types in one package, but the JSON is clearly namespaced by
the file it's in (config.json vs. runtime.json), so there's no need to
disambiguate there.

LK4D4 · 2015-09-02T16:44:59Z

Actually we don't need different go name too.

wking · 2015-09-02T16:48:49Z

On Wed, Sep 02, 2015 at 09:45:00AM -0700, Alexander Morozov wrote:

Actually we don't need different go name too.

We don't? They're both in the specs package in the same directory.
If they were both Mount, how would you access one or the other?

LK4D4 · 2015-09-02T16:51:07Z

@wking It's type name, have nothing in common with field name.

wking · 2015-09-02T18:16:34Z

On Wed, Sep 02, 2015 at 09:51:08AM -0700, Alexander Morozov wrote:

@wking It's type name, have nothing in common with field name.

Ah, right. +1 to making both field names Mounts to match the JSON
keys.

LK4D4 · 2015-09-02T18:21:10Z

@wking @mrunalp I think I addressed comments.

mrunalp · 2015-09-02T18:23:30Z

runtime_config.go

s/configuration/configurations/

wking · 2015-09-02T18:38:56Z

On Wed, Sep 02, 2015 at 11:21:13AM -0700, Alexander Morozov wrote:

@wking @mrunalp I think I addressed comments.

Matching examples for mounts and mount-points is still open 1. But
“it's fine the way it is” is a valid response ;).

LK4D4 · 2015-09-02T18:43:05Z

@wking I actually added example of runtime.conf entry below that line.

wking · 2015-09-02T18:52:16Z

On Wed, Sep 02, 2015 at 11:43:07AM -0700, Alexander Morozov wrote:

@wking I actually added example of runtime.conf entry below that line.

But it only lists ‘proc’, while the runtime.json example in the runtime docs lists proc,
dev, devpts, and data. I'm suggesting we either list all those names
in both examples, or only proc in both examples.

wking · 2015-09-02T19:04:18Z

config.md

Ah, I wasn't suggesting we copy the runtime.json side over here. I want to link to it. But I think the config.json example here should be compatible with the runtime.json example we link to. I'm happy to PR a fixup against your branch if that's easier ;).

wking · 2015-09-02T19:14:20Z

On Wed, Sep 02, 2015 at 11:50:11AM -0700, W. Trevor King wrote:

On Wed, Sep 02, 2015 at 11:43:07AM -0700, Alexander Morozov wrote:

@wking I actually added example of runtime.conf entry below that line.

But it only lists ‘proc’…

I've been explaining this poorly, so here's a PR with my suggested
fixup: LK4D4#1. Feel free to squash into your commit if it
makes sense to you.

wking · 2015-09-02T20:10:43Z

runtime_config.go

We don't want to touch this comment.

I don't see what changed in this comment.
Oh, I see now.

On Wed, Sep 02, 2015 at 01:12:41PM -0700, Alexander Morozov wrote:

Mounts []Mount json:"mounts"

// Hooks are the commands run at various lifecycle events of the container.
…

Mounts map[string]Mount json:"mounts"

// Hooks are the commands run at various lifecycle events of the containers.

I don't see what changed in this comment.

The post-Mounts comment should be about hooks, and there's only one
container for lifecycle events.

// Mounts is a mapping of names to mount configuration.

// Which mounts will be mounted and where should be chosen with MountPoints

// in Spec.

The pre-Mounts comment^, on the other hand should be talking about
“mount configurations”, not “mount configuration”.

wking · 2015-09-02T20:56:33Z

config.md

<nit>Maybe “Mount destination in the container filesystem”? The colon-separated form works, but it took me a reread to understand what it meant.</nit>.

mrunalp · 2015-09-02T20:58:22Z

LGTM

crosbymichael · 2015-09-02T22:54:36Z

LGTM

laijs · 2015-09-02T23:36:26Z

runtime-config.md

Here is a leftover "destination"

laijs · 2015-09-02T23:53:52Z

I think the mount order should be clearly defined in the spec.

Alternative definitions in my mind:

The mount order are exactly the same as this mount points array. The config file MUST list them in reasonable order.
The mount order are mostly the same as this mount points array. But the longer path will be altered downward and be placed after the shorter path which is the ancestor of the longer path.

Any thoughts?

wking · 2015-09-02T23:58:40Z

On Wed, Sep 02, 2015 at 04:53:54PM -0700, Lai Jiangshan wrote:

The mount order are exactly the same as this mount points
array. The config file MUST list them in reasonable order.

This is how I'd do it.

laijs · 2015-09-03T00:30:16Z

This is how I'd do it.

Wait until full discussion?

wking · 2015-09-03T01:48:01Z

On Wed, Sep 02, 2015 at 05:30:18PM -0700, Lai Jiangshan wrote:

This is how I'd do it.

Wait until full discussion?

Probably wise ;). One thing to consider for auto-ordering, is what
would happen if you did:

Bind mount /a/b/c to /d
Mount $x over /a/b

I'm not actually sure ;), but it's possible that /d still looks into
the original /a/b/c, and not into $x/c. If so, that means
auto-ordering could be problematic, because it would see that (1)'s
/a/b/c seemingly depends on (2), when in fact the bundle-author
preferred (1) to happen first.

Added info about MountPoints to config.md. Signed-off-by: Alexander Morozov <[email protected]> Signed-off-by: W. Trevor King <[email protected]>

laijs · 2015-09-03T04:24:58Z

I prefer the strict/exact order. But it needs to be documented in the Spec.

laijs · 2015-09-03T04:25:53Z

LGTM (c18c283)

wking · 2015-09-03T14:19:47Z

On Wed, Sep 02, 2015 at 09:25:54PM -0700, Lai Jiangshan wrote:

LGTM (c18c283)

Looks good to me too. I'm fine punting explicit ordering statements
(and my other comment cleanups [1,2]) to follow-up PRs, if that helps
this one land faster.

LK4D4 · 2015-09-03T16:27:28Z

I think we have enough LGTMs here and I want to fix spec for now. Feel free to post your suggestions as another PR.

Change layout of mountpoints and mounts

I think Go's attribute syntax reads more clearly here, especially since there is no 'Spec.MountPoints' after c18c283 (Change layout of mountpoints and mounts, 2015-09-02, opencontainers#136). Signed-off-by: W. Trevor King <[email protected]>

c18c283 (Change layout of mountpoints and mounts, 2015-09-02, opencontainers#136) removed the destination field from the Go type and examples, but forgot to remove it from the documentation [1]. Fix that with this commit. [1]: opencontainers#109 (comment) Reported-by: 梁辰晔 (Liang Chenye) <[email protected]> Signed-off-by: W. Trevor King <[email protected]>

If we don't specify this, some bundle-authors or runtime-implementers might expect the runtime to intelligently order mounts to get the "right" order [1]. But that's not possible because: $ mkdir -p a/b/c d/e/f h # mount --bind a/b h # mount --bind d a/b $ tree --charset=ascii h h `-- c But in the other order: # umount a/b # umount h # mount --bind d a/b # mount --bind a/b h $ tree --charset=ascii h h `-- e `-- f So there's no "right" order. Allowing the bundle-author to specify their intended order is both easy to implement and unambiguous. [1]: opencontainers#136 (comment) Suggested-by: Lai Jiangshan <[email protected]> Signed-off-by: W. Trevor King <[email protected]>

'destination' has been the path inside the container since c18c283 (Change layout of mountpoints and mounts, 2015-09-02, opencontainers#136). My personal preference is to have an explicit pivot root and allow paths relative to the current working directory [1], but that would be a big shift from the current OCI spec. The only way the current spec lets you turn off the root pivot is by not setting a mount namespace at all (and even then, it's not clear if that turns off the pivot). And the config's root entry is required (despite my attempts to have it made optional [2]), it's not really clear how containers that don't set a mount namespace are supposed to work if they're supported at all. You might be able to get away with something like: When a mount namespace is not set, destination paths are relative to the runtime's initial working directory (or relative to the config.json, or whatever). When a mount namespace is set, destination paths are relative to the mount namespace's root. but with mount-namespace-less containers already so unclear, it seems better to just require absolute destinations. If/when we get clearer support for explicit pivot-root calls or containers that inherit the host mount namespace (without re-joining it and losing their old working directory), we can consider lifting the absolute-path restriction. [1]: https://github.com/wking/ccon/tree/v0.4.0#mount-namespace [2]: https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/6ZKMNWujDhU Date: Wed, 26 Aug 2015 12:54:47 -0700 Subject: Dropping the rootfs requirement and restoring arbitrary bundle content Message-ID: <[email protected]> Signed-off-by: W. Trevor King <[email protected]>

'destination' has been the path inside the container since c18c283 (Change layout of mountpoints and mounts, 2015-09-02, opencontainers#136). My personal preference is to have an explicit pivot root and allow paths relative to the current working directory [1], but that would be a big shift from the current OCI spec. The only way the current spec lets you turn off the root pivot is by not setting a mount namespace at all (and even then, it's not clear if that turns off the pivot). And the config's root entry is required (despite my attempts to have it made optional [2]), so it's not really clear how containers that don't set a mount namespace are supposed to work (if they're supported at all). You might be able to get away with something like: When a mount namespace is not set, destination paths are relative to the runtime's initial working directory (or relative to the config.json, or whatever). When a mount namespace is set, destination paths are relative to the mount namespace's root. but with mount-namespace-less containers already so unclear, it seems better to just require absolute destinations. If/when we get clearer support for explicit pivot-root calls or containers that inherit the host mount namespace (without re-joining it and losing their old working directory), we can consider lifting the absolute-path restriction. [1]: https://github.com/wking/ccon/tree/v0.4.0#mount-namespace [2]: https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/6ZKMNWujDhU Date: Wed, 26 Aug 2015 12:54:47 -0700 Subject: Dropping the rootfs requirement and restoring arbitrary bundle content Message-ID: <[email protected]> Signed-off-by: W. Trevor King <[email protected]>

wking reviewed Sep 2, 2015
View reviewed changes

LK4D4 force-pushed the fix_mounts branch from aaa314c to fe6c3c6 Compare September 2, 2015 16:52

LK4D4 force-pushed the fix_mounts branch from fe6c3c6 to 712d57c Compare September 2, 2015 18:19

mrunalp reviewed Sep 2, 2015
View reviewed changes

runtime_config.go Outdated

Copy link

Contributor

mrunalp Sep 2, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/configuration/configurations/

wking reviewed Sep 2, 2015
View reviewed changes

LK4D4 force-pushed the fix_mounts branch 4 times, most recently from 67313c6 to 497b51f Compare September 2, 2015 19:39

wking reviewed Sep 2, 2015
View reviewed changes

LK4D4 force-pushed the fix_mounts branch from 497b51f to 22a25da Compare September 2, 2015 20:14

wking reviewed Sep 2, 2015
View reviewed changes

laijs reviewed Sep 2, 2015
View reviewed changes

runtime-config.md Outdated

Copy link

Contributor

laijs Sep 2, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a leftover "destination"

Change layout of mountpoints and mounts

c18c283

Added info about MountPoints to config.md. Signed-off-by: Alexander Morozov <[email protected]> Signed-off-by: W. Trevor King <[email protected]>

LK4D4 force-pushed the fix_mounts branch from 22a25da to c18c283 Compare September 3, 2015 04:04

LK4D4 added a commit that referenced this pull request Sep 3, 2015

Merge pull request #136 from LK4D4/fix_mounts

8874000

Change layout of mountpoints and mounts

LK4D4 merged commit 8874000 into opencontainers:master Sep 3, 2015

LK4D4 deleted the fix_mounts branch September 3, 2015 16:27

wking mentioned this pull request Sep 3, 2015

config: Reword the mount-point path description #140

Closed

This was referenced Sep 3, 2015

runtime_config: Replace 'MountPoints in Spec' with 'Spec.Mounts' #141

Closed

config: Require the runtime to mount Spec.Mounts in order #142

Merged

Add config validation and environment tests #109

Closed

This was referenced Sep 9, 2015

runtime-config: Remove 'destination' docs from mounts #165

Merged

add hypervisor volume example for runtime #133

Closed

wking mentioned this pull request Nov 4, 2016

config: Require absolute mount destinations #609

Merged

Change layout of mountpoints and mounts #136

Change layout of mountpoints and mounts #136

Uh oh!

Conversation

LK4D4 commented Sep 2, 2015

Uh oh!

LK4D4 commented Sep 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking commented Sep 2, 2015

Uh oh!

LK4D4 commented Sep 2, 2015

Uh oh!

wking commented Sep 2, 2015

Uh oh!

LK4D4 commented Sep 2, 2015

Uh oh!

wking commented Sep 2, 2015

Uh oh!

LK4D4 commented Sep 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking commented Sep 2, 2015

Uh oh!

LK4D4 commented Sep 2, 2015

Uh oh!

wking commented Sep 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking commented Sep 2, 2015

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 Sep 2, 2015

Uh oh!

crosbymichael commented Sep 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laijs commented Sep 2, 2015

Uh oh!

wking commented Sep 2, 2015

Uh oh!

laijs commented Sep 3, 2015

Uh oh!

wking commented Sep 3, 2015

Uh oh!

laijs commented Sep 3, 2015

Uh oh!

laijs commented Sep 3, 2015

Uh oh!

wking commented Sep 3, 2015

Uh oh!

LK4D4 commented Sep 3, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants