-
Notifications
You must be signed in to change notification settings - Fork 123
Simplify caa multi arch build #2777
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
base: main
Are you sure you want to change the base?
Simplify caa multi arch build #2777
Conversation
e52f407 to
8523c10
Compare
|
I'm not sure how to test this - I tried running against my fork, but for reasons (maybe permissions related?) github won't run the CAA job: https://github.com/stevenhorsman/cloud-api-adaptor/actions/runs/20923681415 and just says "Waiting for pending jobs". As such I guess we should wait until post the 0.18 release to merge this, so we have time to debug & fix/revert any issues? |
36db4d5 to
c2416c4
Compare
ldoktor
left a comment
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.
@stevenhorsman do you intend to convert this from draft? To me it looks better than the current approach. I haven't reviewed thoroughly the new image tags (which is why I'm not giving the ack) but the publishing seems very good and the new tags schema is cleaner.
c2416c4 to
d0d542a
Compare
Thanks for the comments. As I think I mentioned above - as this is potentially disruptive and we should have a release very soon, I'm planning on leaving this as draft until the release is done. I need to do more work to test what I can in my fork as well before that. |
1d8e7bc to
41389cf
Compare
|
Ok, this seems to be tested and working okay in my fork, so maybe it's not risky like I though: https://github.com/stevenhorsman/cloud-api-adaptor/actions/runs/21719552373 |
| manifest="${registry}/${name}:$tag" | ||
| for arch in "${all_arches_array[@]}"; do | ||
| docker pull --platform="linux/$arch" "${registry}/${name}:$tag" | ||
| docker inspect "${registry}/${name}:$tag" |grep Architecture |
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 verification is not included in the hack/publish.sh. The question is whether we care... (I'd rely on docker to generate them correctly)
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, my view is that if we don't trust docker here then we need to pull down and check images in a lot of builds
| # strip the `linux/` from the docker buildx platform format | ||
| arch_suffix="-${arch#"linux/"}" | ||
| # strip the `linux/` from the docker buildx platform format in the tag | ||
| arch_suffix="-${arch#${arch_prefix}}" |
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 confused me a bit, I was looking for the change but it's only an unrelated cleanup :D (very minor, mentioning it just to save other reviewers time)
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.
Btw two lines above is also a typoe "if we ar ebuilding for a single arch" (again, not critical but if you happen to be updating it you might consider including that fix as well)
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, I just spotted that we set a "constant" value and then ignored it and redefined it. I'm happy to just remove arch_prefix="linux/" and revert this line if you'd prefer?
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.
Ideally I'd like to have it in a separate commit along with the other typo fix, but it's fine keeping it here as well as it's improving the situation...
| for tag in "${TAGS[@]}"; do | ||
| amend="" | ||
| for arch in "${MULTI_ARCHES[@]}"; do | ||
| amend+="--amend ${IMAGE_REGISTRY}:${tag}-${arch} " |
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.
The default for registry in caa_build_and_push_all_arches.yaml is quay.io/confidential-containers so this becomes quay.io/confidential-containers:${tag}-${arch}. Shouldn't the cloud-api-adaptor be included as well?
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.
Definitely - I had intended it to be included in the registry, but this isn't the case. I don't want to hard-code it as we probably want to swap the other image builds to use this, so I'll maybe add an IMAGE_NAME parameter too. Thanks for the spot
|
|
||
| function _publish_multiarch_manifest() | ||
| { | ||
| ${IMAGE_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.
I guess you wanted to print these? (this will execute the IMAGE_REGISTRY as a command!)
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.
No - it is supposed to check that the value is set rather than executing it? I can add a better error string though here
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.
Maybe It only works for assignment?
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 mean echo this will check the value, but leaving it on a separate line evaluates that expression :-/. Ai suggests also : ${IMAGE_REGISTRY:?} but it sounds odd to me :-) (Usually I explicitly check by [[ -z "$VAR" ]] in my code...
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.
Ah ok : was the bit I'd seen before that I missed. I reworked it anyway
| function _publish_multiarch_manifest() | ||
| { | ||
| ${IMAGE_REGISTRY:?} | ||
| ${IMAGE_TAGS:?} |
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.
There is an inconsistency between tabs/spaces, could you please pick one and keep the style?
| - name: Generate image manifest | ||
| - name: Publish multi-arch dev manifest | ||
| run: | | ||
| ./src/cloud-api-adaptor/publish.sh publish-multiarch-manifest |
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 already be in src/cloud-api-adaptor dir due to defaults.run.working-directory (previously hack/image-manifest.sh was also in that location)
|
Thanks @stevenhorsman, this unifies the image names (note the |
There is an issue in setup-go that it lacks endian awareness, so port the fix from caa_build_and_push_per_arch.yaml to the standard CAA build workflow, to enable us to run the workflow on the ppc runner, rather than needing to use emulation, which can be slow Drive-by-fix of zizmor warning Signed-off-by: stevenhorsman <[email protected]>
The difference between the caa_build_and_push and caa_build_and_push_per_arch is confusing. I hope to address this in this PR, but let's start by renaming for better clarity. Signed-off-by: stevenhorsman <[email protected]>
41389cf to
c80bae8
Compare
| name: (Callable) Build and push cloud-api-adaptor multi-arch manifest image for all type/arch combinations | ||
|
|
||
| on: | ||
| workflow_call: | ||
| inputs: | ||
| registry: | ||
| default: 'quay.io/confidential-containers' | ||
| description: 'Image registry (e.g. "quay.io/confidential-containers") where the built image will be pushed to' | ||
| required: false | ||
| type: string | ||
| dev_tags: | ||
| description: 'Comma-separated list of tags for the dev built image (e.g. latest,<sha>-dev)' | ||
| required: true | ||
| type: string | ||
| release_tags: | ||
| description: 'Comma-separated list of tags for the release built image (e.g. <sha>)' | ||
| required: true | ||
| type: string | ||
| git_ref: | ||
| default: 'main' | ||
| description: Git ref to checkout the cloud-api-adaptor repository. Defaults to main. | ||
| required: false | ||
| type: string | ||
| secrets: | ||
| QUAY_PASSWORD: | ||
| required: true | ||
|
|
||
| defaults: | ||
| run: | ||
| working-directory: src/cloud-api-adaptor | ||
|
|
||
| permissions: {} | ||
|
|
||
| jobs: | ||
| build_push_job: | ||
| name: build and push single arch dev and release images | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| types: [ | ||
| { arch: linux/amd64, runner: ubuntu-24.04 }, | ||
| { arch: linux/arm64, runner: ubuntu-24.04-arm }, | ||
| { arch: linux/ppc64le, runner: ubuntu-24.04-ppc64le }, | ||
| { arch: linux/s390x, runner: ubuntu-24.04-s390x }, | ||
| ] | ||
| uses: ./.github/workflows/caa_build_and_push.yaml | ||
| with: | ||
| dev_arches: ${{ matrix.types.arch }} | ||
| dev_tags: ${{ inputs.dev_tags }} | ||
| git_ref: ${{ inputs.git_ref}} | ||
| registry: ${{ inputs.registry }} | ||
| release_arches: ${{ matrix.types.arch }} | ||
| release_tags: ${{ inputs.release_tags }} | ||
| runner: ${{ matrix.types.runner}} | ||
| secrets: | ||
| QUAY_PASSWORD: ${{ secrets.QUAY_PASSWORD }} | ||
| permissions: | ||
| packages: write # Needed to push the images to GHCR | ||
|
|
||
| manifest_job: | ||
| name: generate images manifest | ||
| runs-on: ubuntu-24.04 | ||
| needs: [build_push_job] | ||
| steps: | ||
| - name: Checkout the code | ||
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | ||
| with: | ||
| fetch-depth: 0 | ||
| ref: "${{ inputs.git_ref }}" | ||
| persist-credentials: false | ||
|
|
||
| - name: Set up Docker Buildx | ||
| uses: docker/setup-buildx-action@8d2750c68a42422c14e847fe6c8ac0403b4cbd6f # v3.12.0 | ||
|
|
||
| - name: Login to quay Container Registry | ||
| if: ${{ startsWith(inputs.registry, 'quay.io') }} | ||
| uses: docker/login-action@c94ce9fb468520275223c153574b00df6fe4bcc9 # v3.7.0 | ||
| with: | ||
| registry: ${{ inputs.registry }} | ||
| username: ${{ vars.QUAY_USERNAME }} | ||
| password: ${{ secrets.QUAY_PASSWORD }} | ||
|
|
||
| - name: Login to Github Container Registry | ||
| if: ${{ startsWith(inputs.registry, 'ghcr.io') }} | ||
| uses: docker/login-action@c94ce9fb468520275223c153574b00df6fe4bcc9 # v3.7.0 | ||
| with: | ||
| registry: ${{ inputs.registry }} | ||
| username: ${{ github.repository_owner }} | ||
| password: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
||
| - name: Publish multi-arch dev manifest | ||
| run: | | ||
| ./publish.sh publish-multiarch-manifest | ||
| env: | ||
| IMAGE_REGISTRY: ${{ inputs.registry }} | ||
| IMAGE_NAME: "cloud-api-adaptor" | ||
| IMAGE_TAGS: ${{ inputs.dev_tags}} | ||
|
|
||
| - name: Publish multi-arch release manifest | ||
| run: | | ||
| ./publish.sh publish-multiarch-manifest | ||
| env: | ||
| IMAGE_REGISTRY: ${{ inputs.registry }} | ||
| IMAGE_NAME: "cloud-api-adaptor" |
Check warning
Code scanning / zizmor
insufficient job-level concurrency limits Warning
c80bae8 to
df3b3a4
Compare
|
Thanks for the great review @ldoktor - I think I've addressed all the points, and now I've satisfied myself that I was right the first time about the riskiness of this I'm re-running on my fork and have switched it to draft. |
b4760e2 to
39aba27
Compare
Add a helper script that can create one or more multi-arch manifest images for our three supported architectures giving a registry and a list of one, or more tags. Based on the release.sh script from kata-containers Signed-off-by: stevenhorsman <[email protected]>
Currently we have multiple formats of image tags for dev images: - <release>-dev-<arch> for released images - dev-<sha>-<arch> for interim published images - latest-<arch>-dev for daily e2e test images - ci-pr<pr number>-dev (no arch) for the x86 only packer PR e2e test images - ci-pr<pr number>-<arch>-dev for mkosi specific-arch PR e2e test images This shows that we have multiple different code paths, or logic being run to do the same task and we'd like to reduce duplication and increase consistency, so let's move all to the release version: <tag>-dev-<arch> Signed-off-by: stevenhorsman <[email protected]>
Rather than having separate logic and builds for the multi-arch image including the confusing upload/download of a tags file to drive things, we can just swap and use the existing CAA build workflow, to build the images for each arch and the new multi-arch publish to create the multi-arch manifest. Signed-off-by: stevenhorsman <[email protected]>
Nothing should be calling the `image-with-arch` make target anymore now that the process is unified, so remove it and the code that only it called to simplify and remove duplication. Signed-off-by: stevenhorsman <[email protected]>
39aba27 to
a06934c
Compare
The CAA multi-arch build is confusing (at least to me) with uploaded files pointing to sha that are later downloaded to create a dynamic manifest based on them. I think by hardcoded the arches we include in the manifest (which doesn't change often), we can re-use the "standard" caa build and then create a manifest in a more standard way.
Along the way fix some inconsistency with tags and zizmor issues.