Add support for platform (os and architecture) on image import #43103

thaJeztah · 2021-12-23T15:52:53Z

Commit 0380fbf (part of #34642) 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, part of #37350) 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.

- How to verify it

A test has been added, which can be run with:

make DOCKER_GRAPHDRIVER=vfs TEST_SKIP_INTEGRATION_CLI=1 TESTFLAGS='-test.run TestImportWithCustomPlatform' test-integration

...

INFO: Testing against a local daemon
=== RUN   TestImportWithCustomPlatform
=== PAUSE TestImportWithCustomPlatform
=== CONT  TestImportWithCustomPlatform
=== RUN   TestImportWithCustomPlatform/#00
=== RUN   TestImportWithCustomPlatform/_______
=== RUN   TestImportWithCustomPlatform//
=== RUN   TestImportWithCustomPlatform/linux
=== RUN   TestImportWithCustomPlatform/LINUX
=== RUN   TestImportWithCustomPlatform/linux/sparc64
=== RUN   TestImportWithCustomPlatform/macos
=== RUN   TestImportWithCustomPlatform/macos/arm64
=== RUN   TestImportWithCustomPlatform/nintendo64
--- PASS: TestImportWithCustomPlatform (1.44s)
    --- PASS: TestImportWithCustomPlatform/#00 (0.14s)
    --- PASS: TestImportWithCustomPlatform/_______ (0.00s)
    --- PASS: TestImportWithCustomPlatform// (0.00s)
    --- PASS: TestImportWithCustomPlatform/linux (0.13s)
    --- PASS: TestImportWithCustomPlatform/LINUX (0.13s)
    --- PASS: TestImportWithCustomPlatform/linux/sparc64 (0.13s)
    --- PASS: TestImportWithCustomPlatform/macos (0.03s)
    --- PASS: TestImportWithCustomPlatform/macos/arm64 (0.00s)
    --- PASS: TestImportWithCustomPlatform/nintendo64 (0.00s)
PASS

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

samuelkarp

Typo in commit message ("endpoitn") but otherwise LGTM.

thaJeztah · 2021-12-23T20:11:17Z

Typo in commit message ("endpoitn")

Ah! 🤦. I think I'll leave that one be 😅

thaJeztah · 2021-12-23T20:15:54Z

@tonistiigi ptal

tonistiigi · 2022-01-07T22:34:00Z

daemon/images/image_import.go

+		platform = &specs.Platform{}
+	}
+	if platform.Architecture == "" {
+		// TODO platforms.Normalize() is inconsistent: it sets default OS, but does not set default Architecture.


what case is this?

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

func Normalize(platform specs.Platform) specs.Platform {

platform.OS = normalizeOS(platform.OS)

platform.Architecture, platform.Variant = normalizeArch(platform.Architecture, platform.Variant)

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

func normalizeOS(os string) string {

if os == "" {

return runtime.GOOS

}

But normalizeArch() only "normalizes" (but does not set defaults);

moby/vendor/github.com/containerd/containerd/platforms/database.go

Line 83 in ea96e16

func normalizeArch(arch, variant string) (string, string) {

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.

Parse() adds the same runtime.GOARCH if one is not set in the string so this should not be needed.

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.

tonistiigi · 2022-01-07T22:34:47Z

daemon/images/image_import.go

-	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{}


shouldn't this be platforms.DefaultSpec()?

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 · 2022-01-10T15:11:43Z

@tonistiigi updated; PTAL

thaJeztah · 2022-01-10T15:12:45Z

api/swagger.yaml

+            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.


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 😞)

thaJeztah · 2022-01-10T17:25:47Z

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);

=== RUN   TestDockerSuite/TestCpRelativePath
    check_test.go:170: assertion failed: error is not nil: Error response from daemon: container d14484dbcaa7e7d302c1182e17b30e1a87d283475f356db80b903602d1976871: driver "windowsfilter" failed to remove root filesystem: failed to detach VHD: failed to detach virtual disk: The device is not ready.: rename D:\CI\PR-43103\3\daemon\windowsfilter\d14484dbcaa7e7d302c1182e17b30e1a87d283475f356db80b903602d1976871 D:\CI\PR-43103\3\daemon\windowsfilter\d14484dbcaa7e7d302c1182e17b30e1a87d283475f356db80b903602d1976871-removing: Access is denied.: failed to remove d14484dbcaa7e7d302c1182e17b30e1a87d283475f356db80b903602d1976871
    --- FAIL: TestDockerSuite/TestCpRelativePath (3.01s)

thaJeztah · 2022-01-11T15:10:28Z

win-2022/c8d failed to build the Dockerfile (again, similar failiures as I've seen elsewhere with permission denied on renaming files);

INFO: Downloading git...
INFO: Downloading go...
INFO: Downloading compiler 1 of 3...
INFO: Downloading compiler 2 of 3...
INFO: Downloading compiler 3 of 3...
INFO: Extracting git...
Move-Item : Access to the path 'C:\git-tmp\tools\usr' is denied.
At line:1 char:2427
+ ... tory C:\git | Out-Null; Move-Item C:\git-tmp\tools\* C:\git\.; Remove ...
+                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : WriteError: (C:\git-tmp\tools\usr:DirectoryInfo) 
    [Move-Item], IOException
    + FullyQualifiedErrorId : MoveDirectoryItemIOError,Microsoft.PowerShell.Commands.MoveItemCommand

win-2022; similar failure, but during a test:

=== FAIL: github.com/docker/docker/integration-cli TestDockerSuite/TestCreateArgs (0.24s)
    check_test.go:170: assertion failed: error is not nil: Error response from daemon: container 246731ba67ccd21726ce31502329ce11628a20b35dc7fe2f8a42084e39882154: driver "windowsfilter" failed to remove root filesystem: failed to detach VHD: failed to detach virtual disk: The device is not ready.: rename D:\CI\PR-43103\5\daemon\windowsfilter\246731ba67ccd21726ce31502329ce11628a20b35dc7fe2f8a42084e39882154 D:\CI\PR-43103\5\daemon\windowsfilter\246731ba67ccd21726ce31502329ce11628a20b35dc7fe2f8a42084e39882154-removing: Access is denied.: failed to remove 246731ba67ccd21726ce31502329ce11628a20b35dc7fe2f8a42084e39882154
    --- FAIL: TestDockerSuite/TestCreateArgs (0.24s)

win-2022: known flaky test:

=== FAIL: github.com/docker/docker/integration-cli TestDockerSuite/TestSlowStdinClosing (0.91s)
    docker_cli_run_test.go:4177: assertion failed: error is not nil: exit status 127
    --- FAIL: TestDockerSuite/TestSlowStdinClosing (0.91s)

windows rs5 timed out after 2 hours

thaJeztah · 2022-01-20T10:18:52Z

@tonistiigi @tianon PTAL

thaJeztah · 2022-01-20T19:57:12Z

daemon/images/image_import.go

+		if platform.Architecture == "" {
+			// TODO platforms.Normalize() only sets default OS, but does not set default Architecture if missing.
+			platform.Architecture = runtime.GOARCH
+		}


Discussing with @tonistiigi to remove this part, and make it the caller's responsibility to fill in the fields

thaJeztah · 2022-01-20T20:01:20Z

daemon/images/image_import.go

+			// TODO platforms.Normalize() only sets default OS, but does not set default Architecture if missing.
+			platform.Architecture = runtime.GOARCH
+		}
+		p = platforms.Normalize(*platform)


This can also be removed (assuming that the caller has called Parse()

thaJeztah · 2022-01-20T20:36:30Z

@tonistiigi updated as discussed

tonistiigi · 2022-01-20T20:50:31Z

daemon/images/image_import.go

+	// Normalize platform - default to the operating system and architecture if not supplied.
+	var p specs.Platform
+	if platform == nil {
+		p = platforms.DefaultSpec()


you need to set platform

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]>

tianon

👍

thaJeztah · 2022-01-24T18:51:34Z

Windows failure is unrelated (known flaky test)

=== RUN   TestDockerSuite/TestSlowStdinClosing
    docker_cli_run_test.go:4177: assertion failed: error is not nil: exit status 127
    --- FAIL: TestDockerSuite/TestSlowStdinClosing (4.54s)

thaJeztah · 2022-01-24T18:51:47Z

@tonistiigi PTAL

paralin · 2022-08-04T05:36:51Z

This seems to still ignore the --platform in docker import with linux/386 as the platform... it shows up as amd64.

pfuntner · 2022-08-04T14:34:34Z

I also still see the problem with Docker version 20.10.14, build a224086 on Ubuntu 20

thaJeztah · 2022-08-04T14:37:38Z

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.

thaJeztah added status/2-code-review kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. impact/changelog area/images impact/documentation labels Dec 23, 2021

thaJeztah added this to the 21.xx milestone Dec 23, 2021

thaJeztah force-pushed the image_import_platform branch from 7e27502 to bb85375 Compare December 23, 2021 16:16

samuelkarp approved these changes Dec 23, 2021

View reviewed changes

thaJeztah mentioned this pull request Jan 3, 2022

Docker cant find local images on macbook m1 docker/cli#3286

Open

tonistiigi reviewed Jan 7, 2022

View reviewed changes

thaJeztah force-pushed the image_import_platform branch from bb85375 to 9becef6 Compare January 10, 2022 15:11

thaJeztah commented Jan 10, 2022

View reviewed changes

thaJeztah force-pushed the image_import_platform branch from 9becef6 to 18c43d7 Compare January 12, 2022 17:33

thaJeztah commented Jan 20, 2022

View reviewed changes

thaJeztah force-pushed the image_import_platform branch from 18c43d7 to 046c942 Compare January 20, 2022 20:34

tonistiigi reviewed Jan 20, 2022

View reviewed changes

thaJeztah added 4 commits January 24, 2022 17:00

api: postImagesCreate(): use local variable for platform

9839ddd

Signed-off-by: Sebastiaan van Stijn <[email protected]>

integration-cli: TestImportBadURL: use proper rfc6761 "invalid domain"

6b69de6

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]>

thaJeztah force-pushed the image_import_platform branch from 046c942 to 01ae952 Compare January 24, 2022 17:25

tianon approved these changes Jan 24, 2022

View reviewed changes

tonistiigi approved these changes Jan 24, 2022

View reviewed changes

thaJeztah merged commit 559ff0a into moby:master Jan 24, 2022

thaJeztah deleted the image_import_platform branch January 24, 2022 19:29

thaJeztah mentioned this pull request Jan 27, 2022

[BUG] platform argument ignored when running docker import on different arch docker/cli#3408

Closed

mikesep mentioned this pull request Oct 17, 2022

docker image import does not seem to respect --platform docker/for-mac#6361

Closed

3 tasks

huoqifeng mentioned this pull request Oct 20, 2023

Build arch build in docker buildx separately in GHA runner confidential-containers/cloud-api-adaptor#1521

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for platform (os and architecture) on image import #43103

Add support for platform (os and architecture) on image import #43103

thaJeztah commented Dec 23, 2021 •

edited

Loading

samuelkarp left a comment

thaJeztah commented Dec 23, 2021

thaJeztah commented Dec 23, 2021

tonistiigi Jan 7, 2022

thaJeztah Jan 10, 2022

tonistiigi Jan 12, 2022

thaJeztah Jan 12, 2022

tonistiigi Jan 7, 2022

thaJeztah Jan 10, 2022

thaJeztah commented Jan 10, 2022

thaJeztah Jan 10, 2022

thaJeztah commented Jan 10, 2022

thaJeztah commented Jan 11, 2022

thaJeztah commented Jan 20, 2022

thaJeztah Jan 20, 2022

thaJeztah Jan 20, 2022

thaJeztah commented Jan 20, 2022

tonistiigi Jan 20, 2022

tianon left a comment

thaJeztah commented Jan 24, 2022

thaJeztah commented Jan 24, 2022

paralin commented Aug 4, 2022

pfuntner commented Aug 4, 2022

thaJeztah commented Aug 4, 2022

	func Normalize(platform specs.Platform) specs.Platform {
	platform.OS = normalizeOS(platform.OS)
	platform.Architecture, platform.Variant = normalizeArch(platform.Architecture, platform.Variant)

	func normalizeOS(os string) string {
	if os == "" {
	return runtime.GOOS
	}

Add support for platform (os and architecture) on image import #43103

Add support for platform (os and architecture) on image import #43103

Conversation

thaJeztah commented Dec 23, 2021 • edited Loading

samuelkarp left a comment

Choose a reason for hiding this comment

thaJeztah commented Dec 23, 2021

thaJeztah commented Dec 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Jan 10, 2022

Choose a reason for hiding this comment

thaJeztah commented Jan 10, 2022

thaJeztah commented Jan 11, 2022

thaJeztah commented Jan 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Jan 20, 2022

Choose a reason for hiding this comment

tianon left a comment

Choose a reason for hiding this comment

thaJeztah commented Jan 24, 2022

thaJeztah commented Jan 24, 2022

paralin commented Aug 4, 2022

pfuntner commented Aug 4, 2022

thaJeztah commented Aug 4, 2022

thaJeztah commented Dec 23, 2021 •

edited

Loading