Adds the possibility to choose the image by the variant field in a manifest #786

JirkaChadima · 2020-01-01T12:45:34Z

This is part of an containers/skopeo#790 and improves the ChooseImage method so that it respects the variant field which is used on ARM architecture. It also exposes the VariantChoice to the outside world, so it can be passed as a command line argument.

I'm not a Golang person, so it might be a little rusty. I intentionally kept the original behaviour that when there is no match for Variant, the first record matching the platform and architecture is returned. It might be more transparent to fail in that case though.

manifest/docker_schema2_list.go

manifest/oci_index.go

rhatdan · 2020-01-02T15:37:06Z

LGTM
But we need @mtrmac and @vrothberg to review.

mtrmac

Thanks!

See the pre-existing discussion in #753, especially #753 (review) .

manifest/docker_schema2_list.go

manifest/oci_index.go

types/types.go

JirkaChadima · 2020-01-03T08:04:49Z

Thanks for pointing out #753, I will check that out. Do you have any experience with GOARM? That might be used for default detection.

mtrmac · 2020-01-03T15:36:17Z

#753 does point at some pre-existing implementations; at this point, I’m happy to defer to ARM experts as long as the proposed mechanism is not excessively invasive and minimally plausible. We can always improve on the detection if there is interest.

JirkaChadima · 2020-01-06T10:17:53Z

I did some digging around. The GOARM seems to be primarily used when compiling GO programs and is a little controversial, so I would not use it at all, the support table refers to the ability to compile GO programs for those architectures/variants. And from my tests on RPi, it is not even a public property during runtime.

The go to option seems to be the parsing of /proc/cpuinfo. The whole approach is discussed in great detail in this presentation from ARM.

So I'm going to try to port the containerd code while taking a look into moby/moby as well. This is apparently not an easy problem.

mtrmac

Thanks!

(Not a full review, just pointing out some structural aspects in the meantime.)

copy/copy.go

manifest/platform_matcher.go

JirkaChadima · 2020-01-07T08:32:46Z

One thing I'm still not sure about is copy.CheckImageDestinationForCurrentRuntime. What does it do exactly? I wasn't able to get the Variant field out from the Image so I have nothing to compare it to, but maybe I was just looking in the wrong place.

mtrmac · 2020-01-08T21:02:48Z

One thing I'm still not sure about is copy.CheckImageDestinationForCurrentRuntime. What does it do exactly?

This exists so that copies of images that are intended to be run (e.g. “pulls” from remote registries to the local filesystem, as opposed to copies of images from one registry to another, without intending to directly run the image) fail early, i.e. that podman pull ARM-only-image or podman pull Windows-only-image fail on an x86 Linux.

Without this check, it’s perfectly possible to download images and extract them to a filesystem, it would only be running containers from such images that would fail. This allows alerting the user to this earlier, when it’s easy to detect and it can avoid costly/long pulls of unusable data.

I wasn't able to get the Variant field out from the Image so I have nothing to compare it to, but maybe I was just looking in the wrong place.

Indeed; see opencontainers/image-spec#777 . Until that lands (and images start using it), I suppose missing image variant has to be treated as “unknown, assumed compatible” in that code.

…nifest Signed-off-by: Jirka Chadima <[email protected]>

Largely based on https://github.com/moby/moby/blob/bc846d2e8fe5538220e0c31e9d0e8446f6fbc022/distribution/cpuinfo_unix.go https://github.com/containerd/containerd/blob/726dcaea50883e51b2ec6db13caff0e7936b711d/platforms/cpuinfo.go Signed-off-by: Jirka Chadima <[email protected]>

…ate image Signed-off-by: Jirka Chadima <[email protected]>

Signed-off-by: Jirka Chadima <[email protected]>

JirkaChadima · 2020-01-09T14:17:46Z

Until that lands (and images start using it), I suppose missing image variant has to be treated as “unknown, assumed compatible” in that code.

I've added the code, but it's commented out. I'm not sure if there is another way in a typed language.

Otherwise I think it's doing what it should be and I've hopefully addressed all of the (very helpful!) remarks. I'm pretty sure there is still some code that's not very go-like, so let me know what to change.

Thanks for your feedback, it's great to contribute to a project with such kind and welcoming maintainers.

JirkaChadima · 2020-01-16T11:55:25Z

I'm sorry if my last comment sounded definitive. Is there anything else I can do to push this closer to being merged? If not, is there any kind of timeline for this being merged and/or released? Thanks!

mtrmac · 2020-01-16T18:25:17Z

Don’t worry, the comment is perfectly fine; I’m afraid I just got busy with other work.

mtrmac · 2020-01-23T08:48:44Z

FWIW there were a few user reports of broken user cases by the architecture check in checkImageDestinationForCurrentRuntime, so we are going to remove it again.

rhatdan · 2020-01-28T19:40:50Z

@mtrmac Whats the scoop on this one?

mtrmac

I’m sorry it took so long to get back to you.

Do you want to update this, or shall I take the work over and finish it?

mtrmac · 2020-03-16T19:21:50Z

manifest/oci_index.go

 		}
 	}
-	return "", fmt.Errorf("no image found in image index for architecture %s, OS %s", wantedArch, wantedOS)
+	return "", fmt.Errorf("no image found in manifest list for architecture %s, variant %s, OS %s", wantedPlatforms[0].Architecture, wantedPlatforms[0].Variant, wantedPlatforms[0].OS)


“image index” is the OCI terminology, and should be preserved.

mtrmac · 2020-03-16T19:26:38Z

internal/pkg/platform/platform_matcher.go

