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

load: import all platforms (--all-platforms) #316

Merged
merged 1 commit into from
Aug 20, 2021

Conversation

cpuguy83
Copy link
Member

Currently if you try to import a non-native image the blobs will get
GC'd and unpack fails with with "not found" on the blobs.

Import doesn't really have a way to select which platforms to important,
so I don't think it makes sense to make this optional. After all the tar
could just be modified with the desired platforms.

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

This should be gated with --all-platforms as in nerdctl image convert --all-platforms OLD NEW .

// platform flags
&cli.StringSliceFlag{
Name: "platform",
Usage: "Convert content for a specific platform",
Value: &cli.StringSlice{},
},
&cli.BoolFlag{
Name: "all-platforms",
Usage: "Convert content for all platforms",
},

In addition to nerdctl load, nerdctl pull should follow the same semantics, though PR for nerdctl pull can be worked out separately
(Obviously nobody wants all the platforms to be pulled by default)

@AkihiroSuda AkihiroSuda mentioned this pull request Aug 18, 2021
5 tasks
@fahedouch
Copy link
Member

Hello,

@cpuguy83 I do not agree with you. the import retrieves the platform to import from the client which is platforms.DefaultStrict()

the platform is applied on the client here :

client, ctx, cancel, err := newClient(clicontext, containerd.WithDefaultPlatform(platforms.DefaultStrict()))

so then the import function will get it and initialize the platformMatcher with platforms.DefaultStrict() :
https://github.com/containerd/containerd/blob/fda782a7b941de5438e4fbbd38675bf23b69304b/import.go#L128

I agree with @AkihiroSuda making all-platforms by default is not the it is not the best choice. I think we should keep platforms.DefaultStrict() and add --all-platforms flag

@cpuguy83
Copy link
Member Author

Either way is fine.
My main point is the user is supplying the tar to import. It seems strange to not import the whole tar, especially since it seems to be importing them anyway.

@cpuguy83
Copy link
Member Author

Added the --all-platforms flag.

@fahedouch
Copy link
Member

LGTM

@@ -38,6 +38,10 @@ var loadCommand = &cli.Command{
Aliases: []string{"i"},
Usage: "Read from tar archive file, instead of STDIN",
},
&cli.BoolFlag{
Name: "all-platforms",
Usage: "Imports content for all platforms, false by default",
Copy link
Member

Choose a reason for hiding this comment

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

"false by default" can be removed, as urfave/cli/v2 appends (default: false) automatically

@AkihiroSuda AkihiroSuda changed the title load: import all platforms load: import all platforms (--all-platforms) Aug 19, 2021
Currently if you try to import a non-native image the blobs will get
GC'd and unpack fails with with "not found" on the blobs.

Import doesn't really have a way to select which platforms to important,
so I don't think it makes sense to make this optional. After all the tar
could just be modified with the desired platforms.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

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

Successfully merging this pull request may close these issues.

None yet

3 participants