Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

check local image matches the required platform #10546

Merged
merged 1 commit into from May 10, 2023

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented May 10, 2023

What I did
Added a verification that a local image found for service matches the configured platform. Otherwise report error so user can pull the desired image. Please note we can't just force pull as docker engine image store (until configured to run in "containerd image store" mode with Docker Desktop) only stores a single image for a tag.

Related issue
fixes #9740

(not mandatory) A picture of a cute animal, if possible in relation to what you did

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@ndeloof ndeloof requested review from a team, nicksieger, StefanScherer, ulyssessouza, glours, milas and laurazard and removed request for a team May 10, 2023 08:48
@codecov
Copy link

codecov bot commented May 10, 2023

Codecov Report

Patch coverage: 10.00% and project coverage change: -0.14 ⚠️

Comparison is base (0c1a691) 59.81% compared to head (f25e7a8) 59.67%.

Additional details and impacted files
@@            Coverage Diff             @@
##               v2   #10546      +/-   ##
==========================================
- Coverage   59.81%   59.67%   -0.14%     
==========================================
  Files         107      107              
  Lines        9294     9312      +18     
==========================================
- Hits         5559     5557       -2     
- Misses       3166     3185      +19     
- Partials      569      570       +1     
Impacted Files Coverage Δ
pkg/compose/build.go 73.05% <10.00%> (-3.58%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

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

Looks Good To Me #sorryPrivateJoke

@laurazard
Copy link
Member

laurazard commented May 10, 2023

@ndeloof right, this is definitely better than what we had before. I wonder though if we should align with engine behaviour:

$ docker pull --platform=linux/arm64 alpine:3.17
3.17: Pulling from library/alpine
c41833b44d91: Already exists
Digest: sha256:124c7d2707904eea7431fffe91522a01e5a861a624ee31d03372cc1d138a3126
Status: Downloaded newer image for alpine:3.17
docker.io/library/alpine:3.1

$ docker run --platform=linux/amd64 --rm -it alpine:3.17 sh
Unable to find image 'alpine:3.17' locally
3.17: Pulling from library/alpine
f56be85fc22e: Pull complete
Digest: sha256:124c7d2707904eea7431fffe91522a01e5a861a624ee31d03372cc1d138a3126
Status: Downloaded newer image for alpine:3.17
/ # exit

$ docker images
REPOSITORY       TAG       IMAGE ID       CREATED        SIZE
alpine           3.17      9ed4aefc74f6   5 weeks ago    7.05MB
alpine           <none>    51e60588ff2c   5 weeks ago    7.46M

$ docker inspect alpine:3.17
[
    {
        "Id": "sha256:9ed4aefc74f6792b5a804d1d146fe4b4a2299147b0f50eaf2b08435d7b38c27e",
        [...],
        "RepoTags": [
            "alpine:3.17"
        ],
        "RepoDigests": [],
        "Architecture": "amd64",
        [...],
    }
]

$ docker inspect 51e60588ff2c
[
    {
        "Id": "sha256:51e60588ff2cd9f45792b23de89bfface0a7fbd711d17c5f5ce900a4f6b16260",
        [...],
        "RepoTags": [],
        "RepoDigests": [
            "alpine@sha256:124c7d2707904eea7431fffe91522a01e5a861a624ee31d03372cc1d138a3126
        ],
        "Architecture": "arm64",
        "Variant": "v8",
        [...],
    }
]

@ndeloof
Copy link
Contributor Author

ndeloof commented May 10, 2023

I don't strictly have the same behavior, I get a WARNING:

$ docker pull --platform=linux/arm64 alpine:3.17
3.17: Pulling from library/alpine
Digest: sha256:124c7d2707904eea7431fffe91522a01e5a861a624ee31d03372cc1d138a3126
Status: Downloaded newer image for alpine:3.17
docker.io/library/alpine:3.17

$ docker run --platform=linux/amd64 --rm -it alpine:3.17 sh
Unable to find image 'alpine:3.17' locally
3.17: Pulling from library/alpine
f56be85fc22e: Pull complete 
Digest: sha256:124c7d2707904eea7431fffe91522a01e5a861a624ee31d03372cc1d138a3126
Status: Downloaded newer image for alpine:3.17
WARNING: image with reference alpine was found but does not match the specified platform: wanted linux/amd64, actual: linux/arm64/v8
/ #

Anyway we probably can do something comparable, just considering image is missing and get it pulled. Let me give this a try

@ndeloof
Copy link
Contributor Author

ndeloof commented May 10, 2023

I ran some tests, we can support a comparable workflow, while this requires some additional changes so that we don't track required images by tag, but need to add platforms so we can handle some funky compose file like this one:

services:
  hello:
    image: alpine:3.17
    platform: linux/arm64
  toto:
    image: alpine:3.17
    platform: linux/amd64

if you don't mind, I'll keep this for future improvement, so that we - at least - get a correct error message reported to the user when we don't get the expected platform for requested image.

@laurazard
Copy link
Member

Sounds good! I made that suggestion but more for discussion, it's possible that we might not even want to do it (and I didn't consider Compose files like that). This is still better than the previous state.

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

@ndeloof ndeloof merged commit b776826 into docker:v2 May 10, 2023
22 of 24 checks passed
@ndeloof ndeloof deleted the check_image_platform branch May 10, 2023 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants