Merge pull request #321 from mlaventure/cherry-pick-exec-fix-for-release

Fixes for Docker 1.12.2

api/grpc/types/api.proto

@@ -192,7 +192,7 @@ message UpdateContainerRequest {
 }
 
 message UpdateResource {
-	uint64 blkioWeight = 1;
+	uint64 blkioWeight =1;
 	uint64 cpuShares = 2;
 	uint64 cpuPeriod = 3;
 	uint64 cpuQuota = 4;
@@ -203,28 +203,6 @@ message UpdateResource {
 	uint64 memoryReservation = 9;
 	uint64 kernelMemoryLimit = 10;
 	uint64 kernelTCPMemoryLimit = 11;
-	uint64 blkioLeafWeight = 12;
-	repeated WeightDevice blkioWeightDevice = 13;
-	repeated ThrottleDevice blkioThrottleReadBpsDevice = 14;
-	repeated ThrottleDevice blkioThrottleWriteBpsDevice = 15;
-	repeated ThrottleDevice blkioThrottleReadIopsDevice = 16;
-	repeated ThrottleDevice blkioThrottleWriteIopsDevice = 17;
-}
-
-message BlockIODevice {
-	int64 major = 1;
-	int64 minor = 2;
-}
-
-message WeightDevice {
-	BlockIODevice blkIODevice = 1;
-	uint32 weight = 2;
-	uint32 leafWeight = 3;
-}
-
-message ThrottleDevice {
-	BlockIODevice blkIODevice = 1;
-	uint64 rate = 2;
 }
 
 message UpdateContainerResponse {

containerd-shim/main.go

@@ -106,7 +106,7 @@ func start(log *os.File) error {
 		case s := <-signals:
 			switch s {
 			case syscall.SIGCHLD:
-				exits, _ := osutils.Reap()
+				exits, _ := osutils.Reap(false)
 				for _, e := range exits {
 					// check to see if runtime is one of the processes that has exited
 					if e.Pid == p.pid() {
@@ -117,8 +117,17 @@ func start(log *os.File) error {
 			}
 			// runtime has exited so the shim can also exit
 			if exitShim {
-				// Let containerd take care of calling the runtime delete
+				// Let containerd take care of calling the runtime
+				// delete.
+				// This is needed to be done first in order to ensure
+				// that the call to Reap does not block until all
+				// children of the container have died if init was not
+				// started in its own PID namespace.
 				f.Close()
+				// Wait for all the childs this process may have
+				// created (needed for exec and init processes when
+				// they join another pid namespace)
+				osutils.Reap(true)
 				p.Wait()
 				return nil
 			}

containerd/main.go

@@ -23,7 +23,6 @@ import (
 	"github.com/docker/containerd/api/grpc/server"
 	"github.com/docker/containerd/api/grpc/types"
 	"github.com/docker/containerd/api/http/pprof"
-	"github.com/docker/containerd/osutils"
 	"github.com/docker/containerd/supervisor"
 	"github.com/docker/docker/pkg/listeners"
 	"github.com/rcrowley/go-metrics"
@@ -160,7 +159,6 @@ func main() {
 func daemon(context *cli.Context) error {
 	s := make(chan os.Signal, 2048)
 	signal.Notify(s, syscall.SIGTERM, syscall.SIGINT)
-	osutils.SetSubreaper(1)
 	sv, err := supervisor.New(
 		context.String("state-dir"),
 		context.String("runtime"),

osutils/reaper.go

@@ -12,13 +12,17 @@ type Exit struct {
 
 // Reap reaps all child processes for the calling process and returns their
 // exit information
-func Reap() (exits []Exit, err error) {
+func Reap(wait bool) (exits []Exit, err error) {
 	var (
 		ws  syscall.WaitStatus
 		rus syscall.Rusage
 	)
+	flag := syscall.WNOHANG
+	if wait {
+		flag = 0
+	}
 	for {
-		pid, err := syscall.Wait4(-1, &ws, syscall.WNOHANG, &rus)
+		pid, err := syscall.Wait4(-1, &ws, flag, &rus)
 		if err != nil {
 			if err == syscall.ECHILD {
 				return exits, nil

runtime/container.go

@@ -14,6 +14,7 @@ import (
 	"github.com/Sirupsen/logrus"
 	"github.com/docker/containerd/specs"
 	ocs "github.com/opencontainers/runtime-spec/specs-go"
+	"golang.org/x/sys/unix"
 )
 
 // Container defines the operations allowed on a container
@@ -480,12 +481,33 @@ func (c *container) createCmd(pid string, cmd *exec.Cmd, p *process) error {
 		}
 		return err
 	}
-	go func() {
-		err := p.cmd.Wait()
-		if err == nil {
-			p.cmdSuccess = true
-		}
-		close(p.cmdDoneCh)
+	// We need the pid file to have been written to run
+	defer func() {
+		go func() {
+			err := p.cmd.Wait()
+			if err == nil {
+				p.cmdSuccess = true
+			}
+
+			if same, err := p.isSameProcess(); same && p.pid > 0 {
+				// The process changed its PR_SET_PDEATHSIG, so force
+				// kill it
+				logrus.Infof("containerd: %s:%s (pid %v) has become an orphan, killing it", p.container.id, p.id, p.pid)
+				err = unix.Kill(p.pid, syscall.SIGKILL)
+				if err != nil && err != syscall.ESRCH {
+					logrus.Errorf("containerd: unable to SIGKILL %s:%s (pid %v): %v", p.container.id, p.id, p.pid, err)
+				} else {
+					for {
+						err = unix.Kill(p.pid, 0)
+						if err != nil {
+							break
+						}
+						time.Sleep(5 * time.Millisecond)
+					}
+				}
+			}
+			close(p.cmdDoneCh)
+		}()
 	}()
 	if err := c.waitForCreate(p, cmd); err != nil {
 		return err

runtime/process.go

@@ -36,7 +36,7 @@ type Process interface {
 	ExitFD() int
 	// ExitStatus returns the exit status of the process or an error if it
 	// has not exited
-	ExitStatus() (int, error)
+	ExitStatus() (uint32, error)
 	// Spec returns the process spec that created the process
 	Spec() specs.ProcessSpec
 	// Signal sends the provided signal to the process
@@ -228,24 +228,31 @@ func (p *process) Resize(w, h int) error {
 	return err
 }
 
-func (p *process) handleSigkilledShim(rst int, rerr error) (int, error) {
+func (p *process) updateExitStatusFile(status uint32) (uint32, error) {
+	p.stateLock.Lock()
+	p.state = Stopped
+	p.stateLock.Unlock()
+	err := ioutil.WriteFile(filepath.Join(p.root, ExitStatusFile), []byte(fmt.Sprintf("%u", status)), 0644)
+	return status, err
+}
+
+func (p *process) handleSigkilledShim(rst uint32, rerr error) (uint32, error) {
 	if p.cmd == nil || p.cmd.Process == nil {
 		e := unix.Kill(p.pid, 0)
 		if e == syscall.ESRCH {
-			return rst, rerr
+			logrus.Warnf("containerd: %s:%s (pid %d) does not exist", p.container.id, p.id, p.pid)
+			// The process died while containerd was down (probably of
+			// SIGKILL, but no way to be sure)
+			return p.updateExitStatusFile(UnknownStatus)
 		}
 
 		// If it's not the same process, just mark it stopped and set
-		// the status to 255
+		// the status to the UnknownStatus value (i.e. 255)
 		if same, err := p.isSameProcess(); !same {
 			logrus.Warnf("containerd: %s:%s (pid %d) is not the same process anymore (%v)", p.container.id, p.id, p.pid, err)
-			p.stateLock.Lock()
-			p.state = Stopped
-			p.stateLock.Unlock()
 			// Create the file so we get the exit event generated once monitor kicks in
-			// without going to this all process again
-			rerr = ioutil.WriteFile(filepath.Join(p.root, ExitStatusFile), []byte("255"), 0644)
-			return 255, nil
+			// without having to go through all this process again
+			return p.updateExitStatusFile(UnknownStatus)
 		}
 
 		ppid, err := readProcStatField(p.pid, 4)
@@ -255,19 +262,21 @@ func (p *process) handleSigkilledShim(rst int, rerr error) (int, error) {
 		if ppid == "1" {
 			logrus.Warnf("containerd: %s:%s shim died, killing associated process", p.container.id, p.id)
 			unix.Kill(p.pid, syscall.SIGKILL)
+			if err != nil && err != syscall.ESRCH {
+				return UnknownStatus, fmt.Errorf("containerd: unable to SIGKILL %s:%s (pid %v): %v", p.container.id, p.id, p.pid, err)
+			}
+
 			// wait for the process to die
 			for {
 				e := unix.Kill(p.pid, 0)
 				if e == syscall.ESRCH {
 					break
 				}
-				time.Sleep(10 * time.Millisecond)
+				time.Sleep(5 * time.Millisecond)
 			}
-
-			rst = 128 + int(syscall.SIGKILL)
 			// Create the file so we get the exit event generated once monitor kicks in
-			// without going to this all process again
-			rerr = ioutil.WriteFile(filepath.Join(p.root, ExitStatusFile), []byte(fmt.Sprintf("%d", rst)), 0644)
+			// without having to go through all this process again
+			return p.updateExitStatusFile(128 + uint32(syscall.SIGKILL))
 		}
 
 		return rst, rerr
@@ -286,29 +295,8 @@ func (p *process) handleSigkilledShim(rst int, rerr error) (int, error) {
 	if shimStatus.Signaled() && shimStatus.Signal() == syscall.SIGKILL {
 		logrus.Debugf("containerd: ExitStatus(container: %s, process: %s): shim was SIGKILL'ed reaping its child with pid %d", p.container.id, p.id, p.pid)
 
-		var (
-			status unix.WaitStatus
-			rusage unix.Rusage
-			wpid   int
-		)
-
-		// Some processes change their PR_SET_PDEATHSIG, so force kill them
-		unix.Kill(p.pid, syscall.SIGKILL)
-
-		for wpid == 0 {
-			wpid, e = unix.Wait4(p.pid, &status, unix.WNOHANG, &rusage)
-			if e != nil {
-				logrus.Debugf("containerd: ExitStatus(container: %s, process: %s): Wait4(%d): %v", p.container.id, p.id, p.pid, rerr)
-				return rst, rerr
-			}
-		}
-
-		if wpid == p.pid {
-			rerr = nil
-			rst = 128 + int(shimStatus.Signal())
-		} else {
-			logrus.Errorf("containerd: ExitStatus(container: %s, process: %s): unexpected returned pid from wait4 %v (expected %v)", p.container.id, p.id, wpid, p.pid)
-		}
+		rerr = nil
+		rst = 128 + uint32(shimStatus.Signal())
 
 		p.stateLock.Lock()
 		p.state = Stopped
@@ -318,7 +306,7 @@ func (p *process) handleSigkilledShim(rst int, rerr error) (int, error) {
 	return rst, rerr
 }
 
-func (p *process) ExitStatus() (rst int, rerr error) {
+func (p *process) ExitStatus() (rst uint32, rerr error) {
 	data, err := ioutil.ReadFile(filepath.Join(p.root, ExitStatusFile))
 	defer func() {
 		if rerr != nil {
@@ -327,17 +315,19 @@ func (p *process) ExitStatus() (rst int, rerr error) {
 	}()
 	if err != nil {
 		if os.IsNotExist(err) {
-			return -1, ErrProcessNotExited
+			return UnknownStatus, ErrProcessNotExited
 		}
-		return -1, err
+		return UnknownStatus, err
 	}
 	if len(data) == 0 {
-		return -1, ErrProcessNotExited
+		return UnknownStatus, ErrProcessNotExited
 	}
 	p.stateLock.Lock()
 	p.state = Stopped
 	p.stateLock.Unlock()
-	return strconv.Atoi(string(data))
+
+	i, err := strconv.ParseUint(string(data), 10, 32)
+	return uint32(i), err
 }
 
 func (p *process) Spec() specs.ProcessSpec {

runtime/runtime.go

@@ -47,6 +47,10 @@ const (
 	// StartTimeFile holds the name of the file in which the process
 	// start time is saved
 	StartTimeFile = "starttime"
+
+	// UnknownStatus is the value returned when a process exit
+	// status cannot be determined
+	UnknownStatus = 255
 )
 
 // Checkpoint holds information regarding a container checkpoint

supervisor/delete.go

@@ -11,7 +11,7 @@ import (
 type DeleteTask struct {
 	baseTask
 	ID      string
-	Status  int
+	Status  uint32
 	PID     string
 	NoEvent bool
 	Process runtime.Process

supervisor/exit.go

@@ -63,7 +63,7 @@ type ExecExitTask struct {
 	baseTask
 	ID      string
 	PID     string
-	Status  int
+	Status  uint32
 	Process runtime.Process
 }
 
@@ -73,13 +73,19 @@ func (s *Supervisor) execExit(t *ExecExitTask) error {
 	if err := container.RemoveProcess(t.PID); err != nil {
 		logrus.WithField("error", err).Error("containerd: find container for pid")
 	}
-	t.Process.Wait()
-	s.notifySubscribers(Event{
-		Timestamp: time.Now(),
-		ID:        t.ID,
-		Type:      StateExit,
-		PID:       t.PID,
-		Status:    t.Status,
-	})
+	// If the exec spawned children which are still using its IO
+	// waiting here will block until they die or close their IO
+	// descriptors.
+	// Hence, we use a go routine to avoid block all other operations
+	go func() {
+		t.Process.Wait()
+		s.notifySubscribers(Event{
+			Timestamp: time.Now(),
+			ID:        t.ID,
+			Type:      StateExit,
+			PID:       t.PID,
+			Status:    t.Status,
+		})
+	}()
 	return nil
 }

supervisor/sort_test.go

@@ -46,8 +46,8 @@ func (p *testProcess) ExitFD() int {
 	return -1
 }
 
-func (p *testProcess) ExitStatus() (int, error) {
-	return -1, nil
+func (p *testProcess) ExitStatus() (uint32, error) {
+	return runtime.UnknownStatus, nil
 }
 
 func (p *testProcess) Container() runtime.Container {

supervisor/supervisor.go

@@ -186,7 +186,7 @@ type Event struct {
 	Type      string    `json:"type"`
 	Timestamp time.Time `json:"timestamp"`
 	PID       string    `json:"pid,omitempty"`
-	Status    int       `json:"status,omitempty"`
+	Status    uint32    `json:"status,omitempty"`
 }
 
 // Events returns an event channel that external consumers can use to receive updates

version.go

@@ -9,7 +9,7 @@ const VersionMajor = 0
 const VersionMinor = 2
 
 // VersionPatch holds the release patch number
-const VersionPatch = 3
+const VersionPatch = 4
 
 // Version holds the combination of major minor and patch as a string
 // of format Major.Minor.Patch