[meta-virtualization] [PATCH] runc-docker: Allow "run start ..." to daemonize with $SIGUSR1_PARENT_PID
Bruce Ashfield
bruce.ashfield at gmail.com
Sun Dec 10 20:31:57 PST 2017
On Sun, Dec 10, 2017 at 11:20 PM, Jason Wessel <jason.wessel at windriver.com>
wrote:
> On 12/10/2017 09:38 PM, Bruce Ashfield wrote:
>
>
>
> On Fri, Dec 8, 2017 at 10:53 AM, Jason Wessel <jason.wessel at windriver.com>
> wrote:
>
>> The runc-docker has all the code in it to properly run a stop hook if
>> you use it in the foreground. It doesn't work in the back ground
>> because there is no way for a golang application to fork a child exit
>> out of the parent process because all the golang threads stay with the
>> parent.
>>
>
> Can you define background for the commit log purposes ? Is it a container
> that allocates a tty and detaches from the current terminal ? Something
> else ?
>
>
>
> You are correct that it is ambiguous based on the terminal part.
>
>
> For the purpose of the background for this commit it is about interaction
> with a bash shell for any other typical process.
>
> When you use "runc run " it is running in the "foreground", in the sense
> it takes over your existing terminal.
>
> The runc-docker doesn't have a way to start it with "runc run&" where you
> can send it to the background and have everything work. With this commit,
> it does allow you to do that and have all the stop hooks fire at the time
> what ever runc started exits.
>
>
>
> Also why does the threads staying with the parent mean the stop hooks
> don't run. That isn't clear to me. Is it due to lack of notification if
> they exit ?
>
>
>
> We want the parent process to go away because that is how we daemonize.
> If the golang threads stay with the parent, after a fork no exit handlers
> run.
>
>
> Lets take a quick look at what "runc run" does today:
>
> * Starts a whole pile of threads
> * Sets up all name spaces
> * Starts child process for container and leaves it paused at image
> activation
> * runs start hooks
> * executes "continue" for continue process
> * waits for container app to exit
> * executes stop hooks
>
> Now lets look at "runc create/start"
> runc create
> * Starts a whole pile of threads
> * Sets up all name spaces
> * Starts child process for container and leaves it paused at image
> activation
> * exits ---- DOH, this is the guy we want to keep
> runc start
> * runs start hooks
> * executes "continue" for continue process
>
> At this point when the container app exists nothing was waiting for it.
>
>
> --- What happens with the patch? ---
>
> runc create
> * Starts a whole pile of threads
> * Sets up all name spaces
> * Starts child process for container and leaves it paused at image
> activation
> * closes stdin/stderr/stdout
> * Sends SIGUSR1 to wrapper process and the wrapper just exits leaving
> runc start behind.
> * waits for child process to go away
> * runs stop hooks
> runc start
> * runs start hooks
> * executes "continue" for continue process
>
> At this point when the container app exists nothing was waiting for it.
>
>
>
ahaha. This makes it much clearer to me (even the block of code below).
Can you put those steps in the commit log ? I'd like to keep them around
for reference
when I end up merging more patches in the similar area.
I can add this to my test queue in the morning and get it merged.
Bruce
>
>
>> This patch has three parts that happen ONLY when $SIGUSR1_PARENT_PID
>> is set.
>>
>
> Where do you see that environment variable being set ? In the config.json ?
> Somewhere else ? (i.e. the launching environment).
>
>
>
> This is only set by the launch wrapper.
>
> The launch wrapper will set the variable to the PID of itself because it
> wants the notification.
>
>
>
>
>
>>
>> 1) The code was copied which performs the normal the signal handling
>> block which is used for the foreground operation of runc.
>>
>> 2) At the point where runc start would normally exit, it closes
>> stdin/stdout/stderr so it would be possible to daemonize "runc start
>> ...".
>>
>
> I can't say I completely follow point #2. Maybe it is with me not having
> looked at what is required to daemonize for a loooong time.
>
> See one question on the patch below.
>
>
>> 3) The code to send a SIGUSR1 to the parent process was added. The
>> idea being that a parent process would simply exit at that point
>> because it was blocking until runc performed everything it was
>> required to perform.
>>
>> Signed-off-by: Jason Wessel <jason.wessel at windriver.com>
>> ---
>> .../0001-runc-docker-SIGUSR1-daemonize.patch | 131
>> +++++++++++++++++++++
>> recipes-containers/runc/runc-docker_git.bb | 1 +
>> 2 files changed, 132 insertions(+)
>> create mode 100644 recipes-containers/runc/runc-d
>> ocker/0001-runc-docker-SIGUSR1-daemonize.patch
>>
>> diff --git a/recipes-containers/runc/runc-docker/0001-runc-docker-SIGUSR1-daemonize.patch
>> b/recipes-containers/runc/runc-docker/0001-runc-docker-SIGUS
>> R1-daemonize.patch
>> new file mode 100644
>> index 0000000..b3bd068
>> --- /dev/null
>> +++ b/recipes-containers/runc/runc-docker/0001-runc-docker-SIGUS
>> R1-daemonize.patch
>> @@ -0,0 +1,131 @@
>> +From cd7d76a6d1ecb1856f6ed666fb5c30dc105aa94e Mon Sep 17 00:00:00 2001
>> +From: Jason Wessel <jason.wessel at windriver.com>
>> +Date: Tue, 5 Dec 2017 18:28:28 -0800
>> +Subject: [PATCH] runc-docker: Allow "run start ..." to daemonize with
>> $SIGUSR1_PARENT_PID
>> +
>> +The runc-docker has all the code in it to properly run a stop hook if
>> +you use it in the foreground. It doesn't work in the back ground
>> +because there is no way for a golang application to fork a child exit
>> +out of the parent process because all the golang threads stay with the
>> +parent.
>> +
>> +This patch has three parts that happen ONLY when $SIGUSR1_PARENT_PID
>> +is set.
>> +
>> +1) The code was copied which performs the normal the signal handling
>> + block which is used for the foreground operation of runc.
>> +
>> +2) At the point where runc start would normally exit, it closes
>> + stdin/stdout/stderr so it would be possible to daemonize "runc start
>> ...".
>> +
>> +3) The code to send a SIGUSR1 to the parent process was added. The
>> + idea being that a parent process would simply exit at that point
>> + because it was blocking until runc performed everything it was
>> + required to perform.
>> +
>> +Signed-off-by: Jason Wessel <jason.wessel at windriver.com>
>> +---
>> + signals.go | 54 ++++++++++++++++++++++++++++++
>> ++++++++++++++++++++----
>> + utils_linux.go | 2 +-
>> + 2 files changed, 51 insertions(+), 5 deletions(-)
>> +
>> +diff --git a/src/import/signals.go b/src/import/signals.go
>> +index 910ea1ee..b6a23476 100644
>> +--- a/src/import/signals.go
>> ++++ b/src/import/signals.go
>> +@@ -6,6 +6,7 @@ import (
>> + "os"
>> + "os/signal"
>> + "syscall" // only for Signal
>> ++ "strconv"
>> +
>> + "github.com/Sirupsen/logrus"
>> + "github.com/opencontainers/runc/libcontainer"
>> +@@ -56,9 +57,6 @@ type signalHandler struct {
>> + func (h *signalHandler) forward(process *libcontainer.Process, tty
>> *tty, detach bool) (int, error) {
>> + // make sure we know the pid of our main process so that we can
>> return
>> + // after it dies.
>> +- if detach && h.notifySocket == nil {
>> +- return 0, nil
>> +- }
>> +
>> + pid1, err := process.Pid()
>> + if err != nil {
>> +@@ -68,12 +66,60 @@ func (h *signalHandler) forward(process
>> *libcontainer.Process, tty *tty, detach
>> + if h.notifySocket != nil {
>> + if detach {
>> + h.notifySocket.run(pid1)
>> +- return 0, nil
>> + } else {
>> + go h.notifySocket.run(0)
>> + }
>> + }
>> +
>> ++ if (detach) {
>> ++ // This allows the parent process to daemonize this
>> process
>> ++ // so long as stdin/stderr/stdout are closed
>> ++ if envVal := os.Getenv("SIGUSR1_PARENT_PID"); envVal !=
>> "" {
>> ++ // Close stdin/stdout/stderr
>> ++ os.Stdin.Close()
>> ++ os.Stdout.Close()
>> ++ os.Stderr.Close()
>> ++ // Notify parent to detach
>> ++ i, err := strconv.Atoi(envVal)
>> ++ if (err != nil) {
>> ++ return 0, nil
>> ++ }
>>
>
> So all the code below here is the copied block and it deals with the
> normal exit ? i.e. is it shutting the container down ?
>
>
>
> Yes. It follows the sequence I mentioned in this mail for the "runc run"
> case. I tried to make it more clear.
>
>
> The reason I ask, is that I thought this block of code was running on
> startup, when the container first detaches .. and it didn't make sense
> in that context.
>
>
>
>
> This block of code is the wait loop for signals OR exit, and there is some
> additional code that runs after it returns where it runs the cleanup
> handler.
>
>
> Specifically if you look further down in the patch it is called
> r.destroy() and nothing is modified there other to call it. In the case
> you don't have the environment variable set the exit is much earlier.
>
> Jason.
>
>
>
>
>
> Bruce
>
>
>> ++ unix.Kill(i, unix.SIGUSR1)
>> ++ // Loop waiting on the child to signal or exit,
>> ++ // after which all stop hooks will be run
>> ++ for s := range h.signals {
>> ++ switch s {
>> ++ case unix.SIGCHLD:
>> ++ exits, err := h.reap()
>> ++ if err != nil {
>> ++ logrus.Error(err)
>> ++ }
>> ++ for _, e := range exits {
>> ++
>> logrus.WithFields(logrus.Fields{
>> ++ "pid": e.pid,
>> ++ "status":
>> e.status,
>> ++ }).Debug("process exited")
>> ++ if e.pid == pid1 {
>> ++ // call Wait() on
>> the process even though we already have the exit
>> ++ // status because
>> we must ensure that any of the go specific process
>> ++ // fun such as
>> flushing pipes are complete before we return.
>> ++ process.Wait()
>> ++ if h.notifySocket
>> != nil {
>> ++
>> h.notifySocket.Close()
>> ++ }
>> ++ return e.status,
>> nil
>> ++ }
>> ++ }
>> ++ default:
>> ++ logrus.Debugf("sending signal to
>> process %s", s)
>> ++ if err := unix.Kill(pid1,
>> s.(syscall.Signal)); err != nil {
>> ++ logrus.Error(err)
>> ++ }
>> ++ }
>> ++ }
>> ++ }
>> ++ return 0, nil
>> ++ }
>> + // perform the initial tty resize.
>> + tty.resize()
>> + for s := range h.signals {
>> +diff --git a/src/import/utils_linux.go b/src/import/utils_linux.go
>> +index e6d31b35..1bb80a63 100644
>> +--- a/src/import/utils_linux.go
>> ++++ b/src/import/utils_linux.go
>> +@@ -308,7 +308,7 @@ func (r *runner) run(config *specs.Process) (int,
>> error) {
>> + if err != nil {
>> + r.terminate(process)
>> + }
>> +- if detach {
>> ++ if (detach && os.Getenv("SIGUSR1_PARENT_PID") == "") {
>> + return 0, nil
>> + }
>> + r.destroy()
>> +--
>> +2.11.0
>> +
>> diff --git a/recipes-containers/runc/runc-docker_git.bb
>> b/recipes-containers/runc/runc-docker_git.bb
>> index 9db48ee..f31b82e 100644
>> --- a/recipes-containers/runc/runc-docker_git.bb
>> +++ b/recipes-containers/runc/runc-docker_git.bb
>> @@ -10,6 +10,7 @@ SRC_URI = "git://github.com/docker/runc.
>> git;nobranch=1;name=runc-docker \
>> file://0001-runc-Add-console-socket-dev-null.patch \
>> file://0001-Use-correct-go-cross-compiler.patch \
>> file://0001-Disable-building-recvtty.patch \
>> + file://0001-runc-docker-SIGUSR1-daemonize.patch \
>> "
>>
>> RUNC_VERSION = "1.0.0-rc3"
>> --
>> 2.11.0
>>
>> --
>> _______________________________________________
>> meta-virtualization mailing list
>> meta-virtualization at yoctoproject.org
>> https://lists.yoctoproject.org/listinfo/meta-virtualization
>>
>
>
>
> --
> "Thou shalt not follow the NULL pointer, for chaos and madness await thee
> at its end"
>
>
>
--
"Thou shalt not follow the NULL pointer, for chaos and madness await thee
at its end"
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.yoctoproject.org/pipermail/meta-virtualization/attachments/20171210/27def888/attachment-0001.html>
More information about the meta-virtualization
mailing list