-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Skip over non-native platforms when unpacking image #3983
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Another possible extension here could be to allow a csv list of platforms, to allow more fine-grained control. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Building a single non-native platform image still fails.
84e2d4d
to
a94f681
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, it still fails 😅
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
a94f681
to
4941808
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
exporter/containerimage/export.go
Outdated
} | ||
found := false | ||
for _, p2 := range ps.Platforms { | ||
if p2.ID == p { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need a slice? Should we not take the first one found and break instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/amd64
image on alinux/arm64
BuildKit).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.