Skip to content
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

Jump to bottom

c8d: Adjust "image list" to return only a single item for each image store entry #45967

Merged
merged 1 commit into from Feb 26, 2024
Merged

c8d: Adjust "image list" to return only a single item for each image store entry #45967

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
60 changes: 53 additions & 7 deletions daemon/containerd/image_list.go
View file
Open in desktop
Expand Up @@ -11,6 +11,7 @@ import (
cerrdefs "github.com/containerd/containerd/errdefs"
"github.com/containerd/containerd/images"
"github.com/containerd/containerd/labels"
cplatforms "github.com/containerd/containerd/platforms"
"github.com/containerd/containerd/snapshots"
"github.com/containerd/log"
"github.com/distribution/reference"
Expand All @@ -21,6 +22,7 @@ import (
"github.com/docker/docker/container"
"github.com/docker/docker/errdefs"
"github.com/docker/docker/image"
dockerspec "github.com/moby/docker-image-spec/specs-go/v1"
"github.com/opencontainers/go-digest"
"github.com/opencontainers/image-spec/identity"
ocispec "github.com/opencontainers/image-spec/specs-go/v1"
Expand Down Expand Up @@ -123,6 +125,9 @@ func (i *ImageService) Images(ctx context.Context, opts imagetypes.ListOptions)
}
}

// TODO: Allow platform override?
platformMatcher := matchAllWithPreference(cplatforms.Default())

for _, img := range imgs {
isDangling := isDanglingImage(img)

Expand Down Expand Up @@ -154,7 +159,14 @@ func (i *ImageService) Images(ctx context.Context, opts imagetypes.ListOptions)
allContainers = i.containers.List()
}

type tempImage struct {
img *ImageManifest
indexPlatform *ocispec.Platform
dockerImage *dockerspec.DockerOCIImage
}

for _, img := range uniqueImages {
var presentImages []tempImage
err := i.walkImageManifests(ctx, img, func(img *ImageManifest) error {
if isPseudo, err := img.IsPseudoImage(ctx); isPseudo || err != nil {
return err
Expand All @@ -174,26 +186,60 @@ func (i *ImageService) Images(ctx context.Context, opts imagetypes.ListOptions)
return nil
}

image, chainIDs, err := i.singlePlatformImage(ctx, contentStore, tagsByDigest[img.RealTarget.Digest], img, opts, allContainers)
conf, err := img.Config(ctx)
if err != nil {
return err
}

summaries = append(summaries, image)
var dockerImage dockerspec.DockerOCIImage
if err := readConfig(ctx, contentStore, conf, &dockerImage); err != nil {
return err
}

presentImages = append(presentImages, tempImage{
img: img,
indexPlatform: img.Target().Platform,
dockerImage: &dockerImage,
})
return nil
})
if err != nil {
return nil, err
}

if len(presentImages) == 0 {
// TODO we should probably show *something* for images we've pulled
// but are 100% shallow or an empty manifest list/index
// ("tianon/scratch:index" is an empty example image index and
// "tianon/scratch:list" is an empty example manifest list)
Comment on lines +211 to +214
Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes, was about to ask if we could test this. As these are minimal images, perhaps we could include them in our "frozen images"?

I'm fine with doing that in a follow-up if it's complicated though.

Copy link
Contributor

@vvoland vvoland Feb 15, 2024 •
edited

Choose a reason for hiding this comment

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

Not sure if it makes sense to handle this right now.

  1. It's impossible to pull such image, you can only load it via docker load (or going via containerd store directly).
  2. Even if it is loaded, there's nothing that can be done with that image (except removing/pushing it).

Copy link
Member

Choose a reason for hiding this comment

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

Oh! I should read properly, I thought this one also described "multi-arch" image to test. But this was only for the "lots' of emptiness" case.

I mostly meant; we should have a test to verify that multiple arches of an image are properly grouped into a single image when using docker image ls (i.e., with this PR merged)

continue
}

if opts.SharedSize {
root = append(root, &chainIDs)
for _, id := range chainIDs {
layers[id] = layers[id] + 1
sort.SliceStable(presentImages, func(i, j int) bool {
platformFromIndexOrConfig := func(idx int) ocispec.Platform {
if presentImages[i].indexPlatform != nil {
return *presentImages[i].indexPlatform
}
return presentImages[i].dockerImage.Platform
}

return nil
return platformMatcher.Less(platformFromIndexOrConfig(i), platformFromIndexOrConfig(j))
})

best := presentImages[0].img
image, chainIDs, err := i.singlePlatformImage(ctx, contentStore, tagsByDigest[best.RealTarget.Digest], best, opts, allContainers)
if err != nil {
return nil, err
}

summaries = append(summaries, image)

if opts.SharedSize {
root = append(root, &chainIDs)
for _, id := range chainIDs {
layers[id] = layers[id] + 1
}
}
}

if opts.SharedSize {
Expand Down