Merge pull request #684 from vrothberg/dangling

fix "dangling" filter and pruning

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
 	}
 }
 

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
 }
 
@@ -271,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.
@@ -282,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.
 
@@ -294,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 {
@@ -306,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()}
@@ -333,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
 	}
@@ -354,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)
 
@@ -374,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
 		}
 	}
 
@@ -407,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()...)
 
@@ -417,27 +428,24 @@ 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
 	}
 
-	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 {
-		return nil
+	if !danglingParent {
+		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

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)

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...)