c8d/list: Add test and combine size by vvoland · Pull Request #47449 · moby/moby

vvoland · 2024-02-26T16:59:42Z

- What I did

Add unit test for Images (image list)
Skip images which don't match platform matcher
Extract and simplify some code
Count containers using ImageManifest

- How I did it
See commits 🙈

- How to verify it
CI

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

vvoland · 2024-02-27T10:37:25Z

$ docker pull busybox
Using default tag: latest
latest: Pulling from library/busybox
c2bf9493c1bf: Download complete 
Digest: sha256:6d9ac9237a84afe1516540f40a0fafdc86859b2141954b4d643af7066d598b74
Status: Downloaded newer image for busybox:latest
docker.io/library/busybox:latest

$ docker images
REPOSITORY   TAG       IMAGE ID       CREATED       SIZE
busybox      latest    6d9ac9237a84   5 weeks ago   6.09MB

$ docker pull --platform linux/amd64 busybox
Using default tag: latest
latest: Pulling from library/busybox
9ad63333ebc9: Download complete 
Digest: sha256:6d9ac9237a84afe1516540f40a0fafdc86859b2141954b4d643af7066d598b74
Status: Image is up to date for busybox:latest
docker.io/library/busybox:latest

$ docker images
REPOSITORY   TAG       IMAGE ID       CREATED       SIZE
busybox      latest    6d9ac9237a84   5 weeks ago   12.7MB

thaJeztah · 2024-02-27T11:49:15Z

daemon/containerd/image_list.go

 	// TODO(thaJeztah): do we need to take multiple snapshotters into account? See https://github.com/moby/moby/issues/45273
-	snapshotter := i.client.SnapshotService(i.snapshotter)
+	snapshotter := i.snapshotterService


Hmm.. this means we won't be able to use multiple snapshotters (see the TODO comment above). If we do this, we should store it indexed by snapshotter.

Hmmm fair enough, done.

thaJeztah · 2024-02-27T12:00:25Z

daemon/containerd/image_list.go

-		return nil, nil, errors.Wrapf(err, "failed to get rootfs of image %s", imageManifest.Name())
-	}
-
+func (i *ImageService) singlePlatformSize(ctx context.Context, imgMfst *ImageManifest) (totalSize int64, contentSize int64, _ error) {


This name is a bit confusing. I need to look into what exactly it returns; totalSize is the size for all platforms? what is contentSize in this context?

Oh! This is for a single-arch image (so "total" for the image, in this case being the total for a single arch, correct?)

Yes, total here means both content (distributed blobs) and "unpacked" size (snaphots).

thaJeztah · 2024-03-07T13:17:26Z

daemon/containerd/service.go

+		return s
+	}
+
+	return i.client.SnapshotService(snapshotter)


Should this be stored in the map, or was it intentional to only keep a reference to the default one?

internal/testutils/specialimage/multilayer.go

daemon/containerd/image_test.go

thaJeztah · 2024-03-07T13:33:35Z

daemon/containerd/image_list.go


