platforms: define selectors for platforms#1403
Conversation
platforms/database.go
Outdated
There was a problem hiding this comment.
if variant == "v8" { variant = ""}? (for better support for future non-v8 variants)
There was a problem hiding this comment.
arm64 always defaults to v8. If there are future variants, they will be qualified as such.
platforms/database.go
Outdated
There was a problem hiding this comment.
I think we only need linux, windows, and solaris entries here.
At least, android, plan9, nacl, and zos are really unlikely to be supported in containerd.
Also, I think it is ok to use typed struct here, as the upstream godoc list won't be updated so frequently.
There was a problem hiding this comment.
@AkihiroSuda We are going to support the full Go space for platform declaration. This does not imply actual support. This is just identification.
tianon
left a comment
There was a problem hiding this comment.
Overall I really like the idea of creating a standard "specifier" for these things, but I'm a little confused about the intended usage of this as-is, and how it's expected to work (which I've expanded on a little in my further line comments).
For example, on 32-bit ARM, alpine supports v6, which would be the appropriate image choice for a v7 machine, but debian only support v5 and v7, so for a v6 chip, v5 is the only option unless one switches to Raspbian.
Do you think you could lay out a few of your intended scenarios to help clear up how you imagine this being used? 😇
platforms/database.go
Outdated
There was a problem hiding this comment.
Won't these also match substrings like fly, c64, or 64p32?
There was a problem hiding this comment.
Yes, needs to be fixed.
platforms/database.go
Outdated
There was a problem hiding this comment.
Why is v6 the implied default? Isn't arm always ambiguous?
There was a problem hiding this comment.
We have to choose a default because there are a lot of images that are just "arm".
Blame "arm", not the messenger.
There was a problem hiding this comment.
Right, so shouldn't this go the other way in that case? ie, if there isn't a variant, then v6 is implied?
There was a problem hiding this comment.
Uh no, v6 doesn't need to be qualified, so we are "normalizing" it.
platforms/platforms_test.go
Outdated
There was a problem hiding this comment.
This seems bizarre to me -- is the intended usage of this PR for something like a --platform flag to docker pull and friends?
I'd personally be very surprised to find that docker pull --platform linux/arm/v6 xxx ended up getting converted to simply linux/arm, and likewise, I'd be surprised if --platform linux/arm implied only v6. I'd honestly want linux/arm to throw an explicit specificity error, or to imply linux/arm/* (and thus choose the first image which is compatible with my current platform).
There was a problem hiding this comment.
@tianon This is kind of the de facto standard for unqualified arm. Go's runtime defaults to 6 and we kind of need to choose a common denominator that makes this easy. armhf seems to have a default of v6, as well. If you can show evidence or documentation showing
The idea is not to make this too strict and too have some reasonable defaults.
There was a problem hiding this comment.
In Debian armhf is v7 with hardfloat see this (this is the reason for the existence of Rasbian which rebuilds as v6 for compat with RPi hardware). I believe at the time there was cross distro agreement on that as a baseline (at least Ubuntu and Fedora IIRC) for the ARM distros of the day.
There was a problem hiding this comment.
Yeah, I think this default might need to change based on where it's being used -- for ARM-based images that don't specify a variant, I think v7 would be the safer choice/assumption, but for a user pulling images, they probably want to allow v6 if all they specify is "ARM".
e48c018 to
f42ac4c
Compare
platforms/database.go
Outdated
There was a problem hiding this comment.
linux/s390x should also be in here, I'm not sure why it's not listed here on the go page.
There was a problem hiding this comment.
I am not sure if we'll even enforce this list. These are effectively the officially supported Go platforms, which is a subset of the platforms that might run containers. I'm not sure that this is necessary to maintain in this package.
There was a problem hiding this comment.
@tophj-ibm I've added a few test cases for linux/s390x to ensure its covered. I'm going to structure this database a little more carefully to avoid these kinds of problems. I don't want this package to become an impediment to wider platform support!
3a6de93 to
395c9b3
Compare
Codecov Report
@@ Coverage Diff @@
## master #1403 +/- ##
=======================================
Coverage 40.82% 40.82%
=======================================
Files 23 23
Lines 2920 2920
=======================================
Hits 1192 1192
Misses 1451 1451
Partials 277 277Continue to review full report at Codecov.
|
platforms/database.go
Outdated
platforms/platforms.go
Outdated
There was a problem hiding this comment.
minor nit, but should this be marked as TODO?
platforms/database.go
Outdated
There was a problem hiding this comment.
I'm not sure one wants to do this matching above of commonly used but not part of the OCI spec names for architectures.
I would suggest to simply just use the names from the spec and that's it. Otherwise you'd have to do the same of OSes too. darwin is now officially called macOS, for example. If you translate x86-64 to amd64 why not translate macOS to darwin?
There was a problem hiding this comment.
We can add a translation for macOS.
There was a problem hiding this comment.
I'm not sure one wants to do this matching above of commonly used but not part of the OCI spec names for architectures.
I'd prefer a strict approach, but I think we can make everyone happy here because the sets (os/arch) are disjoint. To keep this nice property across projects, the normalization needs to be centralized.
Do you see an issue with this in the future?
There was a problem hiding this comment.
I have to say that at time I find the mismatch between uname -m and Go architectures (and therefor OCI architectures) quite annoying, so I can see an argument for tools to accept both (and then internally translate to the canonical GOARCH/GOOS format). But I can also see an argument for keeping it simple and just use OCI names and leave it to the user to translate.
For now I don't see any issues in the future (I left my crystal ball at home) so if the general desire is to allow commonly used alternative arch/OS names to be used, centralising it in one place like this PR tries certainly a good idea.
platforms/platforms.go
Outdated
platforms/platforms.go
Outdated
bfeeba3 to
26f88fd
Compare
platforms/database.go
Outdated
There was a problem hiding this comment.
We normalise (below) linux/arm into linux/arm/v7 but here we are normalizing linux/armhf into just linux/arm, which seems a bit non-normalysed to me.
IOW if the result of normalizeArch was actually normative (as I think it should be) then the assertion in the following should always be true:
foo := normalizeArch(input)
assert(foo == normalizeArch(foo))
but that doesn't appear to be the case when input == "linux/armhf"
There was a problem hiding this comment.
TL;DR: I think you can ignore my previous comment.
I think I misread the code handling the "arm" case, that switch doesn't actually have the default case I thought it did so normalizeArch("arm") == "arm" as required.
(I'm also aware I was taking some liberties with the actual arguments, there should be split("/", foo) stuff sprinkled around in my examples to make them closer to actually working examples)
There was a problem hiding this comment.
Just added a test case to ensure the round trip.
26f88fd to
517b25b
Compare
estesp
left a comment
There was a problem hiding this comment.
this is looking good, IMO. The make vet error is the only CI issue.
platforms/platforms_test.go
Outdated
There was a problem hiding this comment.
the round trip addition has an issue from make vet:
platforms/platforms_test.go:198: wrong number of args for format in Fatalf call: 1 needed but 2 args
58b29dc to
1a05466
Compare
platforms/database.go
Outdated
platforms/database.go
Outdated
There was a problem hiding this comment.
Why include the v7 case, since it doesn't change? Is it just for clarity, to assign variant in all non-default cases?
There was a problem hiding this comment.
Mostly to prevent it going down other control flow paths.
platforms/database.go
Outdated
There was a problem hiding this comment.
Since variant can also contain letters (e.g. v7), should you also normalize it to lowercase?
platforms/defaults.go
Outdated
platforms/platforms.go
Outdated
platforms/platforms.go
Outdated
There was a problem hiding this comment.
One comment says the minimum requirement for a platform selector is the operating system, but the next paragraph says operating system or architecture, which matches the format specification. Remove the incorrect sentence and probably combine the two paragraphs into one.
platforms/platforms.go
Outdated
There was a problem hiding this comment.
I think this needs a * for allowing multiple characters: ^[A-Za-z0-9_-]*$
There was a problem hiding this comment.
... This was completely broken ...
For supporting selection of images and runtimes in the containerized world, there is thin support for selecting objects by platform. This package defines a syntax to display to users that can express specific platforms in addition to wild cards for matching platforms. The plan is to extend this to provide support for parsing flag arguments and displaying platform types for images. This package will also provide a configurable matcher to allow match of platforms against arbitrary targets, invariant to the Go compilation. The internals are based the OCI Image Spec model. This changeset is being submitted for feedback on the approach before this gets larger. Specifically, review the unit tests and raise any concerns. Signed-off-by: Stephen J Day <stephen.day@docker.com>
9d863ff to
fb06883
Compare
|
Matcher support has been added and the tests have been updated accordingly. PTAL |
estesp
left a comment
There was a problem hiding this comment.
LGTM with two minor typos in comments and a comment that seems incorrect
platforms/platforms.go
Outdated
platforms/platforms.go
Outdated
There was a problem hiding this comment.
wouldn't if ok actually be a match? Unless I'm misunderstanding either the ok should be !ok or the comment should be "does match"
platforms/platforms_test.go
Outdated
Matching support is now implemented in the platforms package. The `Parse` function now returns a matcher object that can be used to match OCI platform specifications. We define this as an interface to allow the creation of helpers oriented around platform selection. Signed-off-by: Stephen J Day <stephen.day@docker.com>
721e70a to
94f6be5
Compare
platforms/platforms.go
Outdated
| // While the OCI platform specifications provide a tool for components to | ||
| // specify structured information, user input typically doesn't need the full | ||
| // context and much can be inferred. To solve this problem, we introduced | ||
| // "specifiers". A specifier has the format `<os|arch>[/<arch>[/<variant>]]`. |
There was a problem hiding this comment.
Should this be <os[/<arch>[/<variant>]]|arch[/<variant>]>?
Unless I'm reading it incorrectly, it would currently match something like amd64/arm.
There was a problem hiding this comment.
This isn't really an official grammar. The variant can never be expressed without OS and Arch. I think maybe this would work:
<os>|<arch>|<os>/<arch>/<variant>
There was a problem hiding this comment.
Is variant optional in the last case?
platforms/platforms.go
Outdated
|
|
||
| // Parse parses the platform specifier syntax into a platform declaration. | ||
| // | ||
| // Platform specifiers are in the format <os|arch>[/<arch>[/<variant>]]. The |
|
LGTM |
Signed-off-by: Stephen J Day <stephen.day@docker.com>
|
LGTM |
| platform.OS = normalizeOS(platform.OS) | ||
| platform.Architecture, platform.Variant = normalizeArch(platform.Architecture, platform.Variant) | ||
|
|
||
| // these fields are deprecated, remove them |
There was a problem hiding this comment.
@stevvooe I just noticed this comment (we were discussing these fields last week in context for Windows images (which may need an OSVersion to match the correct variant); Do you have a reference where these fields are marked deprecated? The spec doesn't mention that; https://github.com/opencontainers/image-spec/blob/v1.0.1/specs-go/v1/descriptor.go#L53-L59 (if they're deprecated, I can open a PR to mark them as such in the spec)
vendor: update golang.org/x/sys 52ab431487773bc9dd1b0766228b1cf3944126bf
For supporting selection of images and runtimes in the containerized
world, there is thin support for selecting objects by platform. This
package defines a syntax to display to users that can express specific
platforms in addition to wild cards for matching platforms.
The plan is to extend this to provide support for parsing flag
arguments and displaying platform types for images. This package will
also provide a configurable matcher to allow match of platforms against
arbitrary targets, invariant to the Go compilation.
The internals are based the OCI Image Spec model.
This changeset is being submitted for feedback on the approach before
this gets larger. Specifically, review the unit tests and raise any
concerns.
Signed-off-by: Stephen J Day stephen.day@docker.com
cc @jhowardmsft @darrenstahlmsft @tianon @estesp