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

docker images: Multi-platform builds for docker images using self-hosted runners #135

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

dsotirakis
Copy link
Contributor

@dsotirakis dsotirakis commented May 28, 2024

image

This PR introduces a reusable workflow which can be used for multi-platform docker image builds using our self-hosted runners, to build images for both arm and amd architectures.

The reusable workflow calls a few composite actions that already exist as shared workflows.

Running example: https://github.com/grafana/grafana-ci-sandbox/actions/runs/9794036255/attempts/6

Notes:

Eventually this will be the only push to GAR workflow we will use, and will replace the already existing push-to-gar-docker action.

The workflow file can be as simple as: https://github.com/grafana/grafana-ci-sandbox/actions/runs/9794036255/workflow

Also, in case the workflow is complicated and we cannot use the reusable action as happened above, we can use the composite actions instead. Example workflow:
https://github.com/grafana/k6-cloud-github-workflows/pull/18/files
Result: https://github.com/grafana/k6-cloud-metrics-api/actions/runs/9877247789/job/27308095542

@dsotirakis dsotirakis requested a review from a team as a code owner May 28, 2024 08:47
@dsotirakis dsotirakis marked this pull request as draft May 28, 2024 08:48
@dsotirakis dsotirakis self-assigned this May 28, 2024
@dsotirakis dsotirakis force-pushed the multiarch-docker-builds-using-sh-runners branch 4 times, most recently from 1bd94df to f313444 Compare May 28, 2024 14:54
@dsotirakis dsotirakis changed the title docker images: Multi-platform builds for docker images using self-hosted runners [WIP] docker images: Multi-platform builds for docker images using self-hosted runners May 29, 2024
@dsotirakis dsotirakis force-pushed the multiarch-docker-builds-using-sh-runners branch 8 times, most recently from 5998e3c to 7aeff14 Compare June 3, 2024 15:52
@dsotirakis dsotirakis force-pushed the multiarch-docker-builds-using-sh-runners branch 5 times, most recently from 3c1eb95 to 219c6da Compare June 20, 2024 15:03
@dsotirakis dsotirakis force-pushed the multiarch-docker-builds-using-sh-runners branch 9 times, most recently from 50b3970 to 2448fb2 Compare July 5, 2024 14:09
@dsotirakis dsotirakis changed the title [WIP] docker images: Multi-platform builds for docker images using self-hosted runners docker images: Multi-platform builds for docker images using self-hosted runners Jul 5, 2024
@dsotirakis dsotirakis added the enhancement New feature or request label Jul 8, 2024
@dsotirakis dsotirakis force-pushed the multiarch-docker-builds-using-sh-runners branch 12 times, most recently from 7955733 to 0980241 Compare July 11, 2024 08:07
@dsotirakis dsotirakis marked this pull request as ready for review July 11, 2024 08:19
@dsotirakis dsotirakis force-pushed the multiarch-docker-builds-using-sh-runners branch from 3f78e9e to 879ba70 Compare July 11, 2024 14:15
default: "docker-container"

outputs:
full_image_name:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dirty but for now re-calculating the whole image name for GAR is difficult, we can get away with it using a simple output.

@@ -0,0 +1,56 @@
# build-and-push-image-digests

This is a composite GitHub Action, used to build and push image digests to GitHub, so then they can be picked up by the `push-to-gar-docker-multiarch` action, to create a manifest and push the actual image to GAR.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This is a composite GitHub Action, used to build and push image digests to GitHub, so then they can be picked up by the `push-to-gar-docker-multiarch` action, to create a manifest and push the actual image to GAR.
This is a composite GitHub Action, used to build and push image digests to GitHub, so then they can be picked up by the `push-to-gar-docker-multiarch` workflow, to create a manifest and push the actual image to GAR.

description: |
The dockerfile to use.
required: false
platform:
Copy link
Contributor

Choose a reason for hiding this comment

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

In the README that input is called platforms, which is more fitting IMO 🙂

workflow_call:
inputs:
platforms:
default: "['amd64']"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it perhaps be better to also use the same format as we have with the docker/build-push-action?

linux/amd64
linux/arm64
...

path: /tmp/digests
pattern: digests-*-${{ inputs.tag }}-${{ inputs.environment }}
- name: Set up Docker Buildx
uses: docker/setup-buildx-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.

Please pin

uses: docker/setup-buildx-action@v3
- id: meta
name: Docker meta
uses: docker/metadata-action@v5
Copy link
Contributor

Choose a reason for hiding this comment

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

Please pin

using: composite
steps:
- name: Checkout
uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the version indicator to allow auto-updates.

Copy link
Member

@iainlane iainlane left a comment

Choose a reason for hiding this comment

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

