From 4e9d29a408f2f14a8d2ce4793fbbdf793a218499 Mon Sep 17 00:00:00 2001
From: Jimmi Dyson <jimmidyson@gmail.com>
Date: Thu, 14 Jan 2016 11:55:53 +0000
Subject: [PATCH] Fix FS usage goroutine leaks

---
 container/container.go      |  4 +++
 container/docker/handler.go |  8 +++---
 container/mock.go           |  2 ++
 container/raw/handler.go    |  3 +++
 manager/container.go        |  3 +++
 manager/manager.go          | 52 +++++++++++++++----------------------
 6 files changed, 38 insertions(+), 34 deletions(-)

diff --git a/container/container.go b/container/container.go
index f41c006a..f925a5e5 100644
--- a/container/container.go
+++ b/container/container.go
@@ -81,4 +81,8 @@ type ContainerHandler interface {
 
 	// Cleanup frees up any resources being held like fds or go routines, etc.
 	Cleanup()
+
+	// Start starts any necessary background goroutines - must be cleaned up in Cleanup().
+	// It is expected that most implementations will be a no-op.
+	Start()
 }
diff --git a/container/docker/handler.go b/container/docker/handler.go
index e31d71d8..7e400f77 100644
--- a/container/docker/handler.go
+++ b/container/docker/handler.go
@@ -142,9 +142,6 @@ func newDockerContainerHandler(
 		fsHandler:          newFsHandler(time.Minute, storageDirs, fsInfo),
 	}
 
-	// Start the filesystem handler.
-	handler.fsHandler.start()
-
 	// We assume that if Inspect fails then the container is not known to docker.
 	ctnr, err := client.InspectContainer(id)
 	if err != nil {
@@ -172,6 +169,11 @@ func newDockerContainerHandler(
 	return handler, nil
 }
 
+func (self *dockerContainerHandler) Start() {
+	// Start the filesystem handler.
+	self.fsHandler.start()
+}
+
 func (self *dockerContainerHandler) Cleanup() {
 	self.fsHandler.stop()
 }
diff --git a/container/mock.go b/container/mock.go
index 40a80419..0306b404 100644
--- a/container/mock.go
+++ b/container/mock.go
@@ -51,6 +51,8 @@ func (self *MockContainerHandler) ContainerReference() (info.ContainerReference,
 	return args.Get(0).(info.ContainerReference), args.Error(1)
 }
 
+func (self *MockContainerHandler) Start() {}
+
 func (self *MockContainerHandler) Cleanup() {}
 
 func (self *MockContainerHandler) GetSpec() (info.ContainerSpec, error) {
diff --git a/container/raw/handler.go b/container/raw/handler.go
index 4e0a22d3..b7dd5005 100644
--- a/container/raw/handler.go
+++ b/container/raw/handler.go
@@ -166,6 +166,9 @@ func (self *rawContainerHandler) GetRootNetworkDevices() ([]info.NetInfo, error)
 	return nd, nil
 }
 
+// Nothing to start up.
+func (self *rawContainerHandler) Start() {}
+
 // Nothing to clean up.
 func (self *rawContainerHandler) Cleanup() {}
 
diff --git a/manager/container.go b/manager/container.go
index 2d6381b2..07667218 100644
--- a/manager/container.go
+++ b/manager/container.go
@@ -361,6 +361,9 @@ func (self *containerData) nextHousekeeping(lastHousekeeping time.Time) time.Tim
 
 // TODO(vmarmol): Implement stats collecting as a custom collector.
 func (c *containerData) housekeeping() {
+	// Start any background goroutines - must be cleaned up in c.handler.Cleanup().
+	c.handler.Start()
+
 	// Long housekeeping is either 100ms or half of the housekeeping interval.
 	longHousekeeping := 100 * time.Millisecond
 	if *HousekeepingInterval/2 < longHousekeeping {
diff --git a/manager/manager.go b/manager/manager.go
index d2be1461..6bc16b31 100644
--- a/manager/manager.go
+++ b/manager/manager.go
@@ -746,6 +746,18 @@ func (m *manager) registerCollectors(collectorConfigs map[string]string, cont *c
 
 // Create a container.
 func (m *manager) createContainer(containerName string) error {
+	m.containersLock.Lock()
+	defer m.containersLock.Unlock()
+
+	namespacedName := namespacedContainerName{
+		Name: containerName,
+	}
+
+	// Check that the container didn't already exist.
+	if _, ok := m.containers[namespacedName]; ok {
+		return nil
+	}
+
 	handler, accept, err := container.NewContainerHandler(containerName, m.inHostNamespace)
 	if err != nil {
 		return err
@@ -775,35 +787,15 @@ func (m *manager) createContainer(containerName string) error {
 		return err
 	}
 
-	// Add to the containers map.
-	alreadyExists := func() bool {
-		m.containersLock.Lock()
-		defer m.containersLock.Unlock()
-
-		namespacedName := namespacedContainerName{
-			Name: containerName,
-		}
-
-		// Check that the container didn't already exist.
-		_, ok := m.containers[namespacedName]
-		if ok {
-			return true
-		}
-
-		// Add the container name and all its aliases. The aliases must be within the namespace of the factory.
-		m.containers[namespacedName] = cont
-		for _, alias := range cont.info.Aliases {
-			m.containers[namespacedContainerName{
-				Namespace: cont.info.Namespace,
-				Name:      alias,
-			}] = cont
-		}
-
-		return false
-	}()
-	if alreadyExists {
-		return nil
+	// Add the container name and all its aliases. The aliases must be within the namespace of the factory.
+	m.containers[namespacedName] = cont
+	for _, alias := range cont.info.Aliases {
+		m.containers[namespacedContainerName{
+			Namespace: cont.info.Namespace,
+			Name:      alias,
+		}] = cont
 	}
+
 	glog.V(3).Infof("Added container: %q (aliases: %v, namespace: %q)", containerName, cont.info.Aliases, cont.info.Namespace)
 
 	contSpec, err := cont.handler.GetSpec()
@@ -827,9 +819,7 @@ func (m *manager) createContainer(containerName string) error {
 	}
 
 	// Start the container's housekeeping.
-	cont.Start()
-
-	return nil
+	return cont.Start()
 }
 
 func (m *manager) destroyContainer(containerName string) error {