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

Fix deprecated aarch image #563

Merged
merged 6 commits into from
Nov 2, 2022

Conversation

mpeiffer
Copy link
Contributor

@mpeiffer mpeiffer commented Oct 24, 2022

Summary of Changes

Addresses Issue #559

  • Merged all Dockerfiles into a single Dockerfile
  • Replaced the deprecated aarch image with alpine:latest
  • Updated the Makefile to build using the ARCH field set by export command.

Testing

Running make build:

testa

@mpeiffer mpeiffer force-pushed the bugfix/deprecated-dockerfile branch from b67cae9 to 98c42be Compare October 24, 2022 23:35
Copy link
Member

@johnwalicki johnwalicki left a comment

Choose a reason for hiding this comment

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

This is fantastic! Nice work. Approved

@johnwalicki
Copy link
Member

Where is the Makefile change?

Signed-off-by: mpeiffer <[email protected]>
@mpeiffer
Copy link
Contributor Author

Whoops, forgot to commit that file! It's added now.

Copy link
Member

@johnwalicki johnwalicki left a comment

Choose a reason for hiding this comment

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

Nice use of --build-arg in the Makefile and Dockerfile

Approved

@johnwalicki
Copy link
Member

@mpeiffer The build pipeline is failing with:

 Step 12/16 : COPY --from=0 /build/bin/${ARCH}_gps /gps
invalid from flag value 0: image with reference sha256:c1346d0f7e401c44cec06a72b15a1a04213467bf7eede44e4c287c041b8a4e67 was found but does not match the specified platform: wanted linux/arm, actual: linux/amd64
make[1]: Leaving directory '/home/runner/work/examples/examples/edge/services/gps'
make[1]: *** [Makefile:19: build] Error 1
make: *** [Makefile:30: test-all-arches] Error 2

The intermediate build stage is called go_build not 0
The attribution comments in line 1 and 2 are unnecessary and can be deleted. We can see that from the git blame.

@johnwalicki
Copy link
Member

I always run the make build whatever on my system before creating a PR. That would quickly shake out these typo bugs. Sometimes its complex to reproduce the build environment, but in this case, its straightforward (says the Linux developer for 20+ yrs... ;-) )

@johnwalicki
Copy link
Member

While you are preparing another commit, can you also rebase to the golang:1.19-alpine image ? Thx

@mpeiffer
Copy link
Contributor Author

Ah, sorry about that! Unfortunately I won't have my laptop until this Friday, as I will be out of town but I will fix it as soon as I get back. Do you want me to close the PR until then and reopen it after I make the fix?

@joewxboy
Copy link
Member

Ah, sorry about that! Unfortunately I won't have my laptop until this Friday, as I will be out of town but I will fix it as soon as I get back. Do you want me to close the PR until then and reopen it after I make the fix?

No worries. We'll keep it open and warm until you get back and ready to tackle it again.

@mpeiffer mpeiffer force-pushed the bugfix/deprecated-dockerfile branch from 7f4572f to 9886cca Compare October 29, 2022 23:13
@joewxboy joewxboy linked an issue Oct 29, 2022 that may be closed by this pull request
Copy link
Member

@johnwalicki johnwalicki left a comment

Choose a reason for hiding this comment

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

All good now. Merging.

@johnwalicki
Copy link
Member

Hmm The pipeline test build failed. I need to run it on my system to understand better.

https://github.com/open-horizon/examples/actions/runs/3353298023/jobs/5556541067

Will hold off on the merge until Monday.

@johnwalicki
Copy link
Member

Using the Dockerfile and Makefile from this PR, I successfully built all three arch {amd64, arm, arm64} containers on my Fedora system. I manually tested the containers on devices running those three architectures. They ran successfully.

I don't why the test pipeline has a problem (Its running Image: ubuntu-20.04 )

@mpeiffer
For giggles, I added a LABEL to the Dockerfile immediate stage (which is a good idea so that we clean up immediate stages)

FROM golang:1.19-alpine as go_build
LABEL stage=builder

and modified the Makefile to prune the intermediate stage after each build step.

build:
	docker build --build-arg ARCH=$(ARCH) --platform linux/$(ARCH) -t $(DOCKER_IMAGE_BASE)_$(ARCH):$(SERVICE_VERSION) -f ./Dockerfile .
	@docker image prune --filter label=stage=builder --force

You might want to add the above and see if that helps the test-all-arches succeed.

@mpeiffer
Copy link
Contributor Author

@jonwalicki Thank you for manually testing on your end! I was also able to build and run all three arch containers on windows. I'll try adding those changes and see if it fixes anything.

@johnwalicki
Copy link
Member

I think the error is in the GHA Build / test pipeline, not the code here.
Let's chat with @t-fine

@mpeiffer
Copy link
Contributor Author

That sounds good to me. Would it be possible to add the hacktoberfest-accepted label to this while we try to resolve the issue? If it turns out there's something on my end that needs to get done I can continue working on it past October.

@t-fine
Copy link
Collaborator

t-fine commented Oct 31, 2022

@mpeiffer @johnwalicki I'll add hacktoberfest-accepted label to this since you were able to build and run the services on all arches ... I'll see if I can't figure out what's going on with the GitHub action

@mpeiffer
Copy link
Contributor Author

mpeiffer commented Nov 1, 2022

@t-fine Thanks for looking into it! A label was added but for it to be counted it should be hacktoberfest-accepted, not hacktoberfest-approved.

Copy link
Collaborator

@t-fine t-fine left a comment

Choose a reason for hiding this comment

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

@johnwalicki @mpeiffer I figured it out! The problem was the arm arch build reusing the golang:1.19-alpine image that is built in the amd64 run and not rebuilding it for arm. You can see it's built on line 520 for amd64 then reused on line 659 for the arm build

Successful GitHub action build with the following changes.

@johnwalicki
Copy link
Member

@t-fine Thanks for the investigation and fix. Let's merge.

@t-fine
Copy link
Collaborator

t-fine commented Nov 2, 2022

Will do! Pushed the changes. Merging once the Github action passes again.

@t-fine t-fine merged commit 503f7d5 into open-horizon:master Nov 2, 2022
@mpeiffer
Copy link
Contributor Author

mpeiffer commented Nov 3, 2022

@johnwalicki @t-fine Thank you for looking over this and for pushing the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gps: aarch64/alpine docker container has been deprecated
4 participants