Replace Linux.Device with more specific config by LK4D4 · Pull Request #94 · opencontainers/runtime-spec

LK4D4 · 2015-08-05T22:26:45Z

It appeared that we need more control over devices, for example different permissions and choosing path on caller side.

crosbymichael · 2015-08-05T22:28:38Z

We should probably specify or describe in this doc what each of the fields mean. But overall this PR looks good

I'll combine description from docstrings.

Is there ever a reason not to include null, random, full, tty, zero and urandom? It seems like we should just say every container can expect to have those device nodes available without specifying.

Ah, okay, thought about this just like about json example, not real config :) I'll add all from runc defaults.

@LK4D4 Yes, I will file a separate issue on this one.

@LK4D4 filed #95

philips · 2015-08-05T22:35:16Z

LGTM overall, I do think we should consider making the "obvious" device nodes available as they are a pretty accepted part of the POSIX API.

wking · 2015-08-05T22:40:03Z

On Wed, Aug 05, 2015 at 03:26:47PM -0700, Alexander Morozov wrote:

It appeared that we need more control over devices, for example
different permissions and choosing path on caller side.

Can you elaborate on this or provide a concrete example? I'd guess
it's something like “I translate my UIDs so my container root isn't my
host root, but I still need access to a root-only host device”, but
that seems like it's handled by the current “The runtime should not
only create the device inside the container but ensure that the root
user inside the container has access rights for the device” phrasing.
Any changes you need to make after that can be handled by the
application itself (or a pre-start hook). If it turns out that folks
do need this sort of detailed device control, can't they just populate
rootfs/dev in their bundled filesystem?

mrunalp · 2015-08-05T23:42:46Z

LK4D4 · 2015-08-06T00:37:26Z

@wking For example we want different cgroups permissions for devices. Another case is to creating device in paths which doesn't exists on host. Also this interface maps pretty well to device creation process on the host, I don't see why we should try to cut it off.
PTAL @crosbymichael @mrunalp @philips

philips · 2015-08-06T00:42:14Z

I don't think a full exhaustive list of many devices is necessary in the spec to illustrate but it LGTM.

laijs · 2015-08-06T09:57:00Z

From my instinct, the abbreviation of r/w/m is OK for permissions. But here, I think we should use "character", "block" and "fifo". I don't have strong reason, just feeling.

I think this is okay as more information could be looked up in the man page.

wking · 2015-08-06T17:06:44Z

On Wed, Aug 05, 2015 at 05:37:28PM -0700, Alexander Morozov wrote:

For example we want different cgroups permissions for devices.
Another case is to creating device in paths which doesn't exists on
host.

For “devices in paths which don't exist on the host”, it seems like
the easiest approach would be to just bundle those device files
(e.g. in rootfs/dev/…).

For “I want special cgroups on $PATH”, it seems like that would be
better handled by either a pre-start hook or a more generic
linux.resources entry. Something that directly maps to 1, like:

{
…
"linux": {
…
"resources": {
…
"devices": [
{
"type": "all",
"major": "",
"minor": "",
"access": "rwm",
}
]
}
}
}

As this PR stands, I'm not sure how you'd use wildcards for
major/minor cgroups rules.

LK4D4 · 2015-08-06T17:10:29Z

@wking I can add to description, that -1 will mean wildcard.

wking · 2015-08-06T17:25:52Z

On Thu, Aug 06, 2015 at 10:10:30AM -0700, Alexander Morozov wrote:

@wking I can add to description, that -1 will mean wildcard.

If you have a -1 in the spec, and the device doesn't exist on the
host, what value will you pass to mknod? That's why I want to
separate the mknod step (which can be the current “copy from the host”
or “handled by the bundle author when building the bundled
filesystem”) from the cgroups step (which I think needs a new spec
entry) 1.

Signed-off-by: Alexander Morozov <lk4d4@docker.com>

LK4D4 · 2015-08-06T17:29:08Z

@wking It's up to you which value you will pass, I don't like any copy from the host in spec, because it's not flexible at all.

wking · 2015-08-06T17:50:43Z

On Thu, Aug 06, 2015 at 10:29:10AM -0700, Alexander Morozov wrote:

It's up to you which value you will pass, …

No, I meant “assuming the -1 wildcard 1, if I put a -1 major value
in my bundle config, what will the runtime use when it calls mknod?”

I don't like any copy from the host in spec, because it's not
flexible at all.

I'm ok dropping that section, since bundle authors that want to copy
device nodes from the host are expecting that those copied files are
portable enough to not need further specification. If that's the
case, they are likely to have the device files on the host that
they're using to write the bundle, and they can copy them from there
into their bundled filesystem.

Maybe it would help if I just PRed my suggestion…

LK4D4 · 2015-08-06T17:57:20Z

@wking Yup, example of your thoughts in code would be perfect.

wking · 2015-08-06T18:31:44Z

On Thu, Aug 06, 2015 at 10:57:22AM -0700, Alexander Morozov wrote:

@wking Yup, example of your thoughts in code would be perfect.

Filed as #98.

crosbymichael · 2015-08-06T22:41:00Z

LGTM

Replace Linux.Device with more specific config

wking · 2015-08-06T23:03:53Z

On Thu, Aug 06, 2015 at 03:41:04PM -0700, Michael Crosby wrote:

Merged #94.

