From 97eff6cf6c9b58f8239b28be2f080e23c9da62c0 Mon Sep 17 00:00:00 2001

From: Ulrich Obergfell <uobergfe@redhat.com>

Date: Thu, 11 Jul 2019 14:33:55 +0200

Subject: [PATCH] fix deadlock in restore() function

When containerd starts up, the restore() function walks through the directory

hierarchy under /run/docker/libcontainerd/containerd and imports the state of

processes from files in /run/docker/libcontainerd/containerd/CONTAINERID and

in /run/docker/libcontainerd/containerd/CONTAINERID/PROCESSID. The restore()

function adds an ExitTask entry to the s.tasks queue for each process that is

no longer in state 'running'. The size of the s.tasks queue is hard-coded and

limited to 2048 (defaultBufferSize). If more than 2048 ExitTask entries need

to be added to the queue, the restore() function gets blocked (queue is full).

If this happens, containerd is in a kind of deadlock situation because the

handleTask() function (which would drain the ExitTask entries from the queue)

has not been started in a separate goroutine yet, and the main goroutine is

blocked in the restore() function (unable to start the handleTask() function).

This patch introduces the dynamically allocated restoreExitTasks slice which

the restore() function uses to store the ExitTask entries separately instead

of adding them to the s.tasks queue. The task handler goroutine subsequently

drains all entries from restoreExitTasks frist before it enters the loop that

handles entries from the s.tasks queue.

Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>

---

 supervisor/supervisor.go | 35 +++++++++++++++++++++++++++++++++--

 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/supervisor/supervisor.go b/supervisor/supervisor.go

index 8a26af0..d92de8a 100644

--- a/supervisor/supervisor.go

+++ b/supervisor/supervisor.go

@@ -18,6 +18,16 @@ const (

 	defaultBufferSize = 2048 // size of queue in eventloop

 )

+// Pointers to all ExitTask that are created by the restore() function are stored in this slice.

+var restoreExitTasks []*ExitTask

+

+func max(x, y int) int {

+	if x < y {

+		return y

+	}

+	return x

+}

+

 // New returns an initialized Process supervisor.

 func New(stateDir string, runtimeName, shimName string, runtimeArgs []string, timeout time.Duration, retainCount int) (*Supervisor, error) {

 	startTasks := make(chan *startTask, 10)

@@ -207,7 +217,9 @@ type eventV1 struct {

 // Events returns an event channel that external consumers can use to receive updates

 // on container events

 func (s *Supervisor) Events(from time.Time, storedOnly bool, id string) chan Event {

-	c := make(chan Event, defaultBufferSize)

+	var c chan Event

+

+	c = make(chan Event, defaultBufferSize)

 	if storedOnly {

 		defer s.Unsubscribe(c)

 	}

@@ -216,6 +228,9 @@ func (s *Supervisor) Events(from time.Time, storedOnly bool, id string) chan Eve

 	if !from.IsZero() {

 		// replay old event

 		s.eventLock.Lock()

+		close(c)

+		// Allocate a channel that has enough space for the entire event log.

+		c = make(chan Event, max(defaultBufferSize, len(s.eventLog)))

 		past := s.eventLog[:]

 		s.eventLock.Unlock()

 		for _, e := range past {

@@ -276,6 +291,21 @@ func (s *Supervisor) Start() error {

 		"cpus":        s.machine.Cpus,

 	}).Debug("containerd: supervisor running")

 	go func() {

+		if (len(restoreExitTasks) > 0) {

+			logrus.Infof("containerd: found %d exited processes after restart", len(restoreExitTasks))

+			//

+			// If the restore() function stored any ExitTask in the dedicated slice,

+			// then handle those tasks first. The purpose of the one second delay is

+			// to give dockerd a chance to establish its event stream connection to

+			// containerd. If the connection is established before the ExitTask are

+			// being handled, then dockerd can receive exit notifications directly

+			// (rather than having to replay the notifications from the event log).

+			//

+			time.Sleep(time.Second)

+			for _, e := range restoreExitTasks {

+				s.handleTask(e)

+			}

+		}

 		for i := range s.tasks {

 			s.handleTask(i)

 		}

@@ -385,7 +415,8 @@ func (s *Supervisor) restore() error {

 					Process: p,

 				}

 				e.WithContext(context.Background())

-				s.SendTask(e)

+				// Store pointer to ExitTask in dedicated slice.

+				restoreExitTasks = append(restoreExitTasks, e)

 			}

 		}

 	}