Skip to content

When checking IsDangling make sure image is not in manifest list #2360

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 26, 2025
Merged

When checking IsDangling make sure image is not in manifest list #2360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion libimage/history.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ func (i *Image) History(ctx context.Context) ([]ImageHistory, error) {
return nil, err
}

layerTree, err := i.runtime.newFreshLayerTree()
layerTree, err := i.runtime.newFreshLayerTree(ctx)
if err != nil {
return nil, err
}
Expand Down
21 changes: 15 additions & 6 deletions libimage/image.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,13 +191,21 @@ func (i *Image) IsReadOnly() bool {
}

// IsDangling returns true if the image is dangling, that is an untagged image
// without children.
// without children and not used in a manifest list.
func (i *Image) IsDangling(ctx context.Context) (bool, error) {
return i.isDangling(ctx, nil)
images, layers, err := i.runtime.getImagesAndLayers()
if err != nil {
return false, err
}
tree, err := i.runtime.newLayerTreeFromData(ctx, images, layers, true)
if err != nil {
return false, err
}
return i.isDangling(ctx, tree)
}

// isDangling returns true if the image is dangling, that is an untagged image
// without children. If tree is nil, it will created for this invocation only.
// without children and not used in a manifest list. If tree is nil, it will created for this invocation only.
func (i *Image) isDangling(ctx context.Context, tree *layerTree) (bool, error) {
if len(i.Names()) > 0 {
return false, nil
Expand All @@ -206,7 +214,8 @@ func (i *Image) isDangling(ctx context.Context, tree *layerTree) (bool, error) {
if err != nil {
return false, err
}
return len(children) == 0, nil
_, usedInManfiestList := tree.manifestListDigests[i.Digest()]
return (len(children) == 0 && !usedInManfiestList), nil
}

// IsIntermediate returns true if the image is an intermediate image, that is
Expand Down Expand Up @@ -258,7 +267,7 @@ func (i *Image) TopLayer() string {

// Parent returns the parent image or nil if there is none
func (i *Image) Parent(ctx context.Context) (*Image, error) {
tree, err := i.runtime.newFreshLayerTree()
tree, err := i.runtime.newFreshLayerTree(ctx)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -292,7 +301,7 @@ func (i *Image) Children(ctx context.Context) ([]*Image, error) {
// created for this invocation only.
func (i *Image) getChildren(ctx context.Context, all bool, tree *layerTree) ([]*Image, error) {
if tree == nil {
t, err := i.runtime.newFreshLayerTree()
t, err := i.runtime.newFreshLayerTree(ctx)
if err != nil {
return nil, err
}
Expand Down
5 changes: 3 additions & 2 deletions libimage/image_tree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
package libimage

import (
"context"
"fmt"
"strings"

Expand All @@ -13,7 +14,7 @@ import (
// Tree generates a tree for the specified image and its layers. Use
// `traverseChildren` to traverse the layers of all children. By default, only
// layers of the image are printed.
func (i *Image) Tree(traverseChildren bool) (string, error) {
func (i *Image) Tree(ctx context.Context, traverseChildren bool) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That breaks the API, we don't care about it in c/common so that is fine but it will need an update in podman.
I think the other breaking changes are fixed with containers/podman#25635 so you shuld be good to vendor that without any other fixes in podman needed.

// NOTE: a string builder prevents us from copying to much data around
// and compile the string when and where needed.
sb := &strings.Builder{}
Expand All @@ -37,7 +38,7 @@ func (i *Image) Tree(traverseChildren bool) (string, error) {
fmt.Fprintf(sb, "No Image Layers")
}

layerTree, err := i.runtime.newFreshLayerTree()
layerTree, err := i.runtime.newFreshLayerTree(ctx)
if err != nil {
return "", err
}
Expand Down
31 changes: 26 additions & 5 deletions libimage/layer_tree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

"github.com/containers/storage"
storageTypes "github.com/containers/storage/types"
digest "github.com/opencontainers/go-digest"
ociv1 "github.com/opencontainers/image-spec/specs-go/v1"
"github.com/sirupsen/logrus"
)
Expand All @@ -22,6 +23,10 @@ type layerTree struct {
// emptyImages do not have any top-layer so we cannot create a
// *layerNode for them.
emptyImages []*Image
// manifestList keep track of images based on their digest.
// Library will use this map when checking if a image is dangling.
// If an image is used in a manifestList it is NOT dangling
manifestListDigests map[digest.Digest]struct{}
}

// node returns a layerNode for the specified layerID.
Expand Down Expand Up @@ -90,20 +95,21 @@ func (l *layerNode) repoTags() ([]string, error) {
}

// newFreshLayerTree extracts a layerTree from consistent layers and images in the local storage.
func (r *Runtime) newFreshLayerTree() (*layerTree, error) {
func (r *Runtime) newFreshLayerTree(ctx context.Context) (*layerTree, error) {
images, layers, err := r.getImagesAndLayers()
if err != nil {
return nil, err
}
return r.newLayerTreeFromData(images, layers)
return r.newLayerTreeFromData(ctx, images, layers, false)
}

// newLayerTreeFromData extracts a layerTree from the given the layers and images.
// The caller is responsible for (layers, images) being consistent.
func (r *Runtime) newLayerTreeFromData(images []*Image, layers []storage.Layer) (*layerTree, error) {
func (r *Runtime) newLayerTreeFromData(ctx context.Context, images []*Image, layers []storage.Layer, generateManifestDigestList bool) (*layerTree, error) {
tree := layerTree{
nodes: make(map[string]*layerNode),
ociCache: make(map[string]*ociv1.Image),
nodes: make(map[string]*layerNode),
ociCache: make(map[string]*ociv1.Image),
manifestListDigests: make(map[digest.Digest]struct{}),
}

// First build a tree purely based on layer information.
Expand All @@ -124,6 +130,21 @@ func (r *Runtime) newLayerTreeFromData(images []*Image, layers []storage.Layer)
topLayer := img.TopLayer()
if topLayer == "" {
tree.emptyImages = append(tree.emptyImages, img)
// When img is a manifest list, cache the lists of
// digests refereenced in manifest list. Digests can
// be used to check for dangling images.
if !generateManifestDigestList {
continue
}
if manifestList, _ := img.IsManifestList(ctx); manifestList {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment why we're doing this here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also if we are concerned about performance impact why are we doing this for every layer tree.
The only caller that would care is ListImages(), but the other callers via newFreshLayerTree() do not need that so why should they pay the cost?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this as a huge cost with the new design, you already read the image into memory and I doubt their is a huge amount of manifestlist in anyone's environment. Not sure if hacking this up for a theoretically performance improvement is worth it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cost of storage operations is all the file-based locking (incl. writes to the lock files); it’s not purely in-memory.

But, also, I haven’t measured anything, and that’s the only answer that counts. (We don’t have a Podman performance regression test, do we?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would have to setup a test with a Huge amount of manifest list images to see any cost, and I am not sure if this is even hitting the disk at all, is this data already being collected in the list of images?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NewImageSource…[Get]Manifest is definitely doing locking and reading from files. That might be a cache-only operation, “only” costing syscalls…

… but newLayerTreeFromData does not, prior to this PR, read the manifests at all, so locally for this function reading manifests would be truly new I/O.

And then it depends on the caller — the parent/child queries do read manifests of likely images: so, listing parent/child/dangling data about every single image reads all manifests anyway; but, I think, one-image queries like Image.Parent seem to typically only read a very small subset.

mlist, err := img.ToManifestList()
if err != nil {
return nil, err
}
for _, digest := range mlist.list.Instances() {
tree.manifestListDigests[digest] = struct{}{}
}
}
continue
}
node, exists := tree.nodes[topLayer]
Expand Down
2 changes: 1 addition & 1 deletion libimage/runtime.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,7 +634,7 @@ func (r *Runtime) ListImages(ctx context.Context, options *ListImagesOptions) ([

var tree *layerTree
if needsLayerTree {
tree, err = r.newLayerTreeFromData(images, snapshot.Layers)
tree, err = r.newLayerTreeFromData(ctx, images, snapshot.Layers, true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only run for options.SetListData which does not seem to be set in RemoveImages() which is called by prune so I don't think the true is correctly populated in that case to fix your issue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prune calls listimages with a isdangling flag, which ends up calling here.

podman images --filter dangling also calls here.

if err != nil {
return nil, err
}
Expand Down