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.

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

Implement create and start #827

Merged
merged 11 commits into from Jun 3, 2016

Conversation

Projects
None yet
@crosbymichael
Member

crosbymichael commented May 16, 2016

This implements create and start in runc without the need for a unix socket or other complexity. It implements it by blocking the init process waiting on a SIGCONT before the users process is started.

This does not remove hooks, that can be done separately. This also updates the libcontainer stats to correctly report if the container is created vs running(user code).

It does not bind mount namespaces, if you want namespaces to be bind mounted then you can write code to bind them after create returns and before calling start.

It retains the current functionality of start today by adding a runc run command that does the same workflow as today.

Closes #506

@julz

This comment has been minimized.

Show comment
Hide comment
@julz

julz May 16, 2016

Contributor

Nice.

Contributor

julz commented May 16, 2016

Nice.

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael May 16, 2016

Member

@julz thanks, plz take the time to play with it, runc is fully operational. lets make sure that it will fit and work with all of our needs but I think this implementation is simple and clean and will work fine.

Member

crosbymichael commented May 16, 2016

@julz thanks, plz take the time to play with it, runc is fully operational. lets make sure that it will fit and work with all of our needs but I think this implementation is simple and clean and will work fine.

Show outdated Hide outdated start.go Outdated
return system.Execv(l.config.Args[0], l.config.Args[0:], os.Environ())
// check for the arg before waiting to make sure it exists and it is returned
// as a create time error
name, err := exec.LookPath(l.config.Args[0])

This comment has been minimized.

@wking

wking May 17, 2016

Contributor

If folks intend to never call start (which seems fine to me for some use-cases), then we probably don't want to require a process config at all. If they do intend to call start, whether the command exists at execve-time seems different enough from whether the command exists at create time for folks to want this early check to at least be optional (e.g. “don't worry, by the time I call start I'll have created that command”).

@wking

wking May 17, 2016

Contributor

If folks intend to never call start (which seems fine to me for some use-cases), then we probably don't want to require a process config at all. If they do intend to call start, whether the command exists at execve-time seems different enough from whether the command exists at create time for folks to want this early check to at least be optional (e.g. “don't worry, by the time I call start I'll have created that command”).

This comment has been minimized.

@cyphar

cyphar May 18, 2016

Member

I agree, we shouldn't error out during create if the program doesn't exist at create-time.

@cyphar

cyphar May 18, 2016

Member

I agree, we shouldn't error out during create if the program doesn't exist at create-time.

This comment has been minimized.

@crosbymichael

crosbymichael May 26, 2016

Member

At this time it is required and if we want to change that its a different discussion

@crosbymichael

crosbymichael May 26, 2016

Member

At this time it is required and if we want to change that its a different discussion

This comment has been minimized.

@wking

wking May 26, 2016

Contributor

On Thu, May 26, 2016 at 01:20:02PM -0700, Michael Crosby wrote:

  • // check for the arg before waiting to make sure it exists and it is returned
  • // as a create time error
  • name, err := exec.LookPath(l.config.Args[0])

At this time it is required and if we want to change that its a
different discussion

Required as in “that's what runC will do right now” or required as in
opencontainers/runtime-spec#384 needs to be updated to list this
requirement”? The distinction there is mostly in how compliance is
tested in ocitools.

@wking

wking May 26, 2016

Contributor

On Thu, May 26, 2016 at 01:20:02PM -0700, Michael Crosby wrote:

  • // check for the arg before waiting to make sure it exists and it is returned
  • // as a create time error
  • name, err := exec.LookPath(l.config.Args[0])

At this time it is required and if we want to change that its a
different discussion

Required as in “that's what runC will do right now” or required as in
opencontainers/runtime-spec#384 needs to be updated to list this
requirement”? The distinction there is mostly in how compliance is
tested in ocitools.

