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

refactor: cross compile for arm64 image build #222

Merged
merged 4 commits into from
Jan 17, 2024
Merged

Conversation

KevFan
Copy link
Contributor

@KevFan KevFan commented Oct 24, 2023

Building arm64 image via QEMU takes over 4 hours on github actions which makes the development / release process less than ideal.

Building this image via cross compile decreases this to ~20mins

On Mac M1, tested this with:

  • amd64 - docker pull quay.io/kuadrant/limitador:quicker-workflow --platform=linux/amd64

image

  • arm64 - docker pull quay.io/kuadrant/limitador:quicker-workflow

image

@KevFan KevFan marked this pull request as draft October 24, 2023 08:29
@@ -37,6 +37,8 @@ const_format = "0.2.31"
lazy_static = "1.4.0"
clap = "4.3"
sysinfo = "0.29.7"
openssl = { version = "0.10.57", features = ["vendored"] }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dockerfile Outdated Show resolved Hide resolved
file: ${{ matrix.dockerfile }}
platforms: |
${{ matrix.platform }}
provenance: false
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
Contributor Author

Choose a reason for hiding this comment

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

Failed to use ubi as the build / run stage due issues mentioned by @eguzki in #214 #215

So the main discussion here is are we fine with using community / non-productised images for this dockerfile / build for arm64 ? 🤔

Personally I dont think it should block this due to:

  • Downstream requirements should generally not leak into the upstream
  • Productised builds should be built on a different infra, which should instead use a "productised" Dockerfile which must use productised images as a requirement.
    • The main Dockerfile which use the ubi image can serve as the "productised" dockerfile which both the amd64 & arm64 image can be built from anyway. Not sure is this "productised" dockerfile usually hosted / provided elsewhere anyway in which case the main dockerfile should follow a similar structure to this dockerfile instead as it's simpler overall
  • The quicker build time (20mins) outweighs the downstream requirement to use productised images here since it delays the development flow / community release flow significantly (4 hours to build arm64 image)

Disadvantages to this is that:

  • Need to maintain 2 dockerfiles
  • The dockerfiles uses different build / runtime stages to each other currently
  • Potentially get rate limited by dockerhub occassionally since we use community images

Interested in peoples thoughts on this espeically @alexsnaps @didierofrivia @eguzki @gsaslis

Copy link
Contributor

Choose a reason for hiding this comment

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

in case it helps, the community Dockerfile is different from the product one - but it makes perfect sense to keep the differences to a minimum.

Copy link
Contributor

Choose a reason for hiding this comment

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

UBI images are publicly available and should be preferred whenever possible. Check out Reasons to Use UBI at https://www.redhat.com/en/blog/introducing-red-hat-universal-base-image.

Personally, I don't see this as leaking requirements from the downstream into the upstream, but as recognizing that behind open source there's a business. We should not kill the business, neither punish the upstream. Instead, we need to find a balance that lets both to thrive as efficient as possible. Generally, UBI is a good compromise.

In contrast, our experience in the past of keeping multiple Dockerfiles and especially with different base images between upstream and downstream has been total chaos. A lot worse than waiting 4 hours for a build to finish. Believe me!

If building for arm64 on UBI base is as inefficient as x12 slower, pressing Red Hat to fix that should be our top priority.

