Be more persistent when removing images

kubernetes · Nov 6, 2018 · 9e8df0f · 9e8df0f
1 parent 5f90404
commit 9e8df0f
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 12 deletions.
diff --git a/pkg/kubelet/dockershim/docker_image.go b/pkg/kubelet/dockershim/docker_image.go
@@ -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
 }
 

diff --git a/pkg/kubelet/dockershim/docker_image_test.go b/pkg/kubelet/dockershim/docker_image_test.go
@@ -38,15 +38,36 @@ func TestRemoveImage(t *testing.T) {
 			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: "1111", RepoTags: []string{"foo", "bar"}},
+			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}}),
 			},
 		},
 	}