Show outdated Hide outdated utils_linux.go Outdated
return -1, err
}
}
if r.detach || r.create {

This comment has been minimized.

@wking

wking May 17, 2016

Contributor

My earlier comment applies here too.

@wking

wking May 17, 2016

Contributor

My earlier comment applies here too.

This comment has been minimized.

@crosbymichael

crosbymichael May 17, 2016

Member

negative

@crosbymichael

crosbymichael May 17, 2016

Member

negative

This comment has been minimized.

@wking

wking May 17, 2016

Contributor

On Tue, May 17, 2016 at 11:11:16AM -0700, Michael Crosby wrote:

  • if r.detach || r.create {

negative

Can you unpack that a bit? Why do you need to close the TTY in the
‘create’ case? Why aren't you waiting on the container process and
returning it's error code?

@wking

wking May 17, 2016

Contributor

On Tue, May 17, 2016 at 11:11:16AM -0700, Michael Crosby wrote:

  • if r.detach || r.create {

negative

Can you unpack that a bit? Why do you need to close the TTY in the
‘create’ case? Why aren't you waiting on the container process and
returning it's error code?

This comment has been minimized.

@crosbymichael

crosbymichael May 17, 2016

Member

When create returns without an error then the container is created and ready to be started. You cannot attach because create cannot return.

@crosbymichael

crosbymichael May 17, 2016

Member

When create returns without an error then the container is created and ready to be started. You cannot attach because create cannot return.

This comment has been minimized.

@wking

wking May 17, 2016

Contributor

On Tue, May 17, 2016 at 11:25:06AM -0700, Michael Crosby wrote:

  • if r.detach || r.create {

When create returns without an error then the container is created
and ready to be started. You cannot attach because create cannot
return.

But somebody needs to stay around to collect the container exit code,
tear down cgroups, de-register the state, etc., right? See waitpid
notes in 1. In the ccon implementation the host-side ‘create’
process stays around to reap the container process 2, and folks who
want to get their terminal back to run ‘start’ can background the
‘create’ process 3.

@wking

wking May 17, 2016

Contributor

On Tue, May 17, 2016 at 11:25:06AM -0700, Michael Crosby wrote:

  • if r.detach || r.create {

When create returns without an error then the container is created
and ready to be started. You cannot attach because create cannot
return.

But somebody needs to stay around to collect the container exit code,
tear down cgroups, de-register the state, etc., right? See waitpid
notes in 1. In the ccon implementation the host-side ‘create’
process stays around to reap the container process 2, and folks who
want to get their terminal back to run ‘start’ can background the
‘create’ process 3.

This comment has been minimized.

@crosbymichael

crosbymichael May 17, 2016

Member

Are you asking a question or what are you trying to get at?

No, the create process does not have to stick around to reap, the proc calling create can handle that and orchestrate the entire thing from create to destroy.

@crosbymichael

crosbymichael May 17, 2016

Member

Are you asking a question or what are you trying to get at?

No, the create process does not have to stick around to reap, the proc calling create can handle that and orchestrate the entire thing from create to destroy.

This comment has been minimized.

@wking

wking May 17, 2016

Contributor

On Tue, May 17, 2016 at 01:05:45PM -0700, Michael Crosby wrote:

Are you asking a question or what are you trying to get at?

I'm trying to decide how you figure out the exit code for the
container process. It sounds like you're saying:

“The runtime will not track or provide access to this information.
The process that called ‘create’ should setup its own wait to
collect that information and call ‘delete’.”

wait(1p) is a fairly convenient way to do that from the shell, so I'm
fine with this approach, but it is shifting that reap/cleanup
functionality from the runtime (which used to handle it) to the
caller, so we probably want clear docs to this effect.

@wking

wking May 17, 2016

Contributor

On Tue, May 17, 2016 at 01:05:45PM -0700, Michael Crosby wrote:

Are you asking a question or what are you trying to get at?

I'm trying to decide how you figure out the exit code for the
container process. It sounds like you're saying:

“The runtime will not track or provide access to this information.
The process that called ‘create’ should setup its own wait to
collect that information and call ‘delete’.”

wait(1p) is a fairly convenient way to do that from the shell, so I'm
fine with this approach, but it is shifting that reap/cleanup
functionality from the runtime (which used to handle it) to the
caller, so we probably want clear docs to this effect.

This comment has been minimized.

@crosbymichael

crosbymichael May 17, 2016

Member
package main

import (
    "io/ioutil"
    "log"
    "os"
    "os/exec"
    "strconv"
    "sync"
    "syscall"

    "github.com/docker/containerd/osutils"
)

func main() {
    if err := run(); err != nil {
        log.Fatal(err)
    }
}

func run() error {
    if err := osutils.SetSubreaper(1); err != nil {
        return err
    }
    if err := os.Chdir("/containers/redis"); err != nil {
        return err
    }
    if err := runc("create", "--pid-file", "/tmp/mypid", "test"); err != nil {
        return err
    }
    data, err := ioutil.ReadFile("/tmp/mypid")
    if err != nil {
        return err
    }
    pid, err := strconv.Atoi(string(data))
    if err != nil {
        return err
    }
    p, err := os.FindProcess(pid)
    if err != nil {
        return err
    }

    wg := &sync.WaitGroup{}
    wg.Add(1)
    go func() {
        defer wg.Done()
        s, err := p.Wait()
        if err != nil {
            log.Fatal(err)
        }
        log.Printf("process exited with %d\n", s.Sys().(syscall.WaitStatus).ExitStatus())
        runc("delete", "test")
    }()
    if err := runc("start", "test"); err != nil {
        return err
    }
    wg.Wait()
    return nil
}

func runc(args ...string) error {
    cmd := exec.Command("runc", args...)
    cmd.Stdin = os.Stdin
    cmd.Stdout = os.Stdout
    cmd.Stderr = os.Stderr
    return cmd.Run()
}
@crosbymichael

crosbymichael May 17, 2016

Member
package main

import (
    "io/ioutil"
    "log"
    "os"
    "os/exec"
    "strconv"
    "sync"
    "syscall"

    "github.com/docker/containerd/osutils"
)

func main() {
    if err := run(); err != nil {
        log.Fatal(err)
    }
}

func run() error {
    if err := osutils.SetSubreaper(1); err != nil {
        return err
    }
    if err := os.Chdir("/containers/redis"); err != nil {
        return err
    }
    if err := runc("create", "--pid-file", "/tmp/mypid", "test"); err != nil {
        return err
    }
    data, err := ioutil.ReadFile("/tmp/mypid")
    if err != nil {
        return err
    }
    pid, err := strconv.Atoi(string(data))
    if err != nil {
        return err
    }
    p, err := os.FindProcess(pid)
    if err != nil {
        return err
    }

    wg := &sync.WaitGroup{}
    wg.Add(1)
    go func() {
        defer wg.Done()
        s, err := p.Wait()
        if err != nil {
            log.Fatal(err)
        }
        log.Printf("process exited with %d\n", s.Sys().(syscall.WaitStatus).ExitStatus())
        runc("delete", "test")
    }()
    if err := runc("start", "test"); err != nil {
        return err
    }
    wg.Wait()
    return nil
}

func runc(args ...string) error {
    cmd := exec.Command("runc", args...)
    cmd.Stdin = os.Stdin
    cmd.Stdout = os.Stdout
    cmd.Stderr = os.Stderr
    return cmd.Run()
}

This comment has been minimized.

@wking

wking May 17, 2016

Contributor

On Tue, May 17, 2016 at 01:30:08PM -0700, W. Trevor King wrote:

wait(1p) is a fairly convenient way to do that from the shell…

Actually, this doesn't work:

$ (sh -c 'sleep 60 &') &
$ ps -u wking | grep sleep | grep -v grep
22881 pts/13 00:00:00 sleep
$ wait 22881
bash: wait: pid 22881 is not a child of this shell

And waitpid(2) is talking about child processes, not ancestor
processes. So I'm back to wondering how you collect the container
exit status when the ‘create’ process exits early.

@wking

wking May 17, 2016

Contributor

On Tue, May 17, 2016 at 01:30:08PM -0700, W. Trevor King wrote:

wait(1p) is a fairly convenient way to do that from the shell…

Actually, this doesn't work:

$ (sh -c 'sleep 60 &') &
$ ps -u wking | grep sleep | grep -v grep
22881 pts/13 00:00:00 sleep
$ wait 22881
bash: wait: pid 22881 is not a child of this shell

And waitpid(2) is talking about child processes, not ancestor
processes. So I'm back to wondering how you collect the container
exit status when the ‘create’ process exits early.

This comment has been minimized.

@wking

wking May 17, 2016

Contributor

On Tue, May 17, 2016 at 01:36:38PM -0700, Michael Crosby wrote:

if err := osutils.SetSubreaper(1); err != nil {

This is fairly low-level for “I'd like to collect the container's exit
code”. I think callers should at least have the option of having the
‘create’ process stay attached through container death.

That also allows the create process to do psuedo-terminal copying 1,
although the OCI spec around process.terminal is currently not very
clear to me 2.

 Subject: More detail for process.terminal?
 Date: Thu, 14 Jan 2016 14:44:08 -0800
 Message-ID: <20160114224408.GM6362@odin.tremily.us>
@wking

wking May 17, 2016

Contributor

On Tue, May 17, 2016 at 01:36:38PM -0700, Michael Crosby wrote:

if err := osutils.SetSubreaper(1); err != nil {

This is fairly low-level for “I'd like to collect the container's exit
code”. I think callers should at least have the option of having the
‘create’ process stay attached through container death.

That also allows the create process to do psuedo-terminal copying 1,
although the OCI spec around process.terminal is currently not very
clear to me 2.

 Subject: More detail for process.terminal?
 Date: Thu, 14 Jan 2016 14:44:08 -0800
 Message-ID: <20160114224408.GM6362@odin.tremily.us>

@crosbymichael crosbymichael added this to the 0.2.0 milestone May 17, 2016

@hqhq

This comment has been minimized.

Show comment
Hide comment
@hqhq

hqhq May 20, 2016

Contributor

So if host rebooted, all created containers would be gone? Is that acceptable?

Contributor

hqhq commented May 20, 2016

So if host rebooted, all created containers would be gone? Is that acceptable?

@wking

This comment has been minimized.

Show comment
Hide comment
@wking

wking May 20, 2016

Contributor

On Fri, May 20, 2016 at 01:54:26AM -0700, Qiang Huang wrote:

So if host rebooted, all created containers would be gone? Is that
acceptable?

That's fine for me (it's how all my other processes work ;). If you
want to restore a container, have an init system set it up on boot
(possibly using checkpoint/restore, depending on how much you want to
preserve). But that all seems like it's out of scope for the runtime
layer.

Contributor

wking commented May 20, 2016

On Fri, May 20, 2016 at 01:54:26AM -0700, Qiang Huang wrote:

So if host rebooted, all created containers would be gone? Is that
acceptable?

That's fine for me (it's how all my other processes work ;). If you
want to restore a container, have an init system set it up on boot
(possibly using checkpoint/restore, depending on how much you want to
preserve). But that all seems like it's out of scope for the runtime
layer.

@mrunalp

This comment has been minimized.

Show comment
Hide comment
@mrunalp

mrunalp May 20, 2016

Contributor

@hqhq I think that is okay as this isn't the same as docker create. As long as we are clear in the spec, it should be fine.

Contributor

mrunalp commented May 20, 2016

@hqhq I think that is okay as this isn't the same as docker create. As long as we are clear in the spec, it should be fine.

@vishh

This comment has been minimized.

Show comment
Hide comment
@vishh

vishh May 20, 2016

Contributor

One of the use-cases for hooks was to customize mounts on demand. Would it be possible to not pivot root as part of create and switch root only on start?

Contributor

vishh commented May 20, 2016

One of the use-cases for hooks was to customize mounts on demand. Would it be possible to not pivot root as part of create and switch root only on start?

@wking

This comment has been minimized.

Show comment
Hide comment
@wking

wking May 20, 2016

Contributor

On Fri, May 20, 2016 at 03:31:07PM -0700, Vish Kannan wrote:

Would it be possible to not pivot root as part of create and switch
root only on start?

I don't think that's a good idea, because you may be using the
container process to hold open a namespace. In that case, there's no
reason to call ‘start’, but you still want the pivoted root in the
mount namespace.

I'm still not clear on why the pre-pivot mounts need to be dynamic,
instead of setting them up in config.json's mounts during pre-create
processing.

Contributor

wking commented May 20, 2016

On Fri, May 20, 2016 at 03:31:07PM -0700, Vish Kannan wrote:

Would it be possible to not pivot root as part of create and switch
root only on start?

I don't think that's a good idea, because you may be using the
container process to hold open a namespace. In that case, there's no
reason to call ‘start’, but you still want the pivoted root in the
mount namespace.

I'm still not clear on why the pre-pivot mounts need to be dynamic,
instead of setting them up in config.json's mounts during pre-create
processing.

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael May 24, 2016

Member

FYI

This change makes us require go 1.6 because previous versions of go would not let you handle SIGCONT, it just ignores it and blocks forever.

Member

crosbymichael commented May 24, 2016

FYI

This change makes us require go 1.6 because previous versions of go would not let you handle SIGCONT, it just ignores it and blocks forever.

Device: "mqueue",
Flags: defaultMountFlags,
},
/*

This comment has been minimized.

@mrunalp

mrunalp May 24, 2016

Contributor

Was this temporarily commented out?

@mrunalp

mrunalp May 24, 2016

Contributor

Was this temporarily commented out?

This comment has been minimized.

@crosbymichael
@crosbymichael

crosbymichael May 25, 2016

Member

bug

This comment has been minimized.

@crosbymichael

crosbymichael May 27, 2016

Member

This is from the issue of mqueue not working on debian kernels on the CI in userns

@crosbymichael

crosbymichael May 27, 2016

Member

This is from the issue of mqueue not working on debian kernels on the CI in userns

Show outdated Hide outdated libcontainer/container.go Outdated
Show outdated Hide outdated libcontainer/container_linux.go Outdated
@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 May 25, 2016

Contributor

I see that all test were changed so they use old version. Maybe couple of tests for create/start are needed.

Contributor

LK4D4 commented May 25, 2016

I see that all test were changed so they use old version. Maybe couple of tests for create/start are needed.

@duglin

This comment has been minimized.

Show comment
Hide comment
@duglin

duglin May 25, 2016

Contributor

would it be bad if we had the console flag default to /dev/pts/ptmx for runc create? From a UX perspective it would be easier to do that than to force all users to remember to supply it. W/o some value for that flag I think runc fails anyway.

Contributor

duglin commented May 25, 2016

would it be bad if we had the console flag default to /dev/pts/ptmx for runc create? From a UX perspective it would be easier to do that than to force all users to remember to supply it. W/o some value for that flag I think runc fails anyway.

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael May 25, 2016

Member

@duglin No, because not all containers use a TTY.

Member

crosbymichael commented May 25, 2016

@duglin No, because not all containers use a TTY.

@duglin

This comment has been minimized.

Show comment
Hide comment
@duglin

duglin May 25, 2016

Contributor

@crosbymichael how do I do a runc create w/o a --console flag? I can't seem to get it to not error out.

Contributor

duglin commented May 25, 2016

@crosbymichael how do I do a runc create w/o a --console flag? I can't seem to get it to not error out.

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael May 25, 2016

Member

@duglin does your spec have terminal true?

Member

crosbymichael commented May 25, 2016

@duglin does your spec have terminal true?

@duglin

This comment has been minimized.

Show comment
Hide comment
@duglin

duglin May 25, 2016

Contributor

ah that was it - thanks

Contributor

duglin commented May 25, 2016

ah that was it - thanks

@duglin

This comment has been minimized.

Show comment
Hide comment
@duglin

duglin May 25, 2016

Contributor

:-) we could still have it default to that value when terminal is true

Contributor

duglin commented May 25, 2016

:-) we could still have it default to that value when terminal is true

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 May 25, 2016

Contributor

@crosbymichael I see timeout on CI and somehow it hangs now :/

Contributor

LK4D4 commented May 25, 2016

@crosbymichael I see timeout on CI and somehow it hangs now :/

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael May 25, 2016

Member

@LK4D4 @mrunalp hum, it looks like having mqueue in test template causes it to lockup for some reason

Member

crosbymichael commented May 25, 2016

@LK4D4 @mrunalp hum, it looks like having mqueue in test template causes it to lockup for some reason

@mrunalp

This comment has been minimized.

Show comment
Hide comment
@mrunalp

mrunalp May 25, 2016

Contributor

@crosbymichael Wonder if it is distro specific. I'll try running on Fedora and report back.

Contributor

mrunalp commented May 25, 2016

@crosbymichael Wonder if it is distro specific. I'll try running on Fedora and report back.

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael May 25, 2016

Member

@mrunalp could be, i'm having trouble running on my ubuntu system also, but i'm getting a compile error inthe docker container:

go build -i -ldflags "-X main.gitCommit=1f7eef6c5b414823c98125ec2b911dfd1466f337" -tags "seccomp" -o runc .
# _/go/src/github.com/opencontainers/runc
/usr/local/go/pkg/tool/linux_amd64/link: running gcc failed: exit status 1
/usr/bin/ld: /tmp/go-link-165825633/000000.o: unrecognized relocation (0x2a) in section `.text'
/usr/bin/ld: final link failed: Bad value
collect2: error: ld returned 1 exit status

Makefile:22: recipe for target 'all' failed
make: *** [all] Error 2
Makefile:53: recipe for target 'unittest' failed
make[1]: *** [unittest] Error 2
make[1]: Leaving directory '/home/michael/development/gocode/src/github.com/opencontainers/runc'
Makefile:47: recipe for target 'test' failed
make: *** [test] Error 2

WTH?????

Member

crosbymichael commented May 25, 2016

@mrunalp could be, i'm having trouble running on my ubuntu system also, but i'm getting a compile error inthe docker container:

go build -i -ldflags "-X main.gitCommit=1f7eef6c5b414823c98125ec2b911dfd1466f337" -tags "seccomp" -o runc .
# _/go/src/github.com/opencontainers/runc
/usr/local/go/pkg/tool/linux_amd64/link: running gcc failed: exit status 1
/usr/bin/ld: /tmp/go-link-165825633/000000.o: unrecognized relocation (0x2a) in section `.text'
/usr/bin/ld: final link failed: Bad value
collect2: error: ld returned 1 exit status

Makefile:22: recipe for target 'all' failed
make: *** [all] Error 2
Makefile:53: recipe for target 'unittest' failed
make[1]: *** [unittest] Error 2
make[1]: Leaving directory '/home/michael/development/gocode/src/github.com/opencontainers/runc'
Makefile:47: recipe for target 'test' failed
make: *** [test] Error 2

WTH?????

@mrunalp

This comment has been minimized.

Show comment
Hide comment
@mrunalp

mrunalp May 25, 2016

Contributor

I see these errors on Fedora with or without mqueue.

# (in test file tests/integration/exec.bats, line 22)
#   `[ "$status" -eq 0 ]' failed
not ok 14 runc exec --pid-file
# (in test file tests/integration/exec.bats, line 35)
#   `[ "$status" -eq 0 ]' failed
Contributor

mrunalp commented May 25, 2016

I see these errors on Fedora with or without mqueue.

# (in test file tests/integration/exec.bats, line 22)
#   `[ "$status" -eq 0 ]' failed
not ok 14 runc exec --pid-file
# (in test file tests/integration/exec.bats, line 35)
#   `[ "$status" -eq 0 ]' failed
@mrunalp

This comment has been minimized.

Show comment
Hide comment
@mrunalp

mrunalp Jun 3, 2016

Contributor

Tested. LGTM.

Contributor

mrunalp commented Jun 3, 2016

Tested. LGTM.

@hqhq

This comment has been minimized.

Show comment
Hide comment
@hqhq

hqhq Jun 3, 2016

Contributor

LGTM

Approved with PullApprove

Contributor

hqhq commented Jun 3, 2016

LGTM

Approved with PullApprove

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jun 3, 2016

Member

One of the biggest issues I can see right off the bat is that runc create doesn't work for user namespaced containers because of #814 (you can't set any console path). While I'm okay with that being the case (for the moment while I work on fixing #814 -- which should be considered as blocking 1.0), we should definitely make it an explicit error rather than "here's some cryptic error message" (we can remove the message once we fix the console bug).

As an aside, it's quite annoying that you can't even specify --console /dev/null ... We need to fix this.

/cc @crosbymichael

Member

cyphar commented Jun 3, 2016

One of the biggest issues I can see right off the bat is that runc create doesn't work for user namespaced containers because of #814 (you can't set any console path). While I'm okay with that being the case (for the moment while I work on fixing #814 -- which should be considered as blocking 1.0), we should definitely make it an explicit error rather than "here's some cryptic error message" (we can remove the message once we fix the console bug).

As an aside, it's quite annoying that you can't even specify --console /dev/null ... We need to fix this.

/cc @crosbymichael

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Jun 3, 2016

Member

@cyphar that is unrelated to this PR.

I really don't think you all understand what --console does or is for if you are trying to pass it dev/null. I'll show you want to do in another PR.

Member

crosbymichael commented Jun 3, 2016

@cyphar that is unrelated to this PR.

I really don't think you all understand what --console does or is for if you are trying to pass it dev/null. I'll show you want to do in another PR.

@crosbymichael crosbymichael merged commit c5060ff into opencontainers:master Jun 3, 2016

3 checks passed

code-review/pullapprove Approved by hqhq, LK4D4
Details
docker/dco-signed All commits signed
Details
janky Jenkins build runc-PRs 1758 has succeeded
Details

@crosbymichael crosbymichael deleted the crosbymichael:create-start branch Jun 3, 2016

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jun 6, 2016

Member

@crosbymichael explained on IRC that --console doesn't take a master, it takes a slave path and does no setup (which makes sense if you read the code). So the complaints about --console are not accurate, and our integration tests usage of --console is incorrect.

Member

cyphar commented Jun 6, 2016

@crosbymichael explained on IRC that --console doesn't take a master, it takes a slave path and does no setup (which makes sense if you read the code). So the complaints about --console are not accurate, and our integration tests usage of --console is incorrect.

wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Feb 3, 2017

runtime: Add 'exit' to state for collecting the container exit code
This gives us a more portable way to discover the container exit code
(vs. requiring callers to use subreapers [1] or other
platform-specific approaches which require knowledge of the runtime
implementation).

[1]: opencontainers/runc#827 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>

wking added a commit to wking/oci-command-line-api that referenced this pull request Feb 9, 2017

runtime: Move start's --id to a positional parameter
I added this as an option in 5033c59 (Add an --id option to 'start',
2015-09-15), because some callers might want to leave ID generation to
the runtime.  When there is a long-running host process waiting on the
container process to perform cleanup, the runtime-caller may not need
to know the container ID.  However, runC has been requiring a
user-specified ID since [1], and the coming create/start split will
follow the early-exit 'create' from [2], so require an ID here.  We
can revisit this if we regain a long-running 'create' process.

You can create a config that adds no isolation vs. the runtime
namespace or completely joins another set of existing namespaces.  It
seems odd to call that a new "container", but the ID is really more of
a process ID, and less of a container ID.  The "container" phrasing is
just a useful hint that there might be some isolation going on.  And
we're always creating a new "container process" with 'start' (which
will become 'create').

[1]: opencontainers/runc#541
     opencontainers/runc@a7278ca (Require container id as arg1,
     2016-02-08, opencontainers/runc#541)
[2]: opencontainers/runc#827
     Summary: Implement create and start

Signed-off-by: W. Trevor King <wking@tremily.us>

wking added a commit to wking/oci-command-line-api that referenced this pull request Feb 9, 2017

runtime: Split 'create' and 'start' (and add 'delete')
Catch up with opencontainers/runtime-spec@be59415 (Split create and
start, 2016-04-01, opencontainers/runtime-spec#384).

One benefit of the early-exit 'create' is that the exit code does not
conflate container process exits with "failed to setup the sandbox"
exits.  We can take advantage of that and use non-zero 'create' exits
to allow stderr writing (so the runtime can log errors while dying
without having to successfully connect to syslog or some such).

I still likes the long-running 'create' API because it makes
collecting the exit code easier.  I've proposed an 'event' operation
[1] which would provide a convenient created trigger.  With 'event' in
place, we don't need the 'create' process exit to serve as that
trigger, and could have a long-running 'create' that collects the
container process exit code using the portable waitid() family.  But
the consensus after the 2016-07-13 meeting was to table that while we
land docs for the runC API [2], and runC has an early-exit create [3].

The "Callers MAY block..." wording is going to be hard to enforce, but
with the runC model, clients rely on the command exits to trigger
post-create and post-start activity.  The longer the runtime hangs
around after completing its action, the laggier those triggers will
be.

The "MUST NOT attempt to read from its stdin" means a generic caller
can safely exec the command with a closed or null stdin, and not have
to worry about the command blocking or crashing because of that.  The
stdout spec for start/delete is more lenient, because runtimes are
unlikely to change their behavior because they are unable to write to
stdout.  If this assumption proves troublesome, we may have to tighten
it up later.

The ptrace idea in this commit is from Mrunal [4].

[1]: opencontainers/runtime-spec#508
     Subject: runtime: Add an 'event' operation for subscribing to pushes
[2]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-07-13-17.03.log.html#l-15
[3]: opencontainers/runc#827
     Summary: Implement create and start
[4]: http://ircbot.wl.linuxfoundation.org/eavesdrop/%23opencontainers/%23opencontainers.2016-07-13.log.html#t2016-07-13T18:58:54

Signed-off-by: W. Trevor King <wking@tremily.us>

wking added a commit to wking/oci-command-line-api that referenced this pull request Feb 9, 2017

runtime: Move start's --id to a positional parameter
I added this as an option in 5033c59 (Add an --id option to 'start',
2015-09-15), because some callers might want to leave ID generation to
the runtime.  When there is a long-running host process waiting on the
container process to perform cleanup, the runtime-caller may not need
to know the container ID.  However, runC has been requiring a
user-specified ID since [1], and the coming create/start split will
follow the early-exit 'create' from [2], so require an ID here.  We
can revisit this if we regain a long-running 'create' process.

You can create a config that adds no isolation vs. the runtime
namespace or completely joins another set of existing namespaces.  It
seems odd to call that a new "container", but the ID is really more of
a process ID, and less of a container ID.  The "container" phrasing is
just a useful hint that there might be some isolation going on.  And
we're always creating a new "container process" with 'start' (which
will become 'create').

[1]: opencontainers/runc#541
     opencontainers/runc@a7278ca (Require container id as arg1,
     2016-02-08, opencontainers/runc#541)
[2]: opencontainers/runc#827
     Summary: Implement create and start

Signed-off-by: W. Trevor King <wking@tremily.us>

wking added a commit to wking/oci-command-line-api that referenced this pull request Feb 9, 2017

runtime: Split 'create' and 'start' (and add 'delete')
Catch up with opencontainers/runtime-spec@be59415 (Split create and
start, 2016-04-01, opencontainers/runtime-spec#384).

One benefit of the early-exit 'create' is that the exit code does not
conflate container process exits with "failed to setup the sandbox"
exits.  We can take advantage of that and use non-zero 'create' exits
to allow stderr writing (so the runtime can log errors while dying
without having to successfully connect to syslog or some such).

I still likes the long-running 'create' API because it makes
collecting the exit code easier.  I've proposed an 'event' operation
[1] which would provide a convenient created trigger.  With 'event' in
place, we don't need the 'create' process exit to serve as that
trigger, and could have a long-running 'create' that collects the
container process exit code using the portable waitid() family.  But
the consensus after the 2016-07-13 meeting was to table that while we
land docs for the runC API [2], and runC has an early-exit create [3].

The "Callers MAY block..." wording is going to be hard to enforce, but
with the runC model, clients rely on the command exits to trigger
post-create and post-start activity.  The longer the runtime hangs
around after completing its action, the laggier those triggers will
be.

The "MUST NOT attempt to read from its stdin" means a generic caller
can safely exec the command with a closed or null stdin, and not have
to worry about the command blocking or crashing because of that.  The
stdout spec for start/delete is more lenient, because runtimes are
unlikely to change their behavior because they are unable to write to
stdout.  If this assumption proves troublesome, we may have to tighten
it up later.

The ptrace idea in this commit is from Mrunal [4].

[1]: opencontainers/runtime-spec#508
     Subject: runtime: Add an 'event' operation for subscribing to pushes
[2]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-07-13-17.03.log.html#l-15
[3]: opencontainers/runc#827
     Summary: Implement create and start
[4]: http://ircbot.wl.linuxfoundation.org/eavesdrop/%23opencontainers/%23opencontainers.2016-07-13.log.html#t2016-07-13T18:58:54

Signed-off-by: W. Trevor King <wking@tremily.us>

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

Merge pull request opencontainers#827 from wking/implicit-link-name-s…
…hortcut

config-linux: Use the implicit link name shortcut

wking added a commit to wking/ocitools-v2 that referenced this pull request Jan 11, 2018

validation/util/container: Use --bundle (and stop requiring BundleDir)
Address a previous TODO.  And now that we are using --bundle, we no
longer need to set cmd.Dir.  The TODO mentions a lack of runc support,
but runc supports --bundle since opencontainers/runc@3fe7d7f (Add
create and start command for container lifecycle, 2016-05-13,
opencontainers/runc#827).

wking added a commit to wking/ocitools-v2 that referenced this pull request Jan 11, 2018

validation/util/container: Use --bundle (and stop requiring BundleDir)
Address a previous TODO.  And now that we are using --bundle, we no
longer need to set cmd.Dir.  The TODO mentions a lack of runc support,
but runc supports --bundle since opencontainers/runc@3fe7d7f (Add
create and start command for container lifecycle, 2016-05-13,
opencontainers/runc#827).

Signed-off-by: W. Trevor King <wking@tremily.us>

wking added a commit to wking/ocitools-v2 that referenced this pull request Jan 18, 2018

validation/util/container: Use --bundle (and stop requiring BundleDir)
Address a previous TODO.  And now that we are using --bundle, we no
longer need to set cmd.Dir.  The TODO mentions a lack of runc support,
but runc supports --bundle since opencontainers/runc@3fe7d7f (Add
create and start command for container lifecycle, 2016-05-13,
opencontainers/runc#827).

Signed-off-by: W. Trevor King <wking@tremily.us>

wking added a commit to wking/runc that referenced this pull request Feb 20, 2018

libcontainer/standard_init_linux: Move LookPath to post-wait
This avoids a panic for containers that do not set Process.  And even
if Process was set, there is no reason to require the executable to be
available *at create time* [1].  Subsequent activity could be
scheduled to get a binary in place at the configured location before
'start' is called.

[1]: opencontainers#827 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment