On Sun, Dec 10, 2017 at 11:20 PM, Jason Wessel <[email protected]> wrote:
> On 12/10/2017 09:38 PM, Bruce Ashfield wrote: > > > > On Fri, Dec 8, 2017 at 10:53 AM, Jason Wessel <[email protected]> > 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 <[email protected]> >> --- >> .../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 <[email protected]> >> +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 <[email protected]> >> +--- >> + 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 >> [email protected] >> 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"
-- _______________________________________________ meta-virtualization mailing list [email protected] https://lists.yoctoproject.org/listinfo/meta-virtualization