+// Returns all compatible platforms with the platform specifics possibly overriden by user,
+// the most compatible platform is first.
+// If some option (arch, os, variant) is not present, a value from current platform is detected.
+func WantedPlatforms(ctx *types.SystemContext) ([]imgspecv1.Platform, error) {


Non-blocking: This can currently never fail, and it’s an internal API. The error return value could be dropped, which would simplify callers a bit.

mtrmac · 2020-03-16T19:27:35Z

copy/copy.go

+				return fmt.Errorf("Image operating system mismatch: image uses %q, expecting %q", c.OS, wantedPlatform.OS)
+			}
+			if wantedPlatform.Architecture != c.Architecture {
+				return fmt.Errorf("Image architecture mismatch: image uses %q, expecting %q", c.Architecture, wantedPlatform.Architecture)


This now needs to just use logrus.Infof instead of failing.

JirkaChadima · 2020-03-17T16:21:28Z

@mtrmac I think it would be more efficient if you take over. I don't remember much about this anymore TBH. And I wasn't following any other changes here, so I'm totally out of context anyway.

mtrmac · 2020-03-17T20:58:47Z

Updated in #854. Thanks again!

JirkaChadima mentioned this pull request Jan 1, 2020

Adds the --override-variant command line flag containers/skopeo#793

Merged

JirkaChadima force-pushed the feat-override-variant branch 2 times, most recently from 4676ebe to 90fbb2c Compare January 2, 2020 08:33

rhatdan reviewed Jan 2, 2020

View reviewed changes

manifest/docker_schema2_list.go Outdated Show resolved Hide resolved

manifest/oci_index.go Outdated Show resolved Hide resolved

JirkaChadima force-pushed the feat-override-variant branch from 90fbb2c to e39e4b4 Compare January 2, 2020 15:07

mtrmac reviewed Jan 2, 2020

View reviewed changes

manifest/docker_schema2_list.go Outdated Show resolved Hide resolved

manifest/oci_index.go Outdated Show resolved Hide resolved

types/types.go Show resolved Hide resolved

mtrmac mentioned this pull request Jan 2, 2020

Extended SystemContext to include VariantChoice. #753

Closed

JirkaChadima force-pushed the feat-override-variant branch from e39e4b4 to 50c993c Compare January 6, 2020 10:02

mtrmac reviewed Jan 6, 2020

View reviewed changes

copy/copy.go Outdated Show resolved Hide resolved

manifest/platform_matcher.go Outdated Show resolved Hide resolved

manifest/platform_matcher.go Outdated Show resolved Hide resolved

JirkaChadima force-pushed the feat-override-variant branch 2 times, most recently from e04febb to 84e09f6 Compare January 7, 2020 11:29

JirkaChadima force-pushed the feat-override-variant branch from 7f50fa8 to c361ecf Compare January 9, 2020 12:07

JirkaChadima added 6 commits January 9, 2020 14:02

Adds the possibility to choose the image by the variant field in a ma…

ff9be61

…nifest Signed-off-by: Jirka Chadima <[email protected]>

Allows matching of compatible OS Variants when selecting the appropri…

78151d3

…ate image Signed-off-by: Jirka Chadima <[email protected]>

Makes platform_matcher internal

6a3e032

Signed-off-by: Jirka Chadima <[email protected]>

Improves generality of platform matcher, adds a few tests

345c9c5

Signed-off-by: Jirka Chadima <[email protected]>

Makes copy command ready to check Variant eventually

6389bbc

Signed-off-by: Jirka Chadima <[email protected]>

JirkaChadima force-pushed the feat-override-variant branch from c361ecf to d9fdc58 Compare January 9, 2020 13:02

Improves error messages when matching image to a platform

fc304bb

Signed-off-by: Jirka Chadima <[email protected]>

JirkaChadima force-pushed the feat-override-variant branch from d9fdc58 to fc304bb Compare January 9, 2020 14:08

mtrmac mentioned this pull request Jan 13, 2020

Pulling cross platform images containers/podman#4849

Closed

grooverdan mentioned this pull request Jan 28, 2020

buildah --platform to take os/arch format and set default ARGS {BUILD|TARGET}{PLATFORM|OS|ARCH|VARIANT} containers/buildah#1903

Closed

mtrmac reviewed Mar 16, 2020

View reviewed changes

mtrmac mentioned this pull request Mar 17, 2020

Add the option to choose an image by the variant field in a manifest #854

Merged

mtrmac closed this Mar 17, 2020

Adds the possibility to choose the image by the variant field in a manifest #786

Adds the possibility to choose the image by the variant field in a manifest #786

Uh oh!

Conversation

JirkaChadima commented Jan 1, 2020

Uh oh!

Uh oh!

Uh oh!

rhatdan commented Jan 2, 2020

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JirkaChadima commented Jan 3, 2020

Uh oh!

mtrmac commented Jan 3, 2020

Uh oh!

JirkaChadima commented Jan 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JirkaChadima commented Jan 7, 2020

Uh oh!

mtrmac commented Jan 8, 2020

Uh oh!

JirkaChadima commented Jan 9, 2020

Uh oh!

JirkaChadima commented Jan 16, 2020

Uh oh!

mtrmac commented Jan 16, 2020

Uh oh!

mtrmac commented Jan 23, 2020

Uh oh!

rhatdan commented Jan 28, 2020

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

mtrmac Mar 16, 2020

Choose a reason for hiding this comment

Uh oh!

mtrmac Mar 16, 2020

Choose a reason for hiding this comment

Uh oh!

mtrmac Mar 16, 2020

Choose a reason for hiding this comment

Uh oh!

JirkaChadima commented Mar 17, 2020

Uh oh!

mtrmac commented Mar 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JirkaChadima commented Jan 6, 2020 •

edited

Loading