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

build our docker images for multiple platforms #1059

Merged
merged 1 commit into from
Nov 7, 2024
Merged

Conversation

jmhodges
Copy link
Contributor

@jmhodges jmhodges commented Nov 7, 2024

Following this documentation:
https://docs.docker.com/build/ci/github-actions/multi-platform/

This clears up a warning in our build, and makes it easier for folks on
arm64 (like us macOS people) to pull and use the images.

Along the way, I also fixed the warnings about using lowercase as when
we use uppercase commands in our Dockerfile.

Following this documentation:
https://docs.docker.com/build/ci/github-actions/multi-platform/

This clears up a warning in our build, and makes it easier for folks on
arm64 (like us macOS people) to pull and use the images.

Along the way, I also fixed the warnings about using lowercase `as` when
we use uppercase commands in our Dockerfile.
@jmhodges jmhodges marked this pull request as ready for review November 7, 2024 18:10
@jmhodges jmhodges requested review from a team as code owners November 7, 2024 18:10
@jmhodges jmhodges requested review from say-yawn and removed request for a team November 7, 2024 18:10
@jmhodges jmhodges requested a review from jbuck November 7, 2024 19:06
Comment on lines +23 to +25
- name: Set up QEMU
uses: docker/setup-qemu-action@v3

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is optional if we want to support more platforms and it's recommended we use multiple native nodes or cross-compilation.

It seems the cross-compilation might be better here and it was what we were attempting but having problems. Wondering if:

Set the GOOS and GOARCH environment variables to the values of TARGETOS and TARGETARCH. The Go compiler uses these variables to do cross-compilation.

would have resolved that (example code)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh just checked the errors and they are:

 5 warnings found (use docker --debug to expand):
 - FromAsCasing: 'as' and 'FROM' keywords' casing do not match (line 59)
 - FromPlatformFlagConstDisallowed: FROM --platform flag should not use constant value "linux/amd64" (line 7)
 - JSONArgsRecommended: JSON arguments recommended for CMD to prevent unintended behavior related to OS signals (line 86)
 - FromAsCasing: 'as' and 'FROM' keywords' casing do not match (line 7)
 - FromAsCasing: 'as' and 'FROM' keywords' casing do not match (line 41)

- FromPlatformFlagConstDisallowed: FROM --platform flag should not use constant value "linux/amd64" (line 7)
It's complaining because we should be using pre-defined BUILDPLATFORM variable

Copy link
Contributor Author

@jmhodges jmhodges Nov 7, 2024

Choose a reason for hiding this comment

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

Considered it, but it would require us to also be using arm64 libraries and os in the container itself.

First, we'd have to build libkmsp11 ourselves. The pre-built binary library Google releases has only amd64 builds. Building it ourself would mean having the team learn bazel and good bit more code in the Dockerfile.

Second, even though we don't have any other libraries that we build by hand, we don't actually run in arm64 in production right now. So, we'd have a different arch in our images on our local machines than what we use in prod. Bugs or just different behavior could lurk there.

Third, even with a pre-built arm library, we'd have to special case the dockerfile to work with it.

You didn't mention this, but the docs say the qemu stuff might slow our builds but I don't actually see that when comparing this PR's build image step vs a previous pr's.

So, I think we should stick with the qemu stuff until we have a good reason to move to arm64 (and there are some energy considerations we could make in the future! But we'd need some arm64 nodepools added to Mozilla's kubernetes cluster)

Copy link
Contributor

@say-yawn say-yawn Nov 7, 2024

Choose a reason for hiding this comment

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

Cool, thanks for the explanation. Yes, I did read that the recommendation came with dealing with the slowness, but your other reasons and there being no build time difference makes sense to move forward :shipit:

@jmhodges jmhodges requested a review from say-yawn November 7, 2024 23:50
Copy link
Contributor

@say-yawn say-yawn left a comment

Choose a reason for hiding this comment

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

Tested my local (MacOS) build works and warnings have been removed :shipit:

@jmhodges jmhodges merged commit 9329ceb into main Nov 7, 2024
14 of 15 checks passed
@jmhodges jmhodges deleted the multiarch-docker-push branch November 7, 2024 23:58
jmhodges added a commit to mozilla-services/autograph-edge that referenced this pull request Nov 19, 2024
This is a port of
mozilla-services/autograph#1063 (and
mozilla-services/autograph#1060).

Currently, we're doing a deploy to one of our staging environments on
every merge to main this repo. That wasn't desired.

Along the way, we also build arm64 versions of the docker image for
convenience. See mozilla-services/autograph#1059
jmhodges added a commit to mozilla-services/autograph-edge that referenced this pull request Nov 19, 2024
This is a port of
mozilla-services/autograph#1063 (and
mozilla-services/autograph#1060).

Currently, we're doing a deploy to one of our staging environments on
every merge to main this repo. That wasn't desired.

Along the way, we also build arm64 versions of the docker image for
convenience. See mozilla-services/autograph#1059
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.

3 participants