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

containerd integration: handle multi-platform images #44989

Merged
merged 1 commit into from
Mar 2, 2023

Conversation

laurazard
Copy link
Member

@laurazard laurazard commented Feb 13, 2023

Signed-off-by: Laura Brehm laurabrehm@hey.com

- What I did

Fix listing images with containerd store

Upstreams:

- How I did it

Multiple entries are returned for each platform of an image (more discussion here: rumpl#113) – which isn't fantastic, but is better than the current state where listing images is broken.

- How to verify it

  • docker pull --platform=linux/amd64 alpine:latest
  • docker images (verify it doesn't break)
  • docker pull --platform=[another platform] alpine:latest
  • docker images (verify another entry is returned)

- Description for the changelog

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

image

@rumpl rumpl added area/images containerd-integration Issues and PRs related to containerd integration labels Feb 13, 2023
@tianon
Copy link
Member

tianon commented Feb 13, 2023

This is likely good (for now?), but we probably also want to come to a decision on #44582 before we get too deep. 😅

(I think this is on me and other maintainers -- we need to participate more over there and I'll go leave some of my thoughts we'd talked about in a previous maintainer meeting over there 🙈)

To be clear, I think something beats nothing here, and do not personally believe that should be a blocker for this PR (because I don't think any implementation here is going to make it harder one way or another to implement something else). 👍

@laurazard
Copy link
Member Author

@tianon I agree! I made this because I was running into not being able to list my images while refactoring for the other PRs to solve the platform issues associated with #44958 and #44934.

This PR just makes things a little smoother for now, and we can come to a better solution from discussion in #44582.

Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

We discussed this extensively in the context of #44840 -- there were a couple takeaways:

These PRs both introduce isDanglingImage and <none>:<none> strings -- so we'd like to see the common parts factored out into a third PR that these two can then be re-stacked on.

In particular, centralizing the <none> string constants in the presentation layer and using a sentinel value (possibly the empty string) to represent untagged images in the plumbing layer makes a lot of sense. We should quarantine those values to the API, so that we can possibly deprecate them in favor of client-side presentation in a future API version.

Likewise, isDanglingImage should be factored into a common place for both these PRs (and then consumed by restacking on top of the common PR), as otherwise when both are merged the symbols will collide as they're in the same package.

@laurazard
Copy link
Member Author

As discussed in #44840 (comment) (and since it's been merged in the meantime), I rebased this PR on top of master and addressed @neersighted's comments. PTAL :)

Multiple entries are returned for each platform of an image

Signed-off-by: Laura Brehm <laurabrehm@hey.com>
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

LGTM!

@thaJeztah thaJeztah added this to the v-next milestone Mar 2, 2023
@thaJeztah thaJeztah merged commit 9822185 into moby:master Mar 2, 2023
@thaJeztah
Copy link
Member

First PR in; congrats!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/images containerd-integration Issues and PRs related to containerd integration status/2-code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants