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

Support buildx for amd64 and arm64 builds (#766) #870

Merged
merged 1 commit into from
Jan 13, 2022

Conversation

sethAmazon
Copy link
Member

@sethAmazon sethAmazon commented Jan 10, 2022

Description:
Support Buildx for amd64 and arm64 builds

Link to tracking Issue:
#766

Testing:
(Make creates binaries)
make docker-build
make docker-build-arm

(With binary not created -takes a very long time since it requires go mod download-)
docker buildx build --platform linux/amd64 -t amazon/awscollector:v0.15.0 -f ./cmd/awscollector/Dockerfile .
docker buildx build --platform linux/arm64 -t amazon/awscollector:v0.15.0 -f ./cmd/awscollector/Dockerfile .

(defaults to amd64)
docker build -t amazon/awscollector:latest -f ./cmd/awscollector/Dockerfile .

@sethAmazon sethAmazon requested a review from a team as a code owner January 10, 2022 14:21
@codecov-commenter
Copy link

Codecov Report

Merging #870 (0e6ce52) into main (94d47a0) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #870   +/-   ##
=======================================
  Coverage   50.38%   50.38%           
=======================================
  Files          11       11           
  Lines         385      385           
=======================================
  Hits          194      194           
  Misses        175      175           
  Partials       16       16           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 85c027b...0e6ce52. Read the comment docs.

@jefchien
Copy link
Member

Wouldn't it be simpler to add arm64 support after #860 gets merged in? Could just add it to the platforms https://github.com/docker/build-push-action/blob/master/docs/advanced/multi-platform.md

@sethAmazon
Copy link
Member Author

@sethAmazon
Copy link
Member Author

From @jefchien and I spoke on slack that this will require these changes to the dockerfile

.github/workflows/CI.yml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@sethAmazon sethAmazon force-pushed the suuport-buildx branch 2 times, most recently from 24f1803 to 40a4d9d Compare January 10, 2022 22:58
cmd/awscollector/Dockerfile Outdated Show resolved Hide resolved
cmd/awscollector/Dockerfile Outdated Show resolved Hide resolved
@sethAmazon sethAmazon force-pushed the suuport-buildx branch 2 times, most recently from eda5f29 to 2c8c86e Compare January 11, 2022 17:27
################################
FROM alpine:latest as build

ENV GOPROXY=direct
Copy link
Member Author

Choose a reason for hiding this comment

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

This will take over 1hr to run since no go proxy is used.

@sethAmazon sethAmazon force-pushed the suuport-buildx branch 2 times, most recently from 657f148 to 34009b8 Compare January 11, 2022 18:06
@@ -485,6 +485,9 @@ jobs:
aws-secret-access-key: ${{ secrets.INTEG_TEST_AWS_KEY_SECRET }}
aws-region: us-west-2

- name: Set up QEMU
Copy link
Member Author

Choose a reason for hiding this comment

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

required for multi arc according to docs

@sethAmazon
Copy link
Member Author

Screen Shot 2022-01-11 at 1 37 48 PM

.github/workflows/CI.yml Outdated Show resolved Hide resolved
.github/workflows/CI.yml Outdated Show resolved Hide resolved
.github/workflows/CI.yml Outdated Show resolved Hide resolved
cmd/awscollector/Dockerfile Outdated Show resolved Hide resolved
@sethAmazon sethAmazon force-pushed the suuport-buildx branch 2 times, most recently from 78f805c to a4c752d Compare January 11, 2022 18:58
@@ -0,0 +1,13 @@
#!/bin/sh
BINARY=aoc_linux_x86_64
Copy link
Contributor

Choose a reason for hiding this comment

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

Small nit I would probably output the same aoc_linux binary into a folder with the platform, just to reduce moving parts (renaming files).

Even better could be just awscollector perhaps since that's what we end up renaming to anyways. Results in less names to deal with.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that was for backward compatibility in the make file. Any sort of backwards not compatible change we should have a meeting just to discuss that. Or I can just make more make commands.

Copy link
Member Author

Choose a reason for hiding this comment

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

COPY --from=build /workspace/config/ecs/otel-instance-metrics-config.yaml /etc/ecs/otel-instance-metrics-config.yaml
COPY --from=build /workspace/config/apprunner/apprunner-default-config.yaml /etc/apprunner/apprunner-default-config.yaml
COPY --from=build /workspace/build/linux/aoc_linux /awscollector
COPY config.yaml /etc/otel-config.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

In a future PR it would be good to redo the folder structure in this repo to allow just one glob COPY command to copy in all the configs to reduce this maintenance

Copy link
Member Author

Choose a reason for hiding this comment

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

Will create an issue for this

Copy link
Member Author

Choose a reason for hiding this comment

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

@sethAmazon sethAmazon changed the title Support buildx for amd64 and arm64 builds step 1 (#766) Support buildx for amd64 and arm64 builds (#766) Jan 12, 2022
Copy link
Member

@jefchien jefchien left a comment

Choose a reason for hiding this comment

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

Do you need to change the CD as well or will retagging pull all platforms?

@khanhntd
Copy link
Contributor

Do you need to change the CD as well or will retagging pull all platforms?

Will need to copy multiple platforms since the local image cache can only store a single architecture per image.

@sethAmazon
Copy link
Member Author

@khanhntd it seems you use https://github.com/akhilerm/tag-push-action this should support multi platform out of box according to their docs

@aateeqi aateeqi merged commit 597874b into aws-observability:main Jan 13, 2022
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.

7 participants