Switch to ImageServer.UntagImage in RemoveImage handler

Add an UntagImage() method to pkg/storage/ImageServer, which will check
if the passed-in NameOrID is a name.  If so, it merely removes that name
from the image, removing the image only if it was the last name that the
image had.  If the NameOrID is an image ID, the image is removed, as
RemoveImage() does.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

pkg/storage/image.go

@@ -3,7 +3,7 @@ package storage
 import (
 	"errors"
 	"net"
-	"path/filepath"
+	"path"
 	"strings"
 
 	"github.com/containers/image/copy"
@@ -21,6 +21,8 @@ import (
 var (
 	// ErrCannotParseImageID is returned when we try to ResolveNames for an image ID
 	ErrCannotParseImageID = errors.New("cannot parse an image ID")
+	// ErrImageMultiplyTagged is returned when we try to remove an image that still has multiple names
+	ErrImageMultiplyTagged = errors.New("image still has multiple names applied")
 )
 
 // ImageResult wraps a subset of information about an image: its ID, its names,
@@ -65,6 +67,9 @@ type ImageServer interface {
 	PrepareImage(systemContext *types.SystemContext, imageName string, options *copy.Options) (types.Image, error)
 	// PullImage imports an image from the specified location.
 	PullImage(systemContext *types.SystemContext, imageName string, options *copy.Options) (types.ImageReference, error)
+	// UntagImage removes a name from the specified image, and if it was
+	// the only name the image had, removes the image.
+	UntagImage(systemContext *types.SystemContext, imageName string) error
 	// RemoveImage deletes the specified image.
 	RemoveImage(systemContext *types.SystemContext, imageName string) error
 	// GetStore returns the reference to the storage library Store which
@@ -389,6 +394,57 @@ func (svc *imageService) PullImage(systemContext *types.SystemContext, imageName
 	return destRef, nil
 }
 
+func (svc *imageService) UntagImage(systemContext *types.SystemContext, nameOrID string) error {
+	ref, err := alltransports.ParseImageName(nameOrID)
+	if err != nil {
+		ref2, err2 := istorage.Transport.ParseStoreReference(svc.store, "@"+nameOrID)
+		if err2 != nil {
+			ref3, err3 := istorage.Transport.ParseStoreReference(svc.store, nameOrID)
+			if err3 != nil {
+				return err
+			}
+			ref2 = ref3
+		}
+		ref = ref2
+	}
+
+	img, err := istorage.Transport.GetStoreImage(svc.store, ref)
+	if err != nil {
+		return err
+	}
+
+	if !strings.HasPrefix(img.ID, nameOrID) {
+		namedRef, err := svc.prepareReference(nameOrID, &copy.Options{})
+		if err != nil {
+			return err
+		}
+
+		name := nameOrID
+		if namedRef.DockerReference() != nil {
+			name = namedRef.DockerReference().Name()
+			if tagged, ok := namedRef.DockerReference().(reference.NamedTagged); ok {
+				name = name + ":" + tagged.Tag()
+			}
+			if canonical, ok := namedRef.DockerReference().(reference.Canonical); ok {
+				name = name + "@" + canonical.Digest().String()
+			}
+		}
+
+		prunedNames := make([]string, 0, len(img.Names))
+		for _, imgName := range img.Names {
+			if imgName != name && imgName != nameOrID {
+				prunedNames = append(prunedNames, imgName)
+			}
+		}
+
+		if len(prunedNames) > 0 {
+			return svc.store.SetNames(img.ID, prunedNames)
+		}
+	}
+
+	return ref.DeleteImage(systemContext)
+}
+
 func (svc *imageService) RemoveImage(systemContext *types.SystemContext, nameOrID string) error {
 	ref, err := alltransports.ParseImageName(nameOrID)
 	if err != nil {
@@ -483,7 +539,7 @@ func (svc *imageService) ResolveNames(imageName string) ([]string, error) {
 		if r == "docker.io" && !strings.ContainsRune(remainder, '/') {
 			rem = "library/" + rem
 		}
-		images = append(images, filepath.Join(r, rem))
+		images = append(images, path.Join(r, rem))
 	}
 	return images, nil
 }

server/image_remove.go

@@ -40,7 +40,7 @@ func (s *Server) RemoveImage(ctx context.Context, req *pb.RemoveImageRequest) (r
 		}
 	}
 	for _, img := range images {
-		err = s.StorageImageServer().RemoveImage(s.ImageContext(), img)
+		err = s.StorageImageServer().UntagImage(s.ImageContext(), img)
 		if err != nil {
 			logrus.Debugf("error deleting image %s: %v", img, err)
 			continue

test/helpers.bash

@@ -212,11 +212,11 @@ function start_crio() {
 	if ! [ "$3" = "--no-pause-image" ] ; then
 		"$BIN2IMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTIONS --runroot "$TESTDIR/crio-run" --source-binary "$PAUSE_BINARY"
 	fi
-	"$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTIONS --runroot "$TESTDIR/crio-run" --image-name=redis:alpine --import-from=dir:"$ARTIFACTS_PATH"/redis-image --add-name=docker.io/library/redis:alpine --signature-policy="$INTEGRATION_ROOT"/policy.json
+	"$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTIONS --runroot "$TESTDIR/crio-run" --image-name=docker.io/library/redis:alpine --import-from=dir:"$ARTIFACTS_PATH"/redis-image --signature-policy="$INTEGRATION_ROOT"/policy.json
-	"$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTIONS --runroot "$TESTDIR/crio-run" --image-name=mrunalp/oom --import-from=dir:"$ARTIFACTS_PATH"/oom-image --add-name=docker.io/mrunalp/oom --signature-policy="$INTEGRATION_ROOT"/policy.json
+	"$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTIONS --runroot "$TESTDIR/crio-run" --image-name=docker.io/mrunalp/oom:latest --import-from=dir:"$ARTIFACTS_PATH"/oom-image --signature-policy="$INTEGRATION_ROOT"/policy.json
-	"$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTIONS --runroot "$TESTDIR/crio-run" --image-name=mrunalp/image-volume-test --import-from=dir:"$ARTIFACTS_PATH"/image-volume-test-image --add-name=docker.io/mrunalp/image-volume-test --signature-policy="$INTEGRATION_ROOT"/policy.json
+	"$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTIONS --runroot "$TESTDIR/crio-run" --image-name=docker.io/mrunalp/image-volume-test:latest --import-from=dir:"$ARTIFACTS_PATH"/image-volume-test-image --signature-policy="$INTEGRATION_ROOT"/policy.json
-	"$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTIONS --runroot "$TESTDIR/crio-run" --image-name=busybox:latest --import-from=dir:"$ARTIFACTS_PATH"/busybox-image --add-name=docker.io/library/busybox:latest --signature-policy="$INTEGRATION_ROOT"/policy.json
+	"$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTIONS --runroot "$TESTDIR/crio-run" --image-name=docker.io/library/busybox:latest --import-from=dir:"$ARTIFACTS_PATH"/busybox-image --signature-policy="$INTEGRATION_ROOT"/policy.json
-	"$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTIONS --runroot "$TESTDIR/crio-run" --image-name=runcom/stderr-test:latest --import-from=dir:"$ARTIFACTS_PATH"/stderr-test --add-name=docker.io/runcom/stderr-test:latest --signature-policy="$INTEGRATION_ROOT"/policy.json
+	"$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTIONS --runroot "$TESTDIR/crio-run" --image-name=docker.io/runcom/stderr-test:latest --import-from=dir:"$ARTIFACTS_PATH"/stderr-test --signature-policy="$INTEGRATION_ROOT"/policy.json
 	"$CRIO_BINARY" ${DEFAULT_MOUNTS_OPTS} ${HOOKS_OPTS} --conmon "$CONMON_BINARY" --listen "$CRIO_SOCKET" --cgroup-manager "$CGROUP_MANAGER" --registry "docker.io" --runtime "$RUNTIME_BINARY" --root "$TESTDIR/crio" --runroot "$TESTDIR/crio-run" $STORAGE_OPTIONS --seccomp-profile "$seccomp" --apparmor-profile "$apparmor" --cni-config-dir "$CRIO_CNI_CONFIG" --cni-plugin-dir "$CRIO_CNI_PLUGIN" --signature-policy "$INTEGRATION_ROOT"/policy.json --image-volumes "$IMAGE_VOLUMES" --pids-limit "$PIDS_LIMIT" --enable-shared-pid-namespace=${ENABLE_SHARED_PID_NAMESPACE} --log-size-max "$LOG_SIZE_MAX_LIMIT" --config /dev/null config >$CRIO_CONFIG
 
 	# Prepare the CNI configuration files, we're running with non host networking by default

test/image_remove.bats

@@ -0,0 +1,74 @@
+#!/usr/bin/env bats
+
+load helpers
+
+IMAGE=docker.io/kubernetes/pause
+
+function teardown() {
+	cleanup_test
+}
+
+@test "image remove with multiple names, by name" {
+	start_crio "" "" --no-pause-image
+	# Pull the image, giving it one name.
+	run crioctl image pull "$IMAGE"
+	echo "$output"
+	[ "$status" -eq 0 ]
+	# Add a second name to the image.
+	run "$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTIONS --runroot "$TESTDIR/crio-run" --image-name="$IMAGE":latest --add-name="$IMAGE":othertag --signature-policy="$INTEGRATION_ROOT"/policy.json
+	echo "$output"
+	[ "$status" -eq 0 ]
+	# Get the list of image names and IDs.
+	run crioctl image list
+	echo "$output"
+	[ "$status" -eq 0 ]
+	[ "$output" != "" ]
+	# Cycle through each name, removing it by name.  The image that we assigned a second
+	# name to should still be around when we get to removing its second name.
+	grep ^Tag: <<< "$output" | while read -r header tag ; do
+		run crioctl image remove --id "$tag"
+		echo "$output"
+		[ "$status" -eq 0 ]
+	done
+	# List all images and their names.  There should be none now.
+	run crioctl image list --quiet
+	echo "$output"
+	[ "$status" -eq 0 ]
+	[ "$output" = "" ]
+	printf '%s\n' "$output" | while IFS= read -r id; do
+		echo "$id"
+	done
+	# All done.
+	cleanup_images
+	stop_crio
+}
+
+@test "image remove with multiple names, by ID" {
+	start_crio "" "" --no-pause-image
+	# Pull the image, giving it one name.
+	run crioctl image pull "$IMAGE"
+	echo "$output"
+	[ "$status" -eq 0 ]
+	# Add a second name to the image.
+	run "$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTIONS --runroot "$TESTDIR/crio-run" --image-name="$IMAGE":latest --add-name="$IMAGE":othertag --signature-policy="$INTEGRATION_ROOT"/policy.json
+	echo "$output"
+	[ "$status" -eq 0 ]
+	# Get the image ID of the image we just saved.
+	run crioctl image status --id="$IMAGE"
+	echo "$output"
+	[ "$status" -eq 0 ]
+	[ "$output" != "" ]
+	# Try to remove the image using its ID.  That should succeed.
+	grep ^ID: <<< "$output" | while read -r header id ; do
+		run crioctl image remove --id "$id"
+		echo "$output"
+		[ "$status" -ne 0 ]
+	done
+	# The image should be gone now.
+	run crioctl image status --id="$IMAGE"
+	echo "$output"
+	[ "$status" -ne 0 ]
+	# All done.
+	cleanup_images
+	stop_crio
+}