Addressed comments. Another series of bug fixes.

Modified the docker driver and lmctfy driver to skip containers they cannot handle.

cadvisor.go

@@ -83,15 +83,14 @@ func main() {
 		}
 	})
 
-	errChan := make(chan error)
+	go func() {
-	go containerManager.Start(errChan)
+		log.Fatal(containerManager.Start())
+	}()
 
 	log.Printf("Starting cAdvisor version: %q", info.VERSION)
 	log.Print("About to serve on port ", *argPort)
 
 	addr := fmt.Sprintf(":%v", *argPort)
-	go func() {
+
-		errChan <- http.ListenAndServe(addr, nil)
+	log.Fatal(http.ListenAndServe(addr, nil))
-	}()
-	log.Fatal(<-errChan)
 }

container/docker/factory.go

@@ -28,7 +28,10 @@ import (
 	"github.com/google/cadvisor/info"
 )
 
-var ArgDockerEndpoint = flag.String("docker", "unix:///var/run/docker.sock", "docker endpoint")
+var (
+	ArgDockerEndpoint = flag.String("docker", "unix:///var/run/docker.sock", "docker endpoint")
+	defaultClient     *docker.Client
+)
 
 type dockerFactory struct {
 	machineInfoFactory info.MachineInfoFactory
@@ -56,12 +59,35 @@ func (self *dockerFactory) NewContainerHandler(name string) (handler container.C
 }
 
 // Docker handles all containers under /docker
+// TODO(vishh): Change the CanHandle interface to be able to return errors.
 func (self *dockerFactory) CanHandle(name string) bool {
 	// In systemd systems the containers are: /docker-{ID}
 	if self.useSystemd {
-		return strings.HasPrefix(name, "/docker-")
+		if !strings.HasPrefix(name, "/docker-") {
+			return false
 		}
-	return strings.HasPrefix(name, "/docker/")
+	} else if name == "/" {
+		return false
+	} else if name == "/docker" {
+		// We need the docker driver to handle /docker. Otherwise the aggregation at the API level will break.
+		return true
+	}
+	// Check if the container is known to docker and it is active.
+	_, id, err := splitName(name)
+	if err != nil {
+		return false
+	}
+	if defaultClient == nil {
+		log.Fatal("Default docker client is nil")
+	}
+	ctnr, err := defaultClient.InspectContainer(id)
+	// We assume that if Inspect fails then the container is not known to docker.
+	// TODO(vishh): Detect lxc containers and avoid handling them.
+	if err != nil || !ctnr.State.Running {
+		return false
+	}
+
+	return true
 }
 
 func parseDockerVersion(full_version_string string) ([]int, error) {
@@ -84,12 +110,12 @@ func parseDockerVersion(full_version_string string) ([]int, error) {
 }
 
 // Register root container before running this function!
-func Register(factory info.MachineInfoFactory) error {
+func Register(factory info.MachineInfoFactory) (err error) {
-	client, err := docker.NewClient(*ArgDockerEndpoint)
+	defaultClient, err = docker.NewClient(*ArgDockerEndpoint)
 	if err != nil {
 		return fmt.Errorf("unable to communicate with docker daemon: %v", err)
 	}
-	if version, err := client.Version(); err != nil {
+	if version, err := defaultClient.Version(); err != nil {
 		return fmt.Errorf("unable to communicate with docker daemon: %v", err)
 	} else {
 		expected_version := []int{0, 11, 1}

container/docker/handler.go

@@ -17,13 +17,13 @@ package docker
 import (
 	"bufio"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"math"
 	"os"
 	"path"
 	"path/filepath"
 	"strings"
-	"time"
 
 	"github.com/docker/libcontainer"
 	"github.com/fsouza/go-dockerclient"
@@ -36,9 +36,13 @@ import (
 // Basepath to all container specific information that libcontainer stores.
 const dockerRootDir = "/var/lib/docker/execdriver/native"
 
+var fileNotFound = errors.New("file not found")
+
 type dockerContainerHandler struct {
 	client             *docker.Client
 	name               string
+	parent             string
+	ID                 string
 	aliases            []string
 	machineInfoFactory info.MachineInfoFactory
 	useSystemd         bool
@@ -56,13 +60,15 @@ func newDockerContainerHandler(
 		machineInfoFactory: machineInfoFactory,
 		useSystemd:         useSystemd,
 	}
-	if !handler.isDockerContainer() {
+	if handler.isDockerRoot() {
 		return handler, nil
 	}
-	_, id, err := handler.splitName()
+	parent, id, err := splitName(name)
 	if err != nil {
 		return nil, fmt.Errorf("invalid docker container %v: %v", name, err)
 	}
+	handler.parent = parent
+	handler.ID = id
 	ctnr, err := client.InspectContainer(id)
 	// We assume that if Inspect fails then the container is not known to docker.
 	if err != nil || !ctnr.State.Running {
@@ -79,8 +85,13 @@ func (self *dockerContainerHandler) ContainerReference() (info.ContainerReferenc
 	}, nil
 }
 
-func (self *dockerContainerHandler) splitName() (string, string, error) {
+func (self *dockerContainerHandler) isDockerRoot() bool {
-	parent, id := path.Split(self.name)
+	// TODO(dengnan): Should we consider other cases?
+	return self.name == "/docker"
+}
+
+func splitName(containerName string) (string, string, error) {
+	parent, id := path.Split(containerName)
 	cgroupSelf, err := os.Open("/proc/self/cgroup")
 	if err != nil {
 		return "", "", err
@@ -115,24 +126,11 @@ func (self *dockerContainerHandler) splitName() (string, string, error) {
 	return parent, id, nil
 }
 
-func (self *dockerContainerHandler) isDockerRoot() bool {
+// TODO(vmarmol): Switch to getting this from libcontainer once we have a solID API.
-	// TODO(dengnan): Should we consider other cases?
+func (self *dockerContainerHandler) readLibcontainerConfig() (config *libcontainer.Config, err error) {
-	return self.name == "/docker"
+	configPath := path.Join(dockerRootDir, self.ID, "container.json")
-}
-
-func (self *dockerContainerHandler) isRootContainer() bool {
-	return self.name == "/"
-}
-
-func (self *dockerContainerHandler) isDockerContainer() bool {
-	return (!self.isDockerRoot()) && (!self.isRootContainer())
-}
-
-// TODO(vmarmol): Switch to getting this from libcontainer once we have a solid API.
-func readLibcontainerConfig(id string) (config *libcontainer.Config, err error) {
-	configPath := path.Join(dockerRootDir, id, "container.json")
 	if !utils.FileExists(configPath) {
-		err = container.NotActive
+		err = fileNotFound
 		return
 	}
 	f, err := os.Open(configPath)
@@ -151,10 +149,10 @@ func readLibcontainerConfig(id string) (config *libcontainer.Config, err error)
 	return
 }
 
-func readLibcontainerState(id string) (state *libcontainer.State, err error) {
+func (self *dockerContainerHandler) readLibcontainerState() (state *libcontainer.State, err error) {
-	statePath := path.Join(dockerRootDir, id, "state.json")
+	statePath := path.Join(dockerRootDir, self.ID, "state.json")
 	if !utils.FileExists(statePath) {
-		err = container.NotActive
+		err = fileNotFound
 		return
 	}
 	f, err := os.Open(statePath)
@@ -204,19 +202,14 @@ func libcontainerConfigToContainerSpec(config *libcontainer.Config, mi *info.Mac
 }
 
 func (self *dockerContainerHandler) GetSpec() (spec *info.ContainerSpec, err error) {
-	if !self.isDockerContainer() {
+	if self.isDockerRoot() {
-		spec = new(info.ContainerSpec)
+		return &info.ContainerSpec{}, nil
-		return
 	}
 	mi, err := self.machineInfoFactory.GetMachineInfo()
 	if err != nil {
 		return
 	}
-	_, id, err := self.splitName()
+	libcontainerConfig, err := self.readLibcontainerConfig()
-	if err != nil {
-		return
-	}
-	libcontainerConfig, err := readLibcontainerConfig(id)
 	if err != nil {
 		return
 	}
@@ -226,22 +219,21 @@ func (self *dockerContainerHandler) GetSpec() (spec *info.ContainerSpec, err err
 }
 
 func (self *dockerContainerHandler) GetStats() (stats *info.ContainerStats, err error) {
-	if !self.isDockerContainer() {
+	if self.isDockerRoot() {
-		// Return empty stats for root containers.
+		return &info.ContainerStats{}, nil
-		stats = new(info.ContainerStats)
+	}
-		stats.Timestamp = time.Now()
+	config, err := self.readLibcontainerConfig()
+	if err != nil {
+		if err == fileNotFound {
+			return &info.ContainerStats{}, nil
+		}
 		return
 	}
-	_, id, err := self.splitName()
+	state, err := self.readLibcontainerState()
 	if err != nil {
-		return
+		if err == fileNotFound {
+			return &info.ContainerStats{}, nil
 		}
-	config, err := readLibcontainerConfig(id)
-	if err != nil {
-		return
-	}
-	state, err := readLibcontainerState(id)
-	if err != nil {
 		return
 	}
 
@@ -249,12 +241,6 @@ func (self *dockerContainerHandler) GetStats() (stats *info.ContainerStats, err
 }
 
 func (self *dockerContainerHandler) ListContainers(listType container.ListType) ([]info.ContainerReference, error) {
-	if self.isDockerContainer() {
-		return nil, nil
-	}
-	if self.isRootContainer() && listType == container.LIST_SELF {
-		return []info.ContainerReference{{Name: "/docker"}}, nil
-	}
 	opt := docker.ListContainersOptions{
 		All: true,
 	}
@@ -264,6 +250,10 @@ func (self *dockerContainerHandler) ListContainers(listType container.ListType)
 	}
 	ret := make([]info.ContainerReference, 0, len(containers)+1)
 	for _, c := range containers {
+		if c.ID == self.ID {
+			// Skip self.
+			continue
+		}
 		if !strings.HasPrefix(c.Status, "Up ") {
 			continue
 		}
@@ -275,9 +265,7 @@ func (self *dockerContainerHandler) ListContainers(listType container.ListType)
 		}
 		ret = append(ret, ref)
 	}
-	if self.isRootContainer() {
+
-		ret = append(ret, info.ContainerReference{Name: "/docker"})
-	}
 	return ret, nil
 }
 

container/factory.go

@@ -16,6 +16,7 @@ package container
 
 import (
 	"fmt"
+	"log"
 	"sync"
 )
 
@@ -32,8 +33,10 @@ type ContainerHandlerFactory interface {
 
 // TODO(vmarmol): Consider not making this global.
 // Global list of factories.
-var factories []ContainerHandlerFactory
+var (
-var factoriesLock sync.RWMutex
+	factories     []ContainerHandlerFactory
+	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.
@@ -52,11 +55,12 @@ func NewContainerHandler(name string) (ContainerHandler, error) {
 	// Create the ContainerHandler with the first factory that supports it.
 	for _, factory := range factories {
 		if factory.CanHandle(name) {
+			log.Printf("Using factory %q for container %q", factory.String(), name)
 			return factory.NewContainerHandler(name)
 		}
 	}
 
-	return nil, fmt.Errorf("no known factory can handle creation of container %q", name)
+	return nil, fmt.Errorf("no known factory can handle creation of container")
 }
 
 // Clear the known factories.

container/libcontainer/helpers.go

@@ -6,7 +6,6 @@ import (
 	"github.com/docker/libcontainer"
 	"github.com/docker/libcontainer/cgroups"
 	"github.com/docker/libcontainer/cgroups/fs"
-	"github.com/docker/libcontainer/cgroups/systemd"
 	"github.com/google/cadvisor/info"
 )
 
@@ -20,17 +19,8 @@ func GetStats(config *libcontainer.Config, state *libcontainer.State) (*info.Con
 	return toContainerStats(libcontainerStats), nil
 }
 
-func GetStatsCgroupOnly(cgroup *cgroups.Cgroup, useSystemd bool) (*info.ContainerStats, error) {
+func GetStatsCgroupOnly(cgroup *cgroups.Cgroup) (*info.ContainerStats, error) {
-	var (
+	s, err := fs.GetStats(cgroup)
-		s   *cgroups.Stats
-		err error
-	)
-	// Use systemd paths if systemd is being used.
-	if useSystemd {
-		s, err = systemd.GetStats(cgroup)
-	} else {
-		s, err = fs.GetStats(cgroup)
-	}
 	if err != nil {
 		return nil, err
 	}
@@ -69,6 +59,8 @@ func toContainerStats(libcontainerStats *libcontainer.ContainerStats) *info.Cont
 			ret.Memory.WorkingSet -= v
 		}
 	}
+	// TODO(vishh): Perform a deep copy or alias libcontainer network stats.
 	ret.Network = (*info.NetworkStats)(&libcontainerStats.NetworkStats)
+
 	return ret
 }

container/lmctfy/factory.go

@@ -50,5 +50,11 @@ func (self *lmctfyFactory) NewContainerHandler(name string) (container.Container
 
 func (self *lmctfyFactory) CanHandle(name string) bool {
 	// TODO(vmarmol): Try to attach to the container before blindly saying true.
+	cmd := exec.Command(lmctfyBinary, "stats", "summary", name)
+	_, err := cmd.Output()
+	if err != nil {
+		return false
+	}
+
 	return true
 }

container/raw/handler.go

@@ -58,7 +58,7 @@ func (self *rawContainerHandler) GetStats() (stats *info.ContainerStats, err err
 		Name:   self.name,
 	}
 
-	return libcontainer.GetStatsCgroupOnly(cgroup, false)
+	return libcontainer.GetStatsCgroupOnly(cgroup)
 }
 
 // Lists all directories under "path" and outputs the results as children of "parent".

manager/manager.go

@@ -27,7 +27,7 @@ import (
 
 type Manager interface {
 	// Start the manager, blocks forever.
-	Start(chanErr chan error)
+	Start() error
 
 	// Get information about a container.
 	GetContainerInfo(containerName string, query *info.ContainerInfoRequest) (*info.ContainerInfo, error)
@@ -73,18 +73,16 @@ type manager struct {
 }
 
 // Start the container manager.
-func (m *manager) Start(errChan chan error) {
+func (m *manager) Start() error {
 	// Create root and then recover all containers.
 	_, err := m.createContainer("/")
 	if err != nil {
-		errChan <- err
+		return err
-		return
 	}
 	log.Printf("Starting recovery of all containers")
 	err = m.detectContainers()
 	if err != nil {
-		errChan <- err
+		return err
-		return
 	}
 	log.Printf("Recovery completed")
 
@@ -104,7 +102,7 @@ func (m *manager) Start(errChan chan error) {
 			log.Printf("Global Housekeeping(%d) took %s", t.Unix(), duration)
 		}
 	}
-	errChan <- nil
+	return nil
 }
 
 // Get a container by name.

utils/path.go

@@ -1,3 +1,17 @@
+// Copyright 2014 Google Inc. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
 package utils
 
 import "os"