Skip over non-native platforms when unpacking image #3983
Conversation
There was a problem hiding this comment.
I think this make sense for the default behavior for buildkit. We might want to add non-bool value to unpack all arch, but that can be done later as it is a new feature.
@rumpl @vvoland Can you confirm this works for Moby?
@AkihiroSuda and for nerdctl?
tonistiigi
changed the title
Another possible extension here could be to allow a csv list of platforms, to allow more fine-grained control. |
jedevc
force-pushed
the
silence-notfound-unpack-error
branch
from
84e2d4d
to
a94f681
Compare
exporter/containerimage/export.go
Outdated
| @@ -349,6 +349,13 @@ func (e *imageExporterInstance) pushImage(ctx context.Context, src *exporter.Sou | |||
| } | |||
|
|
|||
| func (e *imageExporterInstance) unpackImage(ctx context.Context, img images.Image, src *exporter.Source, s session.Group) (err0 error) { | |||
| p := platforms.Normalize(platforms.DefaultSpec()) | |||
| ref, ok := src.FindRef(platforms.Format(p)) | |||
| if !ok || ref == nil { | |||
There was a problem hiding this comment.
Running docker buildx build --platform linux/amd64 -t asdf . on linux/arm64:
ok=true and ref is not nil, so it still doesn't return early here.
There was a problem hiding this comment.
Ok, I've just gone and added an explicit check - "is the current platform in the list of platforms we built for". That should hopefully be the check that does this correctly now.
jedevc
force-pushed
the
silence-notfound-unpack-error
branch
from
a94f681
to
4941808
Compare
exporter/containerimage/export.go
Outdated
| } | ||
| found := false | ||
| for _, p2 := range ps.Platforms { | ||
| if p2.ID == p { |
There was a problem hiding this comment.
We should use platforms.Matcher in here instead of the direct comparison.
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
tonistiigi
force-pushed
the
silence-notfound-unpack-error
branch
from
4941808
to
6c1d491
Compare
|
|
||
| ps, err := exptypes.ParsePlatforms(src.Metadata) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| found := false | ||
| matching := []exptypes.Platform{} |
There was a problem hiding this comment.
Why do we need a slice? Should we not take the first one found and break instead?
There was a problem hiding this comment.
This is to get the platform that has the highest priority, not just the first platform from the array. This should match how the specific platform is found when making a container for a specific platform from an image.
🛠️ Fixes #3891
⚠️ Requires #3982
This is a tricky case - essentially, we should not fail the unpack if buildkit has not built for it's native platform (e.g. building a
linux/amd64image on alinux/arm64BuildKit).Ideally, we would try and find a better candidate to unpack, but this is kinda tricky, since determining the intentions of the client is not super easy here. Firstly, we don't what the client's supported architectures are, so we can't determine which ones to unpack, and secondly, we don't even know what the user intends to do with the results.
By just disabling this error for now, with moby/moby#45647, we'll just end-up loading each architecture on demand, while still having a somewhat reasonable behavior when building images with multi-platform images that contain the default platform.
Ideally, if we want some smarter behavior, we could add something into moby to perform the unpack manually (though I seem to remember some limitations with this, cc @vvoland).
I'm not 100% sure about this, so happy to discuss.