Add support for platform (os and architecture) on image import#43103
Conversation
thaJeztah
added
status/2-code-review
kind/enhancement
thaJeztah
force-pushed
the
image_import_platform
branch
from
7e27502 to
bb85375
Compare
samuelkarp
left a comment
samuelkarp
left a comment
There was a problem hiding this comment.
Typo in commit message ("endpoitn") but otherwise LGTM.
Ah! 🤦. I think I'll leave that one be 😅 |
|
@tonistiigi ptal |
| platform = &specs.Platform{} | ||
| } | ||
| if platform.Architecture == "" { | ||
| // TODO platforms.Normalize() is inconsistent: it sets default OS, but does not set default Architecture. |
There was a problem hiding this comment.
This was mostly an observation; the Normalize() code is here;
moby/vendor/github.com/containerd/containerd/platforms/platforms.go
Lines 269 to 271 in ea96e16
That code calls both normalizeOS() and normalizeArch(); I would expect those to act the same, but where normalizeOS() both normalizes the OS, and sets a default;
moby/vendor/github.com/containerd/containerd/platforms/database.go
Lines 69 to 72 in ea96e16
But normalizeArch() only "normalizes" (but does not set defaults);
I would've expected them both to do the same (fill in missing fields if not set, and normalize the fields where needed), so if no architecture is set, it would set the local architecture as default.
There was a problem hiding this comment.
Parse() adds the same runtime.GOARCH if one is not set in the string so this should not be needed.
There was a problem hiding this comment.
Right, but that's assuming it always originates from a string that's parsed using that function 🤔, so it's either string -> parse (which also normalises), or Normalize() which normalises, but slightly different.
| config, err := dockerfile.BuildFromConfig(&container.Config{}, changes, os) | ||
| // Normalize platform - default to the operating system and architecture if not supplied. | ||
| if platform == nil { | ||
| platform = &specs.Platform{} |
There was a problem hiding this comment.
shouldn't this be platforms.DefaultSpec()?
There was a problem hiding this comment.
Ah, hm.. yes, perhaps I could've gone for DefaultSpec(). My train of thought was to have the platforms.Normalize() take care of filling in the missing bits, but I guess if we have no platform at all, we could as well use DefaultSpec(). Let me have a look at that
thaJeztah
force-pushed
the
image_import_platform
branch
from
bb85375 to
9becef6
Compare
|
@tonistiigi updated; PTAL |
| Platform in the format os[/arch[/variant]]. | ||
|
|
||
| When used in combination with the `fromImage` option, the daemon checks | ||
| if the given image is present in the local image cache with the given | ||
| OS and Architecture, and otherwise attempts to pull the image. If the | ||
| option is not set, the host's native OS and Architecture are used. | ||
| If the given image does not exist in the local image cache, the daemon | ||
| attempts to pull the image with the host's native OS and Architecture. | ||
| If the given image does exists in the local image cache, but its OS or | ||
| architecture does not match, a warning is produced. | ||
|
|
||
| When used with the `fromSrc` option to import an image from an archive, | ||
| this option sets the platform information for the imported image. If | ||
| the option is not set, the host's native OS and Architecture are used | ||
| for the imported image. |
There was a problem hiding this comment.
Also tried to improve the description here (but it's complicated to describe the full behaviour as there's many variations possible due to the same endpoint being used both for import and pull 😞)
|
Seeing more failures like this on Windows (not sure what changed; we updated the docker version on the Windows machines, but no other changes afaik); |
|
win-2022/c8d failed to build the Dockerfile (again, similar failiures as I've seen elsewhere with permission denied on renaming files); win-2022; similar failure, but during a test: win-2022: known flaky test: windows rs5 timed out after 2 hours |
thaJeztah
force-pushed
the
image_import_platform
branch
from
9becef6 to
18c43d7
Compare
|
@tonistiigi @tianon PTAL |
| if platform.Architecture == "" { | ||
| // TODO platforms.Normalize() only sets default OS, but does not set default Architecture if missing. | ||
| platform.Architecture = runtime.GOARCH | ||
| } |
There was a problem hiding this comment.
Discussing with @tonistiigi to remove this part, and make it the caller's responsibility to fill in the fields
| // TODO platforms.Normalize() only sets default OS, but does not set default Architecture if missing. | ||
| platform.Architecture = runtime.GOARCH | ||
| } | ||
| p = platforms.Normalize(*platform) |
There was a problem hiding this comment.
This can also be removed (assuming that the caller has called Parse()
thaJeztah
force-pushed
the
image_import_platform
branch
from
18c43d7 to
046c942
Compare
|
@tonistiigi updated as discussed |
| // Normalize platform - default to the operating system and architecture if not supplied. | ||
| var p specs.Platform | ||
| if platform == nil { | ||
| p = platforms.DefaultSpec() |
This error is meant to be used in the output stream, and some comments were added to prevent accidentally using local variables. Renaming the variable instead to make it less ambiguous. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Just a minor nit: make sure we use a designated "bad" domain https://datatracker.ietf.org/doc/html/rfc6761#section-6.4 Signed-off-by: Sebastiaan van Stijn <[email protected]>
Commit 0380fbf added the ability to pass a --platform flag on `docker import` when importing an archive. The intent of that commit was to allow importing a Linux rootfs on a Windows daemon (as part of the experimental LCOW feature). A later commit (337ba71) changed some of this code to take both OS and Architecture into account (for `docker build` and `docker pull`), but did not yet update the `docker image import`. This patch updates the import endpoitn to allow passing both OS and Architecture. Note that currently only matching OSes are accepted, and an error will be produced when (e.g.) specifying `linux` on Windows and vice-versa. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
image_import_platform
branch
from
046c942 to
01ae952
Compare
|
Windows failure is unrelated (known flaky test) |
|
@tonistiigi PTAL |
|
This seems to still ignore the |
|
I also still see the problem with |
|
This change is only in master, and not the 20.10 releases, so that's expected. |
According to moby/moby#43103, when --platform was initially added to docker import, it only honored the OS portion of the argument, and Architecture was silently ignored. That PR, which resolved the issue and made import honor the architecture segment, was merged for 23.0.0. Therefore, in order for docker import --platform to resolve https://gitlab.com/gitlab-org/gitlab-runner/-/issues/30869, we must use at least Docker 23.0.0.
6 participants
fixes #42813
fixes docker/cli#3408
Commit 0380fbf (part of #34642) added the ability to pass a
--platform flagondocker importwhen importing an archive. The intent of that commit was to allow importing a Linux rootfs on a Windows daemon (as part of the experimental LCOW feature).A later commit (337ba71, part of #37350) changed some of this code to take both OS and Architecture into account (for
docker buildanddocker pull), but did not yet update thedocker image import.This patch updates the import endpoitn to allow passing both OS and Architecture. Note that currently only matching OSes are accepted, and an error will be produced when (e.g.) specifying
linuxon Windows and vice-versa.- How to verify it
A test has been added, which can be run with:
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)