chore: switch docker base image, use system rocksdb for faster compilation #487
Conversation
WalkthroughThe changes in this pull request involve updating several workflow files to use Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
.github/workflows/release-please.yml (2)
40-43: Approve with minor suggestions for improvementThe addition of this step to install
librocksdb-devaligns well with the PR objective of using system RocksDB for faster compilation. This should indeed help in reducing build times.A couple of minor suggestions:
- The
-yflag is unnecessary forapt-get updateas it doesn't prompt for confirmation. You can simplify it tosudo apt-get update.- Consider adding error handling or at least using
set -eto ensure the workflow fails if the package installation encounters any issues.Here's a slightly improved version:
- name: Install dependencies run: | set -e sudo apt-get update sudo apt-get install -y librocksdb-dev
40-43: Consider broader implications of this changeThe addition of the RocksDB system package is a positive change that should improve build times. However, consider the following points for the broader project:
Consistency: Are there other jobs or workflows in the project that could benefit from using the system RocksDB? It might be worth reviewing other CI/CD processes to ensure consistency.
Documentation: Ensure that the README, contribution guidelines, or any relevant documentation is updated to reflect this change in the build process. This will help other contributors understand the new build requirements.
README.md (2)
365-372: LGTM: Clear instructions for installing rocksdbThe instructions for installing
rocksdbusing both Ubuntu's package manager and Conda are clear and accurate. This covers two common scenarios, making it easier for developers to set up their environment.Consider adding a note about potential variations in package names or availability across different Ubuntu versions or other Linux distributions.
389-397: LGTM: Clear build and installation instructionsThe instructions for building and installing the project are clear, concise, and use appropriate flags (
--releasefor building and--path .for local installation).Consider adding a brief explanation of what the
--releaseflag does (e.g., "builds with optimizations") and why it's recommended for this project. This would be helpful for developers who are less familiar with Cargo.utils/docker/Dockerfile (1)
Line range hint
63-67: Optimize layer caching and image sizeCombine the cleanup commands into a single
RUNstatement to reduce the number of layers and optimize the image size. Also, consider using--no-install-recommendswithapt-get installto prevent the installation of unnecessary packages.Apply this diff to optimize the installation and cleanup steps:
RUN apt-get update && \ - apt-get install -y libsqlite3-0 && \ - apt-get clean autoclean && \ - apt-get autoremove --yes && \ + apt-get install -y --no-install-recommends libsqlite3-0 && \ + apt-get clean && \ rm -rf /var/lib/{apt,dpkg,cache,log} /var/lib/apt/lists/*
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .github/workflows/release-please.yml (1 hunks)
- README.md (1 hunks)
- utils/docker/Dockerfile (2 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~379-~379: “to” seems less likely than “do”.
Context: ..._DIR = "/usr/lib/" # same as above ``` to.cargo/config.tomlor set the environ...(AI_HYDRA_LEO_CP_TO_DO)
🔇 Additional comments (2)
README.md (2)
362-364: LGTM: Clear introduction to building from scratchThe new section title and introductory sentence effectively explain the purpose of the following instructions, which is to reduce compile times using pre-built versions of
rocksdb.
362-397: Great addition to the README: Comprehensive build instructionsThese new instructions for building the project from scratch are a valuable addition to the README. They align well with the PR objective of switching the Docker base image and using system rocksdb for faster compilation. The step-by-step guide covers dependency installation, configuration options, and build/installation process, making it easier for developers to set up their environment efficiently.
The instructions are well-structured, clear, and cover multiple scenarios (Ubuntu and Conda). This addition will significantly improve the developer experience and potentially reduce issues related to build setup.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~379-~379: “to” seems less likely than “do”.
Context: ..._DIR = "/usr/lib/" # same as above ``` to.cargo/config.tomlor set the environ...(AI_HYDRA_LEO_CP_TO_DO)
| 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: |
There was a problem hiding this comment.
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 aboveCommittable 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.tomlor set the environ...(AI_HYDRA_LEO_CP_TO_DO)
| RUN cargo build --release | ||
|
|
||
| # Build the application | ||
| RUN /root/.cargo/bin/cargo build --release |
There was a problem hiding this comment.
🛠️ 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.
| RUN /root/.cargo/bin/cargo build --release | |
| RUN cargo build --release |
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Runtime | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| FROM debian:bookworm-slim AS runtime | ||
| FROM ubuntu:noble AS runtime |
There was a problem hiding this comment.
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.
| FROM ubuntu:noble AS runtime | |
| FROM ubuntu:22.04 AS runtime |
| # Use ubuntu:noble as the base image | ||
| FROM ubuntu:noble AS builder |
There was a problem hiding this comment.
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.
| # 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 |
| 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 |
There was a problem hiding this comment.
🛠️ 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 rustfmtThis 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.
| 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 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #487 +/- ##
======================================
- Coverage 74% 73% -2%
======================================
Files 42 43 +1
Lines 7365 11648 +4283
======================================
+ Hits 5470 8514 +3044
- Misses 1895 3134 +1239 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
.github/workflows/rust.yml (2)
35-35: Approve changes with suggestions for improvement.The update to Ubuntu 24.04 and the addition of dependency installation are good improvements. However, there are a couple of suggestions:
- Remove
sudofrom the apt commands as GitHub Actions runners already have root privileges.- Consider using
apt-get install -y --no-install-recommendsto minimize the image size.Here's a suggested improvement for the dependency installation step:
- name: Install dependencies run: | - sudo apt-get update -y - sudo apt-get install -y librocksdb-dev libsnappy-dev libsqlite3-dev + apt-get update -y + apt-get install -y --no-install-recommends librocksdb-dev libsnappy-dev libsqlite3-dev + rm -rf /var/lib/apt/lists/*This change removes
sudo, adds--no-install-recommends, and cleans up apt lists to reduce the image size.Also applies to: 40-43
Line range hint
1-94: Overall approval with suggestions for further improvements.The changes to update all jobs to Ubuntu 24.04 and add explicit dependency installation for the Linting job are good improvements. They provide a more consistent and controlled CI environment. However, consider the following suggestions:
- Review if similar dependency installation steps are needed for the Formatting and Testing jobs. This would ensure all jobs have the necessary dependencies explicitly defined.
- Consider creating a composite action or using a setup-rust action that includes all necessary dependencies. This could help reduce duplication and simplify maintenance of the workflow file.
To implement these suggestions, you could:
- Add similar dependency installation steps to the Formatting and Testing jobs if needed.
- Create a composite action in your repository (e.g.,
.github/actions/setup-rust-env/action.yml) that includes all the common setup steps (installing Rust, protoc, and dependencies). Then use this action in each job.Example of using a composite action:
- uses: ./.github/actions/setup-rust-envThis approach can significantly simplify your workflow file and make it easier to maintain consistent environments across jobs.
🧰 Tools
🪛 actionlint
46-46: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- .github/workflows/automerge.yml (1 hunks)
- .github/workflows/conventional-prs.yml (1 hunks)
- .github/workflows/docker-build.yml (1 hunks)
- .github/workflows/release-please.yml (3 hunks)
- .github/workflows/rust.yml (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/release-please.yml
🧰 Additional context used
🔇 Additional comments (3)
.github/workflows/conventional-prs.yml (1)
12-12: Approved: Update to Ubuntu 24.04 for consistency and stabilityThe change from
ubuntu-latesttoubuntu-24.04aligns with the PR objectives and provides more stability to the CI/CD pipeline. This update ensures a consistent environment across different workflows and prevents unexpected changes when new Ubuntu versions are released.To ensure compatibility, please verify that all tools and dependencies used in this workflow are compatible with Ubuntu 24.04. You can run the following script to check the workflow's execution:
If the conclusion is "success", it confirms that the workflow is compatible with Ubuntu 24.04.
.github/workflows/rust.yml (2)
64-64: Approve OS version update with a suggestion for testing.The update to Ubuntu 24.04 for the Testing job is consistent with the other jobs, which is good. However, please ensure that all required dependencies for testing, including those used by cargo-tarpaulin, are available in Ubuntu 24.04.
To verify the availability of required packages for testing, you can run:
#!/bin/bash # Description: Check if required packages for testing are available in Ubuntu 24.04 # Test: Check if cargo-tarpaulin dependencies are available apt-cache search libssl-dev apt-cache search pkg-config # Test: Check if any Rust-related packages are available apt-cache search rust-all
13-13: Approve OS version update with a suggestion.Updating to a specific Ubuntu version (24.04) is a good practice for consistency. However, please ensure that all required dependencies for the Formatting job are available in Ubuntu 24.04.
To verify the availability of required packages, you can run:
| @@ -18,7 +18,7 @@ env: | |||
|
|
|||
| jobs: | |||
| build-and-push-image: | |||
| runs-on: ubuntu-latest | |||
| runs-on: ubuntu-24.04 | |||
There was a problem hiding this comment.
💡 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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
utils/docker/Dockerfile (1)
50-55: Approve touch command and simplify Cargo buildThe touch command is a good practice to ensure fresh builds. However, the Cargo build command can be simplified.
Apply this diff to simplify the Cargo build command:
# Touch main.rs to prevent cached release build. RUN touch /usr/src/varfish-server-worker/src/main.rs # Build the application -RUN /root/.cargo/bin/cargo build --release +RUN cargo build --release
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .github/workflows/protobuf.yml (1 hunks)
- .github/workflows/rust.yml (4 hunks)
- utils/docker/Dockerfile (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/rust.yml
🧰 Additional context used
🔇 Additional comments (8)
.github/workflows/protobuf.yml (3)
13-13: Approve the update to Ubuntu 24.04Updating the runner to use
ubuntu-24.04instead ofubuntu-latestis a good practice. It provides more consistency and predictability in the CI environment, which aligns with the PR's objective of updating the base image (although this is for GitHub Actions, not Docker).
Line range hint
1-30: Consistency with PR objectivesThe changes in this file partially align with the PR objectives by updating the base environment (GitHub Actions runner) to a specific Ubuntu version. However, there are no changes related to RocksDB or Docker in this file. This is not necessarily an issue, as those changes might be in other files not provided for this review.
To ensure all objectives are met, please confirm that RocksDB and Docker-related changes are present in other files of this PR. You can use the following command to check for RocksDB-related changes:
#!/bin/bash # Check for RocksDB-related changes in the PR git diff origin/main...HEAD | grep -i rocksdb
18-21: Verify Python version requirementThe addition of a specific Python setup step is a good practice for ensuring a consistent environment. However, it's important to verify if Python 3.12 is necessary for this workflow, particularly for
protolint-bin.Could you confirm if Python 3.12 is required for
protolint-binor if there are any other dependencies in this workflow that specifically need this version? If not, consider using a slightly older but still well-supported version like 3.10 or 3.11 for better compatibility.To verify the Python version requirement, you can run:
utils/docker/Dockerfile (5)
21-24: Approve additional packages and improve Rust installationThe addition of system packages, especially
librocksdb-dev, aligns with the PR objective. However, the Rust installation process can be improved to ensure proper environment setup.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 rustfmtThis change adds the Rust environment to the
.bashrcfile, ensuring that it's loaded in all subsequent steps.
16-17:⚠️ Potential issueConsider using a stable Ubuntu LTS base image
While switching to Ubuntu aligns with the PR objective, using
ubuntu:noblemight introduce instability as it's not a long-term support (LTS) release. For enhanced stability and support, consider using an LTS version.Apply this diff to change the base image to an LTS version:
-FROM ubuntu:noble AS builder +FROM ubuntu:22.04 AS builder
35-35: 🛠️ Refactor suggestionSimplify Cargo commands
Using explicit paths to the Cargo binary is unnecessary if the Rust environment is properly set up. Simplifying these commands will make the Dockerfile more maintainable.
Apply this diff to simplify the Cargo commands:
-RUN USER=root /root/.cargo/bin/cargo new varfish-server-worker +RUN USER=root cargo new varfish-server-worker -RUN /root/.cargo/bin/cargo build --release +RUN cargo build --releaseAlso applies to: 41-41
61-61:⚠️ Potential issueAlign runtime base image with a stable release
Using
ubuntu:nobleas the runtime base image may lead to compatibility issues ifnobleis 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
Line range hint
63-68: Approve cleanup steps and consider adding librocksdbThe cleanup steps after package installation are good practice for reducing the image size. The installation of
libsqlite3-0is appropriate for runtime.Consider if
librocksdbis needed in the runtime image. If it's used by the application, you may need to add it to the installation command.To verify if
librocksdbis needed, run the following script:If the script returns any results, consider adding
librocksdb-devto the runtime dependencies.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/rust.yml (4 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/rust.yml
63-63: the runner of "actions-rs/clippy-check@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (9)
.github/workflows/rust.yml (9)
13-13: LGTM: Specific Ubuntu version improves reproducibility.Updating to
ubuntu-24.04instead of usingubuntu-latestis a good practice. It ensures consistent behavior across different runs and improves reproducibility of the CI pipeline.
35-35: LGTM: Consistent use of Ubuntu 24.04.This change maintains consistency with the Formatting job and aligns with the PR objective.
40-43: LGTM: System dependencies for RocksDB added.Adding
librocksdb-dev,libsnappy-dev, andlibsqlite3-devas system dependencies aligns with the PR objective of using system RocksDB for faster compilation.
57-60: LGTM: Caching librocksdb-sys for improved performance.Caching the
librocksdb-sysdirectory can significantly improve build times in subsequent runs.
66-66: LGTM: Comprehensive linting with --all-features.Using
--all-featuresensures that all code paths are checked during linting, which can help catch potential issues early.
70-70: LGTM: Consistent use of Ubuntu 24.04 across jobs.This change maintains consistency with the other jobs and aligns with the PR objective.
77-80: LGTM: Consistent system dependencies across jobs.Adding the same system dependencies as in the Linting job ensures consistency and supports the use of system RocksDB.
93-97: LGTM: Updated caching strategy for improved performance.The updated caching strategy with a specific key for coverage testing and including
librocksdb-sysshould improve build times in subsequent runs.
99-107: Verify the switch from cargo-tarpaulin to cargo-llvm-cov.The change from cargo-tarpaulin to cargo-llvm-cov for code coverage is significant. While both tools are reputable, it's important to ensure this change aligns with the project's needs and doesn't negatively impact coverage reporting.
Please confirm:
- Was this change intentional and discussed with the team?
- Are there any specific advantages of cargo-llvm-cov over cargo-tarpaulin for this project?
- Will this change affect the coverage metrics or how they're interpreted in Codecov?
| with: | ||
| cache-directories: | | ||
| ~/.cargo/registry/src/**/librocksdb-sys-* | ||
|
|
||
| - name: Lint with clippy | ||
| uses: actions-rs/clippy-check@v1 |
There was a problem hiding this comment.
Update the actions-rs/clippy-check action version.
The static analysis tool has flagged that the runner for the "actions-rs/clippy-check@v1" action is too old to run on GitHub Actions. It's recommended to update the action's version to ensure compatibility and access to the latest features and bug fixes.
Consider updating the action version:
- - name: Lint with clippy
- uses: actions-rs/clippy-check@v1
+ - name: Lint with clippy
+ uses: actions-rs/clippy-check@v2Please verify the latest version available for this action before applying the change.
Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 actionlint
63-63: the runner of "actions-rs/clippy-check@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
|
@tedil can this be merged? |
Solves #486
Summary by CodeRabbit
New Features
Bug Fixes