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.

Sign up for GitHub

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

chore: switch docker base image, use system rocksdb for faster compilation #487

Merged
merged 9 commits into from
Oct 22, 2024
Merged

chore: switch docker base image, use system rocksdb for faster compilation #487

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
88cfb62
chore: switch docker base image, use system rocksdb for faster compil…
tedil Oct 8, 2024
4f11a7d
switch to ubuntu-24.04 runner, install dev deps
tedil Oct 9, 2024
4f8f23f
fix cargo path
tedil Oct 9, 2024
d1eb5b7
switch to ubuntu-24.04 runner for protobuf workflow as well
tedil Oct 9, 2024
2c0f07a
swap coverage action
tedil Oct 9, 2024
05d01b0
fix cargo path
tedil Oct 9, 2024
19f5e48
use setup-python action
tedil Oct 9, 2024
d3753f2
install rocksdb deps
tedil Oct 10, 2024
0bc5f0d
Adding missing librocksdb8.9 install in Dockerfie
holtgrewe Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/automerge.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ permissions:

jobs:
dependabot:
runs-on: ubuntu-latest
runs-on: ubuntu-24.04
holtgrewe marked this conversation as resolved.
Show resolved Hide resolved
if: ${{ github.actor == 'dependabot[bot]' }}
steps:
- name: Enable auto-merge for Dependabot PRs
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/conventional-prs.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ on:

jobs:
title-format:
runs-on: ubuntu-latest
runs-on: ubuntu-24.04
steps:
- uses: amannn/[email protected]
env:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/docker-build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ env:

jobs:
build-and-push-image:
runs-on: ubuntu-latest
runs-on: ubuntu-24.04
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Update Required: protobuf.yml Still Uses ubuntu-latest

The script identified that .github/workflows/protobuf.yml is still configured to run on ubuntu-latest. To maintain consistent workflow environments, please update this file to use ubuntu-24.04 as well.

  • File to Update:
    • .github/workflows/protobuf.yml
🔗 Analysis chain

Approve the update to Ubuntu 24.04, but verify consistency across workflows

The change from ubuntu-latest to ubuntu-24.04 is in line with the PR objectives to switch the Docker base image. This update ensures a consistent and predictable environment for the workflow execution.

To ensure consistency across all workflow files, please run the following script:

If the script returns any results, consider updating those files to use ubuntu-24.04 as well for consistency.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if all workflow files use ubuntu-24.04

# Test: Search for any occurrences of ubuntu-latest in workflow files
rg --type yaml 'runs-on:.*ubuntu-latest' .github/workflows

Length of output: 118

permissions:
contents: read
packages: write
Expand Down
11 changes: 8 additions & 3 deletions .github/workflows/release-please.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ env:
jobs:
release-please:
if: github.repository_owner == 'varfish-org'
runs-on: ubuntu-latest
runs-on: ubuntu-24.04
outputs:
release_created: ${{ steps.release.outputs.release_created }}
release_name: ${{ steps.release.outputs.major }}.${{ steps.release.outputs.minor }}.${{ steps.release.outputs.patch }}
Expand All @@ -31,12 +31,17 @@ jobs:

cargo-release:
needs: release-please
runs-on: ubuntu-latest
runs-on: ubuntu-24.04
if: ${{ needs.release-please.outputs.release_created }}
steps:
- uses: actions/checkout@v4
if: ${{ needs.release-please.outputs.release_created }}

- name: Install dependencies
run: |
sudo apt-get update -y
sudo apt-get install -y librocksdb-dev libsnappy-dev libsqlite3-dev

- name: Install stable toolchain
uses: actions-rs/toolchain@v1
if: ${{ needs.release-please.outputs.release_created }}
Expand Down Expand Up @@ -64,7 +69,7 @@ jobs:

container-release:
needs: release-please
runs-on: ubuntu-latest
runs-on: ubuntu-24.04
if: ${{ needs.release-please.outputs.release_created }}
steps:
- name: Checkout repository
Expand Down
13 changes: 9 additions & 4 deletions .github/workflows/rust.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ on:

jobs:
Formatting:
runs-on: ubuntu-latest
runs-on: ubuntu-24.04
steps:
- name: Checkout repository
uses: actions/checkout@v4
Expand All @@ -28,15 +28,20 @@ jobs:
repo-token: ${{ secrets.GITHUB_TOKEN }}

- name: Check format
run: |
run:
cargo fmt -- --check

Linting:
runs-on: ubuntu-latest
runs-on: ubuntu-24.04
steps:
- name: Checkout repository
uses: actions/checkout@v4

- name: Install dependencies
run: |
sudo apt-get update -y
sudo apt-get install -y librocksdb-dev libsnappy-dev libsqlite3-dev

- name: Install stable toolchain
uses: actions-rs/toolchain@v1
with:
Expand All @@ -56,7 +61,7 @@ jobs:

Testing:
needs: Formatting
runs-on: ubuntu-latest
runs-on: ubuntu-24.04
steps:
- name: Checkout repository
uses: actions/checkout@v4
Expand Down
36 changes: 36 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,3 +359,39 @@ For running protolint, install it as python package `protolint-bin`:
# source /tmp/varfish-server-worker/bin/activate
# pip install protolint-bin
```

### Building from scratch
To reduce compile times, we recommend using a pre-built version of `rocksdb`, either from the system package manager or e.g. via `conda`:

```bash
# Ubuntu
sudo apt-get install librocksdb-dev

# Conda
conda install -c conda-forge rocksdb
```

In either case, either add
```toml
[env]
ROCKSDB_LIB_DIR = "/usr/lib/" # in case of the system package manager, adjust the path accordingly for conda
SNAPPY_LIB_DIR = "/usr/lib/" # same as above
```
to `.cargo/config.toml` or set the environment variables `ROCKSDB_LIB_DIR` and `SNAPPY_LIB_DIR` to the appropriate paths:

```bash
export ROCKSDB_LIB_DIR=/usr/lib/
export SNAPPY_LIB_DIR=/usr/lib/
```

By default, the environment variables are defined in the `.cargo/config.toml` as described above, i.e. may need adjustments if not using the system package manager.

To build the project, run:
Comment on lines +374 to +389
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

LGTM with a minor correction: Clear configuration instructions

The instructions for configuring the build environment using either .cargo/config.toml or environment variables are clear and provide flexibility for different setups. The note about default values in .cargo/config.toml is helpful for users to understand the existing configuration.

There's a minor typo in the comment on line 379. It should be "do" instead of "to". Please apply the following change:

-SNAPPY_LIB_DIR = "/usr/lib/"  # same as above
+SNAPPY_LIB_DIR = "/usr/lib/"  # do same as above

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~379-~379: “to” seems less likely than “do”.
Context: ..._DIR = "/usr/lib/" # same as above ``` to .cargo/config.toml or set the environ...

(AI_HYDRA_LEO_CP_TO_DO)

```bash
cargo build --release
```

To install the project locally, run:
```bash
cargo install --path .
```
30 changes: 15 additions & 15 deletions utils/docker/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,16 @@
# Builder
# ---------------------------------------------------------------------------

# Pinning Rust version for now because of this issue:
#
# - https://github.com/rust-lang/rust/issues/95926
FROM rust:1-bookworm AS builder
# Use ubuntu:noble as the base image
FROM ubuntu:noble AS builder
Comment on lines +16 to +17
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider using a stable Ubuntu LTS base image

Switching the base image to ubuntu:noble might introduce instability if noble is not a long-term support (LTS) release or is still under development. For enhanced stability and support, consider using an LTS version like ubuntu:22.04.

Apply this diff to change the base image to an LTS version:

-FROM ubuntu:noble AS builder
+FROM ubuntu:22.04 AS builder
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Use ubuntu:noble as the base image
FROM ubuntu:noble AS builder
# Use ubuntu:22.04 as the base image
FROM ubuntu:22.04 AS builder


# Build dependencies first.
#
# Install dependencies for compilation of C code (e.g., rocksdb).
# Install Rust toolchain and dependencies for compilation of C code (e.g., rocksdb)
RUN apt-get update && \
apt-get install -y clang
# Add the needed Cargo components.
RUN rustup component add rustfmt
apt-get install -y unzip wget curl build-essential clang librocksdb-dev libsqlite3-dev && \
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && \
. $HOME/.cargo/env && \
rustup component add rustfmt
Comment on lines +21 to +24
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Ensure Rust and its environment are properly installed

Manually installing Rust using the curl script requires careful handling to set up the environment correctly. To ensure that Rust is available in subsequent commands without sourcing the environment every time, consider adding Rust to the PATH globally.

Apply this diff to update the Rust installation:

 RUN apt-get update && \
     apt-get install -y unzip wget curl build-essential clang librocksdb-dev libsqlite3-dev && \
     curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && \
-    . $HOME/.cargo/env && \
+    echo 'source $HOME/.cargo/env' >> /root/.bashrc && \
     rustup component add rustfmt

This change adds the Rust environment to the .bashrc file, ensuring that it's loaded in all subsequent steps.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
apt-get install -y unzip wget curl build-essential clang librocksdb-dev libsqlite3-dev && \
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && \
. $HOME/.cargo/env && \
rustup component add rustfmt
apt-get install -y unzip wget curl build-essential clang librocksdb-dev libsqlite3-dev && \
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && \
echo 'source $HOME/.cargo/env' >> /root/.bashrc && \
rustup component add rustfmt


# Install build dependency `protoc`.
COPY utils/install-protoc.sh /tmp
RUN PREFIX=/usr/local bash /tmp/install-protoc.sh
Expand All @@ -49,18 +47,20 @@ COPY protos /usr/src/varfish-server-worker/protos/
# COPY utils/alpine-linker-script.sh /usr/src/varfish-server-worker/utils/
# RUN chmod a+rx /usr/src/varfish-server-worker/utils/alpine-linker-script.sh
# COPY .cargo /usr/src/varfish-server-worker/.cargo/
## Touch main.rs to prevent cached release build.

# Touch main.rs to prevent cached release build.
RUN touch /usr/src/varfish-server-worker/src/main.rs
# This is the actual application build.
RUN cargo build --release

# Build the application
RUN /root/.cargo/bin/cargo build --release
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify the Cargo build command

Avoid hardcoding the path to the Cargo binary. Since Rust's bin directory is added to the PATH, you can invoke cargo directly.

Apply this diff to simplify the build command:

-RUN /root/.cargo/bin/cargo build --release
+RUN cargo build --release
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
RUN /root/.cargo/bin/cargo build --release
RUN cargo build --release


# ---------------------------------------------------------------------------
# Runtime
# ---------------------------------------------------------------------------

FROM debian:bookworm-slim AS runtime
FROM ubuntu:noble AS runtime
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Align runtime base image with a stable release

Using ubuntu:noble as the runtime base image may lead to compatibility issues if noble is not an officially released or stable version. For consistency and reliability, consider using the same stable LTS version as in the builder stage.

Apply this diff to update the runtime base image:

-FROM ubuntu:noble AS runtime
+FROM ubuntu:22.04 AS runtime
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
FROM ubuntu:noble AS runtime
FROM ubuntu:22.04 AS runtime


# Install dependencies (and cleanup afterwards)
# Install dependencies (and cleanup afterward)
RUN apt-get update && \
apt-get install -y libsqlite3-0 && \
apt-get clean autoclean && \
Expand Down
Loading