Skip to content

Fixes to ocitools generate to make it work with runc again #24

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 24, 2016
Merged

Fixes to ocitools generate to make it work with runc again #24

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 48 additions & 1 deletion generate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,8 @@ func setupNamespaces(spec *specs.Spec, context *cli.Context) {
spec.Linux.Namespaces = linuxNs
}

func sPtr(s string) *string { return &s }

func getDefaultTemplate() specs.Spec {
spec := specs.Spec{
Version: specs.Version,
Expand Down Expand Up @@ -646,8 +648,53 @@ func getDefaultTemplate() specs.Spec {
},
},
Hostname: "shell",
Mounts: []specs.Mount{},
Mounts: []specs.Mount{
{
Destination: "/proc",
Type: "proc",
Source: "proc",
Options: nil,
Copy link
Contributor

@wking wking Mar 23, 2016

Choose a reason for hiding this comment

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

It looks like this is rolling back 080ee00 (drop-runtime-supplied-devices-and-mounts, #2). Did we miss something there?

Copy link
Contributor Author

@mrunalp mrunalp Mar 23, 2016

Choose a reason for hiding this comment

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

@wking runc supports default devices but not mounts yet.

Copy link
Contributor

@wking wking Mar 23, 2016

Choose a reason for hiding this comment

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

On Wed, Mar 23, 2016 at 03:26:02PM -0700, Mrunal Patel wrote:

@@ -646,8 +648,53 @@ func getDefaultTemplate() specs.Spec {
},
},
Hostname: "shell",

  •   Mounts:   []specs.Mount{},

  •   Mounts: []specs.Mount{

  •       {

  •           Destination: "/proc",

  •           Type:        "proc",

  •           Source:      "proc",

  •           Options:     nil,

@wking runc supports default devices but not mounts yet.

Ah, got it. Maybe add a note to that effect in the commit message?

Copy link
Contributor Author

@mrunalp mrunalp Mar 23, 2016

Choose a reason for hiding this comment

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

@wking Updated the commit message.

Copy link
Contributor

@wking wking Mar 23, 2016

Choose a reason for hiding this comment

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

On Wed, Mar 23, 2016 at 04:04:14PM -0700, Mrunal Patel wrote:

@@ -646,8 +648,53 @@ func getDefaultTemplate() specs.Spec {
},
},
Hostname: "shell",

  •   Mounts:   []specs.Mount{},

  •   Mounts: []specs.Mount{

  •       {

  •           Destination: "/proc",

  •           Type:        "proc",

  •           Source:      "proc",

  •           Options:     nil,

@wking Updated the commit message.

Thanks. 85943a0 looks good to me.

},
{
Destination: "/dev",
Type: "tmpfs",
Source: "tmpfs",
Options: []string{"nosuid", "strictatime", "mode=755", "size=65536k"},
},
{
Destination: "/dev/pts",
Copy link
Contributor

@wking wking Jun 18, 2016

Choose a reason for hiding this comment

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

This one is causing trouble in my ccon-oci tests. The previous mount just put a tmpfs at /dev, so there is no directory at /dev/pts to attach the mount. Testing with a few patches to get ccon-oci and runtime_test.sh working together:

$ ./test_runtime.sh -r 'ccon-oci --verbose'
-----------------------------------------------------------------------------------
VALIDATING RUNTIME: ccon-oci --verbose
-----------------------------------------------------------------------------------
launched container process with PID 22796
write '0 1000 1' to /proc/22796/uid_map
write 'deny' to /proc/22796/setgroups
write '0 1000 1' to /proc/22796/gid_map
mount 0: /tmp/tmp.MZQVJfex0V/busybox/./rootfs to /tmp/tmp.MZQVJfex0V/busybox/./rootfs (type: (null), flags: 4096, data (null))
mount 1: /tmp/tmp.MZQVJfex0V/busybox/proc to /tmp/tmp.MZQVJfex0V/busybox/rootfs/proc (type: proc, flags: 0, data (null))
mount 2: /tmp/tmp.MZQVJfex0V/busybox/tmpfs to /tmp/tmp.MZQVJfex0V/busybox/rootfs/dev (type: tmpfs, flags: 16777218, data mode=755,size=65536k)
mount 3: /tmp/tmp.MZQVJfex0V/busybox/devpts to /tmp/tmp.MZQVJfex0V/busybox/rootfs/dev/pts (type: devpts, flags: 10, data newinstance,ptmxmode=0666,mode=0620,gid=5)
mount: No such file or directory
…

I don't see any language about “the runtime MUST create missing destinations” in the spec, and I don't see a way to support this default template without doing that. Are there suggestions for how other runtimes should handle entries like this until runC sorts out its required filesystems support?

Copy link
Contributor

@wking wking Jan 23, 2017

Choose a reason for hiding this comment

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

I don't see any language about “the runtime MUST create missing destinations” in the spec, and I don't see a way to support this default template without doing that.

The current spec still doesn't talk about this explicitly, but the intended approach is to have the runtime attempt to create the destination file/directory on its own.

Type: "devpts",
Source: "devpts",
Options: []string{"nosuid", "noexec", "newinstance", "ptmxmode=0666", "mode=0620", "gid=5"},
Copy link
Contributor

@wking wking Jun 18, 2016

Choose a reason for hiding this comment

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

The gid=5 option will be problematic for user namespaces (where you can only map a single user). From user_namespaces(7) for unprivileged (in the parent user namespace) processes:

The data written to uid_map (gid_map) must consist of a single line that maps the writing process's effective user ID (group ID) in the parent user namespace to a user ID (group ID) in the user namespace.

So if the caller maps themselves to 0, then all other GIDs (including 5) will be unmapped. Are we not compliance testing unprivileged processes until runC sorts out its required filesystems support? If we are, how should I handle this mount entry?

},
{
Destination: "/dev/shm",
Type: "tmpfs",
Source: "shm",
Options: []string{"nosuid", "noexec", "nodev", "mode=1777", "size=65536k"},
},
{
Destination: "/dev/mqueue",
Type: "mqueue",
Source: "mqueue",
Options: []string{"nosuid", "noexec", "nodev"},
},
{
Destination: "/sys",
Type: "sysfs",
Source: "sysfs",
Options: []string{"nosuid", "noexec", "nodev", "ro"},
},
},
Linux: specs.Linux{
Resources: &specs.Resources{
Devices: []specs.DeviceCgroup{
{
Allow: false,
Access: sPtr("rwm"),
},
},
},
Namespaces: []specs.Namespace{
{
Type: "pid",
Expand Down