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

use multi image to support apple silicon out of the box #157

Closed
wants to merge 1 commit into from

Conversation

sysradium
Copy link

@sysradium sysradium commented Nov 13, 2023

I think it makes sense to use multi image instead since otherwise on new macs without DOCKER_DEFAULT_PLATFORM=linux/amd64 you would get an error:

✦ ➜ docker compose up -d
[+] Running 0/3
 ⠦ postgres Pulling                                                                                                                                                                                                                                                                                                        1.7s
 ⠦ pact-broker Pulling                                                                                                                                                                                                                                                                                                     1.7s
 ⠦ nginx Pulling                                                                                                                                                                                                                                                                                                           1.7s
no matching manifest for linux/arm64 in the manifest list entries

@mefellows mefellows added the smartbear-supported SmartBear engineering team will support this issue. See https://docs.pact.io/help/smartbear label Jan 7, 2024
@mefellows
Copy link
Member

Thanks for this, we'll add it to our backlog for review.

@bethesque
Copy link
Member

These get updated dynamically, so whatever change gets made here will be overwritten at the next release https://github.com/pact-foundation/pact-broker-docker/blob/master/script/release-workflow/prepare-release.sh

@Lewiscowles1986
Copy link

Is there a path-forward on this?

I Just got

docker run --rm --publish 9292:9292 --detach --env PACT_BROKER_DATABASE_URL=sqlite:////tmp/pact_broker.sqlite3 --name pact-broker pactfoundation/pact-broker:latest
Unable to find image 'pactfoundation/pact-broker:latest' locally
docker: Error response from daemon: no match for platform in manifest: not found.

It feels like this (could / should) be a priority for all PACT projects using the dockerfile

@Lewiscowles1986
Copy link

the dockerfile just builds for arm64 btw, so it's not insurmountable, but a pain. I encountered this while trying to troubleshoot this PR

@mefellows
Copy link
Member

So Beth, are you saying we should update that script to default to multi or something else?

@Lewiscowles1986 do you just need to use the -multi variant of the image, or is this PR somehow blocking your issue?

@Lewiscowles1986
Copy link

So for me, I was not using docker-compose, but

docker run --rm --publish 9292:9292 --detach --env PACT_BROKER_DATABASE_URL=sqlite:////tmp/pact_broker.sqlite3 --name pact-broker pactfoundation/pact-broker:latest

I believe latest-multi would work, but surely using buildx or similar would be a better fit, not needing a label considering that downloading this repo and just issuing a docker build, also works...

I leave such determinations to you all. changing label triggers a need to let people know to use that label, and I'm not sure if it solves needing to separately build that label. Docker supports single labels with multiple architectures.

@mefellows
Copy link
Member

mefellows commented Feb 15, 2024

IIRC we were just being cautious before flicking the switch, so to speak. Agree we don't want users to have to do anything different for different platforms.

We use buildx. I think this PR should just be flicking the switch, if we think the image itself is safe.

@YOU54F
Copy link
Member

YOU54F commented Feb 15, 2024

https://github.com/pact-foundation/pact-broker-docker?tab=readme-ov-file#multi-manifest-image

documented in the readme for multiple platforms.

We didn't want to change existing users behaviours, who are probably using DOCKER_DEFAULT_PLATFORM=linux/amd64 or --platform linux/amd64 on arm64 machines, for images that do not have an arm64 variant, until we were happy that the arm64 / arm images had long enough exposure in the wild.

This caught a few issues that would have otherwise broken peoples workflows. We are pretty confident now, and have nothing major to report, the biggest blocker was #148

We've promoted the pact-ruby-cli project to drop the -multi flag, and publish multi-manifests on the vanilla tags and will do in this repo soon.

The actual change would be to to remove the push method, rename push_multi to push and drop the multi tag here

push_multi() {
## These will use cached builds, so wont build every time.
docker buildx build --platform=linux/amd64,linux/arm64,linux/arm \
--output=type=image,push=true \
-t ${DOCKER_IMAGE_ORG_AND_NAME}:$1-multi .
}
push() {
docker buildx build --platform=linux/amd64 \
--output=type=image,push=true \
-t ${DOCKER_IMAGE_ORG_AND_NAME}:$1 .
}

@YOU54F
Copy link
Member

YOU54F commented Feb 15, 2024

you could do any of the following

append -multi to the image

docker run --rm --publish 9292:9292 --detach --env PACT_BROKER_DATABASE_URL=sqlite:////tmp/pact_broker.sqlite3 --name pact-broker pactfoundation/pact-broker:latest-multi

use DOCKER_DEFAULT_PLATFORM env var

DOCKER_DEFAULT_PLATFORM=linux/amd64 docker run --platform=linux/arm64 --rm --publish 9292:9292 --detach --env PACT_BROKER_DATABASE_URL=sqlite:////tmp/pact_broker.sqlite3 --name pact-broker pactfoundation/pact-broker:latest

explicitly force an alternative platform

docker run --platform=linux/arm64 --rm --publish 9292:9292 --detach --env PACT_BROKER_DATABASE_URL=sqlite:////tmp/pact_broker.sqlite3 --name pact-broker pactfoundation/pact-broker:latest

@Lewiscowles1986
Copy link

Hey @YOU54F while I could; every single place would need to change to use that, whereas updating the image tag, RE: your post before would also require no changes.

I'm very interested in the logic, of lets let folks opt into a tag, which might be broken, rather than; if it is broken then they can use platform flag to request amd64 and run non-native.

I wasn't manually running this image, someone seems to have asked the PHP PACT to run this image. I'd imagined you knew about this and that it was common; but perhaps my assumption there is faulty.

@YOU54F
Copy link
Member

YOU54F commented Feb 16, 2024

When I built the images I proposed the question to the author, re the tags prefixed with -multi. Probably an over abundance of caution on my end, but the maintainer agreed

#123 (comment)

I didn't want to do anything that adversely affected anyones existing workflow, as the PB is relied on by a unknown number of parties, the only time we usually find out things are wrong, that we haven't found, is via issue reports.

I wasn't manually running this image, someone seems to have asked the PHP PACT to run this image. I'd imagined you knew about this and that it was common; but perhaps my assumption there is faulty.

I don't recall this specific instance but can recall about 10 occasions where people have asked about a platform specific image, since release. Our prior guidance was to force emulation on non-native systems. I encounted it when moving to an M1 mac, so was keen to get new images out there at the very least for me, and for others, but also very keen not to negatively impact anyone. It certainly wasn't high enough on anyones want list to have made a PR themselves.

It's probably time to switch now though, so for everyone, it just works, I've given it a fair old battle testing, and we've dropped the multi prefix on the pact-cli.

I'd also like to publish the images to GitHub container registry, as we've had a couple (not many) requests from users who are being rate-limited and although not directly on this project, it makes sense for consistency across all of the pact-foundations images

@Lewiscowles1986
Copy link

So this pull should close and it should be built now for ARM as well as x86_64/AMD64?

@YOU54F
Copy link
Member

YOU54F commented Feb 23, 2024

Closing in favour of #171

Thanks for raising Alex

@YOU54F YOU54F closed this Feb 23, 2024
@sysradium sysradium deleted the pin-platform branch April 3, 2024 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
smartbear-supported SmartBear engineering team will support this issue. See https://docs.pact.io/help/smartbear
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants