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.

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

c8d: images/json return image manifests #45948

Closed
wants to merge 2 commits into from

Conversation

rumpl
Copy link
Member

@rumpl rumpl commented Jul 12, 2023

- What I did

Changed the way we return multi-platform images, we used to return the same name/id for each platform of an image. The ID used to be the ID of the index. With this change we now return the ID of each manifest. Also changed it so that we can inspect etc. an image by its manifest ID.

Before:

$ docker images
REPOSITORY   TAG       IMAGE ID       CREATED        SIZE
nginx        latest    08bc36ad5247   30 hours ago   272MB
nginx        latest    08bc36ad5247   30 hours ago   274MB

After:

$ docker images
REPOSITORY   TAG       IMAGE ID       CREATED        SIZE
nginx        latest    1bb5c4b86cb7   30 hours ago   272MB
nginx        latest    b02b0565e769   30 hours ago   274MB

- How I did it

  • stored the manifest in the ImageManifest struct (we used to store the parent index)
  • added a (slow) way to search for an image by its manifest ID

Note: this is slow and gets slower as the number of the images increases, but for now it works, we will have to find a better way to find an image manifest by digest, either by adding metadata to the image we pull/build or by contributing a change to containerd. The ideal would be for containerd to have an API that does what we need.

- How to verify it

Pull multiple platforms of the same image and list them:

$ docker pull --platform linux/arm64 nginx
$ docker pull --platform linux/amd64 nginx

And then list them.

- Description for the changelog
containerd image store: docker images now show the ID of the image a specific platform instead of the ID of the manifest list

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

The ImageManifest used to store the manifest of the parent
index/manifest list, this means that users would see multiple same
entries in the images list when a multi-platform image is pulled/built.

Using the manifest instead makes is so that we now see different entires
for each platform image.

Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
The containerd API doesn't give us anything for this so we need to take
to slow path: list all images and then walk each manifest of these
images until we find the manifest the user is asking.

Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
@tianon
Copy link
Member

tianon commented Jul 13, 2023

As we discussed in the maintainers call today, I think this is a step in the opposite direction of what we need. 😅

As I tried to describe over in #45735 (comment), having docker image ls return one and only one value for each name combination is part of our "unspoken" API (like the magic strings we can't change from certain endpoints), so I think no matter how we resolve this ultimately, the backwards compatible "downgraded" result from the API needs to satisfy that.

What we discussed on the call today was that the short-term fix should be that we update this endpoint to return a single entry for each name/value pair, and that for now, all the "image specific" metadata (labels, config, size) should be either "default platform" or "first platform we have locally" (which then matches the logic for commands like docker image inspect, etc) with everything else (just ID, maybe?) coming from the manifest list/index. That leaves room for the longer-term implementation to add new fields to describe the other images that are part of the manifest list/index.

As for the data model, I think the ID of that "image" being the manifest list/index digest is the only thing that really makes sense (so the current implementation is "more correct" from that respect).

In typing this, I realized we might be talking past each other a bit, so I implemented it as a PR (that probably has problems, but hopefully communicates more clearly): #45967 😇

@rumpl
Copy link
Member Author

rumpl commented Jul 17, 2023

My thinking behind this change is that it's better than what we have today (showing multiple times the same name/id for different platforms of an image). With this change the user at least can have the different IDs for each image they pulled/built so they can do something with them. It's not ideal of course, we are trying to put a square in a circle. I'm not sure #45967 changes are what we want, that change hides information from the users: if a user builds or pulls multiple platforms of an image they have no way of seeing them.

Another reason why I think this is a step in the right direction: no matter what the final API will look like, parts of these changes will be needed: getting an image for a platform by its digest.

About the same name, we kinda already have that:

$ docker images
REPOSITORY                    TAG       IMAGE ID       CREATED          SIZE
docker-dev                    latest    95593035e9ad   7 minutes ago    1.52GB
<none>                        <none>    0465a961d0af   10 minutes ago   1.52GB
<none>                        <none>    f9d8b43c1a4a   12 minutes ago   1.52GB
<none>                        <none>    a81504762528   3 days ago       1.52GB
...

Comparing this output with the output with my changes, in my mind these are closer to one another than the output we have right now with the same name/ID and as a bonus the user can really see all the present images.

Note: I was implementing what @thaJeztah proposed in my first PR and thought I could extract these changes so that the "final" multi-platform PR is smaller. I thought it would be less controversial :D

@rumpl rumpl closed this Jul 17, 2023
@rumpl rumpl reopened this Jul 17, 2023
@rumpl rumpl closed this Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants