Merge pull request #90 from vmarmol/systemd

Change ContainerHandlerFactories to decide what containers they support.

cadvisor.go

@@ -30,7 +30,6 @@ import (
 )
 
 var argPort = flag.Int("port", 8080, "port to listen")
-var argAllowLmctfy = flag.Bool("allow_lmctfy", true, "whether to allow lmctfy as a container handler")
 
 var argDbDriver = flag.String("storage_driver", "memory", "storage driver to use. Options are: memory (default) and influxdb")
 
@@ -47,31 +46,17 @@ func main() {
 		log.Fatalf("Failed to create a Container Manager: %s", err)
 	}
 
-	// Register lmctfy for the root if allowed and available.
+	// Register Docker.
-	registeredRoot := false
+	if err := docker.Register(containerManager); err != nil {
-	if *argAllowLmctfy {
+		log.Printf("Docker registration failed: %v.", err)
-		if err := lmctfy.Register("/"); err != nil {
-			log.Printf("lmctfy registration failed: %v.", err)
-			log.Print("Running in docker only mode.")
-		} else {
-			registeredRoot = true
-		}
 	}
 
-	// Register Docker for root if we were unable to register lmctfy.
+	// Register lmctfy.
-	if !registeredRoot {
+	if err := lmctfy.Register(); err != nil {
-		if err := docker.Register(containerManager, "/"); err != nil {
+		log.Fatalf("lmctfy registration failed: %v.", err)
-			log.Printf("Docker registration failed: %v.", err)
-			log.Fatalf("Unable to continue without root handler.")
-		}
 	}
 
-	// Register Docker for all Docker containers.
+	// TODO(vmarmol): Have a no-op or "raw" factory.
-	if err := docker.Register(containerManager, "/docker"); err != nil {
-		// Ignore this error because we should work with lmctfy only
-		log.Printf("Docker registration failed: %v.", err)
-		log.Print("Running in lmctfy only mode.")
-	}
 
 	// Handler for static content.
 	http.HandleFunc(static.StaticResource, func(w http.ResponseWriter, r *http.Request) {

container/docker/factory.go

@@ -17,9 +17,12 @@ package docker
 import (
 	"flag"
 	"fmt"
+	"log"
 	"regexp"
 	"strconv"
+	"strings"
 
+	"github.com/docker/libcontainer/cgroups/systemd"
 	"github.com/fsouza/go-dockerclient"
 	"github.com/google/cadvisor/container"
 	"github.com/google/cadvisor/info"
@@ -29,6 +32,9 @@ var ArgDockerEndpoint = flag.String("docker", "unix:///var/run/docker.sock", "do
 
 type dockerFactory struct {
 	machineInfoFactory info.MachineInfoFactory
+
+	// Whether this system is using systemd.
+	hasSystemd bool
 }
 
 func (self *dockerFactory) String() string {
@@ -48,6 +54,15 @@ func (self *dockerFactory) NewContainerHandler(name string) (handler container.C
 	return
 }
 
+// Docker handles all containers under /docker
+func (self *dockerFactory) CanHandle(name string) bool {
+	// In systemd systems the containers are: /docker-{ID}
+	if self.hasSystemd {
+		return strings.HasPrefix(name, "/docker-")
+	}
+	return name == "/docker" || strings.HasPrefix(name, "/docker/")
+}
+
 func parseDockerVersion(full_version_string string) ([]int, error) {
 	version_regexp_string := "(\\d+)\\.(\\d+)\\.(\\d+)"
 	version_re := regexp.MustCompile(version_regexp_string)
@@ -68,7 +83,7 @@ func parseDockerVersion(full_version_string string) ([]int, error) {
 }
 
 // Register root container before running this function!
-func Register(factory info.MachineInfoFactory, paths ...string) error {
+func Register(factory info.MachineInfoFactory) error {
 	client, err := docker.NewClient(*ArgDockerEndpoint)
 	if err != nil {
 		return fmt.Errorf("unable to communicate with docker daemon: %v", err)
@@ -92,12 +107,9 @@ func Register(factory info.MachineInfoFactory, paths ...string) error {
 	}
 	f := &dockerFactory{
 		machineInfoFactory: factory,
+		hasSystemd:         systemd.UseSystemd(),
 	}
-	for _, p := range paths {
+	log.Printf("Registering Docker factory")
-		if p != "/" && p != "/docker" {
+	container.RegisterContainerHandlerFactory(f)
-			return fmt.Errorf("%v cannot be managed by docker", p)
-		}
-		container.RegisterContainerHandlerFactory(p, f)
-	}
 	return nil
 }

container/factory.go

@@ -17,113 +17,54 @@ package container
 import (
 	"fmt"
 	"log"
-	"strings"
 	"sync"
 )
 
 type ContainerHandlerFactory interface {
+	// Create a new ContainerHandler using this factory. CanHandle() must have returned true.
 	NewContainerHandler(name string) (ContainerHandler, error)
 
-	// for testability
+	// Returns whether this factory can handle the specified container.
+	CanHandle(name string) bool
+
+	// Name of the factory.
 	String() string
 }
 
-type factoryTreeNode struct {
+// TODO(vmarmol): Consider not making this global.
-	defaultFactory ContainerHandlerFactory
+// Global list of factories.
-	children       map[string]*factoryTreeNode
+var factories []ContainerHandlerFactory
+var factoriesLock sync.RWMutex
+
+// Register a ContainerHandlerFactory. These should be registered from least general to most general
+// as they will be asked in order whether they can handle a particular container.
+func RegisterContainerHandlerFactory(factory ContainerHandlerFactory) {
+	factoriesLock.Lock()
+	defer factoriesLock.Unlock()
+
+	factories = append(factories, factory)
 }
 
-func (self *factoryTreeNode) find(elems ...string) ContainerHandlerFactory {
+// Create a new ContainerHandler for the specified container.
-	node := self
+func NewContainerHandler(name string) (ContainerHandler, error) {
-	for _, elem := range elems {
+	factoriesLock.RLock()
-		if len(node.children) == 0 {
+	defer factoriesLock.RUnlock()
-			break
+
-		}
+	// Create the ContainerHandler with the first factory that supports it.
-		if child, ok := node.children[elem]; ok {
+	for _, factory := range factories {
-			node = child
+		if factory.CanHandle(name) {
-		} else {
+			log.Printf("Using factory %q for container %q", factory.String(), name)
-			return node.defaultFactory
+			return factory.NewContainerHandler(name)
 		}
 	}
 
-	return node.defaultFactory
+	return nil, fmt.Errorf("no known factory can handle creation of container %q", name)
 }
 
-func (self *factoryTreeNode) add(factory ContainerHandlerFactory, elems ...string) {
+// Clear the known factories.
-	node := self
+func ClearContainerHandlerFactories() {
-	for _, elem := range elems {
+	factoriesLock.Lock()
-		if node.children == nil {
+	defer factoriesLock.Unlock()
-			node.children = make(map[string]*factoryTreeNode, 16)
-		}
-		child, ok := self.children[elem]
-		if !ok {
-			child = &factoryTreeNode{
-				defaultFactory: node.defaultFactory,
-				children:       make(map[string]*factoryTreeNode, 16),
-			}
-			node.children[elem] = child
-		}
-		node = child
-	}
-	node.defaultFactory = factory
-}
 
-type factoryManager struct {
+	factories = make([]ContainerHandlerFactory, 0, 4)
-	root *factoryTreeNode
-	lock sync.RWMutex
-}
-
-func dropEmptyString(elems ...string) []string {
-	ret := make([]string, 0, len(elems))
-	for _, e := range elems {
-		if len(e) > 0 {
-			ret = append(ret, e)
-		}
-	}
-	return ret
-}
-
-// Must register factory for root container!
-func (self *factoryManager) Register(path string, factory ContainerHandlerFactory) {
-	self.lock.Lock()
-	defer self.lock.Unlock()
-
-	if self.root == nil {
-		self.root = &factoryTreeNode{
-			defaultFactory: nil,
-			children:       make(map[string]*factoryTreeNode, 10),
-		}
-	}
-
-	elems := dropEmptyString(strings.Split(path, "/")...)
-	self.root.add(factory, elems...)
-}
-
-func (self *factoryManager) NewContainerHandler(path string) (ContainerHandler, error) {
-	self.lock.RLock()
-	defer self.lock.RUnlock()
-
-	if self.root == nil {
-		err := fmt.Errorf("nil factory for container %v: no factory registered", path)
-		return nil, err
-	}
-
-	elems := dropEmptyString(strings.Split(path, "/")...)
-	factory := self.root.find(elems...)
-	if factory == nil {
-		err := fmt.Errorf("nil factory for container %v", path)
-		return nil, err
-	}
-	log.Printf("Container handler factory for %v is %v\n", path, factory)
-	return factory.NewContainerHandler(path)
-}
-
-var globalFactoryManager factoryManager
-
-func RegisterContainerHandlerFactory(path string, factory ContainerHandlerFactory) {
-	globalFactoryManager.Register(path, factory)
-}
-
-func NewContainerHandler(path string) (ContainerHandler, error) {
-	return globalFactoryManager.NewContainerHandler(path)
 }

container/factory_test.go

@@ -15,7 +15,6 @@
 package container
 
 import (
-	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/mock"
@@ -24,53 +23,100 @@ import (
 type mockContainerHandlerFactory struct {
 	mock.Mock
 	Name           string
+	CanHandleValue bool
 }
 
 func (self *mockContainerHandlerFactory) String() string {
 	return self.Name
 }
 
+func (self *mockContainerHandlerFactory) CanHandle(name string) bool {
+	return self.CanHandleValue
+}
+
 func (self *mockContainerHandlerFactory) NewContainerHandler(name string) (ContainerHandler, error) {
 	args := self.Called(name)
 	return args.Get(0).(ContainerHandler), args.Error(1)
 }
 
-func testExpectedFactory(root *factoryTreeNode, path, expectedFactory string, t *testing.T) {
+const testContainerName = "/test"
-	elems := dropEmptyString(strings.Split(path, "/")...)
+
-	factory := root.find(elems...)
+var mockFactory FactoryForMockContainerHandler
-	if factory.String() != expectedFactory {
+
-		t.Errorf("factory %v should be used to create container %v. but %v is selected",
+func TestNewContainerHandler_FirstMatches(t *testing.T) {
-			expectedFactory,
+	ClearContainerHandlerFactories()
-			path,
+
-			factory)
+	// Register one allways yes factory.
+	allwaysYes := &mockContainerHandlerFactory{
+		Name:           "yes",
+		CanHandleValue: true,
+	}
+	RegisterContainerHandlerFactory(allwaysYes)
+
+	// The yes factory should be asked to create the ContainerHandler.
+	mockContainer, err := mockFactory.NewContainerHandler(testContainerName)
+	if err != nil {
+		t.Error(err)
+	}
+	allwaysYes.On("NewContainerHandler", testContainerName).Return(mockContainer, nil)
+
+	cont, err := NewContainerHandler(testContainerName)
+	if err != nil {
+		t.Error(err)
+	}
+	if cont == nil {
+		t.Error("Expected container to not be nil")
 	}
 }
 
-func testAddFactory(root *factoryTreeNode, path string) *factoryTreeNode {
+func TestNewContainerHandler_SecondMatches(t *testing.T) {
-	elems := dropEmptyString(strings.Split(path, "/")...)
+	ClearContainerHandlerFactories()
-	if root == nil {
+
-		root = &factoryTreeNode{
+	// Register one allways no and one always yes factory.
-			defaultFactory: nil,
+	allwaysNo := &mockContainerHandlerFactory{
+		Name:           "no",
+		CanHandleValue: false,
 	}
+	RegisterContainerHandlerFactory(allwaysNo)
+	allwaysYes := &mockContainerHandlerFactory{
+		Name:           "yes",
+		CanHandleValue: true,
 	}
-	f := &mockContainerHandlerFactory{
+	RegisterContainerHandlerFactory(allwaysYes)
-		Name: path,
+
+	// The yes factory should be asked to create the ContainerHandler.
+	mockContainer, err := mockFactory.NewContainerHandler(testContainerName)
+	if err != nil {
+		t.Error(err)
+	}
+	allwaysYes.On("NewContainerHandler", testContainerName).Return(mockContainer, nil)
+
+	cont, err := NewContainerHandler(testContainerName)
+	if err != nil {
+		t.Error(err)
+	}
+	if cont == nil {
+		t.Error("Expected container to not be nil")
 	}
-	root.add(f, elems...)
-	return root
 }
 
-func TestFactoryTree(t *testing.T) {
+func TestNewContainerHandler_NoneMatch(t *testing.T) {
-	root := testAddFactory(nil, "/")
+	ClearContainerHandlerFactories()
-	root = testAddFactory(root, "/docker")
-	root = testAddFactory(root, "/user")
-	root = testAddFactory(root, "/user/special/containers")
 
-	testExpectedFactory(root, "/docker/container", "/docker", t)
+	// Register two allways no factories.
-	testExpectedFactory(root, "/docker", "/docker", t)
+	allwaysNo1 := &mockContainerHandlerFactory{
-	testExpectedFactory(root, "/", "/", t)
+		Name:           "no",
-	testExpectedFactory(root, "/user/deep/level/container", "/user", t)
+		CanHandleValue: false,
-	testExpectedFactory(root, "/user/special/containers", "/user/special/containers", t)
+	}
-	testExpectedFactory(root, "/user/special/containers/container", "/user/special/containers", t)
+	RegisterContainerHandlerFactory(allwaysNo1)
-	testExpectedFactory(root, "/other", "/", t)
+	allwaysNo2 := &mockContainerHandlerFactory{
+		Name:           "no",
+		CanHandleValue: false,
+	}
+	RegisterContainerHandlerFactory(allwaysNo2)
+
+	_, err := NewContainerHandler(testContainerName)
+	if err == nil {
+		t.Error("Expected NewContainerHandler to fail")
+	}
 }

container/lmctfy/factory.go

@@ -22,15 +22,12 @@ import (
 	"github.com/google/cadvisor/container"
 )
 
-func Register(paths ...string) error {
+func Register() error {
 	if _, err := exec.LookPath("lmctfy"); err != nil {
 		return errors.New("cannot find lmctfy")
 	}
-	f := &lmctfyFactory{}
+	log.Printf("Registering lmctfy factory")
-	for _, path := range paths {
+	container.RegisterContainerHandlerFactory(&lmctfyFactory{})
-		log.Printf("register lmctfy under %v", path)
-		container.RegisterContainerHandlerFactory(path, f)
-	}
 	return nil
 }
 
@@ -50,3 +47,8 @@ func (self *lmctfyFactory) NewContainerHandler(name string) (container.Container
 	handler := container.NewBlackListFilter(c, "/user")
 	return handler, nil
 }
+
+func (self *lmctfyFactory) CanHandle(name string) bool {
+	// TODO(vmarmol): Try to attach to the container before blindly saying true.
+	return true
+}

container/mock.go

@@ -12,10 +12,9 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package test
+package container
 
 import (
-	"github.com/google/cadvisor/container"
 	"github.com/google/cadvisor/info"
 	"github.com/stretchr/testify/mock"
 )
@@ -55,17 +54,17 @@ func (self *MockContainerHandler) GetStats() (*info.ContainerStats, error) {
 	return args.Get(0).(*info.ContainerStats), args.Error(1)
 }
 
-func (self *MockContainerHandler) ListContainers(listType container.ListType) ([]info.ContainerReference, error) {
+func (self *MockContainerHandler) ListContainers(listType ListType) ([]info.ContainerReference, error) {
 	args := self.Called(listType)
 	return args.Get(0).([]info.ContainerReference), args.Error(1)
 }
 
-func (self *MockContainerHandler) ListThreads(listType container.ListType) ([]int, error) {
+func (self *MockContainerHandler) ListThreads(listType ListType) ([]int, error) {
 	args := self.Called(listType)
 	return args.Get(0).([]int), args.Error(1)
 }
 
-func (self *MockContainerHandler) ListProcesses(listType container.ListType) ([]int, error) {
+func (self *MockContainerHandler) ListProcesses(listType ListType) ([]int, error) {
 	args := self.Called(listType)
 	return args.Get(0).([]int), args.Error(1)
 }
@@ -79,10 +78,14 @@ func (self *FactoryForMockContainerHandler) String() string {
 	return self.Name
 }
 
-func (self *FactoryForMockContainerHandler) NewContainerHandler(name string) (container.ContainerHandler, error) {
+func (self *FactoryForMockContainerHandler) NewContainerHandler(name string) (ContainerHandler, error) {
 	handler := &MockContainerHandler{}
 	if self.PrepareContainerHandlerFunc != nil {
 		self.PrepareContainerHandlerFunc(name, handler)
 	}
 	return handler, nil
 }
+
+func (self *FactoryForMockContainerHandler) CanHandle(name string) bool {
+	return true
+}

manager/container_test.go

@@ -23,7 +23,6 @@ import (
 	"time"
 
 	"github.com/google/cadvisor/container"
-	ctest "github.com/google/cadvisor/container/test"
 	"github.com/google/cadvisor/info"
 	itest "github.com/google/cadvisor/info/test"
 	"github.com/google/cadvisor/storage"
@@ -32,17 +31,18 @@ import (
 
 func createContainerDataAndSetHandler(
 	driver storage.StorageDriver,
-	f func(*ctest.MockContainerHandler),
+	f func(*container.MockContainerHandler),
 	t *testing.T,
 ) *containerData {
-	factory := &ctest.FactoryForMockContainerHandler{
+	factory := &container.FactoryForMockContainerHandler{
 		Name: "factoryForMockContainer",
-		PrepareContainerHandlerFunc: func(name string, handler *ctest.MockContainerHandler) {
+		PrepareContainerHandlerFunc: func(name string, handler *container.MockContainerHandler) {
 			handler.Name = name
 			f(handler)
 		},
 	}
-	container.RegisterContainerHandlerFactory("/", factory)
+	container.ClearContainerHandlerFactories()
+	container.RegisterContainerHandlerFactory(factory)
 
 	if driver == nil {
 		driver = &stest.MockStorageDriver{}
@@ -56,7 +56,7 @@ func createContainerDataAndSetHandler(
 }
 
 func TestContainerUpdateSubcontainers(t *testing.T) {
-	var handler *ctest.MockContainerHandler
+	var handler *container.MockContainerHandler
 	subcontainers := []info.ContainerReference{
 		{Name: "/container/ee0103"},
 		{Name: "/container/abcd"},
@@ -64,7 +64,7 @@ func TestContainerUpdateSubcontainers(t *testing.T) {
 	}
 	cd := createContainerDataAndSetHandler(
 		nil,
-		func(h *ctest.MockContainerHandler) {
+		func(h *container.MockContainerHandler) {
 			h.On("ListContainers", container.LIST_SELF).Return(
 				subcontainers,
 				nil,
@@ -99,10 +99,10 @@ func TestContainerUpdateSubcontainers(t *testing.T) {
 }
 
 func TestContainerUpdateSubcontainersWithError(t *testing.T) {
-	var handler *ctest.MockContainerHandler
+	var handler *container.MockContainerHandler
 	cd := createContainerDataAndSetHandler(
 		nil,
-		func(h *ctest.MockContainerHandler) {
+		func(h *container.MockContainerHandler) {
 			h.On("ListContainers", container.LIST_SELF).Return(
 				[]info.ContainerReference{},
 				fmt.Errorf("some error"),
@@ -124,7 +124,7 @@ func TestContainerUpdateSubcontainersWithError(t *testing.T) {
 }
 
 func TestContainerUpdateStats(t *testing.T) {
-	var handler *ctest.MockContainerHandler
+	var handler *container.MockContainerHandler
 	var ref info.ContainerReference
 
 	driver := &stest.MockStorageDriver{}
@@ -134,7 +134,7 @@ func TestContainerUpdateStats(t *testing.T) {
 
 	cd := createContainerDataAndSetHandler(
 		driver,
-		func(h *ctest.MockContainerHandler) {
+		func(h *container.MockContainerHandler) {
 			h.On("GetStats").Return(
 				stats,
 				nil,
@@ -156,11 +156,11 @@ func TestContainerUpdateStats(t *testing.T) {
 }
 
 func TestContainerUpdateSpec(t *testing.T) {
-	var handler *ctest.MockContainerHandler
+	var handler *container.MockContainerHandler
 	spec := itest.GenerateRandomContainerSpec(4)
 	cd := createContainerDataAndSetHandler(
 		nil,
-		func(h *ctest.MockContainerHandler) {
+		func(h *container.MockContainerHandler) {
 			h.On("GetSpec").Return(
 				spec,
 				nil,
@@ -179,7 +179,7 @@ func TestContainerUpdateSpec(t *testing.T) {
 }
 
 func TestContainerGetInfo(t *testing.T) {
-	var handler *ctest.MockContainerHandler
+	var handler *container.MockContainerHandler
 	spec := itest.GenerateRandomContainerSpec(4)
 	subcontainers := []info.ContainerReference{
 		{Name: "/container/ee0103"},
@@ -189,7 +189,7 @@ func TestContainerGetInfo(t *testing.T) {
 	aliases := []string{"a1", "a2"}
 	cd := createContainerDataAndSetHandler(
 		nil,
-		func(h *ctest.MockContainerHandler) {
+		func(h *container.MockContainerHandler) {
 			h.On("GetSpec").Return(
 				spec,
 				nil,

manager/manager_test.go

@@ -22,7 +22,6 @@ import (
 	"time"
 
 	"github.com/google/cadvisor/container"
-	ctest "github.com/google/cadvisor/container/test"
 	"github.com/google/cadvisor/info"
 	itest "github.com/google/cadvisor/info/test"
 	stest "github.com/google/cadvisor/storage/test"
@@ -31,15 +30,15 @@ import (
 func createManagerAndAddContainers(
 	driver *stest.MockStorageDriver,
 	containers []string,
-	f func(*ctest.MockContainerHandler),
+	f func(*container.MockContainerHandler),
 	t *testing.T,
 ) *manager {
 	if driver == nil {
 		driver = &stest.MockStorageDriver{}
 	}
-	factory := &ctest.FactoryForMockContainerHandler{
+	factory := &container.FactoryForMockContainerHandler{
 		Name: "factoryForManager",
-		PrepareContainerHandlerFunc: func(name string, handler *ctest.MockContainerHandler) {
+		PrepareContainerHandlerFunc: func(name string, handler *container.MockContainerHandler) {
 			handler.Name = name
 			found := false
 			for _, c := range containers {
@@ -53,7 +52,8 @@ func createManagerAndAddContainers(
 			f(handler)
 		},
 	}
-	container.RegisterContainerHandlerFactory("/", factory)
+	container.ClearContainerHandlerFactories()
+	container.RegisterContainerHandlerFactory(factory)
 	mif, err := New(driver)
 	if err != nil {
 		t.Fatal(err)
@@ -85,7 +85,7 @@ func TestGetContainerInfo(t *testing.T) {
 	}
 
 	infosMap := make(map[string]*info.ContainerInfo, len(containers))
-	handlerMap := make(map[string]*ctest.MockContainerHandler, len(containers))
+	handlerMap := make(map[string]*container.MockContainerHandler, len(containers))
 
 	for _, container := range containers {
 		infosMap[container] = itest.GenerateRandomContainerInfo(container, 4, query, 1*time.Second)
@@ -95,7 +95,7 @@ func TestGetContainerInfo(t *testing.T) {
 	m := createManagerAndAddContainers(
 		driver,
 		containers,
-		func(h *ctest.MockContainerHandler) {
+		func(h *container.MockContainerHandler) {
 			cinfo := infosMap[h.Name]
 			stats := cinfo.Stats
 			samples := cinfo.Samples
@@ -173,7 +173,7 @@ func TestGetContainerInfoWithDefaultValue(t *testing.T) {
 	query = query.FillDefaults()
 
 	infosMap := make(map[string]*info.ContainerInfo, len(containers))
-	handlerMap := make(map[string]*ctest.MockContainerHandler, len(containers))
+	handlerMap := make(map[string]*container.MockContainerHandler, len(containers))
 
 	for _, container := range containers {
 		infosMap[container] = itest.GenerateRandomContainerInfo(container, 4, query, 1*time.Second)
@@ -183,7 +183,7 @@ func TestGetContainerInfoWithDefaultValue(t *testing.T) {
 	m := createManagerAndAddContainers(
 		driver,
 		containers,
-		func(h *ctest.MockContainerHandler) {
+		func(h *container.MockContainerHandler) {
 			cinfo := infosMap[h.Name]
 			stats := cinfo.Stats
 			samples := cinfo.Samples