Fix platform struct passing

[tonistiigi]

replaces #36418
replaces #37346

This PR makes everything that exposes platform field to the API to use oci platform struct instead of a os string, pass it in its complete form to the distribution and use it for manifest filtering.

This fixes cases like FROM --platform=arm , FROM --platform=linux/arm, docker build --platform=linux/arm/v7 etc.

It also fixes a bug on matching Arm variants that were previously ignored using the logic defined by containerd https://github.com/containerd/containerd/blob/08f7ee9828af1783dc98cc5cc1739e915697c667/platforms/platforms.go#L98 . If the variant matches fails it will fall back to the old method for now to avoid breakage.

Another update is that --platform flag can now contain global build arguments.

Things not yet working:

• Pulling arm images with BuildKit enabled requires containerd update with this fix Fix arm platform matching containerd/containerd#2414 cc @dmcgowan
• docker build --platform doesn't work yet with BuildKit. Need to enable passing this in BuildKit first https://github.com/moby/buildkit/pull/464/files#diff-61884a987d7800098b4038b572819f25R41

[codecov]

Codecov Report

Merging #37350 into master will decrease coverage by 0.01%.
The diff coverage is 19.42%.

@@            Coverage Diff             @@
##           master   #37350      +/-   ##
==========================================
- Coverage   34.83%   34.81%   -0.02%     
==========================================
  Files         610      610              
  Lines       44986    45006      +20     
==========================================
- Hits        15672    15670       -2     
- Misses      27201    27221      +20     
- Partials     2113     2115       +2

[tonistiigi]

@jhowardmsft @arm64b @johnstep @thaJeztah

[thaJeztah]

/cc @tianon @yosifkit @duglin

[thaJeztah]

just some first/quick questions / nits

[thaJeztah]

Do we need the OSversion + features in these structs as well, or can those be added at a later stage if needed?

[tonistiigi]

We don't as the containerd puller doesn't use these fields. Should they be needed in the future they can be added in backward compatible manner.

[thaJeztah]

Alright, thanks for checking!

[thaJeztah]

s/bath/path/ :bath:

[lowenna]

https://idioms.thefreedictionary.com/an+early+bath 🇬🇧

[thaJeztah]

^^ also known for http://realworldstudios.com 😇

[thaJeztah]

Looks like these comments are no longer current?

[arm64b]

IMO, except the parser to parse the os/arch/variant/[os version, os feature, ...], seems we need a sanity checker for the parsed unit to offload the parser in containerd code, that's why I have a TODO here. But I am not sure if it's 100% necessary.

[thaJeztah]

Ah, yes, but saw the code above was removed, and some comments still there 😅

[arm64b]

In my PR #37346 , I have below workaround 😄:

+// TODO(@arm64b): place holder for the sanity check of the spec platform
 +func ValidatePlatform(platform *specs.Platform) error {
 +	return nil
 +}

This code should be in the containerd as the partner of Parse function.

[tonistiigi]

Currently containerd doesn't have this function. I'm not sure how they would implement it and it would need a PR there before. Some of what the old function did is handled automatically in Parse. I think it would make sense to validate lcow in windows like it was before so I could add it back. Ideally, we would add supported platforms into daemon config so we can validate the linux cases as well (like the emulators) and correctly pick what nodes can handle the workloads. Eg. https://github.com/moby/buildkit/blob/master/solver/llbsolver/vertex.go#L77 is supported in buildkit.

@thaJeztah So in what state do you want me to leave this. Just remove comments for now or something more?

[thaJeztah]

Adding the supported platforms to the daemon sounds like a bigger (but preferable) approach; we can add just a TODO here, and work on the bigger change in a follow up.

If we need the validation that was removed, I'm also ok with temporarily adding it back

@jhowardmsft wdyt?

[lowenna]

I think it makes sense to keep something in there temporarily, but not critical. I'm not sure how misleading the error message would be without the check though.

[tonistiigi]

Added back the lcow-only validation.

[tiborvass]

@tonistiigi it's unclear if this is desired or a mistake

[tonistiigi]

updated

[tonistiigi]

Removed the custom matcher additions as containerd has been updated now #37355

[tiborvass]

LGTM

[johnstep]

There is one issue I missed in my review of #36418 that should be addressed before merging.

[johnstep]

With this change there can be multiple operating systems in the list, which manifestsByVersion does not currently handle properly. This means if Linux comes before Windows in the manifest list, then Linux will be selected unless an exact Windows build match is found. With Hyper-V containers, other versions of Windows should still be preferred over Linux, but that is not happening (17134 host, 16299 highest in list):

$ docker image pull hello-seattle
Using default tag: latest
latest: Pulling from library/hello-seattle
6d7e9a8e8043: Pull complete
Digest: sha256:93cdcb901826410633ef2a18052b6a1ffc72f14a3323ddc94023ff58f4171d49
Status: Downloaded newer image for hello-seattle:latest

$ (docker image inspect hello-seattle | ConvertFrom-Json).Os
linux

The following change addresses this by always sorting Windows first:

@@ -103,7 +103,8 @@ func (mbv manifestsByVersion) Less(i, j int) bool {
        // TODO: Split version by parts and compare
        // TODO: Prefer versions which have a greater version number
        // Move compatible versions to the top, with no other ordering changes
-       return versionMatch(mbv.list[i].Platform.OSVersion, mbv.version) && !versionMatch(mbv.list[j].Platform.OSVersion, mbv.version)
+       return (strings.EqualFold("windows", mbv.list[i].Platform.OS) && !strings.EqualFold("windows", mbv.list[j].Platform.OS)) ||
+               (versionMatch(mbv.list[i].Platform.OSVersion, mbv.version) && !versionMatch(mbv.list[j].Platform.OSVersion, mbv.version))
 }

With this change, the expected images are pulled:

$ docker image pull hello-seattle
Using default tag: latest
latest: Pulling from library/hello-seattle
bce2fbc256ea: Already exists
83eec61707e8: Already exists
32e7fb1c173a: Pull complete
8d9eb6464441: Pull complete
Digest: sha256:93cdcb901826410633ef2a18052b6a1ffc72f14a3323ddc94023ff58f4171d49
Status: Downloaded newer image for hello-seattle:latest

$ (docker image inspect hello-seattle | ConvertFrom-Json).Os
windows

$ docker image pull --platform linux hello-seattle
Using default tag: latest
latest: Pulling from library/hello-seattle
6d7e9a8e8043: Pull complete
Digest: sha256:93cdcb901826410633ef2a18052b6a1ffc72f14a3323ddc94023ff58f4171d49
Status: Downloaded newer image for hello-seattle:latest

$ (docker image inspect hello-seattle | ConvertFrom-Json).Os
linux

Without Hyper-V, the image build must match the host. In that case, it's possible that none of the Windows images in the manifest list will run, but the Linux one may. However, it might still be acceptable to prefer Windows and show the error, informing the user that the manifest list contains Windows, but the wrong version. The user can then choose to pull the Linux image with the platform switch.

On a related note, Hyper-V does not support running builds that are higher than the host. Those should be filtered out entirely, and never added to the list. I will follow up with a separate fix for that.

@jhowardmsft, please take a look.

[tiborvass]

We can address the issues @johnstep found in a follow up.