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 multi-architecture images #197

Merged
merged 27 commits into from
Jan 12, 2024
Merged

Build multi-architecture images #197

merged 27 commits into from
Jan 12, 2024

Conversation

4141done
Copy link
Collaborator

@4141done 4141done commented Jan 9, 2024

What

Allows us to push multi-architecture builds for linux/amd64 and linux/arm64 on merge to master. Fixes: #196

The flow looks like this:

                                   ┌──────────────────┐        ┌────────────────┐      ┌────────────────┐
    ┌─────────┐     ┌─────────┬────► Build Latest NJS ├────────►Test Latest NJS ├─────►│Push Latest NJS │
    │Build OSS├────►│Test OSS │    └──────────────────┘        └────────────────┘      └────────────────┘
    └─────────┘     └──┬──────┤
                       │      │    ┌──────────────────┐       ┌──────────────────┐     ┌─────────────────┐
                       │      └────►Build Unprivileged├───────►Test Unprivileged ├────►│Push Unprivileged│
                       │           └──────────────────┘       └──────────────────┘     ├────────┬────────┘
                       │                                                               ├────────┤
                       └──────────────────────────────────────────────────────────────►│Push OSS│
                                                                                       └────────┘


Tests are performed by building the relevant image and tagging it with the image name the tests expect. This follows the original implementation in test.sh. This will likely change to an explicit image name specification as we move the test suite in to Javascript but it's being maintained for now in the interest of simplicity.

Goals

  • To decouple image build from the test run. This will help as we refactor the test suite for this issue Translate integration test suite to Javacript #191.
  • Not have CI take a long time due to multi-architecture builds for interim pushes to pull requests.
  • CI should fail fast if any of the image builds or tests fail
  • Parallelize builds as much as possible.

Notes

  • Since the unprivileged and latest-njs images build off the base oss image, there were some strange moves that had to be done to make that work without rebuilding the base image every time. Specifically, we had to set setup-buildx-action to run in docker driver mode so we can simply load the base image. Otherwise we would have had to use a local repository. However, in docker mode upload-artifact doesn't like the file produced so we have to save the file again.
  • I wanted to build images once and then run tests against them and push at the end. However, there was not a clean way to get the full multi-architecture images all the way to the push step so I just build them again against all architectures and perform tests in the runner architecture. This saves us from needing conditionals in the test portions.

@4141done 4141done changed the title [WIP] trial staged build with github actions tooling Build multi-architecture images Jan 11, 2024
@4141done 4141done marked this pull request as ready for review January 11, 2024 22:37
@4141done 4141done requested a review from dekobon January 11, 2024 22:38
@4141done 4141done mentioned this pull request Jan 11, 2024
# A workflow run is made up of one or more jobs that can run sequentially or in parallel

env:
CI: true
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

annotation: This is temporary until we get the whole test suite refactored. For now we skip the image build step in the test script since this pipeline ensures that the correct images are available.

uses: actions/upload-artifact@v3
with:
name: oss
path: /tmp/oss.tar
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

annotation: The other two images use this as a base. Therefore we export the built image for use in:

  • The tests for this image
  • The build process for the latest-njs and unprivileged images

- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v3
with:
driver: docker
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

annotation:
If we use the docker-container driver that buildx uses by default, a docker load does not get the image in a context where the build process can access it. In order to do this you must use a local registry as detailed in this guide: https://docs.docker.com/build/ci/github-actions/named-contexts/#using-with-a-container-builder

Comment on lines +260 to +264
tags: |
ghcr.io/${{ github.repository }}/nginx-oss-s3-gateway:latest-${{ steps.date.outputs.date }}
ghcr.io/${{ github.repository }}/nginx-oss-s3-gateway:latest
nginxinc/nginx-s3-gateway:latest-${{ steps.date.outputs.date }}
nginxinc/nginx-s3-gateway:latest
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

annotation:
This is nice, the plugin lets you log in to both registries then pushes tot the correct one based on the tag.

context: .
push: true
platforms: linux/amd64,linux/arm64
provenance: false
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

annotation:
This is a little workaround for the github packages repo. The provenance file is a docker thing not an OCI thing so you'll get an unknown/unknown architecture listed in gh packages if you don't set this.

ref: https://docs.docker.com/build/attestations/slsa-provenance/

If we want these to go to dockerhub, we could also separate the builds again but I think the simplicity of having them both push is more desireable.

@4141done 4141done requested a review from ciroque January 11, 2024 22:49
Copy link
Collaborator

@dekobon dekobon 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. I'm looking forward to seeing this merged.

uses: actions/download-artifact@v3
with:
name: oss
path: /tmp
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would check to see if github actions has defined an environment variable for the temp path. Generally, it isn't great to assume that /tmp is the system temp directory. Yes, I know it is in this case and this is a nit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for this note. I discovered ${{ runner.temp }} which serves the purpose well.

uses: actions/upload-artifact@v3
with:
name: unprivileged
path: /tmp/unprivileged.tar
Copy link
Collaborator

Choose a reason for hiding this comment

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

Refresh my memory if the uploaded artifacts eventually get cleaned up. I have a vague memory that they do.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's a default setting for the repo which I believe is 30 days. However based on this comment I'm going to update the retention explicitly since so it's obvious from the script.

retention-days: 1 One day is the minimum which is really too long for this but should serve us fine.

Also worth noting that artifacts are only accessible from the workflow in which they were created so there's no chance of us pulling an old artifact which I was worried about initially.

@4141done 4141done merged commit 98f31ef into master Jan 12, 2024
7 checks passed
4141done added a commit that referenced this pull request Jan 13, 2024
…200)

The original pull request #197 had an issue where the final build and push could not find the base image locally.

This change corrects the issue but introducing a local registry to the final build push step as described in https://docs.docker.com/build/ci/github-actions/named-contexts/#using-with-a-container-builder since the `docker-container` driver-based build can't load from the local docker environment.
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.

Arm64 build
2 participants