runtime-config-linux: Separate mknod from cgroups #298

wking · 2016-01-12T05:52:26Z

This is #99 rebased onto the current master, because the idea seems
to have new life. It leaves mknod entries in linux.devices
and moves cgroups entries into linux.resources.devices. Quiet
thread on the issue here.

This makes it easy to distinguish between configs that call for cgroup
adjustments (which have linux.resources entries) from those that
don't. Without this split, folks interested in making that
distinction would have to parse the device section to determine if it
included cgroup changes. This will also make it easy to drop either
portion (mknod or cgroups) independently of the other if the
project decides to do so.

Using seperate sections for mknod and cgroups also allows us to avoid
the complicated validation rules needed for the combined format
mknod/cgroup (#101).

Now that there is a section specific to supplying devices, I shifted
the default device listing over from config-linux. The /dev/ptmx
entry is a bit awkward, since it's not a device, but it seemed to fit
better over here. But I would also be fine leaving it with the other
mounts in config-linux.

The reference links are sorted into two blocks, with kernel-doc links
sorted alphabetically followed by man pages sorted alphabetically by
section.

wking · 2016-01-12T05:55:11Z

@mrunalp, you were going to write up some more motivation for this? It's also possible that you intended to post that motivation to the thread, and that I'm jumping the gun with this reroll. If so, feel free to table/close this PR until the thread has reached a consensus.

mrunalp · 2016-01-15T21:33:44Z

We can continue using this PR. i have responded to the mailing list discussion.

mrunalp · 2016-01-20T18:14:55Z

There are two use cases for this separation:

The user doesn't want to specify any cgroup configuration. This is easy for the other values but the device cgroups are tied up the device configuration.
Running containers under a non-host usernamespace is an issue, because one can't modify the device cgroups in such a scenario.

So, separation will be good for consistency as well as technical reasons.

vishh · 2016-01-20T18:15:38Z

runtime-config-linux.md

Can devices be setup using hooks?

On Wed, Jan 20, 2016 at 10:15:41AM -0800, Vish Kannan wrote:

-devices is an array specifying the list of devices to be created in the container.
+devices is an array specifying the list of devices that the MUST be available in the container.

Can devices be setup using hooks?

Yeah. Or with bind-mounts, or by putting the device nodes in the
rootfs (#98). My attempt to summarize the pushback for dropping the
mknod section is in 1.

wking · 2016-01-20T19:09:25Z

Rerolled dfb5dad → 3f8105e adding @vishh's ‘omitempty’s [1,2,3].

wking · 2016-01-20T19:15:15Z

In today's meeting there was talk about where default-device creation
happens in the container-setup timeline 1, and whether/how config
authors should be able to opt-out of default-device creation 2.
Addressing either of those seems orthogonal to the split in this PR
3, but I'd thought I'd mention them here in case anyone else
disagrees about their orthogonality.

mrunalp · 2016-01-25T19:57:42Z

Needs rebase.

wking · 2016-01-25T20:27:46Z

On Mon, Jan 25, 2016 at 11:57:44AM -0800, Mrunal Patel wrote:

Needs rebase.

I'll wait until #284 lands.

wking · 2016-01-27T18:41:24Z

On Mon, Jan 25, 2016 at 12:27:43PM -0800, W. Trevor King wrote:

Mon, Jan 25, 2016 at 11:57:44AM -0800, Mrunal Patel:

Needs rebase.

I'll wait until #284 lands.

Rebased around #284 with 3f8105e → 81f726e.

$ diff -u <(git show 3f8105e) <(git show 81f726e)

shows no changes outside of diff context.

mrunalp · 2016-01-27T18:47:09Z

config-linux.md

s/that the/that/

On Wed, Jan 27, 2016 at 10:47:11AM -0800, Mrunal Patel wrote:

-devices is an array specifying the list of devices to be created in the container.
+devices is an array specifying the list of devices that the MUST be available in the container.

s/that the/that/

Fixed in 81f726e → a855883.

mrunalp · 2016-01-27T20:13:02Z

config-linux.md

Should we call it an access list instead of a whitelist?

On Wed, Jan 27, 2016 at 12:13:03PM -0800, Mrunal Patel wrote:

+#### Device whitelist

Should we call it an access list instead of a whitelist?

I'm just echoing the kernel docs [1](which moved! [2]). I've pushed
a855883 → ddd56d8 updating the link target.

Whitelist typically carries a positive connotation. In this case, this whitelist can be used to deny access to devices as well.

vishh · 2016-01-27T21:13:12Z

config-linux.md

Are the following fields required?

On Wed, Jan 27, 2016 at 01:13:14PM -0800, Vish Kannan wrote:

+* fileMode (uint32, required) - file mode for the device.

You can also control access to devices with cgroups.

Are the following fields required?

I'm fine making fileMode, uid, and gid optional. Would they default
to the effective uid/gid/umask of the runtime process? Of the
container process as it will be when it execs any user-configured
arguments?

Defaults can also depend on it setgid bit is set on the parent dir I think.
But it shouldn't matter from the Spec level.

On Wed, Jan 27, 2016 at 1:31 PM W. Trevor King [email protected]
wrote:

In config-linux.md
#298 (comment):

-* permissions (string, optional) - cgroup permissions for device. A composition of r (read), w (write), and m (mknod).

-* fileMode (uint32, optional) - file mode for device file

-* uid (uint32, optional) - uid of device owner

-* gid (uint32, optional) - gid of device owner

-fileMode, uid and gid are required if path is given and are otherwise not allowed.
+* type (char, required) - type of device: c, b, u or p.

More info in [mknod(1)][mknod.1].
+* path (string, required) - full path to device inside container.
+* major, minor (int64, required) - [major, minor numbers][devices] for the device.
+* fileMode (uint32, required) - file mode for the device.

You can also control access to devices with cgroups.

On Wed, Jan 27, 2016 at 01:13:14PM -0800, Vish Kannan wrote: > +*
fileMode (uint32, required) - file mode for the device. > + You can
also control access to devices with cgroups. Are the
following fields required?
I'm fine making fileMode, uid, and gid optional. Would they default to the
effective uid/gid/umask of the runtime process? Of the container process as
it will be when it execs any user-configured arguments?

—
Reply to this email directly or view it on GitHub
https://github.com/opencontainers/specs/pull/298/files#r51053200.

What I was saying was that, it is possible to use the struct without
specifying this field. Hence it is optional.
On Wed, Jan 27, 2016 at 1:35 PM Vishnu Kannan [email protected] wrote:

Defaults can also depend on it setgid bit is set on the parent dir I
think. But it shouldn't matter from the Spec level.

On Wed, Jan 27, 2016 at 1:31 PM W. Trevor King [email protected]
wrote:

In config-linux.md
#298 (comment):

-* permissions (string, optional) - cgroup permissions for device. A composition of r (read), w (write), and m (mknod).

-* fileMode (uint32, optional) - file mode for device file

-* uid (uint32, optional) - uid of device owner

-* gid (uint32, optional) - gid of device owner

-fileMode, uid and gid are required if path is given and are otherwise not allowed.
+* type (char, required) - type of device: c, b, u or p.

More info in [mknod(1)][mknod.1].
+* path (string, required) - full path to device inside container.
+* major, minor (int64, required) - [major, minor numbers][devices] for the device.
+* fileMode (uint32, required) - file mode for the device.

You can also control access to devices with cgroups.

On Wed, Jan 27, 2016 at 01:13:14PM -0800, Vish Kannan wrote: > +*
fileMode (uint32, required) - file mode for the device. > + You can
also control access to devices with cgroups. Are the
following fields required?
I'm fine making fileMode, uid, and gid optional. Would they default to
the effective uid/gid/umask of the runtime process? Of the container
process as it will be when it execs any user-configured arguments?

—
Reply to this email directly or view it on GitHub
https://github.com/opencontainers/specs/pull/298/files#r51053200.

Ok, I just pushed ddd56d8 → 7d5b027, which I think addresses all the optional/required issues. I put some comments on the changes with references, etc., in the new commit message if reviewers want to look those over alongside the diff. The commit message cites mknod, but obviously the bind-mount approach to supplying devices would also work without fileMode, uid, or gid specified.

With mknod entries in linux.devices and cgroups entries in linux.resources.devices. Background discussion in [1]. For specifying device cgroups independent of device creation. This makes it easy to distinguish between configs that call for cgroup adjustments (which have linux.resources entries) from those that don't. Without this split, folks interested in making that distinction would have to parse the device section to determine if it included cgroup changes. This will also make it easy to drop either portion (mknod [2] or cgroups [3]) independently of the other if the project decides to do so. Using seperate sections for mknod and cgroups also allows us to avoid the complicated validation rules needed for the combined format mknod/cgroup [4]. Now that there is a section specific to supplying devices, I shifted the default device listing over from config-linux [5]. The /dev/ptmx entry is a bit awkward, since it's not a device, but it seemed to fit better over here. But I would also be fine leaving it with the other mounts in config-linux. fileMode, uid, and gid are optional, because mknod(2) doesn't need them and specifies the handling when they aren't set [6,7]. Similarly, major/minor numbers are only required for S_IFCHR and S_IFBLK [6]. I've left off wording about required runtime behavior for unset values, because I'd rather address that with a blanket rule [8]. For the cgroup, access is optional because the kernel docs show an example that doesn't write an access field to the devices.deny file [9]. The current kernel docs don't go into much detail on this behavior (I expect unset and 'rwm' are equivalent), but if the kernel doesn't need a value written, the spec should get out of the way and allow users to not specify a value. The reference links are sorted into two blocks, with kernel-doc links sorted alphabetically followed by man pages sorted alphabetically by section. The cgroup link is new since 2016-01-13 [10]. [1]: https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/y_Fsa2_jJaM Subject: Separate config entries for device mknod and cgroups? Date: Mon, 5 Oct 2015 12:46:55 -0700 Message-ID: <[email protected]> [2]: opencontainers#98 [3]: https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/qWHoKs8Fsrk Subject: removal of cgroups from the OCI Linux spec Date: Wed, 28 Oct 2015 17:01:59 +0000 Message-ID: <CAD2oYtO1RMCcUp52w-xXemzDTs+J6t4hS5Mm4mX+uBnVONGDfA@mail.gmail.com> [4]: opencontainers#101 [5]: opencontainers#171 (comment) [6]: http://man7.org/linux/man-pages/man2/mknod.2.html#DESCRIPTION [7]: https://github.com/opencontainers/specs/pull/298/files#r51053835 [8]: opencontainers#285 (comment) [9]: https://kernel.org/doc/Documentation/cgroup-v1/devices.txt [10]: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=34a9304a96d6351c2d35dcdc9293258378fc0bd8 Signed-off-by: W. Trevor King <[email protected]>

crosbymichael · 2016-01-27T23:14:00Z

config-linux.md

I don't really understand this allow and access and how they work together. Can you explain better to me?

I think allow is devices.allow vs devices.deny and rwm is read, write, mknod.

On Wed, Jan 27, 2016 at 03:30:42PM -0800, Tõnis Tiigi wrote:

+* allow (boolean, required) - whether the entry is allowed or denied.

I think allow is devices.allow vs devices.deny and 'rwm` is
read, write, mknod.

That's right. What wording should I add to make it more clear?

ahh, i guess i get what would happen

crosbymichael · 2016-01-28T00:12:57Z

LGTM

mrunalp · 2016-01-28T03:01:57Z

LGTM

runtime-config-linux: Separate mknod from cgroups

My pull request landed on 2016-01-27 in 608cb7b [1]. [1]: opencontainers/runtime-spec#298 (comment)

mrunalp added this to the v0.3.0 milestone Jan 13, 2016

vishh reviewed Jan 20, 2016
View reviewed changes

wking force-pushed the separate-device-cgroups-from-mknod branch from dfb5dad to 3f8105e Compare January 20, 2016 19:08

wking force-pushed the separate-device-cgroups-from-mknod branch from 3f8105e to 81f726e Compare January 27, 2016 18:38

mrunalp reviewed Jan 27, 2016
View reviewed changes

wking force-pushed the separate-device-cgroups-from-mknod branch from 81f726e to a855883 Compare January 27, 2016 18:53

mrunalp reviewed Jan 27, 2016
View reviewed changes

wking force-pushed the separate-device-cgroups-from-mknod branch from a855883 to ddd56d8 Compare January 27, 2016 20:19

wking mentioned this pull request Jan 27, 2016

config-linux: Update links to cgroups documentation #318

Merged

vishh reviewed Jan 27, 2016
View reviewed changes

wking force-pushed the separate-device-cgroups-from-mknod branch from ddd56d8 to 7d5b027 Compare January 27, 2016 21:55

crosbymichael reviewed Jan 27, 2016
View reviewed changes

mrunalp pushed a commit that referenced this pull request Jan 28, 2016

Merge pull request #298 from wking/separate-device-cgroups-from-mknod

608cb7b

runtime-config-linux: Separate mknod from cgroups

mrunalp merged commit 608cb7b into opencontainers:master Jan 28, 2016

wking deleted the separate-device-cgroups-from-mknod branch January 28, 2016 04:09

wking added a commit to wking/nmbug-oci that referenced this pull request Jan 28, 2016

Separate config entries for device mknod and cgroups (fixed)

32f9244

My pull request landed on 2016-01-27 in 608cb7b [1]. [1]: opencontainers/runtime-spec#298 (comment)

runtime-config-linux: Separate mknod from cgroups #298

runtime-config-linux: Separate mknod from cgroups #298

Uh oh!

Conversation

wking commented Jan 12, 2016

Uh oh!

wking commented Jan 12, 2016

Uh oh!

mrunalp commented Jan 15, 2016

Uh oh!

mrunalp commented Jan 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking commented Jan 20, 2016

Uh oh!

wking commented Jan 20, 2016

Uh oh!

mrunalp commented Jan 25, 2016

Uh oh!

wking commented Jan 25, 2016

Uh oh!

wking commented Jan 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

-* permissions (string, optional) - cgroup permissions for device. A composition of r (read), w (write), and m (mknod).

-* fileMode (uint32, optional) - file mode for device file

-* uid (uint32, optional) - uid of device owner

-* gid (uint32, optional) - gid of device owner

Uh oh!

Choose a reason for hiding this comment

-* permissions (string, optional) - cgroup permissions for device. A composition of r (read), w (write), and m (mknod).

-* fileMode (uint32, optional) - file mode for device file

-* uid (uint32, optional) - uid of device owner

-* gid (uint32, optional) - gid of device owner

Uh oh!

wking Jan 27, 2016 via email

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!

crosbymichael commented Jan 28, 2016

Uh oh!

mrunalp commented Jan 28, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

-* `permissions` (string, optional) - cgroup permissions for device. A composition of `r` (read), `w` (write), and `m` (mknod).

-* `fileMode` (uint32, optional) - file mode for device file

-* `uid` (uint32, optional) - uid of device owner

-* `gid` (uint32, optional) - gid of device owner

-* `permissions` (string, optional) - cgroup permissions for device. A composition of `r` (read), `w` (write), and `m` (mknod).

-* `fileMode` (uint32, optional) - file mode for device file

-* `uid` (uint32, optional) - uid of device owner

-* `gid` (uint32, optional) - gid of device owner