[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