From fe0066d2e4b78b4225b0f798732276597e32d33b Mon Sep 17 00:00:00 2001 From: Justin Huff Date: Wed, 25 Jun 2014 16:24:20 -0700 Subject: [PATCH] Cleaning up container ID handling inside kubelet --- pkg/kubelet/fake_docker_client.go | 4 +- pkg/kubelet/kubelet.go | 212 +++++++++++++---------------- pkg/kubelet/kubelet_server.go | 16 +-- pkg/kubelet/kubelet_server_test.go | 11 -- pkg/kubelet/kubelet_test.go | 148 ++++---------------- 5 files changed, 129 insertions(+), 262 deletions(-) diff --git a/pkg/kubelet/fake_docker_client.go b/pkg/kubelet/fake_docker_client.go index 0e98c1e1c26..609834dc341 100644 --- a/pkg/kubelet/fake_docker_client.go +++ b/pkg/kubelet/fake_docker_client.go @@ -59,13 +59,13 @@ func (f *FakeDockerClient) CreateContainer(c docker.CreateContainerOptions) (*do func (f *FakeDockerClient) StartContainer(id string, hostConfig *docker.HostConfig) error { f.appendCall("start") - return nil + return f.err } func (f *FakeDockerClient) StopContainer(id string, timeout uint) error { f.appendCall("stop") f.stopped = append(f.stopped, id) - return nil + return f.err } type FakeDockerPuller struct { diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index 8467f333ee0..c8f213c1eeb 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -169,66 +169,39 @@ func (kl *Kubelet) LogEvent(event *api.Event) error { return err } -// Does this container exist on this host? Returns true if so, and the name under which the container is running. -// Returns an error if one occurs. -func (kl *Kubelet) ContainerExists(manifest *api.ContainerManifest, container *api.Container) (exists bool, foundName string, err error) { - containers, err := kl.ListContainers() - if err != nil { - return false, "", err - } - for _, name := range containers { - manifestId, containerName := dockerNameToManifestAndContainer(name) - if manifestId == manifest.Id && containerName == container.Name { - // TODO(bburns) : This leads to an extra list. Convert this to use the returned ID and a straight call - // to inspect - data, err := kl.GetContainerByName(name) - return data != nil, name, err - } - } - return false, "", nil -} - -// GetContainerID looks at the list of containers on the machine and returns the ID of the container whose name -// matches 'name'. It returns the name of the container, or empty string, if the container isn't found. -// it returns true if the container is found, false otherwise, and any error that occurs. -func (kl *Kubelet) GetContainerID(name string) (string, bool, error) { - containerList, err := kl.DockerClient.ListContainers(docker.ListContainersOptions{}) - if err != nil { - return "", false, err - } - for _, value := range containerList { - if strings.Contains(value.Names[0], name) { - return value.ID, true, nil - } - } - return "", false, nil -} - -// Get a container by name. -// returns the container data from Docker, or an error if one exists. -func (kl *Kubelet) GetContainerByName(name string) (*docker.Container, error) { - id, found, err := kl.GetContainerID(name) - if err != nil { - return nil, err - } - if !found { - return nil, nil - } - return kl.DockerClient.InspectContainer(id) -} - -func (kl *Kubelet) ListContainers() ([]string, error) { - result := []string{} +// Return a map of docker containers that we manage. The map key is the docker container ID +func (kl *Kubelet) getDockerContainers() (map[string]docker.APIContainers, error) { + result := map[string]docker.APIContainers{} containerList, err := kl.DockerClient.ListContainers(docker.ListContainersOptions{}) if err != nil { return result, err } for _, value := range containerList { - result = append(result, value.Names[0]) + // Skip containers that we didn't create to allow users to manually + // spin up their own containers if they want. + if !strings.HasPrefix(value.Names[0], "/"+containerNamePrefix+"--") { + continue + } + result[value.ID] = value } return result, err } +// Return Docker's container ID for a manifest's container. Returns an empty string if it doesn't exist +func (kl *Kubelet) getContainerId(manifest *api.ContainerManifest, container *api.Container) (string, error) { + dockerContainers, err := kl.getDockerContainers() + if err != nil { + return "", err + } + for id, dockerContainer := range dockerContainers { + manifestId, containerName := dockerNameToManifestAndContainer(dockerContainer.Names[0]) + if manifestId == manifest.Id && containerName == container.Name { + return id, nil + } + } + return "", nil +} + type dockerPuller struct{} func MakeDockerPuller() DockerPuller { @@ -346,15 +319,14 @@ func makePortsAndBindings(container *api.Container) (map[docker.Port]struct{}, m return exposedPorts, portBindings } -func (kl *Kubelet) RunContainer(manifest *api.ContainerManifest, container *api.Container, netMode string) (name string, err error) { - name = manifestAndContainerToDockerName(manifest, container) - +// Run a single container from a manifest. Returns the docker container ID +func (kl *Kubelet) runContainer(manifest *api.ContainerManifest, container *api.Container, netMode string) (id string, err error) { envVariables := makeEnvironmentVariables(container) volumes, binds := makeVolumesAndBinds(container) exposedPorts, portBindings := makePortsAndBindings(container) opts := docker.CreateContainerOptions{ - Name: name, + Name: manifestAndContainerToDockerName(manifest, container), Config: &docker.Config{ Image: container.Image, ExposedPorts: exposedPorts, @@ -368,25 +340,18 @@ func (kl *Kubelet) RunContainer(manifest *api.ContainerManifest, container *api. if err != nil { return "", err } - return name, kl.DockerClient.StartContainer(dockerContainer.ID, &docker.HostConfig{ + err = kl.DockerClient.StartContainer(dockerContainer.ID, &docker.HostConfig{ PortBindings: portBindings, Binds: binds, NetworkMode: netMode, }) + return dockerContainer.ID, err } -func (kl *Kubelet) KillContainer(name string) error { - id, found, err := kl.GetContainerID(name) - if err != nil { - return err - } - if !found { - // This is weird, but not an error, so yell and then return nil - glog.Infof("Couldn't find container: %s", name) - return nil - } - err = kl.DockerClient.StopContainer(id, 10) - manifestId, containerName := dockerNameToManifestAndContainer(name) +// Kill a docker container +func (kl *Kubelet) killContainer(container docker.APIContainers) error { + err := kl.DockerClient.StopContainer(container.ID, 10) + manifestId, containerName := dockerNameToManifestAndContainer(container.Names[0]) kl.LogEvent(&api.Event{ Event: "STOP", Manifest: &api.ContainerManifest{ @@ -642,19 +607,12 @@ func (kl *Kubelet) WatchEtcd(watchChannel <-chan *etcd.Response, updateChannel c const networkContainerName = "net" -func (kl *Kubelet) networkContainerExists(manifest *api.ContainerManifest) (string, bool, error) { - pods, err := kl.ListContainers() - if err != nil { - return "", false, err - } - for _, name := range pods { - if strings.Contains(name, containerNamePrefix+"--"+networkContainerName+"--"+escapeDash(manifest.Id)+"--") { - return name, true, nil - } - } - return "", false, nil +// Return the docker ID for a manifest's network container. Returns an empty string if it doesn't exist +func (kl *Kubelet) getNetworkContainerId(manifest *api.ContainerManifest) (string, error) { + return kl.getContainerId(manifest, &api.Container{Name: networkContainerName}) } +// Create a network container for a manifest. Returns the docker container ID of the newly created container func (kl *Kubelet) createNetworkContainer(manifest *api.ContainerManifest) (string, error) { var ports []api.Port // Docker only exports ports from the network container. Let's @@ -669,73 +627,69 @@ func (kl *Kubelet) createNetworkContainer(manifest *api.ContainerManifest) (stri Ports: ports, } kl.DockerPuller.Pull("busybox") - return kl.RunContainer(manifest, container, "") + return kl.runContainer(manifest, container, "") } // Sync the configured list of containers (desired state) with the host current state func (kl *Kubelet) SyncManifests(config []api.ContainerManifest) error { glog.Infof("Desired: %#v", config) var err error - desired := map[string]bool{} + dockerIdsToKeep := map[string]bool{} + + // Check for any containers that need starting for _, manifest := range config { - netName, exists, err := kl.networkContainerExists(&manifest) + // Make sure we have a network container + netId, err := kl.getNetworkContainerId(&manifest) if err != nil { glog.Errorf("Failed to introspect network container. (%#v) Skipping container %s", err, manifest.Id) continue } - if !exists { + if netId == "" { glog.Infof("Network container doesn't exist, creating") - netName, err = kl.createNetworkContainer(&manifest) + netId, err = kl.createNetworkContainer(&manifest) if err != nil { - glog.Errorf("Failed to create network container: %#v", err) + glog.Errorf("Failed to create network container: %#v Skipping container %s", err, manifest.Id) + continue } - // Docker list prefixes '/' for some reason, so let's do that... - netName = "/" + netName } - desired[netName] = true - for _, element := range manifest.Containers { - var exists bool - exists, actualName, err := kl.ContainerExists(&manifest, &element) + dockerIdsToKeep[netId] = true + + for _, container := range manifest.Containers { + containerId, err := kl.getContainerId(&manifest, &container) if err != nil { glog.Errorf("Error detecting container: %#v skipping.", err) continue } - if !exists { - glog.Infof("%#v doesn't exist, creating", element) - kl.DockerPuller.Pull(element.Image) + if containerId == "" { + glog.Infof("%#v doesn't exist, creating", container) + kl.DockerPuller.Pull(container.Image) if err != nil { glog.Errorf("Error pulling container: %#v", err) continue } - // netName has the '/' prefix, so slice it off - networkContainer := netName[1:] - actualName, err = kl.RunContainer(&manifest, &element, "container:"+networkContainer) - // For some reason, list gives back names that start with '/' - actualName = "/" + actualName - + containerId, err = kl.runContainer(&manifest, &container, "container:"+netId) if err != nil { // TODO(bburns) : Perhaps blacklist a container after N failures? glog.Errorf("Error creating container: %#v", err) - desired[actualName] = true continue } } else { - glog.V(1).Infof("%#v exists as %v", element.Name, actualName) + glog.V(1).Infof("%#v exists as %v", container.Name, containerId) } - desired[actualName] = true + dockerIdsToKeep[containerId] = true } } - existingContainers, _ := kl.ListContainers() - glog.Infof("Existing: %#v Desired: %#v", existingContainers, desired) - for _, container := range existingContainers { - // Skip containers that we didn't create to allow users to manually - // spin up their own containers if they want. - if !strings.HasPrefix(container, "/"+containerNamePrefix+"--") { - continue - } - if !desired[container] { - glog.Infof("Killing: %s", container) - err = kl.KillContainer(container) + + // Kill any containers we don't need + existingContainers, err := kl.getDockerContainers() + if err != nil { + glog.Errorf("Error listing containers: %#v", err) + return err + } + for id, container := range existingContainers { + if !dockerIdsToKeep[id] { + glog.Infof("Killing: %s", id) + err = kl.killContainer(container) if err != nil { glog.Errorf("Error killing container: %#v", err) } @@ -772,8 +726,31 @@ func (kl *Kubelet) RunSyncLoop(updateChannel <-chan manifestUpdate, handler Sync } } +// getContainerIdFromName looks at the list of containers on the machine and returns the ID of the container whose name +// matches 'name'. It returns the name of the container, or empty string, if the container isn't found. +// it returns true if the container is found, false otherwise, and any error that occurs. +// TODO: This functions exists to support GetContainerInfo and GetContainerStats +// It should be removed once those two functions start taking proper pod.IDs +func (kl *Kubelet) getContainerIdFromName(name string) (string, bool, error) { + containerList, err := kl.DockerClient.ListContainers(docker.ListContainersOptions{}) + if err != nil { + return "", false, err + } + for _, value := range containerList { + if strings.Contains(value.Names[0], name) { + return value.ID, true, nil + } + } + return "", false, nil +} + +// Returns docker info for a container func (kl *Kubelet) GetContainerInfo(name string) (string, error) { - info, err := kl.DockerClient.InspectContainer(name) + dockerId, found, err := kl.getContainerIdFromName(name) + if err != nil || !found { + return "{}", err + } + info, err := kl.DockerClient.InspectContainer(dockerId) if err != nil { return "{}", err } @@ -781,16 +758,17 @@ func (kl *Kubelet) GetContainerInfo(name string) (string, error) { return string(data), err } +//Returns stats (from Cadvisor) for a container func (kl *Kubelet) GetContainerStats(name string) (*api.ContainerStats, error) { if kl.CadvisorClient == nil { return nil, nil } - id, found, err := kl.GetContainerID(name) + dockerId, found, err := kl.getContainerIdFromName(name) if err != nil || !found { return nil, err } - info, err := kl.CadvisorClient.ContainerInfo(fmt.Sprintf("/docker/%v", id)) + info, err := kl.CadvisorClient.ContainerInfo(fmt.Sprintf("/docker/%v", dockerId)) if err != nil { return nil, err diff --git a/pkg/kubelet/kubelet_server.go b/pkg/kubelet/kubelet_server.go index e4fb2446972..c3966bbe3bb 100644 --- a/pkg/kubelet/kubelet_server.go +++ b/pkg/kubelet/kubelet_server.go @@ -35,7 +35,6 @@ type KubeletServer struct { // kubeletInterface contains all the kubelet methods required by the server. // For testablitiy. type kubeletInterface interface { - GetContainerID(name string) (string, bool, error) GetContainerStats(name string) (*api.ContainerStats, error) GetContainerInfo(name string) (string, error) } @@ -78,6 +77,7 @@ func (s *KubeletServer) ServeHTTP(w http.ResponseWriter, req *http.Request) { s.UpdateChannel <- manifestUpdate{httpServerSource, manifests} } case u.Path == "/containerStats": + // NOTE: The master appears to pass a Pod.ID container := u.Query().Get("container") if len(container) == 0 { w.WriteHeader(http.StatusBadRequest) @@ -105,27 +105,23 @@ func (s *KubeletServer) ServeHTTP(w http.ResponseWriter, req *http.Request) { w.Header().Add("Content-type", "application/json") w.Write(data) case u.Path == "/containerInfo": + // NOTE: The master appears to pass a Pod.ID + // The server appears to pass a Pod.ID container := u.Query().Get("container") if len(container) == 0 { w.WriteHeader(http.StatusBadRequest) fmt.Fprint(w, "Missing container selector arg.") return } - id, found, err := s.Kubelet.GetContainerID(container) - if !found { - w.WriteHeader(http.StatusOK) - fmt.Fprint(w, "{}") - return - } - body, err := s.Kubelet.GetContainerInfo(id) + data, err := s.Kubelet.GetContainerInfo(container) if err != nil { w.WriteHeader(http.StatusInternalServerError) fmt.Fprintf(w, "Internal Error: %#v", err) return } - w.Header().Add("Content-type", "application/json") w.WriteHeader(http.StatusOK) - fmt.Fprint(w, body) + w.Header().Add("Content-type", "application/json") + fmt.Fprint(w, data) default: w.WriteHeader(http.StatusNotFound) fmt.Fprint(w, "Not found.") diff --git a/pkg/kubelet/kubelet_server_test.go b/pkg/kubelet/kubelet_server_test.go index e7b69a11288..c52ba621743 100644 --- a/pkg/kubelet/kubelet_server_test.go +++ b/pkg/kubelet/kubelet_server_test.go @@ -32,7 +32,6 @@ import ( type fakeKubelet struct { infoFunc func(name string) (string, error) - idFunc func(name string) (string, bool, error) statsFunc func(name string) (*api.ContainerStats, error) } @@ -40,10 +39,6 @@ func (fk *fakeKubelet) GetContainerInfo(name string) (string, error) { return fk.infoFunc(name) } -func (fk *fakeKubelet) GetContainerID(name string) (string, bool, error) { - return fk.idFunc(name) -} - func (fk *fakeKubelet) GetContainerStats(name string) (*api.ContainerStats, error) { return fk.statsFunc(name) } @@ -122,12 +117,6 @@ func TestContainers(t *testing.T) { func TestContainerInfo(t *testing.T) { fw := makeServerTest() expected := "good container info string" - fw.fakeKubelet.idFunc = func(name string) (string, bool, error) { - if name == "goodcontainer" { - return name, true, nil - } - return "", false, fmt.Errorf("bad container") - } fw.fakeKubelet.infoFunc = func(name string) (string, error) { if name == "goodcontainer" { return expected, nil diff --git a/pkg/kubelet/kubelet_test.go b/pkg/kubelet/kubelet_test.go index 1ad1d5d837d..d2bc6405f1a 100644 --- a/pkg/kubelet/kubelet_test.go +++ b/pkg/kubelet/kubelet_test.go @@ -133,7 +133,7 @@ func TestContainerManifestNaming(t *testing.T) { verifyPackUnpack(t, "_m___anifest", "-_-container") } -func TestContainerExists(t *testing.T) { +func TestGetContainerId(t *testing.T) { fakeDocker := FakeDockerClient{ err: nil, } @@ -149,19 +149,21 @@ func TestContainerExists(t *testing.T) { } fakeDocker.containerList = []docker.APIContainers{ { + ID: "foobar", Names: []string{"/k8s--foo--qux--1234"}, }, { - Names: []string{"/k8s--bar--qux--1234"}, + ID: "barbar", + Names: []string{"/k8s--bar--qux--2565"}, }, } fakeDocker.container = &docker.Container{ ID: "foobar", } - exists, _, err := kubelet.ContainerExists(&manifest, &container) - verifyCalls(t, fakeDocker, []string{"list", "list", "inspect"}) - if !exists { + id, err := kubelet.getContainerId(&manifest, &container) + verifyCalls(t, fakeDocker, []string{"list"}) + if id == "" { t.Errorf("Failed to find container %#v", container) } if err != nil { @@ -170,113 +172,24 @@ func TestContainerExists(t *testing.T) { fakeDocker.clearCalls() missingManifest := api.ContainerManifest{Id: "foobar"} - exists, _, err = kubelet.ContainerExists(&missingManifest, &container) + id, err = kubelet.getContainerId(&missingManifest, &container) verifyCalls(t, fakeDocker, []string{"list"}) - if exists { - t.Errorf("Failed to not find container %#v, missingManifest") + if id != "" { + t.Errorf("Failed to not find container %#v", missingManifest) } } -func TestGetContainerID(t *testing.T) { - fakeDocker := FakeDockerClient{ - err: nil, - } - kubelet := Kubelet{ - DockerClient: &fakeDocker, - DockerPuller: &FakeDockerPuller{}, - } - fakeDocker.containerList = []docker.APIContainers{ - { - Names: []string{"foo"}, - ID: "1234", - }, - { - Names: []string{"bar"}, - ID: "4567", - }, - } - - id, found, err := kubelet.GetContainerID("foo") - verifyBoolean(t, true, found) - verifyStringEquals(t, id, "1234") - verifyNoError(t, err) - verifyCalls(t, fakeDocker, []string{"list"}) - fakeDocker.clearCalls() - - id, found, err = kubelet.GetContainerID("bar") - verifyBoolean(t, true, found) - verifyStringEquals(t, id, "4567") - verifyNoError(t, err) - verifyCalls(t, fakeDocker, []string{"list"}) - fakeDocker.clearCalls() - - id, found, err = kubelet.GetContainerID("NotFound") - verifyBoolean(t, false, found) - verifyNoError(t, err) - verifyCalls(t, fakeDocker, []string{"list"}) -} - -func TestGetContainerByName(t *testing.T) { - fakeDocker := FakeDockerClient{ - err: nil, - } - kubelet := Kubelet{ - DockerClient: &fakeDocker, - DockerPuller: &FakeDockerPuller{}, - } - fakeDocker.containerList = []docker.APIContainers{ - { - Names: []string{"foo"}, - }, - { - Names: []string{"bar"}, - }, - } - fakeDocker.container = &docker.Container{ - ID: "foobar", - } - - container, err := kubelet.GetContainerByName("foo") - verifyCalls(t, fakeDocker, []string{"list", "inspect"}) - if container == nil { - t.Errorf("Unexpected nil container") - } - verifyStringEquals(t, container.ID, "foobar") - verifyNoError(t, err) -} - -func TestListContainers(t *testing.T) { - fakeDocker := FakeDockerClient{ - err: nil, - } - kubelet := Kubelet{ - DockerClient: &fakeDocker, - DockerPuller: &FakeDockerPuller{}, - } - fakeDocker.containerList = []docker.APIContainers{ - { - Names: []string{"foo"}, - }, - { - Names: []string{"bar"}, - }, - } - - containers, err := kubelet.ListContainers() - verifyStringArrayEquals(t, containers, []string{"foo", "bar"}) - verifyNoError(t, err) - verifyCalls(t, fakeDocker, []string{"list"}) -} - func TestKillContainerWithError(t *testing.T) { fakeDocker := FakeDockerClient{ err: fmt.Errorf("sample error"), containerList: []docker.APIContainers{ { - Names: []string{"foo"}, + ID: "1234", + Names: []string{"/k8s--foo--qux--1234"}, }, { - Names: []string{"bar"}, + ID: "5678", + Names: []string{"/k8s--bar--qux--5678"}, }, }, } @@ -284,9 +197,9 @@ func TestKillContainerWithError(t *testing.T) { DockerClient: &fakeDocker, DockerPuller: &FakeDockerPuller{}, } - err := kubelet.KillContainer("foo") + err := kubelet.killContainer(fakeDocker.containerList[0]) verifyError(t, err) - verifyCalls(t, fakeDocker, []string{"list"}) + verifyCalls(t, fakeDocker, []string{"stop"}) } func TestKillContainer(t *testing.T) { @@ -299,19 +212,21 @@ func TestKillContainer(t *testing.T) { } fakeDocker.containerList = []docker.APIContainers{ { - Names: []string{"foo"}, + ID: "1234", + Names: []string{"/k8s--foo--qux--1234"}, }, { - Names: []string{"bar"}, + ID: "5678", + Names: []string{"/k8s--bar--qux--5678"}, }, } fakeDocker.container = &docker.Container{ ID: "foobar", } - err := kubelet.KillContainer("foo") + err := kubelet.killContainer(fakeDocker.containerList[0]) verifyNoError(t, err) - verifyCalls(t, fakeDocker, []string{"list", "stop"}) + verifyCalls(t, fakeDocker, []string{"stop"}) } func TestResponseToContainersNil(t *testing.T) { @@ -491,14 +406,7 @@ func TestSyncManifestsDoesNothing(t *testing.T) { }, }) expectNoError(t, err) - if len(fakeDocker.called) != 5 || - fakeDocker.called[0] != "list" || - fakeDocker.called[1] != "list" || - fakeDocker.called[2] != "list" || - fakeDocker.called[3] != "inspect" || - fakeDocker.called[4] != "list" { - t.Errorf("Unexpected call sequence: %#v", fakeDocker.called) - } + verifyCalls(t, fakeDocker, []string{"list", "list", "list"}) } func TestSyncManifestsDeletes(t *testing.T) { @@ -527,15 +435,11 @@ func TestSyncManifestsDeletes(t *testing.T) { } err := kubelet.SyncManifests([]api.ContainerManifest{}) expectNoError(t, err) - if len(fakeDocker.called) != 5 || - fakeDocker.called[0] != "list" || - fakeDocker.called[1] != "list" || - fakeDocker.called[2] != "stop" || - fakeDocker.called[3] != "list" || - fakeDocker.called[4] != "stop" || + verifyCalls(t, fakeDocker, []string{"list", "stop", "stop"}) + if len(fakeDocker.stopped) != 2 || fakeDocker.stopped[0] != "1234" || fakeDocker.stopped[1] != "9876" { - t.Errorf("Unexpected call sequence: %#v %s", fakeDocker.called, fakeDocker.stopped) + t.Errorf("Unexpected sequence of stopped containers: %s", fakeDocker.stopped) } }