sorry for taking a while to get to this. great work 🚀. I think my comments will sound like a lot, but these are going to be some of the most important interfaces we offer from this repo so it's worth trying to design the interfaces to be the best we can. I'm up for pairing if you want.

here are my main thoughts in addition to the specific comments I left

Generality

It'd be best if we avoid repeating our earlier mistake of having parallel actions for GAR and Docker Hub which behave slightly differently. What you're building here could replace those. But as it is currently, the actions/workflows hardcode GAR quite deeply.

Flexibility

I think we should provide for users who need to run pre/post steps. We should provide building blocks for building/pushing multiarch images, so that if you need to do something before or after running the build it's possible. In addition, we can provide packaged-up versions as reusable workflows too for those who have simple needs.

Imagine I want to check the repo out, run some scripts (fetch generated artifacts, fill in variables with build info, whatever) and then build that state.

Or maybe I want to supply custom mounts, pass some weird Docker options or anything else. I could run the build myself.

tl;dr plenty of projects want to do custom stuff and if we are too packaged then we prevent that.

Layout

Maybe we should start grouping the actions under docker/ to keep all the related ones together. (I know it can't work for reusable workflows - maybe there they could all start with docker-?)

Comment on lines +77 to +90
- name: Checkout
env:
action_repo: ${{ github.action_repository }}
action_ref: ${{ github.action_ref }}
uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
with:
repository: ${{ env.action_repo }}
ref: ${{ env.action_ref }}
- name: Checkout
uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
with:
repository: grafana/shared-workflows
ref: multiarch-docker-builds-using-sh-runners
path: shared-workflows
Copy link
Member

Choose a reason for hiding this comment

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

Aren't these the same? I thought github.action_repository will be shared-workflows when you're running this, ditto action_ref and the selected ref? Or is it different for reusable workflows?

Comment on lines +68 to +71
- name: try printing env
shell: bash
run: |
echo ${{ env.platform_env }}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- name: try printing env
shell: bash
run: |
echo ${{ env.platform_env }}

shell: bash
run: |
echo ${{ env.platform_env }}
- name: Prepare
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment saying what this is doing, or make the name more descriptive?

Comment on lines +83 to +88
- name: Get repository name
id: get-repository-name
shell: bash
run: |
REPO_NAME=$(echo ${{ github.repository }} | awk -F'/' '{print $2}')
echo "repo_name=${REPO_NAME}" >> ${GITHUB_OUTPUT}
Copy link
Member

Choose a reason for hiding this comment

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

This could be merged into prepare I think

Comment on lines +89 to +101
- name: Resolve GCP project
id: resolve-project
shell: bash
run: |
if [ "${{ inputs.environment }}" == 'dev' ]; then
PROJECT="grafanalabs-dev"
elif [ "${{ inputs.environment }}" == 'prod' ]; then
PROJECT="grafanalabs-global"
else
echo "Invalid environment. Valid environment variable inputs: dev, prod"
exit 1
fi
echo "project=${PROJECT}" >> ${GITHUB_OUTPUT}
Copy link
Member

Choose a reason for hiding this comment

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

And this probably, or alternatively made into a standalone action since we repeat it all over the place.

@@ -0,0 +1,53 @@
---
description: Pulls image digests from GitHub and pushes manifests to GAR
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it would be too hard to make this not specific to Artifact Registry.

Mainly removing the login step up to a higher level.

images: ${{ inputs.full-image-name }}
tags: ${{ inputs.tag }}
- id: login-to-gar
uses: grafana/shared-workflows/actions/login-to-gar@main
Copy link
Member

Choose a reason for hiding this comment

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

(will be obsolete if you adopt my last comment)

Everywhere else in this PR this repo is checked out and the action is used from there, to keep things in sync. That's what we should do here too.

shell: bash
run: |
docker buildx imagetools create $(jq -cr '.tags | map("-t " + .) | join(" ")' <<< "$DOCKER_METADATA_OUTPUT_JSON") \
$(printf '${{ inputs.full-image-name }}@sha256:%s ' *)
Copy link
Member

Choose a reason for hiding this comment

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

Two suggestions:

  1. Do the jq thing on a separate line for a bit better readability, also please add a comment showing what it's transforming (from: xxx to: yyy)
  2. Put inputs.full-image-name through an env block on this step. Just so that you can't give it a value like ';) rm -rf /; or whatever and do mean things to the machine. (Applies to the "Inspect image" step too and maybe others where input is handled similarly)

path: shared-workflows
- name: Build multi-arch
id: build-multiarch
uses: ./shared-workflows/actions/build-and-push-image-digest
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
uses: ./shared-workflows/actions/build-and-push-image-digest
uses: ./shared-workflows/actions/build-and-push-image-digests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants