From ef1344003409f4e3c6dfa8b8ffc02078463829ef Mon Sep 17 00:00:00 2001 From: Vishnu Kannan Date: Tue, 22 Jul 2014 02:50:32 +0000 Subject: [PATCH] Addressed comments. Another series of bug fixes. Modified the docker driver and lmctfy driver to skip containers they cannot handle. --- cadvisor.go | 11 ++-- container/docker/factory.go | 38 ++++++++++-- container/docker/handler.go | 98 ++++++++++++++----------------- container/factory.go | 10 +++- container/libcontainer/helpers.go | 16 ++--- container/lmctfy/factory.go | 6 ++ container/raw/handler.go | 2 +- manager/manager.go | 12 ++-- utils/path.go | 14 +++++ 9 files changed, 117 insertions(+), 90 deletions(-) diff --git a/cadvisor.go b/cadvisor.go index 4d60300d..75c12931 100644 --- a/cadvisor.go +++ b/cadvisor.go @@ -83,15 +83,14 @@ func main() { } }) - errChan := make(chan error) - go containerManager.Start(errChan) + go func() { + 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(<-errChan) + + log.Fatal(http.ListenAndServe(addr, nil)) } diff --git a/container/docker/factory.go b/container/docker/factory.go index 1aff45db..a8b3c186 100644 --- a/container/docker/factory.go +++ b/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 + } + } 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 } - return strings.HasPrefix(name, "/docker/") + // 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 { - client, err := docker.NewClient(*ArgDockerEndpoint) +func Register(factory info.MachineInfoFactory) (err error) { + 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} diff --git a/container/docker/handler.go b/container/docker/handler.go index 77eb035f..c6800c1a 100644 --- a/container/docker/handler.go +++ b/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) { - parent, id := path.Split(self.name) +func (self *dockerContainerHandler) isDockerRoot() bool { + // 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(dengnan): Should we consider other cases? - return self.name == "/docker" -} - -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") +// TODO(vmarmol): Switch to getting this from libcontainer once we have a solID API. +func (self *dockerContainerHandler) readLibcontainerConfig() (config *libcontainer.Config, err error) { + configPath := path.Join(dockerRootDir, self.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) { - statePath := path.Join(dockerRootDir, id, "state.json") +func (self *dockerContainerHandler) readLibcontainerState() (state *libcontainer.State, err error) { + 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() { - spec = new(info.ContainerSpec) - return + if self.isDockerRoot() { + return &info.ContainerSpec{}, nil } mi, err := self.machineInfoFactory.GetMachineInfo() if err != nil { return } - _, id, err := self.splitName() - if err != nil { - return - } - libcontainerConfig, err := readLibcontainerConfig(id) + libcontainerConfig, err := self.readLibcontainerConfig() 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() { - // Return empty stats for root containers. - stats = new(info.ContainerStats) - stats.Timestamp = time.Now() + if self.isDockerRoot() { + return &info.ContainerStats{}, nil + } + config, err := self.readLibcontainerConfig() + if err != nil { + if err == fileNotFound { + return &info.ContainerStats{}, nil + } return } - _, id, err := self.splitName() - if err != nil { - return - } - config, err := readLibcontainerConfig(id) - if err != nil { - return - } - state, err := readLibcontainerState(id) + state, err := self.readLibcontainerState() if err != nil { + if err == fileNotFound { + return &info.ContainerStats{}, 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 } diff --git a/container/factory.go b/container/factory.go index f96a23e7..369925d6 100644 --- a/container/factory.go +++ b/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 factoriesLock sync.RWMutex +var ( + 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. diff --git a/container/libcontainer/helpers.go b/container/libcontainer/helpers.go index b021ba08..e5ace212 100644 --- a/container/libcontainer/helpers.go +++ b/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) { - var ( - 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) - } +func GetStatsCgroupOnly(cgroup *cgroups.Cgroup) (*info.ContainerStats, error) { + 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 } diff --git a/container/lmctfy/factory.go b/container/lmctfy/factory.go index bd9e17c8..b8782675 100644 --- a/container/lmctfy/factory.go +++ b/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 } diff --git a/container/raw/handler.go b/container/raw/handler.go index 2f684c20..07a1a82e 100644 --- a/container/raw/handler.go +++ b/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". diff --git a/manager/manager.go b/manager/manager.go index 041599a4..4ec6fc49 100644 --- a/manager/manager.go +++ b/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 + return err } log.Printf("Starting recovery of all containers") err = m.detectContainers() if err != nil { - errChan <- err - return + return err } 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. diff --git a/utils/path.go b/utils/path.go index 115ddf2d..a7aceee6 100644 --- a/utils/path.go +++ b/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"