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

Don't pass --platform to docker/podman #2511

Merged
merged 2 commits into from Jan 3, 2023
Merged

Don't pass --platform to docker/podman #2511

merged 2 commits into from Jan 3, 2023

Conversation

alexcb
Copy link
Collaborator

@alexcb alexcb commented Dec 15, 2022

The --platform flag is only ever passed to docker/podman when both the
user and server platform values are equal. When they mismatch, the
--platform flag is ommitted, and the server's native platform is used.

Since the --platform value is only ever passed when the user and
server platforms are equal, it shouldn't matter; however in practice
there is a podman bug, which causes a pull to occur whenver the
--platform flag is specified: containers/podman#15711

This bug will cause podman to always pull the earthly/buildkitd image from
docker hub, which will either 1) overwrite the local image if the image exists in docker hub,
and ultimately will cause our tests to run against an incorrect image
version, or 2) result in the following 404-error if the tag does not
exist:

exit status 125: Trying to pull docker.io/earthly/buildkitd:dev-HEAD...
Error: initializing source docker://earthly/buildkitd:dev-HEAD: reading manifest dev-HEAD in docker.io/earthly/buildkitd: manifest unknown: manifest unknown: exit status 125

This commit additionally updates the podman tests to use capsh rather
than grep for the RUN --privileged tests, as the Effective capabilities (CapEff)
bits are not always the same between docker and podman

Signed-off-by: Alex Couture-Beil alex@earthly.dev

@alexcb alexcb changed the title [ignore] debugging [ignore] debugging podman test failures Dec 16, 2022
@alexcb alexcb force-pushed the acb/podman-fix branch 10 times, most recently from d1123c0 to 28242f3 Compare December 21, 2022 19:41
@alexcb alexcb force-pushed the acb/podman-fix branch 3 times, most recently from dfd6a02 to 78a7c9a Compare December 29, 2022 21:30
@alexcb alexcb changed the title [ignore] debugging podman test failures Don't pass --platform to docker/podman Dec 29, 2022
@alexcb alexcb force-pushed the acb/podman-fix branch 3 times, most recently from bf316ab to 968e811 Compare December 29, 2022 23:06
The `--platform` flag is only ever passed to docker/podman when both the
user and server platform values are equal. When they mismatch, the
`--platform` flag is ommitted, and the server's native platform is used.

Since the `--platform` value is only ever passed when the user and
server platforms are equal, it shouldn't matter; however in practice
there is a podman bug, which causes a pull to occur whenver the
`--platform` flag is specified: containers/podman#15711

This bug will cause podman to always pull the earthly/buildkitd image from
docker hub, which will either 1) overwrite the local image if the image exists in docker hub,
and ultimately will cause our tests to run against an incorrect image
version, or 2) result in the following 404-error if the tag does not
exist:

    exit status 125: Trying to pull docker.io/earthly/buildkitd:dev-HEAD...
    Error: initializing source docker://earthly/buildkitd:dev-HEAD: reading manifest dev-HEAD in docker.io/earthly/buildkitd: manifest unknown: manifest unknown: exit status 125

This commit additionally updates the podman tests to use capsh rather
than grep for the `RUN --privileged` tests, as the Effective capabilities (CapEff)
bits are not always the same between docker and podman

Signed-off-by: Alex Couture-Beil <alex@earthly.dev>
@alexcb
Copy link
Collaborator Author

alexcb commented Dec 30, 2022

I tested this on the m1:

under /tmp/uname/Earthfile:

FROM alpine
RUN uname -a

Then I ran:

./earthly +for-darwin-m1 && ./build/darwin/arm64/earthly +for-darwin-m1
export DOCKER_HOST=ssh://alex@<ip>:<magic-port>
./build/darwin/arm64/earthly --verbose /tmp/uname+base

which produced

/tmp/uname+base | Linux buildkitsandbox 5.19.0-26-generic #27-Ubuntu SMP PREEMPT_DYNAMIC Wed Nov 23 20:44:15 UTC 2022 x86_64 Linux

@alexcb alexcb marked this pull request as ready for review December 30, 2022 18:48
@alexcb
Copy link
Collaborator Author

alexcb commented Dec 30, 2022

and when running on the m1 without remote docker host, I get linux/arm64:

Linux buildkitsandbox 5.15.49-linuxkit #1 SMP PREEMPT Tue Sep 13 07:51:32 UTC 2022 aarch64 Linux

Copy link
Contributor

@nelsam nelsam left a comment

Choose a reason for hiding this comment

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

Everything seems reasonable to my (untrained) eyes. I particularly like the capsh changes to scrap the magic hex value.

Copy link
Contributor

@r-LaForge r-LaForge left a comment

Choose a reason for hiding this comment

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

Relying on the default behaviour (letting the Docker / Podman server choose the platform) makes sense.

The first change (only adding platform if they matched) was a conservative change since I was new to the area. I don't recall there being a better reason.

There is another tiny thing we could cleanup in this PR; the FrontendInformation function is being added to the shellFrontend structure and we no longer need it.

FrontendInformation func(ctx context.Context) (*FrontendInfo, error)

@alexcb alexcb merged commit 799508d into main Jan 3, 2023
@alexcb alexcb deleted the acb/podman-fix branch January 3, 2023 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants