Skip to content

c8d/snapshot: Create any platform if not specified#47167

Merged
vvoland merged 1 commit intomoby:masterfrom
vvoland:c8d-prefer-default-platform-snapshot
Jan 23, 2024
Merged

c8d/snapshot: Create any platform if not specified#47167
vvoland merged 1 commit intomoby:masterfrom
vvoland:c8d-prefer-default-platform-snapshot

Conversation

@vvoland
Copy link
Contributor

@vvoland vvoland commented Jan 22, 2024

With containerd snapshotters enabled docker run currently fails when creating a container from an image that doesn't have the default host platform without an explicit --platform selection:

$ docker run image:amd64
Unable to find image 'asdf:amd64' locally
docker: Error response from daemon: pull access denied for asdf, repository does not exist or may require 'docker login'.
See 'docker run --help'.

This is confusing and the graphdriver behavior is much better here, because it runs whatever platform the image has, but prints a warning:

$ docker run image:amd64
WARNING: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested

This commits changes the containerd snapshotter behavior to be the same as the graphdriver. This doesn't affect container creation when platform is specified explicitly.

$ docker run --rm --platform linux/arm64 asdf:amd64
Unable to find image 'asdf:amd64' locally
docker: Error response from daemon: pull access denied for asdf, repository does not exist or may require 'docker login'.
See 'docker run --help'.

- What I did

- How I did it

- How to verify it

- Description for the changelog

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

With containerd snapshotters enabled `docker run` currently fails when
creating a container from an image that doesn't have the default host
platform without an explicit `--platform` selection:

```
$ docker run image:amd64
Unable to find image 'asdf:amd64' locally
docker: Error response from daemon: pull access denied for asdf, repository does not exist or may require 'docker login'.
See 'docker run --help'.
```

This is confusing and the graphdriver behavior is much better here,
because it runs whatever platform the image has, but prints a warning:

```
$ docker run image:amd64
WARNING: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested
```

This commits changes the containerd snapshotter behavior to be the same
as the graphdriver. This doesn't affect container creation when platform
is specified explicitly.

```
$ docker run --rm --platform linux/arm64 asdf:amd64
Unable to find image 'asdf:amd64' locally
docker: Error response from daemon: pull access denied for asdf, repository does not exist or may require 'docker login'.
See 'docker run --help'.
```

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvoland vvoland added status/2-code-review kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. containerd-integration Issues and PRs related to containerd integration area/ux labels Jan 22, 2024
@vvoland vvoland added this to the 26.0.0 milestone Jan 22, 2024
@vvoland vvoland self-assigned this Jan 22, 2024
@vvoland vvoland requested a review from rumpl January 22, 2024 15:23
Copy link
Member

@rumpl rumpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s add a test

@thaJeztah
Copy link
Member

Hm. Probably flaky;

=== FAIL: github.com/docker/docker/integration/container TestCopyFromContainerPathIsNotDir (0.53s)
    copy_test.go:47: assertion failed: expected an error, got nil

@vvoland
Copy link
Contributor Author

vvoland commented Jan 22, 2024

Not flaky, just a Windows test that's not passing (yet) with c8d.

@thaJeztah
Copy link
Member

oh! it has the containerd-integration label set. didn't think of that 😅

Copy link
Member

@rumpl rumpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s get this one in but think of a nice way to tests these things

Copy link
Member

@laurazard laurazard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vvoland
Copy link
Contributor Author

vvoland commented Jan 23, 2024

Let me get this in, I'll add a test in a follow up.

@vvoland vvoland merged commit cac52f7 into moby:master Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/ux containerd-integration Issues and PRs related to containerd integration kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. process/cherry-picked status/2-code-review

Projects

Development

Successfully merging this pull request may close these issues.

4 participants