Support for ARM64 build #71

MeghnaPrabhu · 2020-09-15T17:20:00Z

Support for ARM64 build
#44

Description of changes:

PR does not include the changes for sync task. I will publish a separate PR for that.
Pipeline stages have been modified to allow for parallel multi-arch image builds (x86-64 and ARM64).

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

PettitWesley · 2020-09-15T18:07:11Z

scripts/publish.sh

-classic_regions_account_id="906394416424"
+classic_regions_account_id="957584016365"

 cn_regions="
 cn-north-1
 cn-northwest-1
 "

-cn_regions_account_id="128054284489"
+cn_regions_account_id="957584016365"



Guessing this is left over from testing- account IDs should not change

Yes, I will switch these from my personal ID as the last commit before merge.

buildspec.yml

hossain-rayhan · 2020-09-15T20:24:25Z

buildspec.yml

+      - docker manifest inspect ${AWS_ACCOUNT}.dkr.ecr.${AWS_REGION}.amazonaws.com/amazon/aws-for-fluent-bit-test:latest || true
+
+      # Push manifest list
+      - docker manifest push ${AWS_ACCOUNT}.dkr.ecr.${AWS_REGION}.amazonaws.com/amazon/aws-for-fluent-bit-test:latest || true 


Why do we need to do a OR with true ( || true) every time? Shouldn't it return an error code if it fails?

Like I mentioned above, since both instances are running in parallel, only the one that executes last will be able to create the manifest list successfully. We need both the images to be pushed to ECR before the manifest list is created. At the same time, build should not fail if it is waiting for the image of the other architecture to be available, this is why we have the || True

That's fine, but then we need the verify step to check that the manifest list was published properly. Otherwise its possible for both instances to fail without us knowing, right?

That is a good point, I thought about it. However, if the manifest list does not get created properly, it will fail immediately in the integ-test stage. This is because we start each stage by pulling the image from the manifest list that was created during the build.

Adding a verification step in the publish to ecr stage will not be very useful because by then it is established that we have the manifest file working correctly.

Additionally, this case of ||true only occurs in the build stage because we are running both instances in parallel. Other stages like publish to ecr are all running on a single instance. Hence, we can be sure that the manifest list created in the publish to ecr stage has both the images as we will be pushing them individually to ECR before creating the manifest.

Makes sense 👍

hossain-rayhan · 2020-09-15T21:13:20Z

scripts/publish.sh

