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

CI: Add build for linux/arm64 #66

Closed
wants to merge 2 commits into from

Conversation

iainlane
Copy link
Member

@iainlane iainlane commented May 3, 2023

This builds and pushes a multi-arch Docker manifest for amd64 and arm64.

We use Docker's provided action to do this using qemu emulation, and switch to using this to push the images for us.

Split the build/test of the xk6 binary out to a separate job too, so that can also be run on arm64.

@iainlane iainlane force-pushed the iainlane/docker-arm64 branch 6 times, most recently from 95712c4 to 5ea72b7 Compare May 4, 2023 17:28
This builds and pushes a multi-arch Docker manifest for amd64 and arm64.

We use Docker's provided action to do this using qemu emulation, and
switch to using this to push the images for us.

Split the build/test of the `xk6` binary out to a separate job too, so
that can also be run on arm64.
@iainlane
Copy link
Member Author

iainlane commented May 4, 2023

Example runs where I had hacked the action to work on pushes to iainlane/*:

  • master
  • tag (gets latest and 1.0.0, the v is dropped)
  • rc (gets 1.0.0-rc.1 but not latest)

Expand "Set up tags" -> "Docker tags" to see what each run tagged.

See the resulting pushed container images:

@iainlane iainlane marked this pull request as ready for review May 4, 2023 17:54
@iainlane iainlane requested a review from imiric May 4, 2023 17:54
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

Hey, thanks for this! 🙇

Using actions instead of doing this manually certainly helps, though I liked the clarity of raw commands 😄

The main blocker for me would be figuring out how to do the build-test-push workflow, instead of building and pushing in the same step.

.github/workflows/release.yml Show resolved Hide resolved
Comment on lines +77 to +83
- name: Build image
uses: docker/build-push-action@v4
with:
context: .
platforms: linux/amd64,linux/arm64
push: true
tags: ${{ steps.meta.outputs.tags }}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have conditionals on all of these steps, and maybe on the overall job, to only do this on master or tagged commits.

Copy link
Member Author

Choose a reason for hiding this comment

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

The overall workflow has a condition like that, is that enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, right, I missed it. So the conditionals on the Log into steps can be removed, right?

.github/workflows/release.yml Outdated Show resolved Hide resolved
Comment on lines -18 to -23
- name: Checkout code
uses: actions/checkout@v3
with:
fetch-depth: 0
- name: Build image
run: docker build -t "$IMAGE_REPOSITORY" .
Copy link
Contributor

Choose a reason for hiding this comment

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

Without building the image here, the image used below to build k6 would be grafana/xk6:latest. So there's not much point in always testing that particular image.

What we want to do is to test the same image we've just built, before we push it to the registries. From my limited experience with docker buildx, that's apparently not possible, since it only exposes a --push flag to build and push with the same command. This is probably why the action is named docker/build-push-action, since it's not possible to split these two steps.

It also doesn't seem possible to run docker buildx without --push, and then run docker push, since that only pushes the amd64 image, and not the arm64 one. This is the behavior we've noticed in grafana/k6#3015.

If you have any ideas for how to structure a multi-arch workflow in a way that we can build, test and push, in that order, we'd appreciate it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried a bit when making the PR - you can load the image into the docker daemon so it's available in later steps, but this doesn't work with multiarch manifests for some annoying reason (you get an error). Didn't seem to be a workaround for that from the brief searching I did.

What about running the tests from the Dockerfile so building is actually building + testing? Would that be okay?

Alternatively you could probably apply the matrix to the Docker builds themselves. That is: build the images in separate jobs, push foo-ARCH tags to the registry, then have a final job which is dependent on all of the builds succeeding which builds and pushes the manifest. It's a bit more work and I don't know when I'd have time to come back and do that if we decided to go down that route, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Both of your suggestions are interesting, thanks.

What about running the tests from the Dockerfile so building is actually building + testing? Would that be okay?

Ideally, we'd want to test the exact image that users will use, but this could be a reasonable alternative. We just need to make sure to do the test in the first build stage, so that the cached packages used in the test are not part of the final image.

Alternatively you could probably apply the matrix to the Docker builds themselves. That is: build the images in separate jobs, push foo-ARCH tags to the registry, then have a final job which is dependent on all of the builds succeeding which builds and pushes the manifest. It's a bit more work and I don't know when I'd have time to come back and do that if we decided to go down that route, though.

True, this could be an option as well. I think this approach is cleaner and would be our preferred solution, but you're right that it's more complicated to setup.

Practically speaking, either approach would be transparent for users, so if you're short on time, let's go with the first approach for now. The benefit of having ARM images outweighs any potential problems, and I'll create an issue to implement the second approach, which we can address eventually.

Thanks again!

.github/workflows/release.yml Outdated Show resolved Hide resolved
Comment on lines -18 to -23
- name: Checkout code
uses: actions/checkout@v3
with:
fetch-depth: 0
- name: Build image
run: docker build -t "$IMAGE_REPOSITORY" .
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried a bit when making the PR - you can load the image into the docker daemon so it's available in later steps, but this doesn't work with multiarch manifests for some annoying reason (you get an error). Didn't seem to be a workaround for that from the brief searching I did.

What about running the tests from the Dockerfile so building is actually building + testing? Would that be okay?

Alternatively you could probably apply the matrix to the Docker builds themselves. That is: build the images in separate jobs, push foo-ARCH tags to the registry, then have a final job which is dependent on all of the builds succeeding which builds and pushes the manifest. It's a bit more work and I don't know when I'd have time to come back and do that if we decided to go down that route, though.

Comment on lines +77 to +83
- name: Build image
uses: docker/build-push-action@v4
with:
context: .
platforms: linux/amd64,linux/arm64
push: true
tags: ${{ steps.meta.outputs.tags }}
Copy link
Member Author

Choose a reason for hiding this comment

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

The overall workflow has a condition like that, is that enough?

Co-authored-by: Ivan Mirić <ivan@imiric.com>
@pablochacin
Copy link
Collaborator

Closing due to staleness. If this is required we need to open a new PR aligned with the current vision of the tooling ecosystem

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.

4 participants