+			if opts.ContainerCount {
+				i.containers.ApplyAll(func(c *container.Container) {
+					if c.ImageManifest != nil && c.ImageManifest.Digest == img.Target().Digest {


For a follow-up; we should really document the ImageManifest field in container; I went looking to see if this was the "platform-specific" manifest, or if this was the manifest-index ("root" of the image), but the field is currently not documented.

thaJeztah · 2024-03-07T13:36:53Z

daemon/containerd/image_list.go

+				platform = dockerImage.Platform
+			}
+
+			if !platformMatcher.Match(platform) {


Perhaps would be worth adding your commit-message description as a comment here; this would not be obvious immediately when looking at the code.

Currently this won't have any real effect because the platform matcher matches all platform and is only used for sorting.

Curious if this would filter, wouldn't that mean that docker image ls would not include sizes for all images?

I'd assume it would only include the size for matching-platform images. However, I think that makes sense: if the passes in a specific platform matcher, it's because they only want to know about those particular images.

Not sure what's the correct behavior here when we're reducing multiple platforms into one list item.

Hmmm good one. I feel like technically the correct behavior would be to show the total size of all images, because the single entry represents the whole multi-platform image with all platforms.

This would mean, that if we now added the --platform switch, it wouldn't really affect anything in the output.. But perhaps that's actually what we want - it should only affect the platform specific subimages.

EDIT: Actually it would also hide the multiplatform images not supporting that platform, so I think it's actually the expected behavior.

Yeah my train of thought here was that;

abcd ubuntu:latest 200MB

Is the compact view of;

abcd ubuntu:latest 200MB |-- ubuntu:latest X64 100MB |-- ubuntu:latest ARM64 100MB

So when "collapsed", it would show the summary of it all.

For sure, this is a bit of a limitation of the current means we have, and to some extent there's also something to be said for that to be showing "default" platform. But "hiding" the 100MB for the non-matching platform is also surprising (It says I have 200MB used for images, but I only see 100??)

Yes, I think it makes more sense to show the total size. Fixed.

laurazard

LGTM!
Just noticed a little typo/duplicated word in c8d/list: Count containers by their manifest's text:

Move containers counting out out `singlePlatformImage` and count them
based on the `ImageManifest` property.

Use `image.Store` and `content.Store` stored in the ImageService struct instead of fetching it every time from containerd client. Signed-off-by: Paweł Gronowski <[email protected]>

Avoid fetching `SnapshotService` from client every time. Fetch it once and then store when creating the image service. This also allows to pass custom snapshotter implementation for unit testing. Signed-off-by: Paweł Gronowski <[email protected]>

To ease accessing image descriptors in tests. Signed-off-by: Paweł Gronowski <[email protected]>

Add unit test for `Images` implementation. Signed-off-by: Paweł Gronowski <[email protected]>

Multi-platform images are coalesced into one entry now. Signed-off-by: Paweł Gronowski <[email protected]>

Move containers counting out of `singlePlatformImage` and count them based on the `ImageManifest` property. (also remove ChainIDs calculation as they're no longer used) Signed-off-by: Paweł Gronowski <[email protected]>

Don't save all present images, inline the sorting into the loop instead. Signed-off-by: Paweł Gronowski <[email protected]>

Signed-off-by: Paweł Gronowski <[email protected]>

Currently this won't have any real effect because the platform matcher matches all platform and is only used for sorting. Signed-off-by: Paweł Gronowski <[email protected]>

vvoland · 2024-03-07T15:27:55Z

Thanks @laurazard! This one was meant to be out of, not out out 😅

thaJeztah

LGTM

I wonder if (with more complexity, and more iterating needed to collect all the data) if we should benchmark some of this. Do we need to consider caching at some point? Caching is hard though (and we should not optimise if not needed); more so because the list of present images / platforms will change at any moment, so cache invalidation would now be an issue.

thaJeztah · 2024-03-07T16:10:41Z

daemon/containerd/service.go

+	s, ok := i.snapshotterServices[snapshotter]
+	if !ok {
+		s = i.client.SnapshotService(snapshotter)
+		i.snapshotterServices[snapshotter] = s
+	}
+
+	return s


For posterity / future self; I think this is fine to do, but technically this map would be "unbounded", so would allow an "unlimited" number of snapshotters to be referenced here.

I don't think there's any realistic security here (unless someone would want to DOS themselves by having a gazillion snapshotters).

thaJeztah · 2024-03-07T16:19:28Z

Looks like this one is flaky (I think I mentioned the same one on another PR);

=== FAIL: github.com/docker/docker/integration/networking TestBridgeICCWindows/User_defined_nat_network (9.20s)
    bridge_test.go:242: assertion failed: error is not nil: Post "http://%2F%2F.%2Fpipe%2Fdocker_engine/v1.45/containers/e6bc72d3a70cd87e40bdc547f722749fb7038a26c421697cd350ebcced73db0b/start": context deadline exceeded
    panic.go:523: assertion failed: error is not nil: Error response from daemon: error while removing network: network mynat id 01daafb478bec6f00a16d87bd570a8328c8135c212575c12e29cec9a3c4a9500 has active endpoints

=== FAIL: github.com/docker/docker/integration/networking TestBridgeICCWindows (16.80s)

vvoland · 2024-03-07T16:47:46Z

One easy thing to cache would be the readConfig calls - no need to repeatedly fetch the same blobs from the content store. Configs are small enough, and with content addressability there's no cache invalidation needed (content won't change for the same digest).

Not sure how big of an performance improvement it would bring though, it would definitely be nice to have benchmarks!

vvoland self-assigned this Feb 26, 2024

vvoland mentioned this pull request Feb 26, 2024

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

Merged

vvoland added this to the 26.0.0 milestone Feb 26, 2024

vvoland force-pushed the c8d-list-single branch from 4540ff4 to e6d2ed7 Compare February 26, 2024 17:15

vvoland added the containerd-integration Issues and PRs related to containerd integration label Feb 26, 2024

vvoland force-pushed the c8d-list-single branch from e6d2ed7 to 0dea7d9 Compare February 27, 2024 09:08

vvoland force-pushed the c8d-list-single branch from 0dea7d9 to f5ad7e8 Compare February 27, 2024 10:41

vvoland added area/images Image Distribution kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. kind/refactor PR's that refactor, or clean-up code labels Feb 27, 2024

vvoland marked this pull request as ready for review February 27, 2024 11:26

thaJeztah reviewed Feb 27, 2024

View reviewed changes

vvoland force-pushed the c8d-list-single branch 2 times, most recently from a21249e to ffef668 Compare February 27, 2024 12:28

dmcgowan approved these changes Feb 27, 2024

View reviewed changes

vvoland requested a review from thaJeztah February 29, 2024 08:42

vvoland mentioned this pull request Mar 7, 2024

c8d: Multi-platform image list #47526

Merged

thaJeztah reviewed Mar 7, 2024

View reviewed changes

laurazard approved these changes Mar 7, 2024

View reviewed changes

vvoland added 5 commits March 7, 2024 16:25

daemon/c8d: Use i.images and i.content

74e2f23

Use `image.Store` and `content.Store` stored in the ImageService struct instead of fetching it every time from containerd client. Signed-off-by: Paweł Gronowski <[email protected]>

daemon/c8d: Cache SnapshotService

1b108bd

Avoid fetching `SnapshotService` from client every time. Fetch it once and then store when creating the image service. This also allows to pass custom snapshotter implementation for unit testing. Signed-off-by: Paweł Gronowski <[email protected]>

specialimage: Return optional ocispec.Index

a6e7e67

To ease accessing image descriptors in tests. Signed-off-by: Paweł Gronowski <[email protected]>

c8d/list: Add TestImageList

582de4b

Add unit test for `Images` implementation. Signed-off-by: Paweł Gronowski <[email protected]>

c8d/list: Combine size

a535a65

Multi-platform images are coalesced into one entry now. Signed-off-by: Paweł Gronowski <[email protected]>

vvoland force-pushed the c8d-list-single branch from ffef668 to efbdc44 Compare March 7, 2024 15:25

vvoland added 2 commits March 7, 2024 16:26

c8d/list: Count containers by their manifest

b37ced2

Move containers counting out of `singlePlatformImage` and count them based on the `ImageManifest` property. (also remove ChainIDs calculation as they're no longer used) Signed-off-by: Paweł Gronowski <[email protected]>

c8d/list: Simplify "best" image selection

288a14e

Don't save all present images, inline the sorting into the loop instead. Signed-off-by: Paweł Gronowski <[email protected]>

vvoland added 3 commits March 7, 2024 16:27

c8d/list: Extract imageSummary function

52a80b4

Signed-off-by: Paweł Gronowski <[email protected]>

c8d/list: Remove outdated TODO

72f1f82

Signed-off-by: Paweł Gronowski <[email protected]>

c8d/list: Skip images with non matching platform

2f1a32e

Currently this won't have any real effect because the platform matcher matches all platform and is only used for sorting. Signed-off-by: Paweł Gronowski <[email protected]>

vvoland force-pushed the c8d-list-single branch from efbdc44 to 2f1a32e Compare March 7, 2024 15:27

thaJeztah approved these changes Mar 7, 2024

View reviewed changes

vvoland merged commit f4c696e into moby:master Mar 7, 2024

thaJeztah mentioned this pull request Mar 14, 2024

Image list with containerd store is very slow #47564

Closed

vvoland mentioned this pull request Mar 19, 2024

c8d/list: Fix shared size calculation #47597

Merged

vvoland mentioned this pull request Aug 16, 2024

[27.x backport] c8d: Multi-platform image list #48346

Merged

Conversation

vvoland commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vvoland commented Feb 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvoland Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvoland Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laurazard left a comment

Choose a reason for hiding this comment

Uh oh!

vvoland commented Mar 7, 2024

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 7, 2024

Uh oh!

vvoland commented Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vvoland commented Feb 26, 2024 •

edited

Loading

vvoland Feb 27, 2024 •

edited

Loading

vvoland Mar 7, 2024 •

edited

Loading

thaJeztah Mar 7, 2024 •

edited

Loading

vvoland commented Mar 7, 2024 •

edited

Loading