Merge pull request #29316 from ronnielai/docker-image-remove

Automatic merge from submit-queue

Removing images with multiple tags

If an image has multiple tags, we need to remove all the tags in order to make docker image removing successful.

#28491

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
 }

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()
 

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()
+	assert.New(t).NoError(fakeDocker.AssertCalls(calls))
-	defer fakeDocker.Unlock()
-	verifyStringArrayEquals(t, fakeDocker.called, calls)
 }
 
 func verifyStringArrayEquals(t *testing.T, actual, expected []string) {

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
 }