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

Docker image fixes to avoid runtime errors in ubuntu/linux machines #1114

Closed
wants to merge 4 commits into from

Conversation

afsalthaj
Copy link
Contributor

@afsalthaj afsalthaj commented Dec 3, 2024

Why now?

I had to do this to try golem as a user before its 1.1 release to see if anything obvious is missing and patch the code straight away. On top of that the main purpose of this PR is as follows:

In case anyone reports issues with 1.1 release, we got this PR to come back to and see if it solves their problem. As discussed within the team, if this doesn't resolve the issue, we will not be spending much time trying to fix it though

Context

This is NOT regarding cross compilation builds or docker-compose builds, but actually running golem docker images in latest ubuntu, and that resulted in incompatibility errors at runtime (which we don't test anywhere). I don't think anyone tested running golem other than Mac (or if at all they ran, it accidentally worked)

I haven't spent much time finding out the right version of ubuntu or the cleanest approach (or the slimmest image with the right version since this task is unplanned), and therefore simply using latest to avoid incompatibility issues.

Find the details, below to get more clarity:

Tests conducted

Environment

Ubuntu with multipass, with the following version

ubuntu@foo:~$ ldd --version
ldd (Ubuntu GLIBC 2.39-0ubuntu8.3) 2.39
Copyright (C) 2024 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Written by Roland McGrath and Ulrich Drepper
Commands Run

cargo make --profile ci build-release with necessary platform overrides, within ubuntu, and then built images with the current docker-files successfully, however simply running any of them failed with lots of errors.

Errors without this change:

Most importantly trying to run them in an ubuntu, resulted in glibc compatibilities (not to mention other errors encountered before narrowing it down to glibc incompatibilities). The cause was Debian slim image was at 2.34 while the required one for our app built in ubuntu-latest needed a specific version.

With the changes in the PR, there are no more runtime issues running golem images in Ubuntu.

@afsalthaj afsalthaj marked this pull request as ready for review December 4, 2024 05:59
@@ -16,8 +16,8 @@ WORKDIR /app
COPY /target/$RUST_TARGET/release/golem-component-compilation-service ./
COPY /golem-component-compilation-service/config/ ./config/

RUN apt-get update && apt-get install -y libssl-dev
RUN apt-get update && apt-get install -y ca-certificates
RUN apt-get update --allow-releaseinfo-change && apt-get install -y libssl-dev
Copy link

@kmatasfp kmatasfp Dec 4, 2024

Choose a reason for hiding this comment

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

not sure if --allow-releaseinfo-change is needed here. Could be be needed if we used pinned version of ubuntu, but as we use ubuntu:latest it will handle the repo information change like suite, codename, version etc and we will be installing from a most up to date repo always. And I do not see us manually adding to/modifying /etc/apt/sources.list which could warrant this option

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 think I needed --allow-releaseinfo-change to get something working.

Copy link

Choose a reason for hiding this comment

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

Just verified with fresh ubuntu vm that --allow-releaseinfo-change was not needed, also doing apt-get update twice is not needed, all of this can be replaced with

RUN apt-get update && apt-get install -y libssl-dev ca-certificates

# Find more info below if you are having issues running this command(example: Running from MAC may fail)
# Target has to be x86_64-unknown-linux-gnu or aarch64-unknown-linux-gnu-gcc
cargo build --release --target x86_64-unknown-linux-gnu
Use `multipass` or similar platforms, with a reasonable disk space (since it's the easiest to get a ubuntu)
Copy link

@kmatasfp kmatasfp Dec 4, 2024

Choose a reason for hiding this comment

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

might be worth mentioning that we recommend ubuntu box with following dependencies installed on top of rust toolchain to compile golem:

sudo apt install pkg-config libssl-dev libfontconfig1-dev

As they might use their own tooling to get ubuntu vm (vagrant, https://www.vagrantup.com/, for example is really popular) or already have access to some other ubuntu box

Copy link

@kmatasfp kmatasfp Dec 5, 2024

Choose a reason for hiding this comment

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

also dependent on you starting point, this might be needed

sudo apt install build-essential

Will get linker 'cc' not found otherwise

Copy link

Choose a reason for hiding this comment

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

Also this

sudo apt-get install protobuf-compiler

@afsalthaj
Copy link
Contributor Author

@kmatasfp Feel free to raise PRs on what you did on your end to get these things working like https://about.gitea.com/

@kmatasfp
Copy link

kmatasfp commented Dec 5, 2024

After you merge this PR I will create a followup PR that describes how to tag and publish built container images to registry, be it self hosted like gitea or dockerhub, github, ecr so you can actually use these built images on your mac which is end goal for most devs

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.

2 participants