Skip to content

daemon: Don't set empty platform for GetImageOpts#131

Open
vvoland wants to merge 1 commit intorumpl:c8dfrom
vvoland:c8d-list-avoid-emptyplatform
Open

daemon: Don't set empty platform for GetImageOpts#131
vvoland wants to merge 1 commit intorumpl:c8dfrom
vvoland:c8d-list-avoid-emptyplatform

Conversation

@vvoland
Copy link
Copy Markdown
Collaborator

@vvoland vvoland commented Jan 26, 2023

Don't set GetImageOpts.Platform to avoid searching for unknown/unknown platform in image, when for some reason the container.Config.Platform is empty.
This is possible for containers created before run --platform X support was added.

Signed-off-by: Paweł Gronowski pawel.gronowski@docker.com

- What I did

- How I did it

- How to verify it

- Description for the changelog

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

Don't set `GetImageOpts.Platform` to avoid searching for
`unknown/unknown` platform in image, when for some reason the
`container.Config.Platform` is empty.
This is possible for containers created before `run --platform X`
support was added.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Comment thread daemon/list.go
return nil, err
}
opts.Platform = &ctr.Config.Platform
if ctr.Config.Platform.OS != "" {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we always set a normalised Platform ? (i.e., --platform=arm64 gets normalised to linux/arm64?) ISTR fields can be treated as "optional", so if we would not normalise the value, OS could be empty, but Arch possibly not.

Looking at this, I wonder if sending a nil platform is actually problematic;

  • daemon.imageService.GetImage()
  • calls imageService.getImage()
  • calls imageService.GetContainerdImage()
  • calls (is a wrapper for) imageService.resolveImage()

That last one may be problematic if we do not specify a platform;

// resolveImage searches for an image based on the given
// reference or identifier. Returns the descriptor of
// the image, could be manifest list, manifest, or config.
func (i *ImageService) resolveImage(ctx context.Context, refOrID string, platform *ocispec.Platform) (img containerdimages.Image, err error) {

It looks like that function returns;

  • a manifest index if no platform is specified
  • an image manifest (image) if a platform is specified'
  • (or a "not found" if the image was found, but no matching platform)

Which means that we would now switch the container from referencing an image to it referencing a manifest list (?)

I wonder if instead of "no platform", we should either;

  • Set the default platform (I think that's also what would be used if no platform is specified
  • Or to look up the container's image ID, and check the image's platform?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants