From 2e5e92730a74a25e418613294e80ab7a960ef42c Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Mon, 30 Oct 2017 17:18:42 -0400 Subject: [PATCH] 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 --- pkg/storage/image.go | 58 +++++++++++++++++++++++++++++++-- server/image_remove.go | 2 +- test/helpers.bash | 12 +++---- test/image_remove.bats | 74 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 137 insertions(+), 9 deletions(-) create mode 100644 test/image_remove.bats diff --git a/pkg/storage/image.go b/pkg/storage/image.go index a5832e0f..ddff82c1 100644 --- a/pkg/storage/image.go +++ b/pkg/storage/image.go @@ -4,7 +4,7 @@ import ( "errors" "fmt" "net" - "path/filepath" + "path" "regexp" "strings" @@ -51,6 +51,9 @@ type ImageServer interface { ImageStatus(systemContext *types.SystemContext, filter string) (*ImageResult, 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 @@ -263,6 +266,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 nameOrID != img.ID { + namedRef, err := svc.prepareImage(nameOrID, ©.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 { @@ -440,7 +494,7 @@ func (svc *imageService) ResolveNames(imageName string) ([]string, error) { _, rest := splitDomain(r.Name()) images := []string{} for _, r := range svc.registries { - images = append(images, filepath.Join(r, rest)) + images = append(images, path.Join(r, rest)) } return images, nil } diff --git a/server/image_remove.go b/server/image_remove.go index 32ca4066..d15296cc 100644 --- a/server/image_remove.go +++ b/server/image_remove.go @@ -35,7 +35,7 @@ func (s *Server) RemoveImage(ctx context.Context, req *pb.RemoveImageRequest) (* } } 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 diff --git a/test/helpers.bash b/test/helpers.bash index a8d61f52..22955d33 100644 --- a/test/helpers.bash +++ b/test/helpers.bash @@ -234,16 +234,16 @@ 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 # TODO: remove the code below for copying redis:alpine in using a canonical reference once # https://github.com/kubernetes-incubator/cri-o/issues/531 is complete and we can # copy the image using a tagged reference and then subsequently find the image without # having to explicitly record the canonical reference as one of the image's names - "$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTIONS --runroot "$TESTDIR/crio-run" --image-name=redis@sha256:03789f402b2ecfb98184bf128d180f398f81c63364948ff1454583b02442f73b --import-from=dir:"$ARTIFACTS_PATH"/redis-image-digest --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/library/mrunalp/oom --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/library/mrunalp/image-volume-test --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=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/library/redis@sha256:03789f402b2ecfb98184bf128d180f398f81c63364948ff1454583b02442f73b --import-from=dir:"$ARTIFACTS_PATH"/redis-image-digest --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 --signature-policy="$INTEGRATION_ROOT"/policy.json + "$COPYIMG_BINARY" --root "$TESTDIR/crio" $STORAGE_OPTIONS --runroot "$TESTDIR/crio-run" --image-name=docker.io/library/mrunalp/image-volume-test --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=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=docker.io/library/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" --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 diff --git a/test/image_remove.bats b/test/image_remove.bats new file mode 100644 index 00000000..ca2017d0 --- /dev/null +++ b/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 because removing by ID always works. + grep ^ID: <<< "$output" | while read -r header id ; do + run crioctl image remove --id "$id" + echo "$output" + [ "$status" -eq 0 ] + done + # The image should be gone. + run crioctl image status --id="$IMAGE" + echo "$output" + [ "$status" -ne 0 ] + # All done. + cleanup_images + stop_crio +}