diff --git a/cadvisor.go b/cadvisor.go index 5df6e27d..58d0d95a 100644 --- a/cadvisor.go +++ b/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,32 +46,18 @@ func main() { log.Fatalf("Failed to create a Container Manager: %s", err) } - // Register lmctfy for the root if allowed and available. - registeredRoot := false - if *argAllowLmctfy { - 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. - if !registeredRoot { - if err := docker.Register(containerManager, "/"); err != nil { - log.Printf("Docker registration failed: %v.", err) - log.Fatalf("Unable to continue without root handler.") - } - } - - // Register Docker for all Docker containers. - if err := docker.Register(containerManager, "/docker"); err != nil { - // Ignore this error because we should work with lmctfy only + // Register Docker. + if err := docker.Register(containerManager); err != nil { log.Printf("Docker registration failed: %v.", err) - log.Print("Running in lmctfy only mode.") } + // Register lmctfy. + if err := lmctfy.Register(); err != nil { + log.Fatalf("lmctfy registration failed: %v.", err) + } + + // TODO(vmarmol): Have a no-op or "raw" factory. + // Handler for static content. http.HandleFunc(static.StaticResource, func(w http.ResponseWriter, r *http.Request) { err := static.HandleRequest(w, r.URL) diff --git a/container/docker/factory.go b/container/docker/factory.go index 4d8165ab..879595b6 100644 --- a/container/docker/factory.go +++ b/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 { - if p != "/" && p != "/docker" { - return fmt.Errorf("%v cannot be managed by docker", p) - } - container.RegisterContainerHandlerFactory(p, f) - } + log.Printf("Registering Docker factory") + container.RegisterContainerHandlerFactory(f) return nil } diff --git a/container/factory.go b/container/factory.go index 24f82ed1..54cbbc6c 100644 --- a/container/factory.go +++ b/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 { - defaultFactory ContainerHandlerFactory - children map[string]*factoryTreeNode +// TODO(vmarmol): Consider not making this global. +// Global list of factories. +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 { - node := self - for _, elem := range elems { - if len(node.children) == 0 { - break - } - if child, ok := node.children[elem]; ok { - node = child - } else { - return node.defaultFactory +// Create a new ContainerHandler for the specified container. +func NewContainerHandler(name string) (ContainerHandler, error) { + factoriesLock.RLock() + defer factoriesLock.RUnlock() + + // 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 node.defaultFactory + return nil, fmt.Errorf("no known factory can handle creation of container %q", name) } -func (self *factoryTreeNode) add(factory ContainerHandlerFactory, elems ...string) { - node := self - for _, elem := range elems { - if node.children == nil { - 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 { - 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) +// Clear the known factories. +func ClearContainerHandlerFactories() { + factoriesLock.Lock() + defer factoriesLock.Unlock() + + factories = make([]ContainerHandlerFactory, 0, 4) } diff --git a/container/factory_test.go b/container/factory_test.go index 1a688417..b96caab5 100644 --- a/container/factory_test.go +++ b/container/factory_test.go @@ -15,7 +15,6 @@ package container import ( - "strings" "testing" "github.com/stretchr/testify/mock" @@ -23,54 +22,101 @@ import ( type mockContainerHandlerFactory struct { mock.Mock - Name string + 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) { - elems := dropEmptyString(strings.Split(path, "/")...) - factory := root.find(elems...) - if factory.String() != expectedFactory { - t.Errorf("factory %v should be used to create container %v. but %v is selected", - expectedFactory, - path, - factory) +const testContainerName = "/test" + +var mockFactory FactoryForMockContainerHandler + +func TestNewContainerHandler_FirstMatches(t *testing.T) { + ClearContainerHandlerFactories() + + // 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 { - elems := dropEmptyString(strings.Split(path, "/")...) - if root == nil { - root = &factoryTreeNode{ - defaultFactory: nil, - } +func TestNewContainerHandler_SecondMatches(t *testing.T) { + ClearContainerHandlerFactories() + + // Register one allways no and one always yes factory. + allwaysNo := &mockContainerHandlerFactory{ + Name: "no", + CanHandleValue: false, } - f := &mockContainerHandlerFactory{ - Name: path, + RegisterContainerHandlerFactory(allwaysNo) + 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") } - root.add(f, elems...) - return root } -func TestFactoryTree(t *testing.T) { - root := testAddFactory(nil, "/") - root = testAddFactory(root, "/docker") - root = testAddFactory(root, "/user") - root = testAddFactory(root, "/user/special/containers") +func TestNewContainerHandler_NoneMatch(t *testing.T) { + ClearContainerHandlerFactories() - testExpectedFactory(root, "/docker/container", "/docker", t) - testExpectedFactory(root, "/docker", "/docker", t) - testExpectedFactory(root, "/", "/", t) - testExpectedFactory(root, "/user/deep/level/container", "/user", t) - testExpectedFactory(root, "/user/special/containers", "/user/special/containers", t) - testExpectedFactory(root, "/user/special/containers/container", "/user/special/containers", t) - testExpectedFactory(root, "/other", "/", t) + // Register two allways no factories. + allwaysNo1 := &mockContainerHandlerFactory{ + Name: "no", + CanHandleValue: false, + } + RegisterContainerHandlerFactory(allwaysNo1) + allwaysNo2 := &mockContainerHandlerFactory{ + Name: "no", + CanHandleValue: false, + } + RegisterContainerHandlerFactory(allwaysNo2) + + _, err := NewContainerHandler(testContainerName) + if err == nil { + t.Error("Expected NewContainerHandler to fail") + } } diff --git a/container/lmctfy/factory.go b/container/lmctfy/factory.go index 104c6a68..bd9e17c8 100644 --- a/container/lmctfy/factory.go +++ b/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{} - for _, path := range paths { - log.Printf("register lmctfy under %v", path) - container.RegisterContainerHandlerFactory(path, f) - } + log.Printf("Registering lmctfy factory") + container.RegisterContainerHandlerFactory(&lmctfyFactory{}) 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 +} diff --git a/container/test/mock.go b/container/mock.go similarity index 85% rename from container/test/mock.go rename to container/mock.go index fbbe8fe0..90846c54 100644 --- a/container/test/mock.go +++ b/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 +} diff --git a/manager/container_test.go b/manager/container_test.go index c942eb81..5910ed3b 100644 --- a/manager/container_test.go +++ b/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, diff --git a/manager/manager_test.go b/manager/manager_test.go index 99862c5d..590afa6a 100644 --- a/manager/manager_test.go +++ b/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