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

Make base images multiarch #55

Closed
wants to merge 18 commits into from

Conversation

martin-g
Copy link
Contributor

@martin-g martin-g commented Apr 10, 2023

Add support for Linux ARM64 to base-glibc-busybox-bash and base-glibc-debian-bash images.

Update the Github runners to 20.04 because the 18.04 ones never start (they stay in Queued state for days). See https://github.blog/changelog/2022-08-09-github-actions-the-ubuntu-18-04-actions-runner-image-is-being-deprecated-and-will-be-removed-by-12-1-22/

Fix warnings:

  • update actions/checkout to v4, so that it does not use the deprecated Nodejs v12
  • use >> $GITHUB_OUTPUT instead of the deprecated set-output

Multi-arch images built by my fork could be seen here:

@@ -93,8 +102,8 @@ jobs:
# FIX upstream: Quay.io does not support immutable images currently.
# => Try to use the REST API to check for duplicate tags.
respone="$(
curl -sL \
'https://quay.io/api/v1/repository/bioconda/${{ steps.buildah-build.outputs.image }}/image'
curl -sL -H "Authorization: Bearer ${{ secrets.QUAY_BIOCONDA_TOKEN }}" \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not tested because the secrets are not exported to forks/PRs.
Without the Authorization header Quay returns 403 (Forbidden)

@@ -1,3 +1,4 @@
ARG arch=x86_64
# Use the exact conda, mamba versions as used in bioconda-recipes' builds.
ARG bioconda_utils_version='1.1.3'
FROM quay.io/bioconda/bioconda-utils-build-env-cos7:${bioconda_utils_version} as bioconda-build-env
Copy link

Choose a reason for hiding this comment

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

It seems the image should be quay.io/bioconda/bioconda-utils-build-env-cos7 for x86 and quay.io/bioconda/bioconda-utils-build-env-cos7-aarch64 for aarch64?

[1] https://github.com/bioconda/bioconda-utils/pull/866/files#diff-8b50c1c8ac07bc956388add66e256e8336ae22fd2cac8d83f770e1dc63674c00R20-R25

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this has to be updated once bioconda/bioconda-utils#866 is merged!

@@ -13,7 +14,7 @@ FROM quay.io/bioconda/base-glibc-debian-bash as build
WORKDIR /tmp/work
COPY --from=bioconda-build-env /tmp/requirements.txt ./
COPY install-conda print-env-activate create-env ./
ADD https://repo.anaconda.com/miniconda/Miniconda3-latest-Linux-x86_64.sh ./miniconda.sh
ADD https://repo.anaconda.com/miniconda/Miniconda3-latest-Linux-${arch}.sh ./miniconda.sh
Copy link

Choose a reason for hiding this comment

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

curious is it something special so that we have to use build-args but not wget according build arch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, this is outdated. Please see the latest version of the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But now I notice that I need to add multi-arch build to the GHA workflow!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Improved with 1b4b7e2

env:
# The base image is not intended to change often and should be used with
# version tags or checksum IDs, but not via "latest".
IMAGE_VERSION: '2.1.0'
IMAGE_VERSION: '3.0.0'
Copy link

Choose a reason for hiding this comment

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

Is it a normal upgrade or for special fixing?

Copy link
Contributor Author

@martin-g martin-g Apr 11, 2023

Choose a reason for hiding this comment

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

I bumped the major version because now the image would be multi-arch and because of the update of Ubuntu version and the GLIBC coming with it.
But since there are no changes for the x86_64 users maybe we should bump the minor/patch version ?! I am OK to update the PR with the preferred change by the maintainers!

Copy link
Member

Choose a reason for hiding this comment

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

The only reason I can think of to keep the older version is to use an older libc for testing to make sure our packages for on older HPC. But this should be ensure with the build container ... so following this and trusting the build container I think we could bump the runtime container.

@bioconda/build-system any thoughts?

Copy link
Contributor Author

@martin-g martin-g Apr 11, 2023

Choose a reason for hiding this comment

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

I just bumped the version of create-env image too to 3.0.0, so it is in sync with the other images in this PR.
As I said above I'd be happy to change the version to whatever the Bioconda team thinks it should be!

@martin-g
Copy link
Contributor Author

martin-g commented Apr 12, 2023

We are ready with the next PR for bioconda-containers - https://github.com/martin-g/bioconda-containers/pull/2/files
It depends on this PR to be merged and the new multiarch container images to be available at Quay.io. And also depends on the new bioconda-utils-build-env-cos7-aarch64 which will be provided by bioconda/bioconda-utils#866