@@ -259,7 +308,7 @@ match_two_sha() {

 if [ "${1}" = "publish" ]; then
 	if [ "${2}" = "dockerhub" ]; then
-		publish_to_docker_hub amazon/aws-for-fluent-bit:latest amazon/aws-for-fluent-bit:${AWS_FOR_FLUENT_BIT_VERSION}
+		publish_to_docker_hub amazon/aws-for-fluent-bit meghnaprabhu/aws-for-fluent-bit


Need to replace meghnaprabhu with amazon.

Yes, I will replace that as the last step along with the account ID

Also, I can see both of the values are same. If we are not using the 2nd parameter (fluent-bit-version), we can remove it.

hossain-rayhan · 2020-09-15T21:13:45Z

scripts/publish.sh

@@ -417,7 +466,7 @@ fi
 # Following scripts will be called only from the CI/CD pipeline
 if [ "${1}" = "cicd-publish" ]; then
 	if [ "${2}" = "dockerhub" ]; then
-		publish_to_docker_hub amazon/aws-for-fluent-bit:latest amazon/aws-for-fluent-bit:${AWS_FOR_FLUENT_BIT_VERSION}
+		publish_to_docker_hub amazon/aws-for-fluent-bit meghnaprabhu/aws-for-fluent-bit  


hossain-rayhan · 2020-09-15T21:24:59Z

Looks good to me. Once minor feedbacks are addressed, we are good to go.
One thing, I didn't see any changes for the verification. Is there no way to make sure customers are getting the proper ARM64 image?

MeghnaPrabhu · 2020-09-15T21:31:40Z

For verification did you mean verify if docker pull from a manifest list actually gives you ARM image or did you mean verification of the SHA between tags AWS_FOR_FLUENT_BIT_VERSION and latest?

hossain-rayhan · 2020-09-15T21:34:42Z

The first one. We are not verifying from an ARM environment, right?

MeghnaPrabhu · 2020-09-15T21:36:42Z

Yes, we are not verifying that part.
It is guaranteed by docker, if you issue a docker pull on a manifest file, docker will return the image compatible with the underlying architecture.

hossain-rayhan

Looks good to me.

PettitWesley · 2020-09-16T22:57:43Z

integ/resources/create_test_resources.sh

+ARCHITECTURE=$(uname -m)
+if [ ARCHITECTURE=="x86_64" ] 
+then
+    ARCHITECTURE="x86-64"
+fi
+aws cloudformation deploy --template-file ./integ/resources/cfn-kinesis-s3-firehose.yml --stack-name integ-test-fluent-bit-${ARCHITECTURE} --region us-west-2 --capabilities CAPABILITY_NAMED_IAM


Suggestion, replace the if with:

ARCHITECTURE=$(uname -m | tr '_' '-')

PettitWesley · 2020-09-16T22:59:12Z

scripts/publish.sh

+	for arch in "${ARCHITECTURES[@]}"
+	do
+    docker manifest annotate --arch "$arch" \
+		${1}:${tag} \
+		${1}:"$arch"-${AWS_FOR_FLUENT_BIT_VERSION} 


weird indent on one line

PettitWesley · 2020-09-16T22:59:37Z

scripts/publish.sh

+	push_image_ecr ${account_id} ${region}	
+
+    create_manifest_list ${account_id}.dkr.ecr.${region}.amazonaws.com/aws-for-fluent-bit ${AWS_FOR_FLUENT_BIT_VERSION}
+	create_manifest_list ${account_id}.dkr.ecr.${region}.amazonaws.com/aws-for-fluent-bit "latest"


weird indent on one line

MeghnaPrabhu requested review from hossain-rayhan and PettitWesley September 15, 2020 17:20

MeghnaPrabhu requested a review from a team as a code owner September 15, 2020 17:20

PettitWesley reviewed Sep 15, 2020

View reviewed changes

hossain-rayhan reviewed Sep 15, 2020

View reviewed changes

buildspec.yml Show resolved Hide resolved

hossain-rayhan reviewed Sep 15, 2020

View reviewed changes

Support for ARM64 build

5da9c8f

MeghnaPrabhu force-pushed the master branch from b4baa13 to 5da9c8f Compare September 15, 2020 21:49

hossain-rayhan approved these changes Sep 16, 2020

View reviewed changes

PettitWesley reviewed Sep 16, 2020

View reviewed changes

PettitWesley approved these changes Sep 16, 2020

View reviewed changes

hossain-rayhan merged commit 28b23f6 into aws:master Sep 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for ARM64 build #71

Support for ARM64 build #71

MeghnaPrabhu commented Sep 15, 2020 •

edited

Loading

PettitWesley Sep 15, 2020

MeghnaPrabhu Sep 15, 2020

hossain-rayhan Sep 15, 2020

MeghnaPrabhu Sep 15, 2020

PettitWesley Sep 16, 2020

MeghnaPrabhu Sep 16, 2020

MeghnaPrabhu Sep 16, 2020

PettitWesley Sep 16, 2020

hossain-rayhan Sep 15, 2020 •

edited

Loading

MeghnaPrabhu Sep 15, 2020

hossain-rayhan Sep 15, 2020

hossain-rayhan Sep 15, 2020

hossain-rayhan commented Sep 15, 2020

MeghnaPrabhu commented Sep 15, 2020

hossain-rayhan commented Sep 15, 2020

MeghnaPrabhu commented Sep 15, 2020 •

edited

Loading

hossain-rayhan left a comment

PettitWesley Sep 16, 2020 •

edited

Loading

PettitWesley Sep 16, 2020

PettitWesley Sep 16, 2020

Support for ARM64 build #71

Support for ARM64 build #71

Conversation

MeghnaPrabhu commented Sep 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hossain-rayhan Sep 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hossain-rayhan commented Sep 15, 2020

MeghnaPrabhu commented Sep 15, 2020

hossain-rayhan commented Sep 15, 2020

MeghnaPrabhu commented Sep 15, 2020 • edited Loading

hossain-rayhan left a comment

Choose a reason for hiding this comment

PettitWesley Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MeghnaPrabhu commented Sep 15, 2020 •

edited

Loading

hossain-rayhan Sep 15, 2020 •

edited

Loading

MeghnaPrabhu commented Sep 15, 2020 •

edited

Loading

PettitWesley Sep 16, 2020 •

edited

Loading