Use fifo for create / start instead of signal handling #886

crosbymichael · 2016-06-06T23:40:20Z

This removes the use of a signal handler and SIGCONT to signal the init
process to exec the users process.

Closes #871

wking · 2016-06-06T23:50:50Z

libcontainer/factory_linux.go

@@ -168,6 +168,9 @@ func (l *LinuxFactory) Create(id string, config *configs.Config) (Container, err
 	if err := os.MkdirAll(containerRoot, 0700); err != nil {
 		return nil, newGenericError(err, SystemError)
 	}
+	if err := syscall.Mkfifo(filepath.Join(containerRoot, execFifoFilename), 0655); err != nil {


0655 is a pretty strange permission set. I'm not clear on the intended auth here, but the signal-based approach was limited by kill(2)'s CAP_KILL and/or UID requirements (although SIGCONT had a same-session special case). I expect you want 0600 here to match that (ish ;).

LK4D4 · 2016-06-06T23:53:54Z

libcontainer/standard_init_linux.go

+	// use channel 'c' to ensure that the goroutine was scheduled so that the open is called
+	// before we pivot into the rootfs or else we won't have access to the path on the host.
+	go func() {
+		close(goStarted)


Is this really help? Because scheduler still can reschedule on OpenFile call

It gets us closer than without

Yeah, it's like 30% less race chance, but still... It's really bad.

So, this code is definitely racy :( Maybe we can create fifo in rootfs?

could we just call the raw syscall or still an issue?

i don't think putting it in the rootfs will help because of readonly containers and such. we would want to delete it after we are done and could'nt

If there is no race free way to make this work, we could just use either SIGUSR1/SIGUSR2 signals that are reserved for user code.

I fixed it in a race free way

8767396

;)

cyphar · 2016-06-07T05:05:30Z

Personally, I liked the signal approach to create-start. What is the reason for the change? While SIGCONT has issues with Go 1.6, I'm not sure why we aren't just switching to SIGUSR1 -- IMO signals are the right Unix interface here (not FIFOs).

While I understand @wking's concerns with PID namespaces, it feels like we still have several other problems that we can't solve this way (if we used sockets) -- why should runc start work if runc ps or runc exec won't (since the PID is hardcoded into the namespace_paths)?

hqhq · 2016-06-07T12:16:53Z

libcontainer/standard_init_linux.go

-	// clean up the signal handler
-	signal.Stop(s)
-	close(s)
+	// wait for the fifo be be opened on the other side before


typo "be be"

wking · 2016-06-07T12:36:04Z

On Mon, Jun 06, 2016 at 10:05:32PM -0700, Aleksa Sarai wrote:

Personally, I liked the signal approach to create-start. What is the
reason for the change? While SIGCONT has issues with Go 1.6, I'm
not sure why we aren't just switching to SIGUSR1 -- IMO signals
are the right Unix interface here (not FIFOs).

The problem with signals is that you're just launching them, and don't
know:

If/when they are recieved and acted upon, or
Which code will be running when the process recieves the signal 1.

I'm not entirely sure how much of that you fix with a FIFO, but the
ccon implementation [2](which uses a two-way Unix socket for this)
can (but doesn't actually implement yet) have the container process
send a “message received, I'll go exec that code” message back to the
‘start’ process. That way:

The ‘start’ process that actually triggers the exec knows it, and
can exit success. Any parallel ‘start’ requests who are queued up
waiting for the socket will know that they didn't trigger the exec,
and can exit failure (as required by the spec 3).
You know you'll never racily send a signal that is processed by
user-specified code, because the container process should be closing
(and in ccon, the host-process will remove post-post start, once I
have time to code that up, the filesystem portion of the
socket/FIFO).

“Attempting to start a container that does not exist MUST
generate an error. Attempting to start an already started
container MUST have no effect on the container and MUST generate
an error.”

crosbymichael · 2016-06-07T21:20:12Z

@LK4D4 @cyphar i updated and now its "race free' ;)

wking · 2016-06-07T21:22:35Z

On Mon, Jun 06, 2016 at 10:05:32PM -0700, Aleksa Sarai wrote:

While I understand @wking's concerns with PID namespaces, it feels
like we still have several other problems that we can't solve this
way (if we used sockets) -- why should runc start work if runc ps or
runc exec won't (since the PID is hardcoded into the
namespace_paths)?

I don't see “we have out-of-spec operations that break under these
conditions” as a reason to avoid having more reliable support for
in-spec operations (although in this case, my main concern with
signals is the race which may lead to user-specified code handling a
signal intended for the runtime's idler code 1).

And while using a filesystem object (in this cases a named pipe) gets
you out of the “I'm in a different PID namespace” issue 2, it still
has “I'm in a different mount namespace and can't access runC's state
registry”. I don't see a way around that, although I do wish we had
better wording about this issue than the spec's current [3](e.g. [4]).

 Subject: Linux: Adding the PID namespace inode to state JSON
 Date: Wed, 30 Dec 2015 21:34:57 -0800
 Message-ID: <20151231053457.GG3680@odin.tremily.us>

crosbymichael · 2016-06-10T18:30:59Z

ping,

I've been running stress tests and everything looks good with this change

dqminh · 2016-06-13T13:27:13Z

LGTM

LK4D4 · 2016-06-13T16:55:05Z

@crosbymichael LGTM, but need rebase. Feel free to merge yourself after it.

mrunalp · 2016-06-13T17:15:57Z

I am reviewing/testing this.

crosbymichael · 2016-06-13T17:25:48Z

rebased

mrunalp · 2016-06-13T18:04:32Z

libcontainer/container_linux.go

 	}
 	return nil
 }

+func (c *linuxContainer) Exec() error {
+	path := filepath.Join(c.root, execFifoFilename)
+	f, err := os.OpenFile(path, os.O_RDONLY, 0)


If two processes end up opening this file at the same time, then one of them will be stuck waiting on the ReadAll below.

True but we don't support multiple processes handling the container. I can try to protect the libcontainer call with a lock but its not going to help out of process calls.

On Mon, Jun 13, 2016 at 11:12:02AM -0700, Michael Crosby wrote:

} return nil

}

+func (c *linuxContainer) Exec() error {

path := filepath.Join(c.root, execFifoFilename)

f, err := os.OpenFile(path, os.O_RDONLY, 0)

True but we don't support multiple processes handling the container.
I can try to protect the libcontainer call with a lock but its not
going to help out of process calls.

I thought putting a byte in the pipe would help with this 1. Could
we make the reads unblocking? If you got the byte, success. If you
didn't get the byte, exit 1.

@crosbymichael I agree that with current usage this should be fine. We could add flocks in the future if necessary.

mrunalp · 2016-06-13T18:05:36Z

libcontainer/standard_init_linux.go

@@ -21,6 +20,7 @@ import (
 type linuxStandardInit struct {
 	pipe      io.ReadWriteCloser
 	parentPid int
+	rootfd    int


nit: Can we call this statedirfd? rootfd seems to imply that it is a fd to the rootfs.

cyphar · 2016-06-13T18:20:56Z

I still want to take some time to review this. Unfortunately I have exams until Friday. Can we wait on merging until next week?

crosbymichael · 2016-06-13T18:22:23Z

@cyphar we need this to fix the racyness and 1.6 issues that we currently have with using signals

cyphar · 2016-06-13T18:23:06Z

Alright. I'll take a look at this after merging, and make follow-up PRs if necessary.

mrunalp · 2016-06-13T18:26:07Z

LGTM

crosbymichael · 2016-06-13T18:26:22Z

@mrunalp updating your nits and added a lock to that function

mrunalp · 2016-06-13T18:26:40Z

@crosbymichael Thanks! I'll wait for that before merging.

This removes the use of a signal handler and SIGCONT to signal the init process to exec the users process. Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

crosbymichael · 2016-06-13T18:27:05Z

updated

mrunalp · 2016-06-13T18:34:52Z

We have some more references to root instead of state but some of it predates this PR so we can fixup in a follow on.

mrunalp · 2016-06-13T18:34:58Z

LGTM

mrunalp · 2016-06-13T18:35:26Z

@LK4D4 this needs another LGTM as the PR was updated :)

LK4D4 · 2016-06-13T18:36:09Z

@mrunalp CI is not ready too :)

LK4D4 · 2016-06-13T19:36:33Z

LGTM

config-schema: Optimize code

GordonTheTurtle added the status/0-triage label Jun 6, 2016

wking reviewed Jun 6, 2016
View reviewed changes

crosbymichael force-pushed the start-pipe branch from f6952a4 to fddbd97 Compare June 6, 2016 23:53

LK4D4 reviewed Jun 6, 2016
View reviewed changes

cyphar mentioned this pull request Jun 7, 2016

start: use SIGUSR1 for start signal #888

Closed

hqhq reviewed Jun 7, 2016
View reviewed changes

crosbymichael force-pushed the start-pipe branch 5 times, most recently from 70cfec3 to ae91b58 Compare June 7, 2016 18:41

cyphar mentioned this pull request Jun 8, 2016

Cannot start container using instructions from README on latest build #884

Closed

crosbymichael force-pushed the start-pipe branch from ae91b58 to d038207 Compare June 9, 2016 17:41

crosbymichael mentioned this pull request Jun 10, 2016

integration: Fix race in TestFreeze #901

Closed

crosbymichael force-pushed the start-pipe branch from d038207 to 483a68a Compare June 13, 2016 17:25

mrunalp reviewed Jun 13, 2016
View reviewed changes

Use fifo for create/start

3aacff6

This removes the use of a signal handler and SIGCONT to signal the init process to exec the users process. Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

crosbymichael force-pushed the start-pipe branch from 483a68a to 3aacff6 Compare June 13, 2016 18:27

LK4D4 merged commit 85873d9 into opencontainers:master Jun 13, 2016

crosbymichael deleted the start-pipe branch June 13, 2016 19:41

wking mentioned this pull request Jun 13, 2016

state's status detection (based on PID lookups) is imprecise #871

Closed

papey mentioned this pull request Jun 14, 2016

runc-rc1, new workflow and terminal=true containers #910

Closed

cyphar mentioned this pull request Jun 17, 2016

factory: fix FIFO permissions #920

Closed

wking mentioned this pull request Jan 10, 2017

config: Make 'process.args' optional opencontainers/runtime-spec#620

Closed

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

Merge pull request opencontainers#886 from q384566678/config-shchema-fix

63af112

config-schema: Optimize code

Use fifo for create / start instead of signal handling #886

Use fifo for create / start instead of signal handling #886

Conversation

crosbymichael commented Jun 6, 2016 • edited Loading

wking Jun 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar commented Jun 7, 2016 • edited Loading

Choose a reason for hiding this comment

wking commented Jun 7, 2016

crosbymichael commented Jun 7, 2016

wking commented Jun 7, 2016

crosbymichael commented Jun 10, 2016

dqminh commented Jun 13, 2016 • edited by caniszczyk Loading

LK4D4 commented Jun 13, 2016

mrunalp commented Jun 13, 2016

crosbymichael commented Jun 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar commented Jun 13, 2016 • edited Loading

crosbymichael commented Jun 13, 2016

cyphar commented Jun 13, 2016

mrunalp commented Jun 13, 2016 • edited by caniszczyk Loading

crosbymichael commented Jun 13, 2016

mrunalp commented Jun 13, 2016

crosbymichael commented Jun 13, 2016

mrunalp commented Jun 13, 2016

mrunalp commented Jun 13, 2016 • edited by caniszczyk Loading

mrunalp commented Jun 13, 2016

LK4D4 commented Jun 13, 2016

LK4D4 commented Jun 13, 2016 • edited by caniszczyk Loading

crosbymichael commented Jun 6, 2016 •

edited

Loading

wking Jun 6, 2016 •

edited

Loading

cyphar commented Jun 7, 2016 •

edited

Loading

dqminh commented Jun 13, 2016 •

edited by caniszczyk

Loading

cyphar commented Jun 13, 2016 •

edited

Loading

mrunalp commented Jun 13, 2016 •

edited by caniszczyk

Loading

mrunalp commented Jun 13, 2016 •

edited by caniszczyk

Loading

LK4D4 commented Jun 13, 2016 •

edited by caniszczyk

Loading