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

Remove unused type DockerContainers. #16653

Merged
merged 2 commits into from
Nov 3, 2015
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
32 changes: 4 additions & 28 deletions pkg/kubelet/dockertools/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -211,29 +210,6 @@ 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 {
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
}
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.
Expand Down Expand Up @@ -348,10 +324,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
// 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) {
Copy link
Member

Choose a reason for hiding this comment

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

Call sites didn't change because the result of this function is always used in a range statement that works with both the new and old return styles.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's why I want to remove the old type. :)

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I'm just noting for posterity :) It looked funny to me when reviewing
that the call sites didn't change when the type changed.

On Mon, Nov 2, 2015 at 6:16 PM, taotao notifications@github.com wrote:

In pkg/kubelet/dockertools/docker.go
#16653 (comment)
:

// 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) {

Yeah, that's why I want to remove the old type. :)


Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/16653/files#r43698006.

result := []*docker.APIContainers{}
containers, err := client.ListContainers(docker.ListContainersOptions{All: allContainers})
if err != nil {
return nil, err
Expand All @@ -369,7 +345,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
}
23 changes: 21 additions & 2 deletions pkg/kubelet/dockertools/docker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -83,13 +101,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)
Expand Down
2 changes: 1 addition & 1 deletion pkg/kubelet/dockertools/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Expand Down