Meanwhile, maybe building for arm64 could be triggered automatically only when merging to main/release-* branches and manually on-demand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @guicassolato, good points ! 👍 I guess the only thing is Red Hat support of rust seems pretty minimal ( https://catalog.redhat.com/software/containers/rhel8/rust-toolset/5b9c6aa85a134643ef2ecc63?architecture=amd64&image=607765c8d70cc51ae2772196 / https://catalog.redhat.com/software/containers/search?gs&q=rust) and the main issue I believe to building on ubi using cross compile is it's missing a libc6-dev-arm64-cross equivalent that is available in Debian (#215). I can reach out to the ubi team on their thoughts to supporting this, but I'm guessing this may take a while.

I guess my perspective is that for the upstream, I'm not sure do we need to / should confine ourselves to using ubi8 at the expense of the developer experience / release flow for limitador itself. Especially if / when the productised build comes, it will be built elsewhere with another dockerfile that needs to be maintained regardless.

Many other Red Hat projects looks to use the community images in their upstream (golang equivalents seems a lot simpler):

A compromise is that we can have the runtime stage to be ubi but the build stage to use a community image which looks to have worked for me after some tinkering (docker pull quay.io/kuadrant/limitador:ubi9-test) 🤔 :

Yeah, I agree maintaining multiple Dockerfiles may not be worth it in the end (for example, with the current approach, the arm64 build might break on runtime due to glibc versioning mismatch that isn't caught until someone runs this dockerfile / pulls the arm64 image and runs it 🤔). I'll defer to @alexsnaps @didierofrivia @eguzki for the best course of action as they are the main recent contributiors for whether these changes are worth the potential future headache 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

To add one more point of view:

In general, keeping the same parent image between community + product build (and trying to minimize other differences) actually helps us deliver more value to the community - because we need to spend less time porting to the product build those community changes that we want to ship as part of the product. That way, we can spend more of our time working on the community project ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

My 5 cents:

We use rust based images in community. It makes life easier (as of today) for the upstream project. And the upstream is what gets the daily work. Downstream is automated and who cares if it takes 20min or 2H? And maybe downstream does not even require cross compiling. At least for ARM. Maybe requires images for RH products in IBM (powerpc and z mainframes).

I reckon the benefits of being aligned (regarding the base image) with downstream and the use of UBI images. But only when the cost is not higher than the benefit. IMO the cost is higher than the benefits as of today.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me support my opinion with one illustrating example:

  • Rust based dockerfile
FROM rust:1.72 as limitador-build
RUN apt update && apt upgrade -y
RUN apt install -y protobuf-compiler clang
WORKDIR /usr/src/limitador
ARG GITHUB_SHA
ENV GITHUB_SHA=${GITHUB_SHA:-unknown}
ENV RUSTFLAGS="-C target-feature=-crt-static"
COPY . .
RUN cargo build --release``
  • Ubi based dockerfile
FROM registry.access.redhat.com/ubi8/ubi:8.7 as limitador-build
ENV CARGO_NET_GIT_FETCH_WITH_CLI=true
ARG RUSTC_VERSION=1.72.0
# the powertools repo is required for protobuf-c and protobuf-devel
RUN dnf -y --setopt=install_weak_deps=False --setopt=tsflags=nodocs install \
      http://mirror.centos.org/centos/8-stream/BaseOS/`arch`/os/Packages/centos-gpg-keys-8-6.el8.noarch.rpm \
      http://mirror.centos.org/centos/8-stream/BaseOS/`arch`/os/Packages/centos-stream-repos-8-6.el8.noarch.rpm \
 && dnf -y --setopt=install_weak_deps=False --setopt=tsflags=nodocs install epel-release \
 && dnf config-manager --set-enabled powertools
RUN PKGS="gcc-c++ gcc-toolset-12-binutils-gold openssl-devel protobuf-c protobuf-devel git clang kernel-headers" \
    && dnf install --nodocs --assumeyes $PKGS \
    && rpm --verify --nogroup --nouser $PKGS \
    && yum -y clean all
RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- --no-modify-path --profile minimal --default-toolchain ${RUSTC_VERSION} -c rustfmt -y
WORKDIR /usr/src/limitador
ARG GITHUB_SHA
ENV GITHUB_SHA=${GITHUB_SHA:-unknown}
ENV RUSTFLAGS="-C target-feature=-crt-static"
COPY . .
RUN source $HOME/.cargo/env \
    && cargo build --release

UBI based image requires the usage of centos repos in the limitador project (protobuf-c and protobuf-devel packages)

Copy link
Contributor

Choose a reason for hiding this comment

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

In the effort to use the UBI as base image, for ARM images, this PR is building on rust based images and then in the next stage fitting the binary into an ubi image. This is, IMO, an additional cost of building in UBI for the limitador project. Some extra complexity we will need to maintain and that will be the source of future issues for sure.

@KevFan KevFan added the kind/enhancement New feature or request label Dec 7, 2023
@KevFan KevFan self-assigned this Dec 7, 2023
@KevFan KevFan added size/medium kind/investigation dependencies Pull requests that update a dependency file labels Dec 7, 2023
@KevFan KevFan marked this pull request as ready for review December 7, 2023 13:14
@alexsnaps alexsnaps added this to the Limitador Server v1.4.0 milestone Dec 8, 2023
.github/workflows/build-image.yaml Outdated Show resolved Hide resolved
.github/workflows/build-image.yaml Outdated Show resolved Hide resolved
@eguzki
Copy link
Contributor

eguzki commented Dec 12, 2023

LGTM

I would merge this and open discussion about the community base image.

Dockerfile Outdated
@@ -2,19 +2,19 @@
# Build Stage
# ------------------------------------------------------------------------------

FROM registry.access.redhat.com/ubi8/ubi:8.7 as limitador-build
FROM registry.access.redhat.com/ubi9/ubi:9.2 as limitador-build
Copy link

Choose a reason for hiding this comment

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

Could you also use the bullseye image as you do in aarch64? In that way, both of the builds would use the same images and should be much closer than are right now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can, as long we are "happy" that this Dockefile will no longer be a reflection of what the productised Dockerfile will be 🤔

.github/workflows/build-image.yaml Outdated Show resolved Hide resolved
# Build Stage cross compiling
# ------------------------------------------------------------------------------

# Use bullseye as build image instead of Bookworm as ubi9 does not not have GLIBCXX_3.4.30
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

KevFan added 3 commits January 8, 2024 15:22
Building arm64 image via QEMU takes over 4 hours on github actions which makes the development / release process less than ideal.
Building this image via cross compile decreases this to ~20mins
@KevFan
Copy link
Contributor Author

KevFan commented Jan 8, 2024

Rebased with main

dockerfiles: |
./Dockerfile
context: .
labels: ${{ steps.meta.outputs.labels }}
Copy link
Member

Choose a reason for hiding this comment

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

Which labels are set by default from the output of the meta step?

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 the JSON output for this build step (taken from the workflow logs):

{
    "tags": [
      "quay.io/kuadrant/limitador:quicker-workflow"
    ],
    "labels": {
      "org.opencontainers.image.created": "2024-01-08T15:23:11.687Z",
      "org.opencontainers.image.description": "Rate limiter",
      "org.opencontainers.image.licenses": "Apache-2.0",
      "org.opencontainers.image.revision": "a39035119c7899901dfc785e[59](https://github.com/Kuadrant/limitador/actions/runs/7449493828/job/20266248072?pr=222#step:4:68)380d52b9b11986",
      "org.opencontainers.image.source": "https://github.com/Kuadrant/limitador",
      "org.opencontainers.image.title": "limitador",
      "org.opencontainers.image.url": "https://github.com/Kuadrant/limitador",
      "org.opencontainers.image.version": "quicker-workflow"
    },
    "annotations": [
      "manifest:org.opencontainers.image.created=2024-01-08T15:23:11.[68](https://github.com/Kuadrant/limitador/actions/runs/7449493828/job/20266248072?pr=222#step:4:77)7Z",
      "manifest:org.opencontainers.image.description=Rate limiter",
      "manifest:org.opencontainers.image.licenses=Apache-2.0",
      "manifest:org.opencontainers.image.revision=a39035119c7899901dfc785e59380d52b9b11986",
      "manifest:org.opencontainers.image.source=https://github.com/Kuadrant/limitador",
      "manifest:org.opencontainers.image.title=limitador",
      "manifest:org.opencontainers.image.url=https://github.com/Kuadrant/limitador",
      "manifest:org.opencontainers.image.version=quicker-workflow"
    ]
  }

@KevFan KevFan merged commit 4ebe4ac into main Jan 17, 2024
20 checks passed
@eguzki eguzki deleted the quicker-workflow branch January 18, 2024 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file kind/enhancement New feature or request kind/investigation size/medium
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants