From b127901871b4081f65904e73eda786a7710149b5 Mon Sep 17 00:00:00 2001 From: Lantao Liu Date: Sun, 1 Nov 2015 18:49:05 -0800 Subject: [PATCH 1/2] Remove unused type DockerContainers. Type DockerContainers and function FindPodContainer() are never used. Remove them to simplify the docker runtime api. --- pkg/kubelet/dockertools/docker.go | 19 +++++++------------ pkg/kubelet/dockertools/docker_test.go | 5 +++-- pkg/kubelet/dockertools/manager_test.go | 2 +- 3 files changed, 11 insertions(+), 15 deletions(-) diff --git a/pkg/kubelet/dockertools/docker.go b/pkg/kubelet/dockertools/docker.go index 7e194274e2b..6df9cbcd94f 100644 --- a/pkg/kubelet/dockertools/docker.go +++ b/pkg/kubelet/dockertools/docker.go @@ -32,7 +32,6 @@ import ( "k8s.io/kubernetes/pkg/credentialprovider" kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" "k8s.io/kubernetes/pkg/kubelet/leaky" - kubetypes "k8s.io/kubernetes/pkg/kubelet/types" "k8s.io/kubernetes/pkg/types" "k8s.io/kubernetes/pkg/util" utilerrors "k8s.io/kubernetes/pkg/util/errors" @@ -211,16 +210,12 @@ func (p throttledDockerPuller) IsImagePresent(name string) (bool, error) { return p.puller.IsImagePresent(name) } -// TODO (random-liu) Almost never used, should we remove this? -// DockerContainers is a map of containers -type DockerContainers map[kubetypes.DockerID]*docker.APIContainers - -func (c DockerContainers) FindPodContainer(podFullName string, uid types.UID, containerName string) (*docker.APIContainers, bool, uint64) { - for _, dockerContainer := range c { +// An internal helper function. +func findPodContainer(dockerContainers []*docker.APIContainers, podFullName string, uid types.UID, containerName string) (*docker.APIContainers, bool, uint64) { + for _, dockerContainer := range dockerContainers { if len(dockerContainer.Names) == 0 { continue } - // TODO(proppy): build the docker container name and do a map lookup instead? dockerName, hash, err := ParseDockerName(dockerContainer.Names[0]) if err != nil { continue @@ -348,10 +343,10 @@ func milliCPUToShares(milliCPU int64) int64 { } // GetKubeletDockerContainers lists all container or just the running ones. -// Returns a map of docker containers that we manage, keyed by container ID. +// Returns a list of docker containers that we manage, keyed by container ID. // TODO: Move this function with dockerCache to DockerManager. -func GetKubeletDockerContainers(client DockerInterface, allContainers bool) (DockerContainers, error) { - result := make(DockerContainers) +func GetKubeletDockerContainers(client DockerInterface, allContainers bool) ([]*docker.APIContainers, error) { + result := []*docker.APIContainers{} containers, err := client.ListContainers(docker.ListContainersOptions{All: allContainers}) if err != nil { return nil, err @@ -369,7 +364,7 @@ func GetKubeletDockerContainers(client DockerInterface, allContainers bool) (Doc glog.V(3).Infof("Docker Container: %s is not managed by kubelet.", container.Names[0]) continue } - result[kubetypes.DockerID(container.ID)] = container + result = append(result, container) } return result, nil } diff --git a/pkg/kubelet/dockertools/docker_test.go b/pkg/kubelet/dockertools/docker_test.go index 46840b4e1b0..37ab8950ccb 100644 --- a/pkg/kubelet/dockertools/docker_test.go +++ b/pkg/kubelet/dockertools/docker_test.go @@ -83,13 +83,14 @@ func TestGetContainerID(t *testing.T) { t.Errorf("Expected %#v, Got %#v", fakeDocker.ContainerList, dockerContainers) } verifyCalls(t, fakeDocker, []string{"list"}) - dockerContainer, found, _ := dockerContainers.FindPodContainer("qux_ns", "", "foo") + + dockerContainer, found, _ := findPodContainer(dockerContainers, "qux_ns", "", "foo") if dockerContainer == nil || !found { t.Errorf("Failed to find container %#v", dockerContainer) } fakeDocker.ClearCalls() - dockerContainer, found, _ = dockerContainers.FindPodContainer("foobar", "", "foo") + dockerContainer, found, _ = findPodContainer(dockerContainers, "foobar", "", "foo") verifyCalls(t, fakeDocker, []string{}) if dockerContainer != nil || found { t.Errorf("Should not have found container %#v", dockerContainer) diff --git a/pkg/kubelet/dockertools/manager_test.go b/pkg/kubelet/dockertools/manager_test.go index a10358e83ee..1697836907e 100644 --- a/pkg/kubelet/dockertools/manager_test.go +++ b/pkg/kubelet/dockertools/manager_test.go @@ -356,7 +356,7 @@ func apiContainerToContainer(c docker.APIContainers) kubecontainer.Container { } } -func dockerContainersToPod(containers DockerContainers) kubecontainer.Pod { +func dockerContainersToPod(containers []*docker.APIContainers) kubecontainer.Pod { var pod kubecontainer.Pod for _, c := range containers { dockerName, hash, err := ParseDockerName(c.Names[0]) From eff4533efcaa73426a22946533c37cc69796e697 Mon Sep 17 00:00:00 2001 From: Lantao Liu Date: Mon, 2 Nov 2015 09:49:07 -0800 Subject: [PATCH 2/2] Move findPodContainer to docker_test.go --- pkg/kubelet/dockertools/docker.go | 21 +-------------------- pkg/kubelet/dockertools/docker_test.go | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/pkg/kubelet/dockertools/docker.go b/pkg/kubelet/dockertools/docker.go index 6df9cbcd94f..517a92650e7 100644 --- a/pkg/kubelet/dockertools/docker.go +++ b/pkg/kubelet/dockertools/docker.go @@ -210,25 +210,6 @@ func (p throttledDockerPuller) IsImagePresent(name string) (bool, error) { return p.puller.IsImagePresent(name) } -// An internal helper function. -func findPodContainer(dockerContainers []*docker.APIContainers, podFullName string, uid types.UID, containerName string) (*docker.APIContainers, bool, uint64) { - for _, dockerContainer := range dockerContainers { - if len(dockerContainer.Names) == 0 { - continue - } - dockerName, hash, err := ParseDockerName(dockerContainer.Names[0]) - if err != nil { - continue - } - if dockerName.PodFullName == podFullName && - (uid == "" || dockerName.PodUID == uid) && - dockerName.ContainerName == containerName { - return dockerContainer, true, hash - } - } - return nil, false, 0 -} - const containerNamePrefix = "k8s" // Creates a name which can be reversed to identify both full pod name and container name. @@ -343,7 +324,7 @@ func milliCPUToShares(milliCPU int64) int64 { } // GetKubeletDockerContainers lists all container or just the running ones. -// Returns a list of docker containers that we manage, keyed by container ID. +// Returns a list of docker containers that we manage // TODO: Move this function with dockerCache to DockerManager. func GetKubeletDockerContainers(client DockerInterface, allContainers bool) ([]*docker.APIContainers, error) { result := []*docker.APIContainers{} diff --git a/pkg/kubelet/dockertools/docker_test.go b/pkg/kubelet/dockertools/docker_test.go index 37ab8950ccb..82cf3cd3ac5 100644 --- a/pkg/kubelet/dockertools/docker_test.go +++ b/pkg/kubelet/dockertools/docker_test.go @@ -59,6 +59,24 @@ func verifyStringArrayEquals(t *testing.T, actual, expected []string) { } } +func findPodContainer(dockerContainers []*docker.APIContainers, podFullName string, uid types.UID, containerName string) (*docker.APIContainers, bool, uint64) { + for _, dockerContainer := range dockerContainers { + if len(dockerContainer.Names) == 0 { + continue + } + dockerName, hash, err := ParseDockerName(dockerContainer.Names[0]) + if err != nil { + continue + } + if dockerName.PodFullName == podFullName && + (uid == "" || dockerName.PodUID == uid) && + dockerName.ContainerName == containerName { + return dockerContainer, true, hash + } + } + return nil, false, 0 +} + func TestGetContainerID(t *testing.T) { fakeDocker := &FakeDockerClient{} fakeDocker.ContainerList = []docker.APIContainers{