Update runc to fix hang during start and exec #36097

crosbymichael · 2018-01-23T20:04:05Z

Signed-off-by: Michael Crosby [email protected]

runc diff:

diff --git a/libcontainer/cgroups/systemd/apply_systemd.go b/libcontainer/cgroups/systemd/apply_systemd.go
index 5471ebb..45bd3ac 100644
--- a/libcontainer/cgroups/systemd/apply_systemd.go
+++ b/libcontainer/cgroups/systemd/apply_systemd.go
@@ -295,10 +295,13 @@ func (m *Manager) Apply(pid int) error {
 		}
 	}
 
-	if _, err := theConn.StartTransientUnit(unitName, "replace", properties, nil); err != nil && !isUnitExists(err) {
+	statusChan := make(chan string)
+	if _, err := theConn.StartTransientUnit(unitName, "replace", properties, statusChan); err != nil && !isUnitExists(err) {
 		return err
 	}
 
+	<-statusChan
+
 	if err := joinCgroups(c, pid); err != nil {
 		return err
 	}
diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go
index 1ac74b1..cfb05b4 100644
--- a/libcontainer/container_linux.go
+++ b/libcontainer/container_linux.go
@@ -5,6 +5,7 @@ package libcontainer
 import (
 	"bytes"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -267,20 +268,71 @@ func (c *linuxContainer) Exec() error {
 
 func (c *linuxContainer) exec() error {
 	path := filepath.Join(c.root, execFifoFilename)
-	f, err := os.OpenFile(path, os.O_RDONLY, 0)
-	if err != nil {
-		return newSystemErrorWithCause(err, "open exec fifo for reading")
+
+	fifoOpen := make(chan struct{})
+	select {
+	case <-awaitProcessExit(c.initProcess.pid(), fifoOpen):
+		return errors.New("container process is already dead")
+	case result := <-awaitFifoOpen(path):
+		close(fifoOpen)
+		if result.err != nil {
+			return result.err
+		}
+		f := result.file
+		defer f.Close()
+		if err := readFromExecFifo(f); err != nil {
+			return err
+		}
+		return os.Remove(path)
 	}
-	defer f.Close()
-	data, err := ioutil.ReadAll(f)
+}
+
+func readFromExecFifo(execFifo io.Reader) error {
+	data, err := ioutil.ReadAll(execFifo)
 	if err != nil {
 		return err
 	}
-	if len(data) > 0 {
-		os.Remove(path)
-		return nil
+	if len(data) <= 0 {
+		return fmt.Errorf("cannot start an already running container")
 	}
-	return fmt.Errorf("cannot start an already running container")
+	return nil
+}
+
+func awaitProcessExit(pid int, exit <-chan struct{}) <-chan struct{} {
+	isDead := make(chan struct{})
+	go func() {
+		for {
+			select {
+			case <-exit:
+				return
+			case <-time.After(time.Millisecond * 100):
+				stat, err := system.Stat(pid)
+				if err != nil || stat.State == system.Zombie {
+					close(isDead)
+					return
+				}
+			}
+		}
+	}()
+	return isDead
+}
+
+func awaitFifoOpen(path string) <-chan openResult {
+	fifoOpened := make(chan openResult)
+	go func() {
+		f, err := os.OpenFile(path, os.O_RDONLY, 0)
+		if err != nil {
+			fifoOpened <- openResult{err: newSystemErrorWithCause(err, "open exec fifo for reading")}
+			return
+		}
+		fifoOpened <- openResult{file: f}
+	}()
+	return fifoOpened
+}
+
+type openResult struct {
+	file *os.File
+	err  error
 }
 
 func (c *linuxContainer) start(process *Process, isInit bool) error {
@@ -308,11 +360,13 @@ func (c *linuxContainer) start(process *Process, isInit bool) error {
 		c.initProcessStartTime = state.InitProcessStartTime
 
 		if c.config.Hooks != nil {
+			bundle, annotations := utils.Annotations(c.config.Labels)
 			s := configs.HookState{
-				Version: c.config.Version,
-				ID:      c.id,
-				Pid:     parent.pid(),
-				Bundle:  utils.SearchLabels(c.config.Labels, "bundle"),
+				Version:     c.config.Version,
+				ID:          c.id,
+				Pid:         parent.pid(),
+				Bundle:      bundle,
+				Annotations: annotations,
 			}
 			for i, hook := range c.config.Hooks.Poststart {
 				if err := hook.Run(s); err != nil {
@@ -1436,11 +1490,13 @@ func (c *linuxContainer) criuNotifications(resp *criurpc.CriuResp, process *Proc
 		}
 	case notify.GetScript() == "setup-namespaces":
 		if c.config.Hooks != nil {
+			bundle, annotations := utils.Annotations(c.config.Labels)
 			s := configs.HookState{
-				Version: c.config.Version,
-				ID:      c.id,
-				Pid:     int(notify.GetPid()),
-				Bundle:  utils.SearchLabels(c.config.Labels, "bundle"),
+				Version:     c.config.Version,
+				ID:          c.id,
+				Pid:         int(notify.GetPid()),
+				Bundle:      bundle,
+				Annotations: annotations,
 			}
 			for i, hook := range c.config.Hooks.Prestart {
 				if err := hook.Run(s); err != nil {
diff --git a/libcontainer/init_linux.go b/libcontainer/init_linux.go
index 16e94a7..2770be3 100644
--- a/libcontainer/init_linux.go
+++ b/libcontainer/init_linux.go
@@ -494,6 +494,16 @@ func signalAllProcesses(m cgroups.Manager, s os.Signal) error {
 		logrus.Warn(err)
 	}
 
+	subreaper, err := system.GetSubreaper()
+	if err != nil {
+		// The error here means that PR_GET_CHILD_SUBREAPER is not
+		// supported because this code might run on a kernel older
+		// than 3.4. We don't want to throw an error in that case,
+		// and we simplify things, considering there is no subreaper
+		// set.
+		subreaper = 0
+	}
+
 	for _, p := range procs {
 		if s != unix.SIGKILL {
 			if ok, err := isWaitable(p.Pid); err != nil {
@@ -507,9 +517,16 @@ func signalAllProcesses(m cgroups.Manager, s os.Signal) error {
 			}
 		}
 
-		if _, err := p.Wait(); err != nil {
-			if !isNoChildren(err) {
-				logrus.Warn("wait: ", err)
+		// In case a subreaper has been setup, this code must not
+		// wait for the process. Otherwise, we cannot be sure the
+		// current process will be reaped by the subreaper, while
+		// the subreaper might be waiting for this process in order
+		// to retrieve its exit code.
+		if subreaper == 0 {
+			if _, err := p.Wait(); err != nil {
+				if !isNoChildren(err) {
+					logrus.Warn("wait: ", err)
+				}
 			}
 		}
 	}
diff --git a/libcontainer/process_linux.go b/libcontainer/process_linux.go
index 149b112..58980b0 100644
--- a/libcontainer/process_linux.go
+++ b/libcontainer/process_linux.go
@@ -341,11 +341,13 @@ func (p *initProcess) start() error {
 				}
 
 				if p.config.Config.Hooks != nil {
+					bundle, annotations := utils.Annotations(p.container.config.Labels)
 					s := configs.HookState{
-						Version: p.container.config.Version,
-						ID:      p.container.id,
-						Pid:     p.pid(),
-						Bundle:  utils.SearchLabels(p.config.Config.Labels, "bundle"),
+						Version:     p.container.config.Version,
+						ID:          p.container.id,
+						Pid:         p.pid(),
+						Bundle:      bundle,
+						Annotations: annotations,
 					}
 					for i, hook := range p.config.Config.Hooks.Prestart {
 						if err := hook.Run(s); err != nil {
@@ -370,11 +372,13 @@ func (p *initProcess) start() error {
 				}
 			}
 			if p.config.Config.Hooks != nil {
+				bundle, annotations := utils.Annotations(p.container.config.Labels)
 				s := configs.HookState{
-					Version: p.container.config.Version,
-					ID:      p.container.id,
-					Pid:     p.pid(),
-					Bundle:  utils.SearchLabels(p.config.Config.Labels, "bundle"),
+					Version:     p.container.config.Version,
+					ID:          p.container.id,
+					Pid:         p.pid(),
+					Bundle:      bundle,
+					Annotations: annotations,
 				}
 				for i, hook := range p.config.Config.Hooks.Prestart {
 					if err := hook.Run(s); err != nil {
diff --git a/libcontainer/state_linux.go b/libcontainer/state_linux.go
index 1f8c5e7..b45ce23 100644
--- a/libcontainer/state_linux.go
+++ b/libcontainer/state_linux.go
@@ -63,10 +63,12 @@ func destroy(c *linuxContainer) error {
 
 func runPoststopHooks(c *linuxContainer) error {
 	if c.config.Hooks != nil {
+		bundle, annotations := utils.Annotations(c.config.Labels)
 		s := configs.HookState{
-			Version: c.config.Version,
-			ID:      c.id,
-			Bundle:  utils.SearchLabels(c.config.Labels, "bundle"),
+			Version:     c.config.Version,
+			ID:          c.id,
+			Bundle:      bundle,
+			Annotations: annotations,
 		}
 		for _, hook := range c.config.Hooks.Poststop {
 			if err := hook.Run(s); err != nil {
diff --git a/libcontainer/system/linux.go b/libcontainer/system/linux.go
index 4837085..5f124cd 100644
--- a/libcontainer/system/linux.go
+++ b/libcontainer/system/linux.go
@@ -134,3 +134,14 @@ func RunningInUserNS() bool {
 func SetSubreaper(i int) error {
 	return unix.Prctl(PR_SET_CHILD_SUBREAPER, uintptr(i), 0, 0, 0)
 }
+
+// GetSubreaper returns the subreaper setting for the calling process
+func GetSubreaper() (int, error) {
+	var i uintptr
+
+	if err := unix.Prctl(unix.PR_GET_CHILD_SUBREAPER, uintptr(unsafe.Pointer(&i)), 0, 0, 0); err != nil {
+		return -1, err
+	}
+
+	return int(i), nil
+}
diff --git a/tests/integration/spec.bats b/tests/integration/spec.bats
index c9ba2aa..059e4c0 100644
--- a/tests/integration/spec.bats
+++ b/tests/integration/spec.bats
@@ -81,6 +81,9 @@ function teardown() {
   run bash -c "GOPATH='$GOPATH' go get github.com/xeipuuv/gojsonschema"
   [ "$status" -eq 0 ]
 
+  run git -C "${GOPATH}/src/github.com/xeipuuv/gojsonschema" reset --hard 6637feb73ee44cd4640bb3def285c29774234c7f
+  [ "$status" -eq 0 ]
+
   GOPATH="$GOPATH" go build src/runtime-spec/schema/validate.go
   [ -e ./validate ]

Signed-off-by: Michael Crosby <[email protected]>

dnephin

LGTM

vdemeester

LGTM 🐯

cpuguy83

LGTM

See moby/moby#36097.

Update runc to fix hang during start and exec

d10091c

Signed-off-by: Michael Crosby <[email protected]>

crosbymichael requested review from dnephin and tianon as code owners January 23, 2018 20:04

GordonTheTurtle added the status/0-triage label Jan 23, 2018

dnephin approved these changes Jan 23, 2018

View reviewed changes

vdemeester added status/2-code-review and removed status/0-triage labels Jan 23, 2018

vdemeester approved these changes Jan 23, 2018

View reviewed changes

cpuguy83 approved these changes Jan 23, 2018

View reviewed changes

cpuguy83 merged commit 1b9d1e8 into moby:master Jan 23, 2018

thaJeztah mentioned this pull request Jan 24, 2018

[17.12] Update runc to fix hang during start and exec docker-archive/docker-ce#395

Merged

thaJeztah added area/runtime impact/changelog labels Jan 24, 2018

crosbymichael deleted the runc-hang branch January 24, 2018 14:39

This was referenced Jan 26, 2018

Slow docker build #36109

Closed

After upgrading to 17.12.0-ce containers are reported as "unknown" on start #35891

Closed

Docker Swarm Resource/Memory leak #35826

Closed

thaJeztah mentioned this pull request Feb 26, 2018

docker start hangs #36410

Closed

ay0o mentioned this pull request Feb 28, 2018

Can't stop docker container #35933

Closed

eloycoto mentioned this pull request Mar 12, 2018

Nightly: Test flake cannot exec command in container. cilium/cilium#3106

Closed

charleskorn added a commit to batect/batect that referenced this pull request Nov 16, 2019

Use Docker 17.12.1 on CI to work around bug where Docker would hang.

9b04e97

See moby/moby#36097.

liggitt mentioned this pull request Jan 24, 2020

"OCI runtime start failed" runc race causes many CI failures/flakes kubernetes/kubernetes#86312

Closed

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update runc to fix hang during start and exec #36097

Update runc to fix hang during start and exec #36097

crosbymichael commented Jan 23, 2018 •

edited

Loading

dnephin left a comment

vdemeester left a comment

cpuguy83 left a comment

Update runc to fix hang during start and exec #36097

Update runc to fix hang during start and exec #36097

Conversation

crosbymichael commented Jan 23, 2018 • edited Loading

dnephin left a comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

cpuguy83 left a comment

Choose a reason for hiding this comment

crosbymichael commented Jan 23, 2018 •

edited

Loading