actions-runner-controller icon indicating copy to clipboard operation
actions-runner-controller copied to clipboard

chore: use xx-go for build, better buildx caching

Open pratikbin opened this issue 4 years ago • 4 comments

pratikbin avatar Jan 08 '22 13:01 pratikbin

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Feb 07 '22 14:02 stale[bot]

Inspired by this pr, I've enhanced our Dockerfile to use cache for rebuilds as in https://github.com/actions-runner-controller/actions-runner-controller/commit/5030e075a9853c9ebc5081023309c8a02b06a4e5 and https://github.com/actions-runner-controller/actions-runner-controller/commit/25570a0c6d62b83e5cd872df98a9450bdeb96b17. Perhaps the caching part of this pr isn't needed anymore? 🤔

Still unsure what to do for xx-go thing. I'm still learning the real benefit of using it. The cache thing shortened my local rebuild of ARC when there are some Go code changes, from 40s to 3~4s so it's benefit was obvious. I'm still unsure what metric I can consult for xx-go yet.

mumoshu avatar Mar 07 '22 01:03 mumoshu

Inspired by this pr, I've enhanced our Dockerfile to use cache for rebuilds as...

I haven't added cache because then we have to use buildx in local as well and for that we have to change Makefile. Thank you for adding cache

Still unsure what to do for xx-go thing. I'm still learning...

xx-go will assing TARGETPLATFORM to GOOS and GOARCH based on --plateform flag from buildx

pratikbin avatar Mar 07 '22 05:03 pratikbin

@mumoshu We can avoid using xx-go since you want the least dependencies in overall pipeline, so you can close this I guess. Let me know your thoughts

pratikbin avatar Mar 16 '22 01:03 pratikbin

@pratikbin Hey! Sorry for the looong silence. I finally got some time to revisit this and restarted by first reading https://github.com/tonistiigi/xx.

I now understand how xx-go set some envvars according to --platform as you told me in https://github.com/actions-runner-controller/actions-runner-controller/pull/1041#issuecomment-1060220775- great!

In addition to that, there's another benefit that the use of xx along with $BUILTPLATFORM enables you to let buildx merge and cache the build steps until it finally diverges at:

ARG TARGETPLATFORM
RUN xx-go build ...

and it can also prevent buildx cross build from triggerring build sessions for all the target platforms like arm64 and amd64, right?

That might give us extra boost on the build speed, if I'm correct. Our buildx cross builds seem to trigger concurrent builds for all the target platforms. Ideally, there should be the only one that runs natively (if you're on amd64, every build session should be on amd64 for native speed), and the use of BUILDPLATFORM along with xx-go would make it easy...

I'm still learning. But I'm definitely going to try it out myself and reuse some of your works with a lot of thanks.

Just wanted to let you know- Thanks again for your contribution.

mumoshu avatar Sep 25 '22 02:09 mumoshu

and it can also prevent buildx cross build to trigger build sessions for all the target platforms like arm64 and amd64, right?

Correct

That might give us extra boost on the build speed, if I'm correct

Also correct.

pratikbin avatar Sep 25 '22 07:09 pratikbin

@pratikbin Hey!! Thanks a lot for your confirmation. I had been experimenting with xx and my limited observation says #1877 might be the minimal change to achieve the goal of this pull request. WDYT? TL;DR; I couldn't see any build speed up. But xx did make our Dockerfile more readable.

mumoshu avatar Sep 29 '22 12:09 mumoshu

xx did make our Dockerfile more readable.

xx is dependency. And if it's only serving the purpose of readability, then we should avoid it.

PS: I was lowkey for a while and missed so many things in this project, Will go through all stuff once more and will see if I can optimize anything

pratikbin avatar Sep 29 '22 19:09 pratikbin

@pratikbin I greatly appreciate your patience and thoughtful feedback! Agreed. I'll also close my PR and be prepared to review your upcoming PRs in a more timely manner. Thanks again for your contribution 🙏

mumoshu avatar Sep 29 '22 23:09 mumoshu