From 5d6ad845cc8d6a229c61abea69da540808cd1a1e Mon Sep 17 00:00:00 2001 From: Victor Marmol Date: Fri, 13 Feb 2015 13:08:15 -0800 Subject: [PATCH] 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 | 1 + pkg/kubelet/kubelet.go | 2 +- pkg/kubelet/kubelet_test.go | 112 +++++++++++++++++++++--------- 3 files changed, 80 insertions(+), 35 deletions(-) diff --git a/pkg/kubelet/dockertools/docker.go b/pkg/kubelet/dockertools/docker.go index ad4ea20c508..51d5e911e9a 100644 --- a/pkg/kubelet/dockertools/docker.go +++ b/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) { diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index a903e01aff5..f78f4754950 100644 --- a/pkg/kubelet/kubelet.go +++ b/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) } diff --git a/pkg/kubelet/kubelet_test.go b/pkg/kubelet/kubelet_test.go index 3d7199442dd..34dd475650d 100644 --- a/pkg/kubelet/kubelet_test.go +++ b/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) { - t.Errorf("expected: %v, got: %v", test.expectedRemoved, fakeDocker.Removed) - } + verifyStringArrayEqualsAnyOrder(t, test.expectedRemoved, fakeDocker.Removed) } }