Hmm. Can someone clarify how the -1 wildcards 1 are supposed to
work? I still don't see how a single major (or minor) field is
sufficient for both mknod and cgroups [2](which is why I floated #98
and #99).

LK4D4 · 2015-08-07T00:32:15Z

@wking Basically, if you want to use wildcard, then you need to skip Path, because otherwise runtime will try to mknod. If Permissions is not empty - then device is allowed. That's whole relationship between devices and cgroup as I see it. I don't see a big problem if it'll be documented.

wking · 2015-08-07T16:44:35Z

On Thu, Aug 06, 2015 at 05:32:17PM -0700, Alexander Morozov wrote:

Basically, if you want to use wildcard, then you need to skip Path,
because otherwise runtime will try to mknod. If Permissions is not
empty - then device is allowed. That's whole relationship between
devices and cgroup as I see it. I don't see a big problem if it'll
be documented.

I started working on a PR documenting this, but I'm not sure how to
distinguish between allow and deny cgroup rules 1. Do you have
something equivalent to 2?

I'd prefer to handle mknod and device cgroups independently [1,2], to avoid all this "If path is given..." and "If parameters is given..." special casing. But the overloaded approach has landed [3], so this commit documents the indended semantics [4]. I'm not sure how bundle authors are supposed to register deny cgroups rules [5]. [1]: opencontainers#98 [2]: opencontainers#99 [3]: opencontainers#94 (comment) [4]: opencontainers#94 (comment) [5]: opencontainers#94 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>

LK4D4 · 2015-08-07T17:10:36Z

@wking basically idea was to echo a > devices.deny and then add to allow only devices with non-empty permissions.

wking · 2015-08-07T17:17:57Z

On Fri, Aug 07, 2015 at 10:10:38AM -0700, Alexander Morozov wrote:

@wking basically idea was to echo a > devices.deny and then add to
allow only devices with non-empty permissions.

That may be overly strict for child containers. What if you want to
allow your children to add further allows?

wking · 2015-08-07T17:23:47Z

I've floated some more explicit docs in #101, if we want to talk about the semantics and docs for allow vs. deny there.

LK4D4 · 2015-08-07T17:24:09Z

@wking Maybe it's better to discuss this in IRC. It's too much for closed PR.

I filed a PR to keep this separate [1], but it was closed after [2] landed. See also [3], where I point out that putting the mknod stuff in the “control groups” section is awkward. [1]: opencontainers/runtime-spec#99 Add linux.resources.devices [2]: opencontainers/runtime-spec#94 Replace Linux.Device with more specific config [3]: opencontainers/runtime-spec#171 (comment) move the description of user ns mapping and default files to proper file

crosbymichael reviewed Aug 5, 2015
View reviewed changes

LK4D4 force-pushed the expose_more_about_device branch from 62ba998 to 0512e7eCompare August 5, 2015 22:39

mrunalp reviewed Aug 5, 2015
View reviewed changes

Comment threadspec_linux.go Outdated
mrunalpAug 5, 2015
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UID

LK4D4 force-pushed the expose_more_about_device branch 2 times, most recently from e923f70 to 85586b4Compare August 6, 2015 00:33

LK4D4 force-pushed the expose_more_about_device branch from 85586b4 to 456c6bdCompare August 6, 2015 00:43

laijs reviewed Aug 6, 2015
View reviewed changes

Replace Linux.Device with more specific config
5273b3d
Signed-off-by: Alexander Morozov <lk4d4@docker.com>

LK4D4 force-pushed the expose_more_about_device branch from 456c6bd to 5273b3dCompare August 6, 2015 17:26

wking mentioned this pull request Aug 6, 2015
Add linux.resources.devices and drop linux.devices #98
Closed

crosbymichael added a commit that referenced this pull request Aug 6, 2015
Merge pull request#94from LK4D4/expose_more_about_device
cb928bb
Replace Linux.Device with more specific config

crosbymichael merged commit cb928bb into opencontainers:masterAug 6, 2015

wking mentioned this pull request Aug 6, 2015
Add linux.resources.devices #99
Closed

LK4D4 deleted the expose_more_about_device branch August 7, 2015 00:18

wking mentioned this pull request Aug 7, 2015
Additional documentation for overloaded mknod / cgroups in linux.devices #101
Closed

wking mentioned this pull request Aug 11, 2015
Correct err in devices json in config-linux.md #103
Closed

Uh oh!

Conversation

LK4D4 commented Aug 5, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philips commented Aug 5, 2015

Uh oh!

wking commented Aug 5, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Aug 6, 2015

Uh oh!

philips commented Aug 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking commented Aug 6, 2015

Uh oh!

LK4D4 commented Aug 6, 2015

Uh oh!

wking commented Aug 6, 2015

Uh oh!

LK4D4 commented Aug 6, 2015

Uh oh!

wking commented Aug 6, 2015

Uh oh!

LK4D4 commented Aug 6, 2015

Uh oh!

wking commented Aug 6, 2015

Uh oh!

crosbymichael commented Aug 6, 2015

Uh oh!

wking commented Aug 6, 2015

Uh oh!

LK4D4 commented Aug 7, 2015

Uh oh!

wking commented Aug 7, 2015

Uh oh!

LK4D4 commented Aug 7, 2015

Uh oh!

wking commented Aug 7, 2015

Uh oh!

wking commented Aug 7, 2015 via email

Uh oh!

LK4D4 commented Aug 7, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants