From 4f188aa191f1d28be19af64317bd8512f5c0cac0 Mon Sep 17 00:00:00 2001 From: umohnani8 Date: Fri, 6 Jul 2018 11:55:02 -0400 Subject: podman rmi should only untag image if parent of another podman rmi was deleting an image even if it was a parent of another image. This fix just untags the image instead. This also fixes podman rmi to remove intermediate images of an image when the image is removed. Signed-off-by: umohnani8 Closes: #1055 Approved by: mheon --- cmd/podman/images.go | 28 +++++--------- libpod/image/image.go | 101 ++++++++++++++++++++++++++++++++++++++++++++++++-- libpod/runtime_img.go | 36 +++++++++++------- test/e2e/rmi_test.go | 96 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 225 insertions(+), 36 deletions(-) diff --git a/cmd/podman/images.go b/cmd/podman/images.go index fdf2eb02c..092326b1f 100644 --- a/cmd/podman/images.go +++ b/cmd/podman/images.go @@ -7,7 +7,8 @@ import ( "strings" "time" - "github.com/containers/storage" + "github.com/sirupsen/logrus" + "github.com/docker/go-units" digest "github.com/opencontainers/go-digest" "github.com/pkg/errors" @@ -259,12 +260,16 @@ func sortImagesOutput(sortBy string, imagesOutput imagesSorted) imagesSorted { } // getImagesTemplateOutput returns the images information to be printed in human readable format -func getImagesTemplateOutput(ctx context.Context, runtime *libpod.Runtime, images []*image.Image, opts imagesOptions, storageLayers []storage.Layer) (imagesOutput imagesSorted) { +func getImagesTemplateOutput(ctx context.Context, runtime *libpod.Runtime, images []*image.Image, opts imagesOptions) (imagesOutput imagesSorted) { for _, img := range images { // If all is false and the image doesn't have a name, check to see if the top layer of the image is a parent // to another image's top layer. If it is, then it is an intermediate image so don't print out if the --all flag // is not set. - if !opts.all && len(img.Names()) == 0 && !layerIsLeaf(storageLayers, img.TopLayer()) { + isParent, err := img.IsParent() + if err != nil { + logrus.Errorf("error checking if image is a parent %q: %v", img.ID(), err) + } + if !opts.all && len(img.Names()) == 0 && isParent { continue } createdTime := img.Created() @@ -325,17 +330,13 @@ func generateImagesOutput(ctx context.Context, runtime *libpod.Runtime, images [ return nil } var out formats.Writer - storageLayers, err := runtime.GetLayers() - if err != nil { - return errors.Wrap(err, "error getting layers from store") - } switch opts.format { case formats.JSONString: imagesOutput := getImagesJSONOutput(ctx, runtime, images) out = formats.JSONStructArray{Output: imagesToGeneric([]imagesTemplateParams{}, imagesOutput)} default: - imagesOutput := getImagesTemplateOutput(ctx, runtime, images, opts, storageLayers) + imagesOutput := getImagesTemplateOutput(ctx, runtime, images, opts) out = formats.StdoutTemplateArray{Output: imagesToGeneric(imagesOutput, []imagesJSONParams{}), Template: opts.outputformat, Fields: imagesOutput[0].HeaderMap()} } return formats.Writer(out).Out() @@ -391,14 +392,3 @@ func CreateFilterFuncs(ctx context.Context, r *libpod.Runtime, c *cli.Context, i } return filterFuncs, nil } - -// layerIsLeaf goes through the layers in the store and checks if "layer" is -// the parent of any other layer in store. -func layerIsLeaf(storageLayers []storage.Layer, layer string) bool { - for _, storeLayer := range storageLayers { - if storeLayer.Parent == layer { - return false - } - } - return true -} diff --git a/libpod/image/image.go b/libpod/image/image.go index 5dd2c57f3..082c4906f 100644 --- a/libpod/image/image.go +++ b/libpod/image/image.go @@ -59,6 +59,9 @@ type Runtime struct { SignaturePolicyPath string } +// ErrRepoTagNotFound is the error returned when the image id given doesn't match a rep tag in store +var ErrRepoTagNotFound = errors.New("unable to match user input to any specific repotag") + // NewImageRuntimeFromStore creates an ImageRuntime based on a provided store func NewImageRuntimeFromStore(store storage.Store) *Runtime { return &Runtime{ @@ -333,9 +336,39 @@ func (i *Image) TopLayer() string { // Remove an image; container removal for the image must be done // outside the context of images +// TODO: the force param does nothing as of now. Need to move container +// handling logic here eventually. func (i *Image) Remove(force bool) error { - _, err := i.imageruntime.store.DeleteImage(i.ID(), true) - return err + parent, err := i.GetParent() + if err != nil { + return err + } + if _, err := i.imageruntime.store.DeleteImage(i.ID(), true); err != nil { + return err + } + for parent != nil { + nextParent, err := parent.GetParent() + if err != nil { + return err + } + children, err := parent.GetChildren() + if err != nil { + return err + } + // Do not remove if image is a base image and is not untagged, or if + // the image has more children. + if (nextParent == nil && len(parent.Names()) > 0) || len(children) > 0 { + return nil + } + id := parent.ID() + if _, err := i.imageruntime.store.DeleteImage(id, true); err != nil { + logrus.Debugf("unable to remove intermediate image %q: %v", id, err) + } else { + fmt.Println(id) + } + parent = nextParent + } + return nil } // Decompose an Image @@ -902,7 +935,7 @@ func (i *Image) MatchRepoTag(input string) (string, error) { } } if maxCount == 0 { - return "", errors.Errorf("unable to match user input to any specific repotag") + return "", ErrRepoTagNotFound } if len(results[maxCount]) > 1 { return "", errors.Errorf("user input matched multiple repotags for the image") @@ -916,6 +949,68 @@ func splitString(input string) string { return split[len(split)-1] } +// IsParent goes through the layers in the store and checks if i.TopLayer is +// the parent of any other layer in store. Double check that image with that +// layer exists as well. +func (i *Image) IsParent() (bool, error) { + children, err := i.GetChildren() + if err != nil { + return false, err + } + return len(children) > 0, nil +} + +// GetParent returns the image ID of the parent. Return nil if a parent is not found. +func (i *Image) GetParent() (*Image, error) { + images, err := i.imageruntime.GetImages() + if err != nil { + return nil, err + } + layer, err := i.imageruntime.store.Layer(i.TopLayer()) + if err != nil { + return nil, err + } + for _, img := range images { + if img.TopLayer() == layer.Parent { + return img, nil + } + } + return nil, nil +} + +// GetChildren returns a list of the imageIDs that depend on the image +func (i *Image) GetChildren() ([]string, error) { + var children []string + images, err := i.imageruntime.GetImages() + if err != nil { + return nil, err + } + layers, err := i.imageruntime.store.Layers() + if err != nil { + return nil, err + } + + for _, layer := range layers { + if layer.Parent == i.TopLayer() { + if imageID := getImageOfTopLayer(images, layer.ID); len(imageID) > 0 { + children = append(children, imageID...) + } + } + } + return children, nil +} + +// getImageOfTopLayer returns the image ID where layer is the top layer of the image +func getImageOfTopLayer(images []*Image, layer string) []string { + var matches []string + for _, img := range images { + if img.TopLayer() == layer { + matches = append(matches, img.ID()) + } + } + return matches +} + // InputIsID returns a bool if the user input for an image // is the image's partial or full id func (i *Image) InputIsID() bool { diff --git a/libpod/runtime_img.go b/libpod/runtime_img.go index 8b0c08cd3..d127d753f 100644 --- a/libpod/runtime_img.go +++ b/libpod/runtime_img.go @@ -75,7 +75,7 @@ type CopyOptions struct { // RemoveImage deletes an image from local storage // Images being used by running containers can only be removed if force=true -func (r *Runtime) RemoveImage(ctx context.Context, image *image.Image, force bool) (string, error) { +func (r *Runtime) RemoveImage(ctx context.Context, img *image.Image, force bool) (string, error) { r.lock.Lock() defer r.lock.Unlock() @@ -90,50 +90,58 @@ func (r *Runtime) RemoveImage(ctx context.Context, image *image.Image, force boo } imageCtrs := []*Container{} for _, ctr := range ctrs { - if ctr.config.RootfsImageID == image.ID() { + if ctr.config.RootfsImageID == img.ID() { imageCtrs = append(imageCtrs, ctr) } } - if len(imageCtrs) > 0 && len(image.Names()) <= 1 { + if len(imageCtrs) > 0 && len(img.Names()) <= 1 { if force { for _, ctr := range imageCtrs { if err := r.removeContainer(ctx, ctr, true); err != nil { - return "", errors.Wrapf(err, "error removing image %s: container %s using image could not be removed", image.ID(), ctr.ID()) + return "", errors.Wrapf(err, "error removing image %s: container %s using image could not be removed", img.ID(), ctr.ID()) } } } else { - return "", fmt.Errorf("could not remove image %s as it is being used by %d containers", image.ID(), len(imageCtrs)) + return "", fmt.Errorf("could not remove image %s as it is being used by %d containers", img.ID(), len(imageCtrs)) } } - if len(image.Names()) > 1 && !image.InputIsID() { + hasChildren, err := img.IsParent() + if err != nil { + return "", err + } + + if (len(img.Names()) > 1 && !img.InputIsID()) || hasChildren { // If the image has multiple reponames, we do not technically delete // the image. we figure out which repotag the user is trying to refer // to and untag it. - repoName, err := image.MatchRepoTag(image.InputName) + repoName, err := img.MatchRepoTag(img.InputName) + if hasChildren && err == image.ErrRepoTagNotFound { + return "", errors.Errorf("unable to delete %q (cannot be forced) - image has dependent child images", img.ID()) + } if err != nil { return "", err } - if err := image.UntagImage(repoName); err != nil { + if err := img.UntagImage(repoName); err != nil { return "", err } return fmt.Sprintf("Untagged: %s", repoName), nil - } else if len(image.Names()) > 1 && image.InputIsID() && !force { + } else if len(img.Names()) > 1 && img.InputIsID() && !force { // If the user requests to delete an image by ID and the image has multiple // reponames and no force is applied, we error out. - return "", fmt.Errorf("unable to delete %s (must force) - image is referred to in multiple tags", image.ID()) + return "", fmt.Errorf("unable to delete %s (must force) - image is referred to in multiple tags", img.ID()) } - err = image.Remove(force) + err = img.Remove(force) if err != nil && errors.Cause(err) == storage.ErrImageUsedByContainer { - if errStorage := r.rmStorageContainers(force, image); errStorage == nil { + if errStorage := r.rmStorageContainers(force, img); errStorage == nil { // Containers associated with the image should be deleted now, // let's try removing the image again. - err = image.Remove(force) + err = img.Remove(force) } else { err = errStorage } } - return image.ID(), err + return img.ID(), err } // Remove containers that are in storage rather than Podman. diff --git a/test/e2e/rmi_test.go b/test/e2e/rmi_test.go index 399fcd0fc..2d096d7ba 100644 --- a/test/e2e/rmi_test.go +++ b/test/e2e/rmi_test.go @@ -103,4 +103,100 @@ var _ = Describe("Podman rmi", func() { resultForce.WaitWithDefaultTimeout() Expect(resultForce.ExitCode()).To(Equal(0)) }) + + It("podman rmi image that is a parent of another image", func() { + session := podmanTest.Podman([]string{"rmi", "-fa"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + + session = podmanTest.Podman([]string{"run", "--name", "c_test", ALPINE, "true"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + + session = podmanTest.Podman([]string{"commit", "-q", "c_test", "test"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + + session = podmanTest.Podman([]string{"rm", "c_test"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + + session = podmanTest.Podman([]string{"rmi", ALPINE}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + + session = podmanTest.Podman([]string{"images", "-q"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + Expect(len(session.OutputToStringArray())).To(Equal(1)) + + session = podmanTest.Podman([]string{"images", "-q", "-a"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + Expect(len(session.OutputToStringArray())).To(Equal(2)) + untaggedImg := session.OutputToStringArray()[1] + + session = podmanTest.Podman([]string{"rmi", "-f", untaggedImg}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Not(Equal(0))) + }) + + It("podman rmi with cached images", func() { + session := podmanTest.Podman([]string{"rmi", "-fa"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + + dockerfile := `FROM docker.io/library/alpine:latest + RUN mkdir hello + RUN touch test.txt + ENV foo=bar + ` + podmanTest.BuildImage(dockerfile, "test", "true") + + dockerfile = `FROM docker.io/library/alpine:latest + RUN mkdir hello + RUN touch test.txt + RUN mkdir blah + ENV foo=bar + ` + podmanTest.BuildImage(dockerfile, "test2", "true") + + session = podmanTest.Podman([]string{"images", "-q", "-a"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + numOfImages := len(session.OutputToStringArray()) + + session = podmanTest.Podman([]string{"rmi", "test2"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + + session = podmanTest.Podman([]string{"images", "-q", "-a"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + Expect(numOfImages - len(session.OutputToStringArray())).To(Equal(2)) + + session = podmanTest.Podman([]string{"rmi", "test"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + + session = podmanTest.Podman([]string{"images", "-q", "-a"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + Expect(len(session.OutputToStringArray())).To(Equal(1)) + + podmanTest.BuildImage(dockerfile, "test3", "true") + + session = podmanTest.Podman([]string{"rmi", ALPINE}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + + session = podmanTest.Podman([]string{"rmi", "test3"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + + session = podmanTest.Podman([]string{"images", "-q", "-a"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + Expect(len(session.OutputToString())).To(Equal(0)) + }) }) -- cgit v1.2.3-54-g00ecf