-
Notifications
You must be signed in to change notification settings - Fork 178
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
arm64 support #90
arm64 support #90
Conversation
6d6ca05
to
2996fd9
Compare
please merge this, would really appreciate |
Any chance this could be reviewed? looks quite promising! |
I have tried building docker images for arm64 with the changes suggested in this PR and able to build and use the arm64 docker image. It would be helpful, if this PR is merged. |
Is there anything still needed here before Travis can start doing its thing? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, overall this is very nice, thanks. I have some questions and comments, and one thing that definitely needs to be addressed before this can be used.
.travis.yml
Outdated
after_success: | ||
- 'if [[ "$DIST" == "buster" ]] ; then sudo docker tag "bitnami/minideb:$DIST-$PLATFORM" "$BASENAME:latest-$PLATFORM" ; fi' | ||
- 'if [[ "$TRAVIS_BRANCH" == "master" ]] ; then sudo bash pushone $DIST $PLATFORM ; fi' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: This is being done here as it's on a different machine and so it can't be pushed in the deploy stage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly. Since every image is built on a different machine and there is no way to pass a docker image as artifact to another stage, the only way is to push the built image on after_success
after the build is finished.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correction: tar files created by buildone
can maybe passed as artifact and pushed only in deploy stage.
Is there any limit to artifact files on travis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not know.
One thing is that we run tests on the images after importing the tarball to the base image, which requires starting a container from the image. If we want to test all arches then that would have to be done on each arch I believe, which makes this the better way to do that, even with the pushing.
The alternative would be pushing images without tags to some holding registry, then having an artifact that reported the id and then doing the final push in deploy when everything is OK.
I'm not sure what is better, but this seems acceptable to me.
.travis.yml
Outdated
after_success: | ||
- 'if [[ "$DIST" == "buster" ]] ; then sudo docker tag "bitnami/minideb:$DIST-$PLATFORM" "$BASENAME:latest-$PLATFORM" ; fi' | ||
- 'if [[ "$TRAVIS_BRANCH" == "master" ]] ; then sudo bash pushone $DIST $PLATFORM ; fi' | ||
- 'if [[ "$DIST" == "buster" ]] ; then sudo bash pushone latest $PLATFORM ; fi' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (blocking): I think this also needs a guard that it is master branch as otherwise this will push the latest tag from PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right 👍
.travis.yml
Outdated
sudo docker manifest create bitnami/minideb:$DIST bitnami/minideb:$DIST-amd64 bitnami/minideb:$DIST-arm64 | ||
sudo docker manifest push bitnami/minideb:$DIST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: does this require the images to be on this machine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. The docker manifest
commands talks directly to the registry.
This is way the images bitnami/minideb:$DIST-amd64
and bitnami/minideb:$DIST-arm64
needs to be pushed with their own tags before this is executed.
docker tag "$built_image_id" "$BASENAME:$TAG-$PLATFORM" | ||
log "Tagged $built_image_id as $BASENAME:$TAG-$PLATFORM" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: is there some special handling in registries that means that this is understood to be a multiarch image? If not it seems like this means we are no longer going to update the existing tags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing tags will be updated by the docker manifest push
command as a multiarch tag, but the source images of the tag needs to be pushed before the docker manifest
command could be issued.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks.
pushone
Outdated
source <(curl -sSL "$CIRCLE_CI_FUNCTIONS_URL") | ||
# Use '.RepoDigests 0' for getting Dockerhub repo digest as it was the first pushed | ||
DIST_REPO_DIGEST=$(docker image inspect --format '{{index .RepoDigests 0}}' "${BASENAME}:${DIST}-${PLATFORM}") | ||
update_minideb_derived "https://github.com/bitnami/minideb-runtimes" "$DIST-${PLATFORM}" "$DIST_REPO_DIGEST" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: have you checked that this will have the right behaviour? My guess is that this won't do anything because the platform tags aren't recognised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really. And probably it won't work as expected reading the code on test-infra
.
Probably it needs an update on test-infra
, but should be executed after the deploy stage (after the docker manifest push
command succeeds).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that this should be moved out of pushone
in to the deploy stage then? Even if the different arches just build the tarball and the pushing is done at the end it seems like we would want this bit to be run after the manifest step, so that all arches are present and the tags are updated? That would seem to be the best behaviour unless those scripts are modified to understand multi-arch so that they can update each arch separately, but that doesn't seem like it would be useful currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now executing this on deploy stage on travis
qemu_build
Outdated
while do_sudo fuser /var/{lib/{dpkg,apt/lists},cache/apt/archives}/lock >/dev/null 2>&1; do | ||
sleep 1 | ||
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: This is done in the travis file, so also doing it here seems weird, suggest removing it unless there's a reason it needs to be repeated here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qemu_build
file is not intended to be executed on travis.
I often encountered a lock issue while working on this executing apt-get update
, so I copied this while loop from travis file. Is not really required, so can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I figured this out later and that makes sense, thanks.
qemu_build
Outdated
do_sudo apt-get update | ||
do_sudo apt-get install -y qemu-kvm libvirt-bin qemu-utils genisoimage virtinst curl rsync qemu-system-x86 qemu-system-arm cloud-image-utils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: other dependencies are installed from the Makefile
, is it being done here as these aren't always needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is not needed in every case, but only if executing a build with qemu, so they are required to create a build environment (but not on travis for example).
Makefile
install requirements inside the build environment (also on travis).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I would suggest making this another Makefile target? I'm not a fan of apt-get running every time you use a script as it wastes time once the dependencies are installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 added .installed-qemu
make target
qemu_build
Outdated
mkdir -p .kvm-images/{amd64,arm64} | ||
|
||
if [[ ! -f .kvm-images/amd64/bionic-server-cloudimg-amd64.img && "$PLATFORM" == "amd64" ]]; then | ||
curl -SL https://cloud-images.ubuntu.com/bionic/current/bionic-server-cloudimg-amd64.img > .kvm-images/amd64/bionic-server-cloudimg-amd64.img |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: do Debian offer equivalent images?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe debian openstack images could be used (https://cdimage.debian.org/cdimage/openstack/current/) but I'm not sure about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine given that this is mainly for local testing. It just struck me a bit odd to be using Ubuntu to build Debian.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just replaced the images with Debian ones.
qemu_build
Outdated
fi | ||
|
||
|
||
debian_snapshot_id=${3:-} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: not having this with the other params, and using lowercase where they use caps seems out of place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
qemu_build
Outdated
do_ssh "apt-get update && apt-get install -y apt-transport-https debootstrap debian-archive-keyring jq dpkg-dev rsync ca-certificates curl gnupg-agent software-properties-common && mkdir /build" | ||
do_ssh "curl -fsSL https://download.docker.com/linux/ubuntu/gpg | apt-key add -" | ||
do_ssh "add-apt-repository \"deb [arch=$PLATFORM] https://download.docker.com/linux/ubuntu \$(lsb_release -cs) stable\"" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: this duplicates some things that are done elsewhere. I wonder if copying over the Makefile and using that or something similar would make it easier to maintain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, trying to use Makefile
to install common requirements now.
Not sure if useful (maybe not for minideb but for the downstream repos building on it?), but for the record here is a neat example of multi-arch building using docker-tools' joshuamorris3/namecheap-ddns-update#6 It seems The resulting docker tags have digests for all the different architectures: https://hub.docker.com/r/joshuamorris3/namecheap-ddns-update/tags |
a8f347a
to
7779615
Compare
I think there's a couple of things to fix, but I think a maintainer could mark this as ready for review now, and at least we could find out what Travis thinks of building the images. |
7779615
to
1745656
Compare
1745656
to
deb0332
Compare
Hi, This is cool, thanks for your contribution, I think it looks good now. Unfortunately there's nothing more I can do to help at the moment. Hopefully a maintainer sees this and can move the PR forward. Thanks, James |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @alekitto great work here! my apologies for this late response.
It seems most of the questions were resolved and well handled by @james-w (thanks James). Mines are more in the snapshot feature and how it is going to be handle and some other restrictions with our Travis version.
Please, could you take a look at them?
- <<: *build_job | ||
arch: arm64-graviton2 | ||
env: | ||
- DIST=stretch PLATFORM=arm64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I think we need to use arm64
since arm64-graviton2
is only supported in .com
our project is built in .org
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't notice that build was running on .org
. Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway on .org
now there's a warning stating that will be shut down in a few weeks. If the build will be migrated to .com
maybe we should revert to graviton2
which should have better performance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There could be a problem with arm64
architecture on travis-ci.org
: it could run only in unpriviledged LXD container.
To correctly run debootstrap
a full vm is needed, which is only available in arm64-graviton2
.
I also tried to execute a QEMU build into a full amd64 vm, but it is extremely slow.
Stating that .org
is about to shut down, maybe a migration to .com
could be planned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The third option is to try to build the images on GitHub Actions.
It shouldn't be too difficult to make a test workflow. The question is: will the execution be too slow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The question is: will the execution be too slow?
where travis runs stages sequentially on the same machine (multiple machines also doable), github runs jobs in parallel on multiple machines by default. this should solve any speed issues probably?
docker provides a qemu setup action for multiarch GH workflows.
Im I'm not mistaken, GH by default runs up to four jobs on four runners (each runner 2CPU/7GB) concurrently (might be four not sure anymore) which can be decreased as well. all you'd need to do is define a job with a step that runs the qemu_build script with args ${{matrix.distro}} ${{matrix.other_arg}}
, then define your custom matrix on the top of the job and you should be all set for parallel builds :)
for example, this CI runs in parallel on new commits to master, and all commits to all PRs (and emulates Travis's auto-cancel previous runs feature) , plus CD runs once upon a GitHub Release or Prerelease. as alternative to split files, add a push step to the job that only runs on a release/prerelease event:
on:
pull_request:
push:
branches: master
release:
types: [released, prereleased]
workflow_dispatch: # allows triggering manually from the Actions tab
...
- name: Push image
if: github.event_name == 'release'
run: docker push
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the records: I just tried to build the images on GH Actions on my fork (master
branch to test the push job), without using qemu_build
as performance was too bad.
This is the last working run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
14 mins for buster arm64..
+1 for the effort though! plz dont delete the branch :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2ce5677
to
1ad9efe
Compare
1ad9efe
to
555918d
Compare
while sudo fuser /var/{lib/{dpkg,apt/lists},cache/apt/archives}/lock >/dev/null 2>&1; do | ||
sleep 1 | ||
done | ||
- sudo rm -f /usr/local/bin/jq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? (deleting jq)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .installed-requirements
make target fails without removing the travis-included jq executable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhm, which error was it throwing? If the package was already installed it should simply ignore it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't remember the exact error message, but the jq
executable is not installed in travis machine via apt.
Apt simply refuses to overwrite an existing file.
while sudo fuser /var/{lib/{dpkg,apt/lists},cache/apt/archives}/lock >/dev/null 2>&1; do | ||
sleep 1 | ||
done | ||
- sudo rm -f /usr/local/bin/jq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhm, which error was it throwing? If the package was already installed it should simply ignore it.
.travis.yml
Outdated
|
||
if [[ "$DISTS_WITH_SNAPSHOT" =~ (^|[[:space:]])"$DIST"($|[[:space:]]) ]] ; then | ||
SNAPSHOT_NAME="$DIST-snapshot-$(./snapshot_id)" | ||
sudo docker manifest create $SNAPSHOT_NAME:$DIST $SNAPSHOT_NAME:$DIST-amd64 $SNAPSHOT_NAME:$DIST-arm64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you meant $BASENAME:$SNAPSHOT_NAME
(and the -amd64, -arm64 variants)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -43,8 +43,9 @@ log() { | |||
|
|||
build() { | |||
DIST=$1 | |||
PLATFORM=${2:-amd64} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should consider the $PLATFORM name when querying the registry in:
if docker pull "$BASENAME:$TAG" > /dev/null; then
...
}
If not, when comparing the amd64 image (at least now that we only have a amd64 one, but is not labeled -amd64), the check at then end is probably going to determine the image is up to date:
else
log "Image didn't change"
return
fi
And so it will skip tagging it:
docker tag "$built_image_id" "$BASENAME:$TAG-$PLATFORM"
This may not be a problem when all is up and running, but at least for the first time, it will fail because the tag does not exists when creating the manifest.
Maybe just appending it at the beginning would work:
if [ -n "$debian_snapshot_id" ]; then
TAG="${DIST}-snapshot-${debian_snapshot_id}-$PLATFORM"
else
TAG=$DIST-$PLATFORM
fi
That would also require tweaking the "test" script to check the image name correspond to jessie. From:
if [ "jessie" == "$DIST" ]; then
to something like
if [[ "$DIST" == "jessie"* ]]; then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
- stage: deploy | ||
if: branch = master AND type = push | ||
env: | ||
- DISTS="stretch buster latest" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As jessie does not get a multiplatform manifest and we are tagging it as jessie-amd64
, we should add some extra logic to tag it as simply jessie
too, for backwards compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I've added a build job for jessie image push.
Probably there's something to be corrected in update_minideb_derived
to check multiple digests, but is out of scope of this PR.
@alekitto Ok, I believe this is ready to land :). Thanks a lot for your help adding this feature and for your responsive answers to our feedback! |
Sorry, there's an error in |
I already fixed 99 and 100 but missed 57. Don't worry, we are handling it. Thanks for offering! |
Related to #76
I needed arm based images for one of my work, so I modified the build scripts to build and push multiarch images to docker hub.
I'm publishing my work as a draft hoping that could be useful to support arm64 architecture.
The trickiest part was to test the build on my local environment, so I wrote the
qemu_build
script that installs qemu requirements, downloads the ubuntu image for the target platform and executes a build inside an emulated environment. It than exports the images into a tar, transfers it from the vm to the local environment and imports it as docker image with the right tag.The script is not intended to run on CI environment like travis, as without KVM the build is very slow, but could be useful to test multi arch builds in local environments, or test new archs other than amd64 and arm64.
Hope this could help.