* [PATCH] runc-docker: Allow "run start ..." to daemonize with $SIGUSR1_PARENT_PID
@ 2017-12-08 15:53 Jason Wessel
2017-12-11 3:38 ` Bruce Ashfield
0 siblings, 1 reply; 4+ messages in thread
From: Jason Wessel @ 2017-12-08 15:53 UTC (permalink / raw)
To: meta-virtualization
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@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-docker/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-SIGUSR1-daemonize.patch
new file mode 100644
index 0000000..b3bd068
--- /dev/null
+++ b/recipes-containers/runc/runc-docker/0001-runc-docker-SIGUSR1-daemonize.patch
@@ -0,0 +1,131 @@
+From cd7d76a6d1ecb1856f6ed666fb5c30dc105aa94e Mon Sep 17 00:00:00 2001
+From: Jason Wessel <jason.wessel@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@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
++ }
++ 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] runc-docker: Allow "run start ..." to daemonize with $SIGUSR1_PARENT_PID
2017-12-08 15:53 [PATCH] runc-docker: Allow "run start ..." to daemonize with $SIGUSR1_PARENT_PID Jason Wessel
@ 2017-12-11 3:38 ` Bruce Ashfield
2017-12-11 4:20 ` Jason Wessel
0 siblings, 1 reply; 4+ messages in thread
From: Bruce Ashfield @ 2017-12-11 3:38 UTC (permalink / raw)
To: Jason Wessel; +Cc: meta-virtualization
[-- Attachment #1: Type: text/plain, Size: 9997 bytes --]
On Fri, Dec 8, 2017 at 10:53 AM, Jason Wessel <jason.wessel@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 ?
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 ?
>
> 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).
>
> 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@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-docker/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-
> SIGUSR1-daemonize.patch
> new file mode 100644
> index 0000000..b3bd068
> --- /dev/null
> +++ b/recipes-containers/runc/runc-docker/0001-runc-docker-
> SIGUSR1-daemonize.patch
> @@ -0,0 +1,131 @@
> +From cd7d76a6d1ecb1856f6ed666fb5c30dc105aa94e Mon Sep 17 00:00:00 2001
> +From: Jason Wessel <jason.wessel@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@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 ?
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.
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@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"
[-- Attachment #2: Type: text/html, Size: 13981 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] runc-docker: Allow "run start ..." to daemonize with $SIGUSR1_PARENT_PID
2017-12-11 3:38 ` Bruce Ashfield
@ 2017-12-11 4:20 ` Jason Wessel
2017-12-11 4:31 ` Bruce Ashfield
0 siblings, 1 reply; 4+ messages in thread
From: Jason Wessel @ 2017-12-11 4:20 UTC (permalink / raw)
To: Bruce Ashfield; +Cc: meta-virtualization
[-- Attachment #1: Type: text/plain, Size: 14534 bytes --]
On 12/10/2017 09:38 PM, Bruce Ashfield wrote:
>
>
> On Fri, Dec 8, 2017 at 10:53 AM, Jason Wessel <jason.wessel@windriver.com <mailto:jason.wessel@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.
>
> 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@windriver.com <mailto:jason.wessel@windriver.com>>
> ---
> .../0001-runc-docker-SIGUSR1-daemonize.patch | 131 +++++++++++++++++++++
> recipes-containers/runc/runc-docker_git.bb <http://runc-docker_git.bb> | 1 +
> 2 files changed, 132 insertions(+)
> create mode 100644 recipes-containers/runc/runc-docker/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-SIGUSR1-daemonize.patch
> new file mode 100644
> index 0000000..b3bd068
> --- /dev/null
> +++ b/recipes-containers/runc/runc-docker/0001-runc-docker-SIGUSR1-daemonize.patch
> @@ -0,0 +1,131 @@
> +From cd7d76a6d1ecb1856f6ed666fb5c30dc105aa94e Mon Sep 17 00:00:00 2001
> +From: Jason Wessel <jason.wessel@windriver.com <mailto:jason.wessel@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@windriver.com <mailto:jason.wessel@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 <http://github.com/Sirupsen/logrus>"
> + "github.com/opencontainers/runc/libcontainer <http://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 <http://runc-docker_git.bb> b/recipes-containers/runc/runc-docker_git.bb <http://runc-docker_git.bb>
> index 9db48ee..f31b82e 100644
> --- a/recipes-containers/runc/runc-docker_git.bb <http://runc-docker_git.bb>
> +++ b/recipes-containers/runc/runc-docker_git.bb <http://runc-docker_git.bb>
> @@ -10,6 +10,7 @@ SRC_URI = "git://github.com/docker/runc.git;nobranch=1;name=runc-docker <http://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@yoctoproject.org <mailto:meta-virtualization@yoctoproject.org>
> https://lists.yoctoproject.org/listinfo/meta-virtualization <https://lists.yoctoproject.org/listinfo/meta-virtualization>
>
>
>
>
> --
> "Thou shalt not follow the NULL pointer, for chaos and madness await thee at its end"
[-- Attachment #2: Type: text/html, Size: 24486 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] runc-docker: Allow "run start ..." to daemonize with $SIGUSR1_PARENT_PID
2017-12-11 4:20 ` Jason Wessel
@ 2017-12-11 4:31 ` Bruce Ashfield
0 siblings, 0 replies; 4+ messages in thread
From: Bruce Ashfield @ 2017-12-11 4:31 UTC (permalink / raw)
To: Jason Wessel; +Cc: meta-virtualization
[-- Attachment #1: Type: text/plain, Size: 13734 bytes --]
On Sun, Dec 10, 2017 at 11:20 PM, Jason Wessel <jason.wessel@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@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@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@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@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@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"
[-- Attachment #2: Type: text/html, Size: 24852 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-12-11 4:32 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-08 15:53 [PATCH] runc-docker: Allow "run start ..." to daemonize with $SIGUSR1_PARENT_PID Jason Wessel
2017-12-11 3:38 ` Bruce Ashfield
2017-12-11 4:20 ` Jason Wessel
2017-12-11 4:31 ` Bruce Ashfield
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.