From 86afb94e00541ff575d2d4b9b3f4e5369321d968 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Fri, 16 Jul 2021 11:38:42 +0200 Subject: [PATCH 1/3] refine dangling filters As discussed in github.com/containers/podman/issues/10832 the definition of a "dangling" image in Podman has historically been incorrect. While the Docker docs describe a dangling image as an image without a tag, and Podman implemented the filters as such, Docker actually implemented the filters for images without a tag and without children. Refine the dangling filters and hence `IsDangling()` to only return true if an image is untagged and has no children. Also correct the comments of `IsIntermediate()`. Signed-off-by: Valentin Rothberg --- libimage/filters.go | 10 +++++++--- libimage/image.go | 34 ++++++++++++++++++---------------- libimage/image_test.go | 4 +++- 3 files changed, 28 insertions(+), 20 deletions(-) diff --git a/libimage/filters.go b/libimage/filters.go index eae18fd9c..0cc5cc311 100644 --- a/libimage/filters.go +++ b/libimage/filters.go @@ -88,7 +88,7 @@ func (r *Runtime) compileImageFilters(ctx context.Context, filters []string) ([] if err != nil { return nil, errors.Wrapf(err, "non-boolean value %q for dangling filter", value) } - filterFuncs = append(filterFuncs, filterDangling(dangling)) + filterFuncs = append(filterFuncs, filterDangling(ctx, dangling)) case "id": filterFuncs = append(filterFuncs, filterID(value)) @@ -201,9 +201,13 @@ func filterContainers(value bool) filterFunc { } // filterDangling creates a dangling filter for matching the specified value. -func filterDangling(value bool) filterFunc { +func filterDangling(ctx context.Context, value bool) filterFunc { return func(img *Image) (bool, error) { - return img.IsDangling() == value, nil + isDangling, err := img.IsDangling(ctx) + if err != nil { + return false, err + } + return isDangling == value, nil } } diff --git a/libimage/image.go b/libimage/image.go index 19b929dc7..2ac513455 100644 --- a/libimage/image.go +++ b/libimage/image.go @@ -121,24 +121,29 @@ func (i *Image) IsReadOnly() bool { return i.storageImage.ReadOnly } -// IsDangling returns true if the image is dangling. An image is considered -// dangling if no names are associated with it in the containers storage. -func (i *Image) IsDangling() bool { - return len(i.Names()) == 0 +// IsDangling returns true if the image is dangling, that is an untagged image +// without children. +func (i *Image) IsDangling(ctx context.Context) (bool, error) { + if len(i.Names()) > 0 { + return false, nil + } + children, err := i.getChildren(ctx, false) + if err != nil { + return false, err + } + return len(children) == 0, nil } // IsIntermediate returns true if the image is an intermediate image, that is -// a dangling image without children. +// an untagged image with children. func (i *Image) IsIntermediate(ctx context.Context) (bool, error) { - // If the image has tags, it's not an intermediate one. - if !i.IsDangling() { + if len(i.Names()) > 0 { return false, nil } children, err := i.getChildren(ctx, false) if err != nil { return false, err } - // No tags, no children -> intermediate! return len(children) != 0, nil } @@ -420,19 +425,16 @@ func (i *Image) remove(ctx context.Context, rmMap map[string]*RemoveImageReport, return nil } - if !parent.IsDangling() { - return nil - } - - // If the image has siblings, we don't remove the parent. - hasSiblings, err := parent.HasChildren(ctx) + // Only remove the parent if it's dangling, that is being untagged and + // without children. + danglingParent, err := parent.IsDangling(ctx) if err != nil { // See Podman commit fd9dd7065d44: we need to // be tolerant toward corrupted images. logrus.Warnf("error determining if an image is a parent: %v, ignoring the error", err) - hasSiblings = false + danglingParent = false } - if hasSiblings { + if !danglingParent { return nil } diff --git a/libimage/image_test.go b/libimage/image_test.go index af37fcb85..3e27fa98b 100644 --- a/libimage/image_test.go +++ b/libimage/image_test.go @@ -64,7 +64,9 @@ func TestImageFunctions(t *testing.T) { // Below mostly smoke tests. require.False(t, image.IsReadOnly()) - require.False(t, image.IsDangling()) + isDangling, err := image.IsDangling(ctx) + require.NoError(t, err) + require.False(t, isDangling) isIntermediate, err := image.IsIntermediate(ctx) require.NoError(t, err) From b487f0ff6b64036ca8fc250d7b95230ab57a046b Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 19 Jul 2021 11:30:53 +0200 Subject: [PATCH 2/3] libruntime: layer tree: handle empty images Handle images without physical layers when constructing the layer tree and store them in a dedicated slice that can later on be consulted when computing parent/child relations. Such "empty" images can be built with, for instance, with the following Dockerfile: ``` FROM scratch ENV test1=test1 ENV test2=test2 ``` Signed-off-by: Valentin Rothberg --- libimage/layer_tree.go | 90 ++++++++++++++++++++++++++++++++---------- 1 file changed, 70 insertions(+), 20 deletions(-) diff --git a/libimage/layer_tree.go b/libimage/layer_tree.go index 4195b43c0..05f21531b 100644 --- a/libimage/layer_tree.go +++ b/libimage/layer_tree.go @@ -15,6 +15,9 @@ type layerTree struct { // ociCache is a cache for Image.ID -> OCI Image. Translations are done // on-demand. ociCache map[string]*ociv1.Image + // emptyImages do not have any top-layer so we cannot create a + // *layerNode for them. + emptyImages []*Image } // node returns a layerNode for the specified layerID. @@ -105,6 +108,7 @@ func (r *Runtime) layerTree() (*layerTree, error) { img := images[i] // do not leak loop variable outside the scope topLayer := img.TopLayer() if topLayer == "" { + tree.emptyImages = append(tree.emptyImages, img) continue } node, exists := tree.nodes[topLayer] @@ -126,22 +130,13 @@ func (r *Runtime) layerTree() (*layerTree, error) { // either the same top layer as parent or parent being the true parent layer. // Furthermore, the history of the parent and child images must match with the // parent having one history item less. If all is true, all images are -// returned. Otherwise, the first image is returned. +// returned. Otherwise, the first image is returned. Note that manifest lists +// do not have children. func (t *layerTree) children(ctx context.Context, parent *Image, all bool) ([]*Image, error) { if parent.TopLayer() == "" { - return nil, nil - } - - var children []*Image - - parentNode, exists := t.nodes[parent.TopLayer()] - if !exists { - // Note: erroring out in this case has turned out having been a - // mistake. Users may not be able to recover, so we're now - // throwing a warning to guide them to resolve the issue and - // turn the errors non-fatal. - logrus.Warnf("Layer %s not found in layer tree. The storage may be corrupted, consider running `podman system reset`.", parent.TopLayer()) - return children, nil + if isManifestList, _ := parent.IsManifestList(ctx); isManifestList { + return nil, nil + } } parentID := parent.ID() @@ -163,6 +158,38 @@ func (t *layerTree) children(ctx context.Context, parent *Image, all bool) ([]*I return areParentAndChild(parentOCI, childOCI), nil } + var children []*Image + + // Empty images are special in that they do not have any physical layer + // but yet can have a parent-child relation. Hence, compare the + // "parent" image to all other known empty images. + if parent.TopLayer() == "" { + for i := range t.emptyImages { + empty := t.emptyImages[i] + isParent, err := checkParent(empty) + if err != nil { + return nil, err + } + if isParent { + children = append(children, empty) + if !all { + break + } + } + } + return children, nil + } + + parentNode, exists := t.nodes[parent.TopLayer()] + if !exists { + // Note: erroring out in this case has turned out having been a + // mistake. Users may not be able to recover, so we're now + // throwing a warning to guide them to resolve the issue and + // turn the errors non-fatal. + logrus.Warnf("Layer %s not found in layer tree. The storage may be corrupted, consider running `podman system reset`.", parent.TopLayer()) + return children, nil + } + // addChildrenFrom adds child images of parent to children. Returns // true if any image is a child of parent. addChildrenFromNode := func(node *layerNode) (bool, error) { @@ -204,8 +231,37 @@ func (t *layerTree) children(ctx context.Context, parent *Image, all bool) ([]*I } // parent returns the parent image or nil if no parent image could be found. +// Note that manifest lists do not have parents. func (t *layerTree) parent(ctx context.Context, child *Image) (*Image, error) { if child.TopLayer() == "" { + if isManifestList, _ := child.IsManifestList(ctx); isManifestList { + return nil, nil + } + } + + childID := child.ID() + childOCI, err := t.toOCI(ctx, child) + if err != nil { + return nil, err + } + + // Empty images are special in that they do not have any physical layer + // but yet can have a parent-child relation. Hence, compare the + // "child" image to all other known empty images. + if child.TopLayer() == "" { + for _, empty := range t.emptyImages { + if childID == empty.ID() { + continue + } + emptyOCI, err := t.toOCI(ctx, empty) + if err != nil { + return nil, err + } + // History check. + if areParentAndChild(emptyOCI, childOCI) { + return empty, nil + } + } return nil, nil } @@ -219,14 +275,8 @@ func (t *layerTree) parent(ctx context.Context, child *Image) (*Image, error) { return nil, nil } - childOCI, err := t.toOCI(ctx, child) - if err != nil { - return nil, err - } - // Check images from the parent node (i.e., parent layer) and images // with the same layer (i.e., same top layer). - childID := child.ID() images := node.images if node.parent != nil { images = append(images, node.parent.images...) From 39e7214a081ebe6520873e5566bea2f8aaf28a3d Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 19 Jul 2021 18:10:21 +0200 Subject: [PATCH 3/3] libimage: report all removed images Fix a bug where not all removed images were actually reported as such. A regression test will be added to Podman. Signed-off-by: Valentin Rothberg --- libimage/image.go | 34 ++++++++++++++++++++-------------- libimage/runtime.go | 33 ++++++++++++++++++++++----------- 2 files changed, 42 insertions(+), 25 deletions(-) diff --git a/libimage/image.go b/libimage/image.go index 2ac513455..5b060a185 100644 --- a/libimage/image.go +++ b/libimage/image.go @@ -276,7 +276,7 @@ type RemoveImageReport struct { // remove removes the image along with all dangling parent images that no other // image depends on. The image must not be set read-only and not be used by -// containers. +// containers. Returns IDs of removed/untagged images in order. // // If the image is used by containers return storage.ErrImageUsedByContainer. // Use force to remove these containers. @@ -287,7 +287,12 @@ type RemoveImageReport struct { // // This function is internal. Users of libimage should always use // `(*Runtime).RemoveImages()`. -func (i *Image) remove(ctx context.Context, rmMap map[string]*RemoveImageReport, referencedBy string, options *RemoveImagesOptions) error { +func (i *Image) remove(ctx context.Context, rmMap map[string]*RemoveImageReport, referencedBy string, options *RemoveImagesOptions) ([]string, error) { + processedIDs := []string{} + return i.removeRecursive(ctx, rmMap, processedIDs, referencedBy, options) +} + +func (i *Image) removeRecursive(ctx context.Context, rmMap map[string]*RemoveImageReport, processedIDs []string, referencedBy string, options *RemoveImagesOptions) ([]string, error) { // If referencedBy is empty, the image is considered to be removed via // `image remove --all` which alters the logic below. @@ -299,7 +304,7 @@ func (i *Image) remove(ctx context.Context, rmMap map[string]*RemoveImageReport, logrus.Debugf("Removing image %s", i.ID()) if i.IsReadOnly() { - return errors.Errorf("cannot remove read-only image %q", i.ID()) + return processedIDs, errors.Errorf("cannot remove read-only image %q", i.ID()) } if i.runtime.eventChannel != nil { @@ -311,7 +316,7 @@ func (i *Image) remove(ctx context.Context, rmMap map[string]*RemoveImageReport, if exists { // If the image has already been removed, we're done. if report.Removed { - return nil + return processedIDs, nil } } else { report = &RemoveImageReport{ID: i.ID()} @@ -338,7 +343,7 @@ func (i *Image) remove(ctx context.Context, rmMap map[string]*RemoveImageReport, if options.WithSize { size, err := i.Size() if handleError(err) != nil { - return err + return processedIDs, err } report.Size = size } @@ -359,18 +364,18 @@ func (i *Image) remove(ctx context.Context, rmMap map[string]*RemoveImageReport, byDigest := strings.HasPrefix(referencedBy, "sha256:") if !options.Force { if byID && numNames > 1 { - return errors.Errorf("unable to delete image %q by ID with more than one tag (%s): please force removal", i.ID(), i.Names()) + return processedIDs, errors.Errorf("unable to delete image %q by ID with more than one tag (%s): please force removal", i.ID(), i.Names()) } else if byDigest && numNames > 1 { // FIXME - Docker will remove the digest but containers storage // does not support that yet, so our hands are tied. - return errors.Errorf("unable to delete image %q by digest with more than one tag (%s): please force removal", i.ID(), i.Names()) + return processedIDs, errors.Errorf("unable to delete image %q by digest with more than one tag (%s): please force removal", i.ID(), i.Names()) } } // Only try to untag if we know it's not an ID or digest. if !byID && !byDigest { if err := i.Untag(referencedBy); handleError(err) != nil { - return err + return processedIDs, err } report.Untagged = append(report.Untagged, referencedBy) @@ -379,14 +384,15 @@ func (i *Image) remove(ctx context.Context, rmMap map[string]*RemoveImageReport, } } + processedIDs = append(processedIDs, i.ID()) if skipRemove { - return nil + return processedIDs, nil } // Perform the actual removal. First, remove containers if needed. if options.Force { if err := i.removeContainers(options.RemoveContainerFunc); err != nil { - return err + return processedIDs, err } } @@ -412,7 +418,7 @@ func (i *Image) remove(ctx context.Context, rmMap map[string]*RemoveImageReport, } if _, err := i.runtime.store.DeleteImage(i.ID(), true); handleError(err) != nil { - return err + return processedIDs, err } report.Untagged = append(report.Untagged, i.Names()...) @@ -422,7 +428,7 @@ func (i *Image) remove(ctx context.Context, rmMap map[string]*RemoveImageReport, // Check if can remove the parent image. if parent == nil { - return nil + return processedIDs, nil } // Only remove the parent if it's dangling, that is being untagged and @@ -435,11 +441,11 @@ func (i *Image) remove(ctx context.Context, rmMap map[string]*RemoveImageReport, danglingParent = false } if !danglingParent { - return nil + return processedIDs, nil } // Recurse into removing the parent. - return parent.remove(ctx, rmMap, "", options) + return parent.removeRecursive(ctx, rmMap, processedIDs, "", options) } // Tag the image with the specified name and store it in the local containers diff --git a/libimage/runtime.go b/libimage/runtime.go index ea1f94b3c..26a04dad5 100644 --- a/libimage/runtime.go +++ b/libimage/runtime.go @@ -587,11 +587,10 @@ func (r *Runtime) RemoveImages(ctx context.Context, names []string, options *Rem rmErrors = append(rmErrors, err) } - orderedIDs := []string{} // determinism and relative order deleteMap := make(map[string]*deleteMe) // ID -> deleteMe - + toDelete := []string{} // Look up images in the local containers storage and fill out - // orderedIDs and the deleteMap. + // toDelete and the deleteMap. switch { case len(names) > 0: // Look up the images one-by-one. That allows for removing @@ -605,15 +604,12 @@ func (r *Runtime) RemoveImages(ctx context.Context, names []string, options *Rem } dm, exists := deleteMap[img.ID()] if !exists { - orderedIDs = append(orderedIDs, img.ID()) + toDelete = append(toDelete, img.ID()) dm = &deleteMe{image: img} deleteMap[img.ID()] = dm } dm.referencedBy = append(dm.referencedBy, resolvedName) } - if len(orderedIDs) == 0 { - return nil, rmErrors - } default: filteredImages, err := r.ListImages(ctx, nil, &ListImagesOptions{Filters: options.Filters}) @@ -622,14 +618,21 @@ func (r *Runtime) RemoveImages(ctx context.Context, names []string, options *Rem return nil, rmErrors } for _, img := range filteredImages { - orderedIDs = append(orderedIDs, img.ID()) + toDelete = append(toDelete, img.ID()) deleteMap[img.ID()] = &deleteMe{image: img} } } + // Return early if there's no image to delete. + if len(deleteMap) == 0 { + return nil, rmErrors + } + // Now remove the images in the given order. rmMap := make(map[string]*RemoveImageReport) - for _, id := range orderedIDs { + orderedIDs := []string{} + visitedIDs := make(map[string]bool) + for _, id := range toDelete { del, exists := deleteMap[id] if !exists { appendError(errors.Errorf("internal error: ID %s not in found in image-deletion map", id)) @@ -639,9 +642,17 @@ func (r *Runtime) RemoveImages(ctx context.Context, names []string, options *Rem del.referencedBy = []string{""} } for _, ref := range del.referencedBy { - if err := del.image.remove(ctx, rmMap, ref, options); err != nil { + processedIDs, err := del.image.remove(ctx, rmMap, ref, options) + if err != nil { appendError(err) - continue + } + // NOTE: make sure to add given ID only once to orderedIDs. + for _, id := range processedIDs { + if visited := visitedIDs[id]; visited { + continue + } + orderedIDs = append(orderedIDs, id) + visitedIDs[id] = true } } }