-
-
Notifications
You must be signed in to change notification settings - Fork 62
feat(build): use action-build-push-images #1802
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
Conversation
d12af1e
to
9c06ef6
Compare
… e2e ci" This reverts commit 9c06ef6.
|
||
assemble-ghcr: | ||
needs: [build-setup, build-image] | ||
if: "needs.build-setup.outputs.full_ci == 'true'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is now done on every commit since e2e now needs ghcr.io/getsentry/symbolicator:${{ github.sha }} because it can't download symbolicator-amd64.tar anymore
not a big deal imo, very little overhead since actually building the final docker image is really fast
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this workflow works the way you set it up. It never builds the multi arch image, which a majority of the workflow did you ended up replacing. Have you checked that the output/uploaded image is actually multi arch?
Also this workflow now no longer works on forks because it needs push permissions where previously it ran fine for external contributors.
there's no more right now the built image in ghcr has linux/amd64 and an unknown/unknown (looking into this now; i can't edit: github ui displays it as that because attestations: https://github.com/orgs/community/discussions/45969 edit 2: the unknown/unknown is just a glitch, in reality linux/arm64 wasn't being added - now fixed! https://github.com/getsentry/symbolicator/pkgs/container/symbolicator/538778621?tag=5de2c8ee9aafca6593611792bd90eb7cccf98e52 |
|
||
- *assemble | ||
image_name: 'symbolicator' | ||
platforms: linux/amd64,linux/arm64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need multiplatform image for production? Perhaps @Dav1dde knows better here, but I guess there is no harm having it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need multi arch for production and self hosted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we need to make sure multi-arch is available for self-hosted on ghcr, but are we running symbolicator on arm64 and amd64 in prod? Since this is pushing to artifact registry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but are we running symbolicator on arm64 and amd64 in prod? Since this is pushing to artifact registry
We are.
with: | ||
project_name: symbolicator | ||
image_url: symbolicator-amd64 | ||
image_url: ghcr.io/getsentry/symbolicator:${{ github.event.pull_request.head.sha || github.sha }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will still fail on forks, we may need to ensure we're building the image on forks, saving the artifact, and loading it here to ensure it works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something similar to https://github.com/getsentry/relay/blob/master/.github/workflows/ci.yml#L521-L531
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i thought we want to skip publishing (so that CI can still pass overall) if it's a run from a fork?
(then again, we lose self hosted e2e testing)
it's okay to have the permissions and push images to GHCR then? i feel like it's fine (as long as nightly
isn't published)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should definitely skip publishing for forks, I think I linked the wrong thing above over there, but it's still possible to build the image and save the image as an artifact to be used for the self-hosted e2e tests here even on a fork. Essentially that would require keeping the existing logic for
- name: Download Docker Image
uses: actions/download-artifact@v5
with:
pattern: symbolicator-image@amd64
path: /tmp
- name: Load Docker Image
run: docker load --input /tmp/symbolicator-amd64.tar
and adding a job to save the docker image as an artifact to be reused later. Here's the right link: https://github.com/getsentry/relay/blob/master/.github/workflows/ci.yml#L485-L503
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, looks like i'll have to keep that stuff around
|
||
- *assemble | ||
image_name: 'symbolicator' | ||
platforms: linux/amd64,linux/arm64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you actually tested the images you're building? You're creating a amd64 image with a arm64 binary and vice versa.
I don't think this setup can even work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshuarli This is accurate, we need to ensure that the cargo build
step is separate from building the docker image as right now we're publishing the multi arch image with the wrong binary twice. We need to ensure that the docker image build and publish happens once and copies the correct binary when building. This may require necessary changes to the dockerfile to make sure we're copying the right binary.
Example in relay: https://github.com/getsentry/relay/blob/468fc6fca10a0d29bb78f39149b1b3fa358758f9/Dockerfile.release#L8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hubertdeng123 do you think it would be feasible to adjust the build action to support assembly from other images, similar to what is done here by loading multiple different architectures? It's just unfortunate that docker still doesn't provide good tools to deal with multi arch images.
The setup here was inspired from objectstore
, so we'll have to do the same changes to that repo (eventually).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah right now both images have the amd64 binary, i need to rework this pr
docker run --platform=linux/arm64 ghcr.io/getsentry/symbolicator:55ae450aac76c48d1c12340d96282d8c0eabc09c
rosetta error: failed to open elf at /lib64/ld-linux-x86-64.so.2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dav1dde Do you mean add logic to the composite action to take existing images and assemble them into the multiplatform manifest?
Currently there exists two processes for building multiplatform images. It's not possible for us to add assembly of images to the 2nd approach since we can't add a matrix strategy to a composite action, it can only be done from the job that calls the composite action.
- Artifact build on platform specific runners, image build on one runner with emulator (relay is doing this)
action-build-and-push-images
is called once and directly published multi platform image without performance degradation since the most time consuming part of building artifacts is done on the appropriate runners- requires dockerfile to handle copying the binary with the correct platform
- Image build happens on platform specific runners, assembly is done manually at the end
action-build-and-push-images
is called twice and builds and publishes images with suffix of eitherarm64
oramd64
- assembly for multiplatform image can only happen after the two jobs are complete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently there exists two processes for building multiplatform images. It's not possible for us to add assembly of images to the 2nd approach since we can't add a matrix strategy to a composite action, it can only be done from the job that calls the composite action.
I am more thinking, let the matrix build the images (like Symbolicator/Objectstore do). And then the action-build-and-push-images
can run outside of the matrix and be just passed paths to the raw images (as they are passed around via CI artifacts).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created an item to address that, makes sense to have
superseded by #1805 |
this replaces image assembly with https://github.com/getsentry/action-build-and-push-images
the goal is to comply with the artifact management standard here: https://www.notion.so/sentry/Standard-Spec-Artifact-Management-22a8b10e4b5d80ecb6dcca3eb4558f5b