Skip to content

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

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

Open
vvoland wants to merge 1 commit into
base: c8d
Choose a base branch
from
Open

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

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion daemon/list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,7 +592,9 @@ func (daemon *Daemon) refreshImage(ctx context.Context, s *container.Snapshot, f
if err != nil || ctr == nil {
return nil, err
}
opts.Platform = &ctr.Config.Platform
if ctr.Config.Platform.OS != "" {
Copy link
Copy Markdown
Collaborator

@thaJeztah thaJeztah Jan 28, 2023

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;

moby/daemon/containerd/image.go

Lines 149 to 152 in a723ff5

// 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?

opts.Platform = &ctr.Config.Platform
}
}
img, err := daemon.imageService.GetImage(ctx, tmpImage, opts)

Expand Down