From e25da2150315c1a6db775fc31184c39a3d295ed8 Mon Sep 17 00:00:00 2001 From: Ron Lai Date: Tue, 19 Jul 2016 17:36:48 -0700 Subject: [PATCH] Clear tags to remove images with multiple tags --- pkg/kubelet/dockertools/docker_manager.go | 11 ++- .../dockertools/docker_manager_test.go | 17 ++++ pkg/kubelet/dockertools/docker_test.go | 5 +- pkg/kubelet/dockertools/fake_docker_client.go | 79 ++++++++++--------- 4 files changed, 72 insertions(+), 40 deletions(-) diff --git a/pkg/kubelet/dockertools/docker_manager.go b/pkg/kubelet/dockertools/docker_manager.go index f699a5deade..ba865821b5d 100644 --- a/pkg/kubelet/dockertools/docker_manager.go +++ b/pkg/kubelet/dockertools/docker_manager.go @@ -859,7 +859,16 @@ func (dm *DockerManager) IsImagePresent(image kubecontainer.ImageSpec) (bool, er // Removes the specified image. func (dm *DockerManager) RemoveImage(image kubecontainer.ImageSpec) error { - // TODO(harryz) currently Runtime interface does not provide other remove options. + // If the image has multiple tags, we need to remove all the tags + if inspectImage, err := dm.client.InspectImage(image.Image); err == nil && len(inspectImage.RepoTags) > 1 { + for _, tag := range inspectImage.RepoTags { + if _, err := dm.client.RemoveImage(tag, dockertypes.ImageRemoveOptions{PruneChildren: true}); err != nil { + return err + } + } + return nil + } + _, err := dm.client.RemoveImage(image.Image, dockertypes.ImageRemoveOptions{PruneChildren: true}) return err } diff --git a/pkg/kubelet/dockertools/docker_manager_test.go b/pkg/kubelet/dockertools/docker_manager_test.go index c9302d2b0a1..0e767a4304a 100644 --- a/pkg/kubelet/dockertools/docker_manager_test.go +++ b/pkg/kubelet/dockertools/docker_manager_test.go @@ -387,6 +387,23 @@ func TestListImages(t *testing.T) { } } +func TestDeleteImage(t *testing.T) { + manager, fakeDocker := newTestDockerManager() + fakeDocker.Image = &dockertypes.ImageInspect{ID: "1111", RepoTags: []string{"foo"}} + manager.RemoveImage(kubecontainer.ImageSpec{Image: "1111"}) + fakeDocker.AssertCallDetails([]calledDetail{{name: "inspect_image"}, {name: "remove_image", + arguments: []interface{}{"1111", dockertypes.ImageRemoveOptions{PruneChildren: true}}}}) +} + +func TestDeleteImageWithMultipleTags(t *testing.T) { + manager, fakeDocker := newTestDockerManager() + fakeDocker.Image = &dockertypes.ImageInspect{ID: "1111", RepoTags: []string{"foo", "bar"}} + manager.RemoveImage(kubecontainer.ImageSpec{Image: "1111"}) + fakeDocker.AssertCallDetails([]calledDetail{{name: "inspect_image"}, + {name: "remove_image", arguments: []interface{}{"foo", dockertypes.ImageRemoveOptions{PruneChildren: true}}}, + {name: "remove_image", arguments: []interface{}{"bar", dockertypes.ImageRemoveOptions{PruneChildren: true}}}}) +} + func TestKillContainerInPod(t *testing.T) { manager, fakeDocker := newTestDockerManager() diff --git a/pkg/kubelet/dockertools/docker_test.go b/pkg/kubelet/dockertools/docker_test.go index bb2ef8a790f..2f3c85c3158 100644 --- a/pkg/kubelet/dockertools/docker_test.go +++ b/pkg/kubelet/dockertools/docker_test.go @@ -30,6 +30,7 @@ import ( dockertypes "github.com/docker/engine-api/types" dockernat "github.com/docker/go-connections/nat" cadvisorapi "github.com/google/cadvisor/info/v1" + "github.com/stretchr/testify/assert" "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/apis/componentconfig" "k8s.io/kubernetes/pkg/client/record" @@ -43,9 +44,7 @@ import ( ) func verifyCalls(t *testing.T, fakeDocker *FakeDockerClient, calls []string) { - fakeDocker.Lock() - defer fakeDocker.Unlock() - verifyStringArrayEquals(t, fakeDocker.called, calls) + assert.New(t).NoError(fakeDocker.AssertCalls(calls)) } func verifyStringArrayEquals(t *testing.T, actual, expected []string) { diff --git a/pkg/kubelet/dockertools/fake_docker_client.go b/pkg/kubelet/dockertools/fake_docker_client.go index c6246665acd..822c570dc1c 100644 --- a/pkg/kubelet/dockertools/fake_docker_client.go +++ b/pkg/kubelet/dockertools/fake_docker_client.go @@ -32,6 +32,11 @@ import ( "k8s.io/kubernetes/pkg/api" ) +type calledDetail struct { + name string + arguments []interface{} +} + // FakeDockerClient is a simple fake docker client, so that kubelet can be run for testing without requiring a real docker setup. type FakeDockerClient struct { sync.Mutex @@ -41,7 +46,7 @@ type FakeDockerClient struct { Image *dockertypes.ImageInspect Images []dockertypes.Image Errors map[string]error - called []string + called []calledDetail pulled []string // Created, Stopped and Removed all container docker ID @@ -95,13 +100,21 @@ func (f *FakeDockerClient) ClearErrors() { func (f *FakeDockerClient) ClearCalls() { f.Lock() defer f.Unlock() - f.called = []string{} + f.called = []calledDetail{} f.Stopped = []string{} f.pulled = []string{} f.Created = []string{} f.Removed = []string{} } +func (f *FakeDockerClient) getCalledNames() []string { + names := []string{} + for _, detail := range f.called { + names = append(names, detail.name) + } + return names +} + // Because the new data type returned by engine-api is too complex to manually initialize, we need a // fake container which is easier to initialize. type FakeContainer struct { @@ -178,6 +191,17 @@ func (f *FakeDockerClient) AssertCalls(calls []string) (err error) { f.Lock() defer f.Unlock() + if !reflect.DeepEqual(calls, f.getCalledNames()) { + err = fmt.Errorf("expected %#v, got %#v", calls, f.getCalledNames()) + } + + return +} + +func (f *FakeDockerClient) AssertCallDetails(calls []calledDetail) (err error) { + f.Lock() + defer f.Unlock() + if !reflect.DeepEqual(calls, f.called) { err = fmt.Errorf("expected %#v, got %#v", calls, f.called) } @@ -216,24 +240,6 @@ func (f *FakeDockerClient) AssertStopped(stopped []string) error { return nil } -func (f *FakeDockerClient) AssertUnorderedCalls(calls []string) (err error) { - f.Lock() - defer f.Unlock() - - expected := make([]string, len(calls)) - actual := make([]string, len(f.called)) - copy(expected, calls) - copy(actual, f.called) - - sort.StringSlice(expected).Sort() - sort.StringSlice(actual).Sort() - - if !reflect.DeepEqual(actual, expected) { - err = fmt.Errorf("expected(sorted) %#v, got(sorted) %#v", expected, actual) - } - return -} - func (f *FakeDockerClient) popError(op string) error { if f.Errors == nil { return nil @@ -252,7 +258,7 @@ func (f *FakeDockerClient) popError(op string) error { func (f *FakeDockerClient) ListContainers(options dockertypes.ContainerListOptions) ([]dockertypes.Container, error) { f.Lock() defer f.Unlock() - f.called = append(f.called, "list") + f.called = append(f.called, calledDetail{name: "list"}) err := f.popError("list") containerList := append([]dockertypes.Container{}, f.RunningContainerList...) if options.All { @@ -269,7 +275,7 @@ func (f *FakeDockerClient) ListContainers(options dockertypes.ContainerListOptio func (f *FakeDockerClient) InspectContainer(id string) (*dockertypes.ContainerJSON, error) { f.Lock() defer f.Unlock() - f.called = append(f.called, "inspect_container") + f.called = append(f.called, calledDetail{name: "inspect_container"}) err := f.popError("inspect_container") if container, ok := f.ContainerMap[id]; ok { return container, err @@ -282,7 +288,7 @@ func (f *FakeDockerClient) InspectContainer(id string) (*dockertypes.ContainerJS func (f *FakeDockerClient) InspectImage(name string) (*dockertypes.ImageInspect, error) { f.Lock() defer f.Unlock() - f.called = append(f.called, "inspect_image") + f.called = append(f.called, calledDetail{name: "inspect_image"}) err := f.popError("inspect_image") return f.Image, err } @@ -306,7 +312,7 @@ func (f *FakeDockerClient) normalSleep(mean, stdDev, cutOffMillis int) { func (f *FakeDockerClient) CreateContainer(c dockertypes.ContainerCreateConfig) (*dockertypes.ContainerCreateResponse, error) { f.Lock() defer f.Unlock() - f.called = append(f.called, "create") + f.called = append(f.called, calledDetail{name: "create"}) if err := f.popError("create"); err != nil { return nil, err } @@ -329,7 +335,7 @@ func (f *FakeDockerClient) CreateContainer(c dockertypes.ContainerCreateConfig) func (f *FakeDockerClient) StartContainer(id string) error { f.Lock() defer f.Unlock() - f.called = append(f.called, "start") + f.called = append(f.called, calledDetail{name: "start"}) if err := f.popError("start"); err != nil { return err } @@ -352,7 +358,7 @@ func (f *FakeDockerClient) StartContainer(id string) error { func (f *FakeDockerClient) StopContainer(id string, timeout int) error { f.Lock() defer f.Unlock() - f.called = append(f.called, "stop") + f.called = append(f.called, calledDetail{name: "stop"}) if err := f.popError("stop"); err != nil { return err } @@ -390,7 +396,7 @@ func (f *FakeDockerClient) StopContainer(id string, timeout int) error { func (f *FakeDockerClient) RemoveContainer(id string, opts dockertypes.ContainerRemoveOptions) error { f.Lock() defer f.Unlock() - f.called = append(f.called, "remove") + f.called = append(f.called, calledDetail{name: "remove"}) err := f.popError("remove") if err != nil { return err @@ -413,7 +419,7 @@ func (f *FakeDockerClient) RemoveContainer(id string, opts dockertypes.Container func (f *FakeDockerClient) Logs(id string, opts dockertypes.ContainerLogsOptions, sopts StreamOptions) error { f.Lock() defer f.Unlock() - f.called = append(f.called, "logs") + f.called = append(f.called, calledDetail{name: "logs"}) return f.popError("logs") } @@ -422,7 +428,7 @@ func (f *FakeDockerClient) Logs(id string, opts dockertypes.ContainerLogsOptions func (f *FakeDockerClient) PullImage(image string, auth dockertypes.AuthConfig, opts dockertypes.ImagePullOptions) error { f.Lock() defer f.Unlock() - f.called = append(f.called, "pull") + f.called = append(f.called, calledDetail{name: "pull"}) err := f.popError("pull") if err == nil { authJson, _ := json.Marshal(auth) @@ -445,21 +451,21 @@ func (f *FakeDockerClient) CreateExec(id string, opts dockertypes.ExecConfig) (* f.Lock() defer f.Unlock() f.execCmd = opts.Cmd - f.called = append(f.called, "create_exec") + f.called = append(f.called, calledDetail{name: "create_exec"}) return &dockertypes.ContainerExecCreateResponse{ID: "12345678"}, nil } func (f *FakeDockerClient) StartExec(startExec string, opts dockertypes.ExecStartCheck, sopts StreamOptions) error { f.Lock() defer f.Unlock() - f.called = append(f.called, "start_exec") + f.called = append(f.called, calledDetail{name: "start_exec"}) return nil } func (f *FakeDockerClient) AttachToContainer(id string, opts dockertypes.ContainerAttachOptions, sopts StreamOptions) error { f.Lock() defer f.Unlock() - f.called = append(f.called, "attach") + f.called = append(f.called, calledDetail{name: "attach"}) return nil } @@ -468,12 +474,13 @@ func (f *FakeDockerClient) InspectExec(id string) (*dockertypes.ContainerExecIns } func (f *FakeDockerClient) ListImages(opts dockertypes.ImageListOptions) ([]dockertypes.Image, error) { - f.called = append(f.called, "list_images") + f.called = append(f.called, calledDetail{name: "list_images"}) err := f.popError("list_images") return f.Images, err } func (f *FakeDockerClient) RemoveImage(image string, opts dockertypes.ImageRemoveOptions) ([]dockertypes.ImageDelete, error) { + f.called = append(f.called, calledDetail{name: "remove_image", arguments: []interface{}{image, opts}}) err := f.popError("remove_image") if err == nil { for i := range f.Images { @@ -503,14 +510,14 @@ func (f *FakeDockerClient) updateContainerStatus(id, status string) { func (f *FakeDockerClient) ResizeExecTTY(id string, height, width int) error { f.Lock() defer f.Unlock() - f.called = append(f.called, "resize_exec") + f.called = append(f.called, calledDetail{name: "resize_exec"}) return nil } func (f *FakeDockerClient) ResizeContainerTTY(id string, height, width int) error { f.Lock() defer f.Unlock() - f.called = append(f.called, "resize_container") + f.called = append(f.called, calledDetail{name: "resize_container"}) return nil } @@ -555,7 +562,7 @@ func (f *FakeDockerPuller) IsImagePresent(name string) (bool, error) { func (f *FakeDockerClient) ImageHistory(id string) ([]dockertypes.ImageHistory, error) { f.Lock() defer f.Unlock() - f.called = append(f.called, "image_history") + f.called = append(f.called, calledDetail{name: "image_history"}) history := f.ImageHistoryMap[id] return history, nil }