Skip to content

Commit

Permalink
Merge pull request kubernetes#22154 from sjenning/pick-70647-3.11
Browse files Browse the repository at this point in the history 
UPSTREAM: 70647: Always run untag when removing docker image

Origin-commit: 8d848f9d6ecbae401811e1b1856bfb5b5073ec66
  • Loading branch information
@k8s-publishing-bot
k8s-publishing-bot committed Mar 13, 2019
2 parents 41e4c97 + 11d3555 commit 0f51d76
Show file tree
Hide file tree
Showing 3 changed files with 91 additions and 53 deletions.
25 changes: 14 additions & 11 deletions pkg/kubelet/dockershim/docker_image.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,24 +120,27 @@ func (ds *dockerService) RemoveImage(_ context.Context, r *runtimeapi.RemoveImag
// TODO: We assume image.Image is image ID here, which is true in the current implementation
// of kubelet, but we should still clarify this in CRI.
imageInspect, err := ds.client.InspectImageByID(image.Image)
if err == nil && imageInspect != nil && len(imageInspect.RepoTags) > 1 {
for _, tag := range imageInspect.RepoTags {
if _, err := ds.client.RemoveImage(tag, dockertypes.ImageRemoveOptions{PruneChildren: true}); err != nil && !libdocker.IsImageNotFoundError(err) {
return nil, err
}
}
return &runtimeapi.RemoveImageResponse{}, nil
}

// dockerclient.InspectImageByID doesn't work with digest and repoTags,
// it is safe to continue removing it since there is another check below.
if err != nil && !libdocker.IsImageNotFoundError(err) {
return nil, err
}

_, err = ds.client.RemoveImage(image.Image, dockertypes.ImageRemoveOptions{PruneChildren: true})
if err != nil && !libdocker.IsImageNotFoundError(err) {
return nil, err
// An image can have different numbers of RepoTags and RepoDigests.
// Iterating over both of them plus the image ID ensures the image really got removed.
// It also prevents images from being deleted, which actually are deletable using this approach.
var images []string
images = append(images, imageInspect.RepoTags...)
images = append(images, imageInspect.RepoDigests...)
images = append(images, image.Image)

for _, image := range images {
if _, err := ds.client.RemoveImage(image, dockertypes.ImageRemoveOptions{PruneChildren: true}); err != nil && !libdocker.IsImageNotFoundError(err) {
return nil, err
}
}

return &runtimeapi.RemoveImageResponse{}, nil
}

Expand Down
65 changes: 50 additions & 15 deletions pkg/kubelet/dockershim/docker_image_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,22 +30,57 @@ import (
)

func TestRemoveImage(t *testing.T) {
ds, fakeDocker, _ := newTestDockerService()
id := "1111"
fakeDocker.InjectImageInspects([]dockertypes.ImageInspect{{ID: id, RepoTags: []string{"foo"}}})
ds.RemoveImage(getTestCTX(), &runtimeapi.RemoveImageRequest{Image: &runtimeapi.ImageSpec{Image: id}})
fakeDocker.AssertCallDetails(libdocker.NewCalledDetail("inspect_image", nil),
libdocker.NewCalledDetail("remove_image", []interface{}{id, dockertypes.ImageRemoveOptions{PruneChildren: true}}))
}
tests := map[string]struct {
image dockertypes.ImageInspect
calledDetails []libdocker.CalledDetail
}{
"single tag": {
dockertypes.ImageInspect{ID: "1111", RepoTags: []string{"foo"}},
[]libdocker.CalledDetail{
libdocker.NewCalledDetail("inspect_image", nil),
libdocker.NewCalledDetail("remove_image", []interface{}{"foo", dockertypes.ImageRemoveOptions{PruneChildren: true}}),
libdocker.NewCalledDetail("remove_image", []interface{}{"1111", dockertypes.ImageRemoveOptions{PruneChildren: true}}),
},
},
"multiple tags": {
dockertypes.ImageInspect{ID: "2222", RepoTags: []string{"foo", "bar"}},
[]libdocker.CalledDetail{
libdocker.NewCalledDetail("inspect_image", nil),
libdocker.NewCalledDetail("remove_image", []interface{}{"foo", dockertypes.ImageRemoveOptions{PruneChildren: true}}),
libdocker.NewCalledDetail("remove_image", []interface{}{"bar", dockertypes.ImageRemoveOptions{PruneChildren: true}}),
libdocker.NewCalledDetail("remove_image", []interface{}{"2222", dockertypes.ImageRemoveOptions{PruneChildren: true}}),
},
},
"single tag multiple repo digests": {
dockertypes.ImageInspect{ID: "3333", RepoTags: []string{"foo"}, RepoDigests: []string{"foo@3333", "example.com/foo@3333"}},
[]libdocker.CalledDetail{
libdocker.NewCalledDetail("inspect_image", nil),
libdocker.NewCalledDetail("remove_image", []interface{}{"foo", dockertypes.ImageRemoveOptions{PruneChildren: true}}),
libdocker.NewCalledDetail("remove_image", []interface{}{"foo@3333", dockertypes.ImageRemoveOptions{PruneChildren: true}}),
libdocker.NewCalledDetail("remove_image", []interface{}{"example.com/foo@3333", dockertypes.ImageRemoveOptions{PruneChildren: true}}),
libdocker.NewCalledDetail("remove_image", []interface{}{"3333", dockertypes.ImageRemoveOptions{PruneChildren: true}}),
},
},
"no tags multiple repo digests": {
dockertypes.ImageInspect{ID: "4444", RepoTags: []string{}, RepoDigests: []string{"foo@4444", "example.com/foo@4444"}},
[]libdocker.CalledDetail{
libdocker.NewCalledDetail("inspect_image", nil),
libdocker.NewCalledDetail("remove_image", []interface{}{"foo@4444", dockertypes.ImageRemoveOptions{PruneChildren: true}}),
libdocker.NewCalledDetail("remove_image", []interface{}{"example.com/foo@4444", dockertypes.ImageRemoveOptions{PruneChildren: true}}),
libdocker.NewCalledDetail("remove_image", []interface{}{"4444", dockertypes.ImageRemoveOptions{PruneChildren: true}}),
},
},
}

func TestRemoveImageWithMultipleTags(t *testing.T) {
ds, fakeDocker, _ := newTestDockerService()
id := "1111"
fakeDocker.InjectImageInspects([]dockertypes.ImageInspect{{ID: id, RepoTags: []string{"foo", "bar"}}})
ds.RemoveImage(getTestCTX(), &runtimeapi.RemoveImageRequest{Image: &runtimeapi.ImageSpec{Image: id}})
fakeDocker.AssertCallDetails(libdocker.NewCalledDetail("inspect_image", nil),
libdocker.NewCalledDetail("remove_image", []interface{}{"foo", dockertypes.ImageRemoveOptions{PruneChildren: true}}),
libdocker.NewCalledDetail("remove_image", []interface{}{"bar", dockertypes.ImageRemoveOptions{PruneChildren: true}}))
for name, test := range tests {
t.Run(name, func(t *testing.T) {
ds, fakeDocker, _ := newTestDockerService()
fakeDocker.InjectImageInspects([]dockertypes.ImageInspect{test.image})
ds.RemoveImage(getTestCTX(), &runtimeapi.RemoveImageRequest{Image: &runtimeapi.ImageSpec{Image: test.image.ID}})
err := fakeDocker.AssertCallDetails(test.calledDetails...)
assert.NoError(t, err)
})
}
}

func TestPullWithJSONError(t *testing.T) {
Expand Down
54 changes: 27 additions & 27 deletions pkg/kubelet/dockershim/libdocker/fake_client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,14 @@ import (
"k8s.io/apimachinery/pkg/util/clock"
)

type calledDetail struct {
type CalledDetail struct {
name string
arguments []interface{}
}

// NewCalledDetail create a new call detail item.
func NewCalledDetail(name string, arguments []interface{}) calledDetail {
return calledDetail{name: name, arguments: arguments}
func NewCalledDetail(name string, arguments []interface{}) CalledDetail {
return CalledDetail{name: name, arguments: arguments}
}

// FakeDockerClient is a simple fake docker client, so that kubelet can be run for testing without requiring a real docker setup.
Expand All @@ -58,7 +58,7 @@ type FakeDockerClient struct {
Images []dockertypes.ImageSummary
ImageIDsNeedingAuth map[string]dockertypes.AuthConfig
Errors map[string]error
called []calledDetail
called []CalledDetail
pulled []string
EnableTrace bool
RandGenerator *rand.Rand
Expand Down Expand Up @@ -132,7 +132,7 @@ func (f *FakeDockerClient) WithRandSource(source rand.Source) *FakeDockerClient
return f
}

func (f *FakeDockerClient) appendCalled(callDetail calledDetail) {
func (f *FakeDockerClient) appendCalled(callDetail CalledDetail) {
if f.EnableTrace {
f.called = append(f.called, callDetail)
}
Expand Down Expand Up @@ -183,7 +183,7 @@ func (f *FakeDockerClient) ClearErrors() {
func (f *FakeDockerClient) ClearCalls() {
f.Lock()
defer f.Unlock()
f.called = []calledDetail{}
f.called = []CalledDetail{}
f.pulled = []string{}
f.Created = []string{}
f.Started = []string{}
Expand Down Expand Up @@ -286,7 +286,7 @@ func (f *FakeDockerClient) AssertCalls(calls []string) (err error) {
return
}

func (f *FakeDockerClient) AssertCallDetails(calls ...calledDetail) (err error) {
func (f *FakeDockerClient) AssertCallDetails(calls ...CalledDetail) (err error) {
f.Lock()
defer f.Unlock()

Expand Down Expand Up @@ -390,7 +390,7 @@ func (f *FakeDockerClient) popError(op string) error {
func (f *FakeDockerClient) ListContainers(options dockertypes.ContainerListOptions) ([]dockertypes.Container, error) {
f.Lock()
defer f.Unlock()
f.appendCalled(calledDetail{name: "list"})
f.appendCalled(CalledDetail{name: "list"})
err := f.popError("list")
containerList := append([]dockertypes.Container{}, f.RunningContainerList...)
if options.All {
Expand Down Expand Up @@ -470,7 +470,7 @@ func toDockerContainerStatus(state string) string {
func (f *FakeDockerClient) InspectContainer(id string) (*dockertypes.ContainerJSON, error) {
f.Lock()
defer f.Unlock()
f.appendCalled(calledDetail{name: "inspect_container"})
f.appendCalled(CalledDetail{name: "inspect_container"})
err := f.popError("inspect_container")
if container, ok := f.ContainerMap[id]; ok {
return container, err
Expand All @@ -487,7 +487,7 @@ func (f *FakeDockerClient) InspectContainer(id string) (*dockertypes.ContainerJS
func (f *FakeDockerClient) InspectContainerWithSize(id string) (*dockertypes.ContainerJSON, error) {
f.Lock()
defer f.Unlock()
f.appendCalled(calledDetail{name: "inspect_container_withsize"})
f.appendCalled(CalledDetail{name: "inspect_container_withsize"})
err := f.popError("inspect_container_withsize")
if container, ok := f.ContainerMap[id]; ok {
return container, err
Expand All @@ -504,7 +504,7 @@ func (f *FakeDockerClient) InspectContainerWithSize(id string) (*dockertypes.Con
func (f *FakeDockerClient) InspectImageByRef(name string) (*dockertypes.ImageInspect, error) {
f.Lock()
defer f.Unlock()
f.appendCalled(calledDetail{name: "inspect_image"})
f.appendCalled(CalledDetail{name: "inspect_image"})
if err := f.popError("inspect_image"); err != nil {
return nil, err
}
Expand All @@ -519,7 +519,7 @@ func (f *FakeDockerClient) InspectImageByRef(name string) (*dockertypes.ImageIns
func (f *FakeDockerClient) InspectImageByID(name string) (*dockertypes.ImageInspect, error) {
f.Lock()
defer f.Unlock()
f.appendCalled(calledDetail{name: "inspect_image"})
f.appendCalled(CalledDetail{name: "inspect_image"})
if err := f.popError("inspect_image"); err != nil {
return nil, err
}
Expand Down Expand Up @@ -555,7 +555,7 @@ func GetFakeContainerID(name string) string {
func (f *FakeDockerClient) CreateContainer(c dockertypes.ContainerCreateConfig) (*dockercontainer.ContainerCreateCreatedBody, error) {
f.Lock()
defer f.Unlock()
f.appendCalled(calledDetail{name: "create"})
f.appendCalled(CalledDetail{name: "create"})
if err := f.popError("create"); err != nil {
return nil, err
}
Expand All @@ -581,7 +581,7 @@ func (f *FakeDockerClient) CreateContainer(c dockertypes.ContainerCreateConfig)
func (f *FakeDockerClient) StartContainer(id string) error {
f.Lock()
defer f.Unlock()
f.appendCalled(calledDetail{name: "start"})
f.appendCalled(CalledDetail{name: "start"})
if err := f.popError("start"); err != nil {
return err
}
Expand Down Expand Up @@ -619,7 +619,7 @@ func (f *FakeDockerClient) StartContainer(id string) error {
func (f *FakeDockerClient) StopContainer(id string, timeout time.Duration) error {
f.Lock()
defer f.Unlock()
f.appendCalled(calledDetail{name: "stop"})
f.appendCalled(CalledDetail{name: "stop"})
if err := f.popError("stop"); err != nil {
return err
}
Expand Down Expand Up @@ -657,7 +657,7 @@ func (f *FakeDockerClient) StopContainer(id string, timeout time.Duration) error
func (f *FakeDockerClient) RemoveContainer(id string, opts dockertypes.ContainerRemoveOptions) error {
f.Lock()
defer f.Unlock()
f.appendCalled(calledDetail{name: "remove"})
f.appendCalled(CalledDetail{name: "remove"})
err := f.popError("remove")
if err != nil {
return err
Expand Down Expand Up @@ -693,7 +693,7 @@ func (f *FakeDockerClient) UpdateContainerResources(id string, updateConfig dock
func (f *FakeDockerClient) Logs(id string, opts dockertypes.ContainerLogsOptions, sopts StreamOptions) error {
f.Lock()
defer f.Unlock()
f.appendCalled(calledDetail{name: "logs"})
f.appendCalled(CalledDetail{name: "logs"})
return f.popError("logs")
}

Expand All @@ -710,7 +710,7 @@ func (f *FakeDockerClient) isAuthorizedForImage(image string, auth dockertypes.A
func (f *FakeDockerClient) PullImage(image string, auth dockertypes.AuthConfig, opts dockertypes.ImagePullOptions) error {
f.Lock()
defer f.Unlock()
f.appendCalled(calledDetail{name: "pull"})
f.appendCalled(CalledDetail{name: "pull"})
err := f.popError("pull")
if err == nil {
if !f.isAuthorizedForImage(image, auth) {
Expand Down Expand Up @@ -742,21 +742,21 @@ func (f *FakeDockerClient) CreateExec(id string, opts dockertypes.ExecConfig) (*
f.Lock()
defer f.Unlock()
f.execCmd = opts.Cmd
f.appendCalled(calledDetail{name: "create_exec"})
f.appendCalled(CalledDetail{name: "create_exec"})
return &dockertypes.IDResponse{ID: "12345678"}, nil
}

func (f *FakeDockerClient) StartExec(startExec string, opts dockertypes.ExecStartCheck, sopts StreamOptions) error {
f.Lock()
defer f.Unlock()
f.appendCalled(calledDetail{name: "start_exec"})
f.appendCalled(CalledDetail{name: "start_exec"})
return nil
}

func (f *FakeDockerClient) AttachToContainer(id string, opts dockertypes.ContainerAttachOptions, sopts StreamOptions) error {
f.Lock()
defer f.Unlock()
f.appendCalled(calledDetail{name: "attach"})
f.appendCalled(CalledDetail{name: "attach"})
return nil
}

Expand All @@ -767,15 +767,15 @@ func (f *FakeDockerClient) InspectExec(id string) (*dockertypes.ContainerExecIns
func (f *FakeDockerClient) ListImages(opts dockertypes.ImageListOptions) ([]dockertypes.ImageSummary, error) {
f.Lock()
defer f.Unlock()
f.appendCalled(calledDetail{name: "list_images"})
f.appendCalled(CalledDetail{name: "list_images"})
err := f.popError("list_images")
return f.Images, err
}

func (f *FakeDockerClient) RemoveImage(image string, opts dockertypes.ImageRemoveOptions) ([]dockertypes.ImageDeleteResponseItem, error) {
f.Lock()
defer f.Unlock()
f.appendCalled(calledDetail{name: "remove_image", arguments: []interface{}{image, opts}})
f.appendCalled(CalledDetail{name: "remove_image", arguments: []interface{}{image, opts}})
err := f.popError("remove_image")
if err == nil {
for i := range f.Images {
Expand Down Expand Up @@ -833,14 +833,14 @@ func (f *FakeDockerClient) updateContainerStatus(id, status string) {
func (f *FakeDockerClient) ResizeExecTTY(id string, height, width uint) error {
f.Lock()
defer f.Unlock()
f.appendCalled(calledDetail{name: "resize_exec"})
f.appendCalled(CalledDetail{name: "resize_exec"})
return nil
}

func (f *FakeDockerClient) ResizeContainerTTY(id string, height, width uint) error {
f.Lock()
defer f.Unlock()
f.appendCalled(calledDetail{name: "resize_container"})
f.appendCalled(CalledDetail{name: "resize_container"})
return nil
}

Expand Down Expand Up @@ -884,7 +884,7 @@ func dockerTimestampToString(t time.Time) string {
func (f *FakeDockerClient) ImageHistory(id string) ([]dockerimagetypes.HistoryResponseItem, error) {
f.Lock()
defer f.Unlock()
f.appendCalled(calledDetail{name: "image_history"})
f.appendCalled(CalledDetail{name: "image_history"})
history := f.ImageHistoryMap[id]
return history, nil
}
Expand Down Expand Up @@ -916,6 +916,6 @@ func (f *FakeDockerPuller) GetImageRef(image string) (string, error) {
func (f *FakeDockerClient) GetContainerStats(id string) (*dockertypes.StatsJSON, error) {
f.Lock()
defer f.Unlock()
f.appendCalled(calledDetail{name: "getContainerStats"})
f.appendCalled(CalledDetail{name: "getContainerStats"})
return nil, fmt.Errorf("not implemented")
}

0 comments on commit 0f51d76

Please sign in to comment.