Conversation
e0c4d58 to
c8ae8fb
Compare
Codecov Report
@@ Coverage Diff @@
## master #36418 +/- ##
=========================================
Coverage ? 34.94%
=========================================
Files ? 613
Lines ? 45563
Branches ? 0
=========================================
Hits ? 15924
Misses ? 27550
Partials ? 2089 |
|
ping @johnstep @salah-khan PTAL |
c8ae8fb to
53bdc73
Compare
|
Rebased. |
53bdc73 to
98fc141
Compare
Signed-off-by: John Howard <jhoward@microsoft.com> Addresses moby#35089 (comment). This change enables the daemon to automatically select an image under LCOW that can be used if the API doesn't specify an explicit platform. For example: FROM supertest2014/nyan ADD Dockerfile / And docker build . will download the linux image (not a multi-manifest image) And similarly docker pull ubuntu will match linux/amd64
98fc141 to
6baf604
Compare
|
@tonistiigi @dnephin @johnstep Thanks for reviewing #35089. Now that is merged, this is rebased and ready for review. It addresses your outstanding comments about platform auto-select if the daemon finds an image which it can use. See the description at the top of the PR for examples. |
| } | ||
| if os == "windows" { | ||
| if foundWindowsMatch { | ||
| sort.Stable(manifestsByVersion{osVersion, matches}) |
There was a problem hiding this comment.
Maybe note that this is how a Windows image is selected over Linux image, for an unspecified OS with LCOW enabled. This is probably fine since the Windows build number must match anyway.
| func filterManifests(manifests []manifestlist.ManifestDescriptor, requestedOS string) []manifestlist.ManifestDescriptor { | ||
| version := system.GetOSVersion() | ||
| osVersion := fmt.Sprintf("%d.%d.%d", version.MajorVersion, version.MinorVersion, version.Build) | ||
| logrus.Debugf("will prefer Windows entries with version %s", osVersion) |
There was a problem hiding this comment.
Maybe the above 3 lines should only be called if requestedOS is not linux? Calling system.GetOSVersion() is not useful in that case, and it seems weird to output the message. However, I guess it will be output anyway, when the OS is not specified, and the manifest contains only Linux images. That could be addressed by only getting the version once the first Windows image is found.
There was a problem hiding this comment.
Or simply put "will prefer any found Windows entries with version ….". This is only a debug statement ;) To note, this is in a _windows.go file, so it won't be called when running on Linux.
|
Ping @tonistiigi again. PTAL. This was an issue you (and to some extent) @dnephin were both insisting was done post #35089, yet it's been sitting un-reviewed for a month now. I assume you still want this functionality? Thanks. |
|
ping @tonistiigi yet again. PTAL. @johnstep - any further comments? |
|
I was asked to provide my thoughts on whether the overall approach here makes sense, and this:
Seems like sane logic IMO -- if the user hasn't specified otherwise, and we're Windows and the image we want to run is Windows, run Windows, else if we can Linux, let's try Linux. 👍 |
|
Ping again.... |
|
Currently, the user experience horks with this bizarre message:
#36773 (comment). It would make things much better. However, if we get the real helpful advice in output:
that would equally help. Just imaging the world where we will have BSD based dockers, so running Dragonfly on Windows when mode is Linux should show me the error, instead of auto selecting stuff. |
|
Ping @thaJeztah @carlfischer1, on the 4th month anniversary of the PR not being merged, how can it be moved forward? |
|
Opened #37350 that builds on this and combines with #37346 and updated parser pkg. @thaJeztah We could still merge this separately, but we shouldn't release without the full platform support. |
|
@tonistiigi is this one still needed now that #37350 was merged? |
|
Ah, no. Closing |
|
Thanks! I wasn't sure if there was anything left in this one 🤗 |
Signed-off-by: John Howard jhoward@microsoft.com
Fixes #36560
Addresses #35089 (comment) and #35089 (comment).
With this PR, an LCOW daemon will choose to pull the Linux image (both for multi-manifest and single tag images) if
So for example:
Note microsoft/dotnet has both Linux and multiple Windows images in it's manifest list, so it should pull the best matching Windows image, not the Linux one.
And the above through the builder
And an explicit Linux request of microsoft/dotnet
And using scratch