Use Docker name (not ID) to parse Kubernetes components.

Since the parsing function doesn't return an error all the components
returned empty strings. This caused us to enforce the MaxContainerLimit
as a global limit instead of a per-container limit.

Fixes #4413.

pkg/kubelet/dockertools/docker.go

@@ -578,6 +578,7 @@ func BuildDockerName(podUID types.UID, podFullName string, container *api.Contai
 		rand.Uint32())
 }
 
+// TODO(vmarmol): This should probably return an error.
 // Unpacks a container name, returning the pod full name and container name we would have used to
 // construct the docker name. If the docker name isn't the one we created, we may return empty strings.
 func ParseDockerName(name string) (podFullName string, podUID types.UID, containerName string, hash uint64) {

pkg/kubelet/kubelet.go

@@ -395,7 +395,7 @@ func (kl *Kubelet) GarbageCollectContainers() error {
 	}
 	uidToIDMap := map[string][]string{}
 	for _, container := range containers {
-		_, uid, name, _ := dockertools.ParseDockerName(container.ID)
+		_, uid, name, _ := dockertools.ParseDockerName(container.Names[0])
 		uidName := string(uid) + "." + name
 		uidToIDMap[uidName] = append(uidToIDMap[uidName], container.ID)
 	}

pkg/kubelet/kubelet_test.go

@@ -95,6 +95,27 @@ func verifyStringArrayEquals(t *testing.T, actual, expected []string) {
 	}
 }
 
+func verifyStringArrayEqualsAnyOrder(t *testing.T, actual, expected []string) {
+	invalid := len(actual) != len(expected)
+	if !invalid {
+		for _, exp := range expected {
+			found := false
+			for _, act := range actual {
+				if exp == act {
+					found = true
+					break
+				}
+			}
+			if !found {
+				t.Errorf("Expected element %s not found in %#v", exp, actual)
+			}
+		}
+	}
+	if invalid {
+		t.Errorf("Expected: %#v, Actual: %#v", expected, actual)
+	}
+}
+
 func verifyBoolean(t *testing.T, expected, value bool) {
 	if expected != value {
 		t.Errorf("Unexpected boolean.  Expected %t.  Found %t", expected, value)
@@ -1634,6 +1655,7 @@ func TestKubeletGarbageCollection(t *testing.T) {
 		containerDetails map[string]*docker.Container
 		expectedRemoved  []string
 	}{
+		// Remove oldest containers.
 		{
 			containers: []docker.APIContainers{
 				{
@@ -1651,21 +1673,6 @@ func TestKubeletGarbageCollection(t *testing.T) {
 					Names: []string{"/k8s_POD_foo.new.test_.deadbeef_42"},
 					ID:    "3876",
 				},
-				{
-					// pod infra container
-					Names: []string{"/k8s_POD_foo.new.test_.deadbeef_42"},
-					ID:    "4876",
-				},
-				{
-					// pod infra container
-					Names: []string{"/k8s_POD_foo.new.test_.deadbeef_42"},
-					ID:    "5876",
-				},
-				{
-					// pod infra container
-					Names: []string{"/k8s_POD_foo.new.test_.deadbeef_42"},
-					ID:    "6876",
-				},
 			},
 			containerDetails: map[string]*docker.Container{
 				"1876": {
@@ -1678,6 +1685,7 @@ func TestKubeletGarbageCollection(t *testing.T) {
 			},
 			expectedRemoved: []string{"1876"},
 		},
+		// Only remove non-running containers.
 		{
 			containers: []docker.APIContainers{
 				{
@@ -1700,21 +1708,6 @@ func TestKubeletGarbageCollection(t *testing.T) {
 					Names: []string{"/k8s_POD_foo.new.test_.deadbeef_42"},
 					ID:    "4876",
 				},
-				{
-					// pod infra container
-					Names: []string{"/k8s_POD_foo.new.test_.deadbeef_42"},
-					ID:    "5876",
-				},
-				{
-					// pod infra container
-					Names: []string{"/k8s_POD_foo.new.test_.deadbeef_42"},
-					ID:    "6876",
-				},
-				{
-					// pod infra container
-					Names: []string{"/k8s_POD_foo.new.test_.deadbeef_42"},
-					ID:    "7876",
-				},
 			},
 			containerDetails: map[string]*docker.Container{
 				"1876": {
@@ -1734,6 +1727,7 @@ func TestKubeletGarbageCollection(t *testing.T) {
 			},
 			expectedRemoved: []string{"2876"},
 		},
+		// Less than maxContainerCount doesn't delete any.
 		{
 			containers: []docker.APIContainers{
 				{
@@ -1743,10 +1737,62 @@ func TestKubeletGarbageCollection(t *testing.T) {
 				},
 			},
 		},
+		// maxContainerCount applies per container..
+		{
+			containers: []docker.APIContainers{
+				{
+					// pod infra container
+					Names: []string{"/k8s_POD_foo2.new.test_.beefbeef_40"},
+					ID:    "1706",
+				},
+				{
+					// pod infra container
+					Names: []string{"/k8s_POD_foo2.new.test_.beefbeef_40"},
+					ID:    "2706",
+				},
+				{
+					// pod infra container
+					Names: []string{"/k8s_POD_foo2.new.test_.beefbeef_40"},
+					ID:    "3706",
+				},
+				{
+					// pod infra container
+					Names: []string{"/k8s_POD_foo.new.test_.deadbeef_42"},
+					ID:    "1876",
+				},
+				{
+					// pod infra container
+					Names: []string{"/k8s_POD_foo.new.test_.deadbeef_42"},
+					ID:    "2876",
+				},
+				{
+					// pod infra container
+					Names: []string{"/k8s_POD_foo.new.test_.deadbeef_42"},
+					ID:    "3876",
+				},
+			},
+			containerDetails: map[string]*docker.Container{
+				"1706": {
+					State: docker.State{
+						Running: false,
+					},
+					ID:      "1706",
+					Created: time.Now(),
+				},
+				"1876": {
+					State: docker.State{
+						Running: false,
+					},
+					ID:      "1876",
+					Created: time.Now(),
+				},
+			},
+			expectedRemoved: []string{"1706", "1876"},
+		},
 	}
 	for _, test := range tests {
 		kubelet, fakeDocker := newTestKubelet(t)
-		kubelet.maxContainerCount = 5
+		kubelet.maxContainerCount = 2
 		fakeDocker.ContainerList = test.containers
 		fakeDocker.ContainerMap = test.containerDetails
 		fakeDocker.Container = &docker.Container{ID: "error", Created: time.Now()}
@@ -1754,9 +1800,7 @@ func TestKubeletGarbageCollection(t *testing.T) {
 		if err != nil {
 			t.Errorf("unexpected error: %v", err)
 		}
-		if !reflect.DeepEqual(fakeDocker.Removed, test.expectedRemoved) {
+		verifyStringArrayEqualsAnyOrder(t, test.expectedRemoved, fakeDocker.Removed)
-			t.Errorf("expected: %v, got: %v", test.expectedRemoved, fakeDocker.Removed)
-		}
 	}
 }