Clean up dockertools/manager.go and add more unit tests.

This change refactors the GetPods function and add some basic unit tests.
We should start migrating docker specific tests from kubelet_test to
manager_test.go.

pkg/kubelet/dockertools/manager.go

@@ -627,6 +627,67 @@ func makeCapabilites(capAdd []api.CapabilityType, capDrop []api.CapabilityType)
 	return addCaps, dropCaps
 }
 
+// A helper function to get the KubeletContainerName and hash from a docker
+// container.
+func getDockerContainerNameInfo(c *docker.APIContainers) (*KubeletContainerName, uint64, error) {
+	if len(c.Names) == 0 {
+		return nil, 0, fmt.Errorf("cannot parse empty docker container name: %#v", c.Names)
+	}
+	dockerName, hash, err := ParseDockerName(c.Names[0])
+	if err != nil {
+		return nil, 0, fmt.Errorf("parse docker container name %q error: %v", c.Names[0], err)
+	}
+	return dockerName, hash, nil
+}
+
+// Converts docker.APIContainers to kubecontainer.Container.
+func convertDockerToRuntimeContainer(c *docker.APIContainers) (*kubecontainer.Container, error) {
+	dockerName, hash, err := getDockerContainerNameInfo(c)
+	if err != nil {
+		return nil, err
+	}
+	return &kubecontainer.Container{
+		ID:      types.UID(c.ID),
+		Name:    dockerName.ContainerName,
+		Image:   c.Image,
+		Hash:    hash,
+		Created: c.Created,
+	}, nil
+}
+
+// Get pod UID, name, and namespace by examining the container names.
+func getPodInfoFromContainer(c *docker.APIContainers) (types.UID, string, string, error) {
+	dockerName, _, err := getDockerContainerNameInfo(c)
+	if err != nil {
+		return types.UID(""), "", "", err
+	}
+	name, namespace, err := kubecontainer.ParsePodFullName(dockerName.PodFullName)
+	if err != nil {
+		return types.UID(""), "", "", fmt.Errorf("parse pod full name %q error: %v", dockerName.PodFullName, err)
+	}
+	return dockerName.PodUID, name, namespace, nil
+}
+
+// GetContainers returns a list of running containers if |all| is false;
+// otherwise, it returns all containers.
+func (dm *DockerManager) GetContainers(all bool) ([]*kubecontainer.Container, error) {
+	containers, err := GetKubeletDockerContainers(dm.client, all)
+	if err != nil {
+		return nil, err
+	}
+	// Convert DockerContainers to []*kubecontainer.Container
+	result := make([]*kubecontainer.Container, 0, len(containers))
+	for _, c := range containers {
+		converted, err := convertDockerToRuntimeContainer(c)
+		if err != nil {
+			glog.Errorf("Error examining the container: %v", err)
+			continue
+		}
+		result = append(result, converted)
+	}
+	return result, nil
+}
+
 func (dm *DockerManager) GetPods(all bool) ([]*kubecontainer.Pod, error) {
 	pods := make(map[types.UID]*kubecontainer.Pod)
 	var result []*kubecontainer.Pod
@@ -638,35 +699,28 @@ func (dm *DockerManager) GetPods(all bool) ([]*kubecontainer.Pod, error) {
 
 	// Group containers by pod.
 	for _, c := range containers {
-		if len(c.Names) == 0 {
+		converted, err := convertDockerToRuntimeContainer(c)
-			glog.Warningf("Cannot parse empty docker container name: %#v", c.Names)
-			continue
-		}
-		dockerName, hash, err := ParseDockerName(c.Names[0])
 		if err != nil {
-			glog.Warningf("Parse docker container name %q error: %v", c.Names[0], err)
+			glog.Errorf("Error examining the container: %v", err)
 			continue
 		}
-		pod, found := pods[dockerName.PodUID]
+
+		podUID, podName, podNamespace, err := getPodInfoFromContainer(c)
+		if err != nil {
+			glog.Errorf("Error examining the container: %v", err)
+			continue
+		}
+
+		pod, found := pods[podUID]
 		if !found {
-			name, namespace, err := kubecontainer.ParsePodFullName(dockerName.PodFullName)
-			if err != nil {
-				glog.Warningf("Parse pod full name %q error: %v", dockerName.PodFullName, err)
-				continue
-			}
 			pod = &kubecontainer.Pod{
-				ID:        dockerName.PodUID,
+				ID:        podUID,
-				Name:      name,
+				Name:      podName,
-				Namespace: namespace,
+				Namespace: podNamespace,
 			}
-			pods[dockerName.PodUID] = pod
+			pods[podUID] = pod
 		}
-		pod.Containers = append(pod.Containers, &kubecontainer.Container{
+		pod.Containers = append(pod.Containers, converted)
-			ID:      types.UID(c.ID),
-			Name:    dockerName.ContainerName,
-			Hash:    hash,
-			Created: c.Created,
-		})
 	}
 
 	// Convert map to list.

pkg/kubelet/dockertools/manager_test.go

@@ -17,12 +17,40 @@ limitations under the License.
 package dockertools
 
 import (
+	"reflect"
+	"sort"
 	"testing"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/client/record"
+	kubecontainer "github.com/GoogleCloudPlatform/kubernetes/pkg/kubelet/container"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/kubelet/network"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/types"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 	"github.com/fsouza/go-dockerclient"
 )
 
+func NewFakeDockerManager() (*DockerManager, *FakeDockerClient) {
+	fakeDocker := &FakeDockerClient{Errors: make(map[string]error), RemovedImages: util.StringSet{}}
+	fakeRecorder := &record.FakeRecorder{}
+	readinessManager := kubecontainer.NewReadinessManager()
+	containerRefManager := kubecontainer.NewRefManager()
+	networkPlugin, _ := network.InitNetworkPlugin([]network.NetworkPlugin{}, "", network.NewFakeHost(nil))
+
+	dockerManager := NewDockerManager(
+		fakeDocker,
+		fakeRecorder,
+		readinessManager,
+		containerRefManager,
+		PodInfraContainerImage,
+		0, 0, "",
+		kubecontainer.FakeOS{},
+		networkPlugin,
+		nil)
+
+	return dockerManager, fakeDocker
+}
+
 func TestSetEntrypointAndCommand(t *testing.T) {
 	cases := []struct {
 		name      string
@@ -87,3 +115,102 @@ func TestSetEntrypointAndCommand(t *testing.T) {
 		}
 	}
 }
+
+func TestConvertDockerToRuntimeContainer(t *testing.T) {
+	dockerContainer := &docker.APIContainers{
+		ID:      "ab2cdf",
+		Image:   "bar_image",
+		Created: 12345,
+		Names:   []string{"/k8s_bar.5678_foo_ns_1234_42"},
+	}
+	expected := &kubecontainer.Container{
+		ID:      types.UID("ab2cdf"),
+		Name:    "bar",
+		Image:   "bar_image",
+		Hash:    0x5678,
+		Created: 12345,
+	}
+
+	actual, err := convertDockerToRuntimeContainer(dockerContainer)
+	if err != nil {
+		t.Fatalf("unexpected error %v", err)
+	}
+	if !reflect.DeepEqual(expected, actual) {
+		t.Errorf("expected %#v, got %#v", expected, actual)
+	}
+}
+
+// verifyPods returns true if the two pod slices are equal.
+func verifyPods(a, b []*kubecontainer.Pod) bool {
+	if len(a) != len(b) {
+		return false
+	}
+
+	// Sort the containers within a pod.
+	for i := range a {
+		sort.Sort(containersByID(a[i].Containers))
+	}
+	for i := range b {
+		sort.Sort(containersByID(b[i].Containers))
+	}
+
+	// Sort the pods by UID.
+	sort.Sort(podsByID(a))
+	sort.Sort(podsByID(b))
+
+	return reflect.DeepEqual(a, b)
+}
+
+func TestGetPods(t *testing.T) {
+	manager, fakeDocker := NewFakeDockerManager()
+	dockerContainers := []docker.APIContainers{
+		{
+			ID:    "1111",
+			Names: []string{"/k8s_foo_qux_new_1234_42"},
+		},
+		{
+			ID:    "2222",
+			Names: []string{"/k8s_bar_qux_new_1234_42"},
+		},
+		{
+			ID:    "3333",
+			Names: []string{"/k8s_bar_jlk_wen_5678_42"},
+		},
+	}
+
+	// Convert the docker containers. This does not affect the test coverage
+	// because the conversion is tested separately in
+	// TestConvertDockerToRuntimeContainer.
+	containers := make([]*kubecontainer.Container, len(dockerContainers))
+	for i := range containers {
+		c, err := convertDockerToRuntimeContainer(&dockerContainers[i])
+		if err != nil {
+			t.Fatalf("unexpected error %v", err)
+		}
+		containers[i] = c
+	}
+
+	expected := []*kubecontainer.Pod{
+		{
+			ID:         types.UID("1234"),
+			Name:       "qux",
+			Namespace:  "new",
+			Containers: []*kubecontainer.Container{containers[0], containers[1]},
+		},
+		{
+			ID:         types.UID("5678"),
+			Name:       "jlk",
+			Namespace:  "wen",
+			Containers: []*kubecontainer.Container{containers[2]},
+		},
+	}
+
+	fakeDocker.ContainerList = dockerContainers
+	actual, err := manager.GetPods(false)
+	if err != nil {
+		t.Fatalf("unexpected error %v", err)
+	}
+	if !verifyPods(expected, actual) {
+		t.Errorf("expected %#v, got %#v", expected, actual)
+	}
+}