[WIP] Add ARM64 support #803
Conversation
|
|
|
@radu-matei I added building of multi-arch manifest.
https://cloud.docker.com/repository/docker/alxandr/brigade-generic-gateway/tags |
|
Question: I'm getting some warnings while building the worker: However, they don't seem to directly affect building the image. Do we at least know what is causing them? |
|
I have the same warnings, and no. I'm not quite sure. As you said, they don't seem to be affecting the image. Google has been most unhelpful. What operating system are you running on? I was hoping that building on linux might not give this error (or a different one). That being said, thinking about it (while typing); I would guess the issue is yarn downloading native (pre-built) dependencies of some sort? |
|
I'm currently testing on Windows, but in a couple of hours I can also test on Linux and macOS. |
|
I need to actually test that the resulting arm images work first. I'm looking at the helm chart to see what values I need to modify to deploy to my cluster. |
|
I tested the build, build images, push images, and push manifest bits of this PR. Let us know how actually deploying Brigade on an ARM cluster goes, and I'd also like to ping @vdice and/or @adamreese since this PR pokes at the Makefile. Thanks! |
|
Also on the TODO: the brigade.js file might need changing. I've not really looked at it though, but I assume it also builds the project? |
|
Yeah, and all releases are actually done through Brigade, so that will also need updating. |
|
I'm getting the following error from vacuum: Any idea what could be causing this? |
|
Figured it out. I was putting the amd64 binary in the arm image -.- I can now run jobs successfully: |
|
@vdice - could you have a look at this when you get a couple of minutes? Thanks! |
|
@vdice I did a major overhaul of the makefile. Targets that are based on files (like how make is supposed to work) as far as is possible etc. I've not tested the manifest creation yet, but building single or multiple arches works. The dockerfiles now also have arguments, such that they will not go out of sync. However, as a downside, the makefile is now harder to understand (though I guess that's subjective). Please take a look at it and see if you like it, if not I can go back to scratch and see if it can be done in another way. Though considering how you're using make (all of the targets are |
There was a problem hiding this comment.
This is fantastic work, @Alxandr ! I, for one, am all in favor of an overhaul of the Makefile it it means an improved and more make-appropriate use. The immediate benefits I see are:
- Finally harnessing the power/functionality of Make with file-based targets where appropriate, thereby only processing/re-building files when their source has changed.
- Dynamic generation of arch-specific Dockerfiles
- Target generation for the various combinations of: brigade component, os, arch now fully captured by re-usable functions and corresponding foreach loops
As you've mentioned, this approach definitely requires a bit more time to understand. I'm hoping adding more comments (with examples), as suggested in a PR comment here, helps in this regard.
There remain a few important points that I think we want to ensure before going this direction:
maketargets continue to work for all platforms. Here, I'm most interested in @radu-matei 's WSL experience and not so much worried about Mac (my testing looks great w/ some caveats mentioned in PR comments) -- and certainly less so w/ Linux :)- Ensure
make buildby default only builds the os-arch combination (for brigade component binaries inbin/) matching the developer's host machine. I'm still seeing all three oflinux,darwinandwindowsbeing built when I run this target. (Also mentioned in a PR comment on this review.) - Ensure the momentum to cut/release Brigade v1.0.0 isn't disrupted (not super worried, just anticipating the CI/
brigade.jschanges)
I'm glad to help sorting out changes needed in brigade.js when we get some of the details above sorted out. Again, the work put in on this overhaul (make-over 😄) is very much appreciated and I personally do like the direction.
Makefile
Outdated
| # This is for debugging the makefile | ||
| .PHONY: list-targets | ||
| list-targets: | ||
| @$(MAKE) -pRrq -f $(lastword $(MAKEFILE_LIST)) : 2>/dev/null | awk -v RS= -F: '/^# File/,/^# Finished Make data base/ {if ($$1 !~ "^[#.]") {print $$1}}' | sort | egrep -v -e '^[^[:alnum:]]' -e '^$@$$' | xargs |
There was a problem hiding this comment.
Nice -- and crucial -- with how the targets are now generated.
Can we separate each target by a newline? I did so via xargs -0.
Makefile
Outdated
|
|
||
| .PHONY: build build-images push-images dockerfiles | ||
|
|
||
| define GO_CMD_TARGETS |
There was a problem hiding this comment.
I'm thinking some further comments around each of the define sections would help new users -- and even us after time goes by! -- more quickly grok how it all ties together.
For example, maybe we add, godoc style :
# GO_CMD_TARGETS generates go build targets using the provided
# Brigade component ($1) and architecture ($2), e.g., brig/rootfs/amd64/rootfs
#
# See the `foreach` loop that invokes this function - elsewhere in this file - for how these targets are generated
define GO_CMD_TARGETS
...
Definitely open as to wording, but at a high-level being sure to mention what each arg ($1, $2, etc.) corresponds to and a literal example.
Makefile
Outdated
| $(foreach cmd,$(BINS),$(eval $(call BRIG_STATIC_ARCH_TARGETS,$2,$1,$(cmd)))) | ||
| endef | ||
|
|
||
| define GO_ARCH_TARGETS |
There was a problem hiding this comment.
I'm wondering if we move lines 128-140 further up at the top, before the definitions of the functions that are invoked in each foreach statement (BRIG_STATIC_TARGETS, etc.).
Then the flow to understand would be: seeing these meta targets and foreach loops, then digging in to each looped function for how each parameterized target is built...
There was a problem hiding this comment.
I'm an F#-er, so I'm used to reading files with the exports at the bottom, and the implementation details at the top, hence the ordering (up to a point where I confused myself I think). Will try to move targets that are more likely to be called higher up.
Makefile
Outdated
| $1: $1-images | ||
|
|
||
| create-$1-manifest: push-$1-images | ||
| # Remove old manifest if present |
There was a problem hiding this comment.
Are there any drawbacks in removing the need for this cleanup logic and instead adding --amend to the docker manifest create?
I explored this option as the cleanup logic broke down for me when my registry was a non-docker.io registry (ACR) -- so the manifest file was named differently ($(DOCKER_REGISTRY)_$1-$(IMAGE_VERSION)). Instead of updating the cleanup logic, I thought it'd be great if we could get rid of it altogether if docker manifest create --amend ... would work for us.
There was a problem hiding this comment.
Honestly, I don't know. The docker manifest commands are so badly documented, that I'm not even sure what they really do. I've just found a way to use them that works 😛. My main issue with doing amend would be that you would possibly just elongate the manifest list in repeated builds. This shouldn't be a problem as long as the manifest tag and the image tags are the same, but I would have to test it. Also, when doing manifest push, we can add --purge (with the option to disable it for debugging purposes), which will purge after pushing.
Makefile
Outdated
| arm64_DOCKER_ANNOTATIONS = --os linux --arch arm64 --variant v8 | ||
| arm32_DOCKER_ANNOTATIONS = --os linux --arch arm | ||
|
|
||
| .PHONY: build build-images push-images dockerfiles |
There was a problem hiding this comment.
Should we add a top-level manifests target as well? (Or, update-manifests, etc., to follow pattern of <verb>-<noun>)
Then, we could add the following in the DOCKER_MANIFEST_TARGETS function:
manifests: create-$1-manifest
Flow for CI would then be: make build-images push-images manifests for building/pushing/updating manifests.
There was a problem hiding this comment.
Yeah, the names should probably be walked over (put in a list, drawn in a tree, and then painted on a wall a few times, to get a good feel for them 😛). I spent quite a while on this, re-discovering make while doing it (been a while since I did a make template), so my state of mind probably changed quite a bit while writing it, leading to inconsistencies. Doing a dependency-graph in comments at the top of the file would probably be a good idea.
git-sidecar/Dockerfile.arm64v8
Outdated
| @@ -0,0 +1,13 @@ | |||
| FROM arm64v8/alpine:3.8 | |||
There was a problem hiding this comment.
Was there a reason to keep this arch-specific Dockerfile around? Or can it be removed now that they are dynamically generated?
Makefile
Outdated
|
|
||
| # Create arch dockerfile | ||
| $1/Dockerfile.$2: $1/Dockerfile | ||
| @cat $1/Dockerfile | ARCH="$2" QEMU_IMPORT="$$($2_QEMU_IMPORT)" DOCKER_ARCH="$$($2_DOCKER_ARCH)" envsubst '$$$${ARCH} $$$${QEMU_IMPORT} $$$${DOCKER_ARCH}' > $1/Dockerfile.$2 |
There was a problem hiding this comment.
We'll need to check for the existence of envsubst and either install in a platform-agnostic manner if it doesn't exist -- or exit out and inform the user that it is a required dependency to run the targets needing it.
As a Mac user, I didn't have it by default, although I did have the package that contained it (gettext). For me, the key was the latter command from: brew install gettext && brew link --force gettext
There was a problem hiding this comment.
I did the same command :). Alternatively, using something like go templates might be a better idea?
There was a problem hiding this comment.
Just from half a minute of googeling, go get github.com/hairyhenderson/gomplate/cmd/gomplate maybe?
There was a problem hiding this comment.
Nice -- if that utility works, a go get-able option would be a good choice.
Makefile
Outdated
|
|
||
| .PHONY: helm-install | ||
| helm-install: helm-upgrade | ||
|
|
||
| .PHONY: helm-upgrade | ||
| helm-upgrade: | ||
| helm upgrade --install $(BRIGADE_RELEASE) brigade/brigade --namespace $(BRIGADE_NAMESPACE) \ | ||
| --set brigade-github-app.enabled=true \ | ||
| --set gw.service.type=ClusterIP \ |
There was a problem hiding this comment.
I'd like to go back to how master looks for this target (gw.* sets removed in place of --set brigade-github-app.enabled=true), as we hope to deprecate the classic GH gateway (gw) in favor of the brigade-github-app gateway.
There was a problem hiding this comment.
This was probably just a merge conflict. Pretty sure I didn't intentionally touch the helm-upgrade target.
Makefile
Outdated
|
|
||
| define GO_CMD_TARGETS | ||
| # Build command | ||
| $1/rootfs/$2/$1: vendor $(shell find $1 -type f -name '*.go') |
There was a problem hiding this comment.
I must admit I'm still trying to figure out where the change is needed :) but I'm seeing these (docker binary) image targets being called when I run make build.
I think I prefer maintaining the pattern of make build only building go binaries (and placing into /bin) corresponding to the os+arch of the host machine.
Then, these docker binary build targets (putting the linux+arch binaries in /rootfs) would only be run, as needed, by make build-images (and/or, if we had the need to run separately, e.g. via make build-docker-bins)
There was a problem hiding this comment.
Fair enough. I'll try drawing a dependency graph with the names. Then we can discuss how it should work after it's more visible. Makes it easier to make naming changes like this.
|
Hi @Alxandr , an update from today's (2/19/19) Brigade Dev Meeting. Although we/I will still continue to test/review the functionality presented in this pull request, the main concern at this time is to reduce the potential for speed bumps/issues in releasing Brigade 1.0.0. As the Makefile has been fully overhauled and the core maintainers have yet to fully understand its new design/mechanisms, the expectation is that the earliest this work would be merged would be after this milestone. |
|
Fair enough. I have things running on my cluster, which was my main concern. I have one issue though, which I would probably label as a "bug" (but would not matter if this branch got merged): I need to modify the worker of every new project I make, even though I set the worker in the values for the helm template. Other than that, things are working just as they should on my end. |
|
I am also interested in Linux/ARM64 Docker image release for ‘brigade’. According to the previous comments, it seems that the Linux/ARM64 docker image addition was in your backlog, but due to v1.0.0 milestone, the activity was on-hold. May I know why this activity is still on-hold? Kindly let me know if there is anything I can do, I will be happy to help and contribute. |
|
@odidev I cannot speak directly to why this PR stalled out two years ago. I can tell you that, currently, all our available resources are being directed at Brigade 2.0. We would love to add ARM64 support and It's certain to happen in some version 2.x > 2.0. I'm also certain the state of the art in building multi-architecture Docker images has advanced in the two years since this PR was touched. Assistance is welcome with multi-arch builds for 2.0. See the v2 branch. |
|
One issue was that we didn't have the proper infrastructure for CI - and I'm still not sure if we have now. I believe @squillace may have some more info around ARM CI? |
|
I've (personally) moved from ARM back to X86, so I don't really have a stake in this anymore, but I did keep up to date on multi-arch images etc, so I can say that there are several new(ish) things there that make things way easier than when I started on that journey (I don't remember where in the spectrum this image falls). For anyone who wants to look at this, I recommend looking at "docker buildx bake". I will close this PR as I have no idea what's in it anymore :P. |
|
@radu-matei @krancour @Alxandr Thank you for the clarifications. I tried building the package (v2 branch) locally on Linux x86_64 and ARM64 machines using ‘make’ command. The build was successful on the x86_64 machine, and amd64 binaries were generated in the ‘bin’ folder. May I know if you are planning to use Github actions to include the linux jobs to the CI in the development branch ‘v2’? If yes, then it will be feasible to add the Linux/ARM64 docker image build and release process via GA CI. Also, are you planning to add Linux/ARM64 support in the 2.0 release? If yes, then I will be happy to contribute for the ARM64 docker image release process. Kindly share your opinion on this. |
We have used GitHub Actions for only one thing-- building the Windows log agent image. And the reason we're doing that is because building a Window-based Docker image inside another Windows-based Docker container does not seem to be possible or practical yet. With this one exception, we're avoiding GitHub Actions for the sake of "dogfooding" Brigade. Our CI and CD are, and will remain, based on Brigade. (Full transparency, we're currently using Brigade 1.4.something to build the
Reiterating what I already said:
Also, @radu-matei has raised an excellent point that we may still be lacking the infrastructure to accomplish this. i.e. I'm not sure we have access to ARM64 VMs right now. |
@krancour @radu-matei , Can't we use Travis-CI for the same as it has support for multi-architecture builds. |
|
@odidev While Brigade isn't exclusively for CI, CI is a popular use case for Brigade. To the greatest extent we're able, we prefer to implement our CI by "dogfooding" our own product. I think I've previously indicated this preference for dogfooding. For the time being, using Travis or Circle or similar is off the table. I can see you're keen to get ARM64 support. You're welcome to explore whatever options may exist that don't violate our preference for dogfooding. |
|
Per #803 (comment), @radu-matei, I will insist on ARM outputs at some point, and I will arrange to pay for them as well. Lemme look into this more deeply. (I'm on Team Pinebook over here.) As to anything but Brigade in the Brigade automation, I'm completely on Team Dogfood. FWIW and if anyone cares. |
|
@krancour , I've built the package on my local Arm64 system. It is getting built successfully there but to build this we need the Docker images of |
What this PR does / why we need it: It adds ARM64 support to brigade, allowing it to run on ARM (and hybrid ARM64/AMD64) clusters.
If applicable:
No code changes have been performed, so it should not need unit tests, nor break backwards compatibility. It might warrant an update to docs just to say it supports ARM though?
Resolves #773