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

feat(#8841): Support multiplatform images #8918

Merged
merged 19 commits into from
Mar 15, 2024
Merged

Conversation

1yuv
Copy link
Member

@1yuv 1yuv commented Mar 7, 2024

Description

Build multi-platform images
#8841

Code review checklist

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on cht-docs
  • Tested: Unit and/or e2e where appropriate
  • Internationalised: All user facing text
  • Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

Compose URLs

If Build CI hasn't passed, these may 404:

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

@1yuv
Copy link
Member Author

1yuv commented Mar 7, 2024

Working demo of external contributor: this build and internal contributor

Copy link
Contributor

@mrjones-plip mrjones-plip left a comment

Choose a reason for hiding this comment

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

yay!!! This has been so long in the coming, it's just a real joy to finally see it in the form of PR - nice work!

tl;dr - I think we may need to change const BUILD_PLATFORMS = ['linux/amd64', 'linux/arm64']; to have /v8 at the end like this: const BUILD_PLATFORMS = ['linux/amd64', 'linux/arm64/v8'];

This SO post talks about it, but that talks about linux/arm64/v8 normalizes to linux/arm64 - but what about the other way around?

read below for my logic


I'm doing some more research, but in my earlier tests I noted:

I do note that it's not arm64 AND amd64, just amd64

This is because I tested the cht-sentinel:4.6.0-8841-multiplatform-images image. Running a complete test, there's some more disparities, specifically that both sentinel and API are only amd64. They're both based on alpine:3.19 upstream. I'm running tests with, sentinel, for example:

docker run --rm mplatform/mquery public.ecr.aws/medic/cht-sentinel:4.6.0-8841-multiplatform-images

Running an exhaustive test we see that none of the upstream images support arm64 and they all support arm64/v8. All CHT images are the 4.6.0-8841-multiplatform-images tag:

Medic Images linux/amd64 linux/arm64 linux/arm64/v8
cht-sentinel Y N N
cht-couchdb Y Y N
cht-nginx Y Y N
cht-haproxy-healthcheck Y Y N
cht-haproxy Y Y N
cht-api Y N N
Upstream Images
alpine:3.19 (sentinel) Y N Y
couchdb:3.3.3 (couchdb) Y N Y
nginx:1.25.1-alpine (nginx) Y N Y
python:3.10.13-alpine (haproxy-healthcheck) Y N Y
haproxy:2.6 (haproxy) Y N Y
alpine:3.19 (api) Y N Y

@garethbowen garethbowen removed their request for review March 8, 2024 06:36
@mrjones-plip
Copy link
Contributor

thanks @1yuv ! You know how long it takes to build new images after a commit/push based on your earlier efforts?

@1yuv 1yuv requested a review from mrjones-plip March 8, 2024 16:06
@1yuv
Copy link
Member Author

1yuv commented Mar 8, 2024

Hi @mrjones-plip , thanks for extensive test and details on the image and platforms they support. After updating build platfroms to linux/arm64/v8, I can see all medic images now support both amd64 and arm64.

Medic Images linux/amd64 linux/arm64
cht-sentinel Y Y
cht-couchdb Y Y
cht-nginx Y Y
cht-haproxy-healthcheck Y Y
cht-haproxy Y Y
cht-api Y Y

@mrjones-plip
Copy link
Contributor

Awesome work @1yuv ! I was hoping we'd bet linux/arm64/v8 out of this and not just linux/arm64 🤔 Still, I'll approve this as is - I believe it's good to go. My local tests on Linux show no regressions, which is great.

That said I think we should get at least one other engineer to sign off on this before merging to master.

@dianabarsan or @nydr - any thoughts or concerns here? I kinda wanna get another set of eyes on this given that the new images can have far reaching implications if something goes off the rails. Maybe you can let us know before next Tue (Mar 12th) so we can keep this PR moving?

NB - I'll be out starting Mar 11th, back in the office Mar 18th.

@mrjones-plip mrjones-plip requested a review from nydr March 8, 2024 16:42
Copy link
Contributor

@mrjones-plip mrjones-plip left a comment

Choose a reason for hiding this comment

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

tentatively approving pending sign off from @dianabarsan or @nydr

@1yuv
Copy link
Member Author

1yuv commented Mar 8, 2024

Regarding time taken for building and publishing image, given we're building 2 platform images for 6 services, app compilation time has gone up by nearly 1 minute 30 seconds, specially publish for testing where we're building multi-platform images. This is in comparison with recent single platform build action .

However, the overall build and test time is unaffected or reduced in comparison with some previous builds since we saved time in Publishing branch build by using regctl which detects pre-existing layers and prevent unnecessary i/o and has resulted in faster publishing of images compared to using docker pull, tag, and push.

@mrjones-plip mrjones-plip modified the milestone: 4.7.0 Mar 8, 2024
@1yuv 1yuv changed the title fix(#8841): Build multiplatform images feat(#8841): Support multiplatform images Mar 9, 2024
Copy link
Member

@dianabarsan dianabarsan left a comment

Choose a reason for hiding this comment

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

Cool. Thanks for getting this done, @1yuv !

@1yuv 1yuv merged commit 448c700 into master Mar 15, 2024
30 checks passed
@1yuv 1yuv deleted the 8841-multiplatform-images branch March 15, 2024 01:34
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.

4 participants