The new/next PR will provide images create-env-x86_64 and create-env-aarch64 (https://github.com/martin-g?tab=packages&tab=packages&q=create-)

Copy link

@Yikun Yikun left a comment

Choose a reason for hiding this comment

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

bioconda-utils build --docker --mulled-test --packages py2bit --docker-base-image ghcr.io/yikun/bioconda-utils-build-env-cos7-aarch64 --mulled-conda-image ghcr.io/martin-g/create-env-aarch64 --force

I also do a e2e test with martin-g#2 (ghcr.io/martin-g/create-env-aarch64), it works! So LGTM except inline comment/question.

.github/workflows/create-env.yaml Outdated Show resolved Hide resolved
container="$( buildah from "${image_id}" )"
buildah config --label=glibc="${glibc}" "${container}"
buildah config --label=debian="${debian}" "${container}"
labels="
Copy link

Choose a reason for hiding this comment

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

buildah config --label=glibc="${glibc}" "${container}"
buildah config --label=debian="${debian}" "${container}"

Not sure what did the original buildah do? Just label variable? Was it used by some other where?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

OK, great!

@martin-g
Copy link
Contributor Author

Friendly ping!

@martin-g
Copy link
Contributor Author

martin-g commented Jun 5, 2023

Any feedback on the proposed changes ?

@julien-faye
Copy link

These changes look like exactly what I need!
Let me test them and I'll comment back!

@julien-faye
Copy link

I had to build bioconda-utils-build-env-cos7-aarch64 myself locally although bioconda/bioconda-utils#866 is merged and I expected to find it at https://quay.io/repository/bioconda/bioconda-utils-build-env-cos7-aarch64

Then I built create-env-aarch64 by using this PR!

And following the steps explained at bioconda/bioconda-recipes#23454 (comment) I was able to build several Bioconda packages for Linux aarch64!

Great work, @martin-g !

I hope this PR is merged soon and all aarch64 Docker images are uploaded to Quay.io so that there is no need to use patches and build them locally !

@emiliofernandes
Copy link

+1 for this PR!
Looking forward for it to be merged soon!

@johanneskoester
Copy link

I have never touched this repo so far, so any help with fixing the remaining issues would be greatly appreciated.

@martin-g
Copy link
Contributor Author

martin-g commented Oct 4, 2023

@johanneskoester Do you have an idea what is needed for #55 (comment) ?
https://quay.io/repository/bioconda/bioconda-utils-build-env-cos7-aarch64 gives 404.
The CI job for bioconda/bioconda-utils#866 failed to publish it due to authentication issues.

@johanneskoester
Copy link

@johanneskoester Do you have an idea what is needed for #55 (comment) ? https://quay.io/repository/bioconda/bioconda-utils-build-env-cos7-aarch64 gives 404. The CI job for bioconda/bioconda-utils#866 failed to publish it due to authentication issues.

Thanks for the reminder. Looking into that now. I think the bot did not have the permissions to push, but that should be solved now. Let's see whether the next release (today) succeeds.

@johanneskoester
Copy link

@johanneskoester Do you have an idea what is needed for #55 (comment) ? https://quay.io/repository/bioconda/bioconda-utils-build-env-cos7-aarch64 gives 404. The CI job for bioconda/bioconda-utils#866 failed to publish it due to authentication issues.

Thanks for the reminder. Solved now and 2.5.0 has been properly pushed!

@martin-g
Copy link
Contributor Author

martin-g commented Oct 4, 2023

Great!
I will update this PR to use it!

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

Try with redhat-actions/buildah-build and redhat-actions/push-to-registry

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
…ay.io

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
To be in sync with the other image versions in this PR

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Its value is newline separated and breaks the loop syntax

Fix the name of a step - it is steps.build.outputs.image, not
buildah-build anymore

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
@martin-g
Copy link
Contributor Author

martin-g commented Oct 4, 2023

I am going to merge martin-g#2 into this PR.

@martin-g
Copy link
Contributor Author

martin-g commented Oct 4, 2023

The CI jobs fail with:

W: The repository 'http://security.debian.org/debian-security stretch/updates Release' does not have a Release file.
W: The repository 'http://deb.debian.org/debian stretch Release' does not have a Release file.
W: The repository 'http://deb.debian.org/debian stretch-updates Release' does not have a Release file.
E: Failed to fetch http://security.debian.org/debian-security/dists/stretch/updates/main/binary-amd64/Packages  404  Not Found [IP: 151.101.2.132 80]
E: Failed to fetch http://deb.debian.org/debian/dists/stretch/main/binary-amd64/Packages  404  Not Found [IP: 146.75.38.132 80]
E: Failed to fetch http://deb.debian.org/debian/dists/stretch-updates/main/binary-amd64/Packages  404  Not Found [IP: 146.75.38.132 80]
E: Some index files failed to download. They have been ignored, or old ones used instead.

for debian:9.
The comment says that the usage of Debian 9 is intentional (to use old version of Glibc) but the image is broken these days ...

@martin-g
Copy link
Contributor Author

martin-g commented Oct 4, 2023

Is it OK to use Debian 10 instead ?
Or should I remove the problematic entries in /etc/apt/sources.list before executing apt-get update ?

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
@martin-g
Copy link
Contributor Author

martin-g commented Oct 5, 2023

With c1e64e2 I updated sources.list to use http://archive.debian.org for the debian:9 Docker image.

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
@martin-g
Copy link
Contributor Author

martin-g commented Oct 5, 2023

Now I remember why create-env needs to be in a second step:

FROM quay.io/bioconda/base-glibc-debian-bash as build

here it depends on quay.io/bioconda/base-glibc-debian-bash - the ARM64 flavor of this image is created by this PR.

But this PR CI checks cannot pass because

curl -H "Authorization: Bearer $TOKEN" \
-sL \
'https://quay.io/api/v1/repository/bioconda/${{ steps.buildah-build.outputs.image }}/image'
makes a REST API call to Quay.io that fails with error 403 (authentication) in PRs because Github secrets are not exported to PRs.

@martin-g
Copy link
Contributor Author

martin-g commented Oct 5, 2023

@johanneskoester Could you please allow the execution of the checks one more time? Thanks!

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
@mbargull
Copy link
Member

mbargull commented Oct 7, 2023

Hi @martin-g,

thank you (and also everyone else above!) for working on this, much appreciated.
I'm currently reviewing this (looking good thus far!) and will help to get these changes published during the next few days.

(Incidentally, Debian 12.2 is also scheduled to be released today, so we might combine updating the base images with that version. I'm doing some sanity checks for that currently.)

I'll get back to you tomorrow or Monday at the latest.

Cheers,
Marcel

@mbargull
Copy link
Member

mbargull commented Oct 9, 2023

Quick update:
I'm still checking and debugging some bits and bobs here.

Currently, I'm digging into why busybox wget does not work under QEMU (haven't checked on native ARM64 yet).
It's not just our build of BusyBox but also the one from docker-library (both glibc and musl) and Debian.
Alpine's one work fine (they dyn. link to musl and have a bunch of patches and custom config).
Native x86_64 work fine, too, of course.

# podman run --arch=arm64 --rm docker://arm64v8/busybox:1.36.1-glibc busybox wget -q -S --no-check-certificate https://www.busybox.net/news.html
wget: error getting response: Connection reset by peer
# podman run --arch=arm64 --rm docker://arm64v8/busybox:1.36.1-musl busybox wget -q -S --no-check-certificate https://www.busybox.net/news.html
wget: error getting response: Connection reset by peer
# podman run --arch=arm64 --rm docker://arm64v8/debian:12.1-slim sh -c 'apt-get update -qq && DEBIAN_FRONTEND=noninteractive apt-get install -qy busybox > /dev/null && busybox wget -q -S --no-check-certificate https://www.busybox.net/news.html'
debconf: delaying package configuration, since apt-utils is not installed
wget: error getting response: Connection reset by peer
# podman run --arch=arm64 --rm docker://arm64v8/alpine:3.18.4 busybox wget -q -S --no-check-certificate https://www.busybox.net/news.html
  HTTP/1.1 200 OK
  Date: Mon, 09 Oct 2023 20:46:02 GMT
  Server: Apache
  Strict-Transport-Security: max-age=63072000
  Accept-Ranges: bytes
  X-Content-Type-Options: nosniff
  X-Frame-Options: DENY
  X-XSS-Protection: 1
  Connection: close
  Transfer-Encoding: chunked
  Content-Type: text/html
  
# podman run --arch=amd64 --rm docker://amd64/busybox:1.36.1-glibc busybox wget -q -S --no-check-certificate https://www.busybox.net/news.html 
  HTTP/1.1 200 OK
  Date: Mon, 09 Oct 2023 20:46:53 GMT
  Server: Apache
  Strict-Transport-Security: max-age=63072000
  Accept-Ranges: bytes
  X-Content-Type-Options: nosniff
  X-Frame-Options: DENY
  X-XSS-Protection: 1
  Connection: close
  Transfer-Encoding: chunked
  Content-Type: text/html
  
# podman run --arch=amd64 --rm docker://amd64/debian:12.1-slim sh -c 'apt-get update -qq && DEBIAN_FRONTEND=noninteractive apt-get install -qy busybox > /dev/null && busybox wget -q -S --no-check-certificate https://www.busybox.net/news.html'
debconf: delaying package configuration, since apt-utils is not installed
  HTTP/1.1 200 OK
  Date: Mon, 09 Oct 2023 20:47:25 GMT
  Server: Apache
  Strict-Transport-Security: max-age=63072000
  Accept-Ranges: bytes
  X-Content-Type-Options: nosniff
  X-Frame-Options: DENY
  X-XSS-Protection: 1
  Connection: close
  Transfer-Encoding: chunked
  Content-Type: text/html
  

Once that one's figured out, I'll cherry-pick your changes into multiple PRs to update the base images and then the other images.

(docker://debian:12.2 is also not out yet but likely happening in the next days; though we don't have to wait for that.)

@martin-g
Copy link
Contributor Author

There are many reports for busybox + wget + TLS: https://github.com/docker-library/busybox/issues?q=Connection+reset+by+peer and https://github.com/mirror/busybox/issues?q=Connection+reset+by+peer

The recommendation is to use alpine: docker-library/busybox#64 (comment) and docker-library/busybox#134 (comment)

$ docker run --platform=arm64 -it --rm alpine wget -S -c https://www.busybox.net/news.html

works fine!

I tried many older versions of busybox but all of them fail here.

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
@mbargull
Copy link
Member

There are many reports for busybox + wget + TLS [...]

Yes, you'd normally not want to use BusyBox's wget with TLS etc. without backing it with a well-tested TLS library (it supports using openssl s_client).

The recommendation is to use alpine [...]

For some use cases that makes sense, yes.
For us not so much. If we were to use Alpine we'd essentially have images with the size of Alpine plus our current image's size (minus the ~800 kB for BusyBox) because we want an image with glibc and Bash (which comprise most of the image's size). It's also not bad to have the two base images somewhat similar.


I just wanted to make sure that we don't introduce (avoidable) differences between the amd64 and aarch64 images.
I checked on native ARM everything works as expected too -- so all good!
(Turns out the problem is just that BusyBox expects some (not standard-mandated) behavior of vfork which QEMU doesn't meet. So, only a problem if someone runs our images via QEMU. We can easily patch that the wget applet -- no idea if other (more important) applet are affected too...)


I'm currently working on incorporating the version updates and hope we can finish the images tomorrow.

@mbargull
Copy link
Member

After more unfortunate delays, we are nearly there with the container builds.
gh-59 is still building bioconda/create-env currently (I need to trim down the tests for that; 10 minutes or so previous run time is north of an hour under the emulator... not very economically...).

In gh-58 I cherry-picked your commits for the base images and those are available on quay.io/bioconda (well, after fixing a little hiccup with gh-60).

There were quite some unexpected hurdles with differences under QEMU and/or aarch64, Buildah versions, bumpy manifest handling, directory layout changes in Debian 12, etc. pp.
The build scripts are, admittedly, a lengthy, under-documented, redundancy-laden mess (I'll try to rectify this in the near-ish future) -- but the image contents seem fine as far as my local inspections/tests and the CI tests show thus far.

I'll wait for one test run of gh-59 to finish and then trim down/skip some tests (at least when run via QEMU), so that we can have those built and pushed today.

We now have x86_64 and aarch64 base images updated to the current Debian/BusyBox versions (also prepared for minimal work needed for future Debian >=13 updates) available at quay.io/bioconda/base-glibc-busybox-bash:3.0and quay.io/bioconda/base-glibc-debian-bash:3.0.

Thanks very much for keeping to push this -- half a year on this PR, finally coming to fruition!

@martin-g
Copy link
Contributor Author

w00t!
#59 is green and (I think) fast enough!

I am going to close this PR since it is done with #58 (and #60).

@martin-g martin-g closed this Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants