Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix name resolution in Systemd systems #220

Merged
merged 1 commit into from
Sep 29, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 4 additions & 5 deletions container/docker/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ package docker
import (
"flag"
"fmt"
"path"
"regexp"
"strconv"
"strings"
Expand Down Expand Up @@ -78,11 +77,12 @@ func (self *dockerFactory) CanHandle(name string) bool {
} else if !strings.HasPrefix(name, "/docker/") {
return false
}

// Check if the container is known to docker and it is active.
id := path.Base(name)
ctnr, err := self.client.InspectContainer(id)
id := containerNameToDockerId(name, self.useSystemd)

// We assume that if Inspect fails then the container is not known to docker.
// TODO(vishh): Detect lxc containers and avoid handling them.
ctnr, err := self.client.InspectContainer(id)
if err != nil || !ctnr.State.Running {
return false
}
Expand Down Expand Up @@ -157,7 +157,6 @@ func Register(factory info.MachineInfoFactory) error {
if f.useSystemd {
glog.Infof("System is using systemd")
}
glog.Infof("Registering Docker factory")
container.RegisterContainerHandlerFactory(f)
return nil
}
47 changes: 28 additions & 19 deletions container/docker/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,12 @@ 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
libcontainerStateDir string
cgroup cgroups.Cgroup
}

func newDockerContainerHandler(
Expand All @@ -63,12 +63,15 @@ func newDockerContainerHandler(
machineInfoFactory: machineInfoFactory,
useSystemd: useSystemd,
libcontainerStateDir: path.Join(dockerRootDir, pathToLibcontainerState),
cgroup: cgroups.Cgroup{
Parent: "/",
Name: name,
},
}
if handler.isDockerRoot() {
return handler, nil
}
id := path.Base(name)
handler.parent = path.Dir(name)
id := containerNameToDockerId(name, useSystemd)
handler.id = id
ctnr, err := client.InspectContainer(id)
// We assume that if Inspect fails then the container is not known to docker.
Expand All @@ -79,6 +82,25 @@ func newDockerContainerHandler(
return handler, nil
}

func containerNameToDockerId(name string, useSystemd bool) string {
id := path.Base(name)

// Turn systemd cgroup name into Docker ID.
if useSystemd {
const systemdDockerPrefix = "docker-"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we instead export "docker-" prefix from libcontainer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libcontainer as in container/libcontainer/helpers.go?

I think the right place for this is here since it is a Docker concept and not a libcontainer one (as in, Docker prefixes it's containers with that).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant docker/libcontainer. If libcontainer is not the source of the prefix, then the current code is fine as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libcontainer does some sad mangling of container names I'm trying to remove in a PR :( once that goes in, Docker will be the one doing the prefixing.

if strings.HasPrefix(id, systemdDockerPrefix) {
id = id[len(systemdDockerPrefix):]
}

const systemdScopeSuffix = ".scope"
if strings.HasSuffix(id, systemdScopeSuffix) {
id = id[:len(id)-len(systemdScopeSuffix)]
}
}

return id
}

func (self *dockerContainerHandler) ContainerReference() (info.ContainerReference, error) {
return info.ContainerReference{
Name: self.name,
Expand Down Expand Up @@ -112,8 +134,7 @@ func (self *dockerContainerHandler) readLibcontainerConfig() (config *libcontain
config = retConfig

// Replace cgroup parent and name with our own since we may be running in a different context.
config.Cgroups.Parent = self.parent
config.Cgroups.Name = self.id
*config.Cgroups = self.cgroup

return
}
Expand Down Expand Up @@ -199,14 +220,6 @@ func (self *dockerContainerHandler) GetStats() (stats *info.ContainerStats, err
if self.isDockerRoot() {
return &info.ContainerStats{}, nil
}
config, err := self.readLibcontainerConfig()
if err != nil {
if err == fileNotFound {
glog.Errorf("Libcontainer config not found for container %q", self.name)
return &info.ContainerStats{}, nil
}
return
}
state, err := self.readLibcontainerState()
if err != nil {
if err == fileNotFound {
Expand All @@ -216,7 +229,7 @@ func (self *dockerContainerHandler) GetStats() (stats *info.ContainerStats, err
return
}

return containerLibcontainer.GetStats(config, state)
return containerLibcontainer.GetStats(&self.cgroup, state)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this mean we will loose network stats? If so, why not make this change only on systemd systems?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we still have the state so GetStats fetches network stats. I checked just in case :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh ok :)

}

func (self *dockerContainerHandler) ListContainers(listType container.ListType) ([]info.ContainerReference, error) {
Expand Down Expand Up @@ -258,11 +271,7 @@ func (self *dockerContainerHandler) ListThreads(listType container.ListType) ([]
}

func (self *dockerContainerHandler) ListProcesses(listType container.ListType) ([]int, error) {
c := &cgroups.Cgroup{
Parent: self.parent,
Name: self.id,
}
return fs.GetPids(c)
return fs.GetPids(&self.cgroup)
}

func (self *dockerContainerHandler) WatchSubcontainers(events chan container.SubcontainerEvent) error {
Expand Down
14 changes: 10 additions & 4 deletions container/libcontainer/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,23 @@ import (
"github.com/docker/libcontainer"
"github.com/docker/libcontainer/cgroups"
cgroupfs "github.com/docker/libcontainer/cgroups/fs"
"github.com/docker/libcontainer/network"
"github.com/google/cadvisor/info"
)

// Get stats of the specified container
func GetStats(config *libcontainer.Config, state *libcontainer.State) (*info.ContainerStats, error) {
func GetStats(cgroup *cgroups.Cgroup, state *libcontainer.State) (*info.ContainerStats, error) {
// TODO(vmarmol): Use libcontainer's Stats() in the new API when that is ready.
libcontainerStats, err := libcontainer.GetStats(config, state)
stats := &libcontainer.ContainerStats{}

var err error
stats.CgroupStats, err = cgroupfs.GetStats(cgroup)
if err != nil {
return nil, err
return &info.ContainerStats{}, err
}
return toContainerStats(libcontainerStats), nil

stats.NetworkStats, err = network.GetStats(&state.NetworkState)
return toContainerStats(stats), nil
}

func GetStatsCgroupOnly(cgroup *cgroups.Cgroup) (*info.ContainerStats, error) {
Expand Down