fix(tikv/rust-rocksdb): migrate CI from CentOS 7 to Rocky Linux 8#4564
Conversation
|
|
![ti-chi-bot[bot]](https://avatars.githubusercontent.com/in/214286?s=60&v=4){kind=small}
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary:
This PR updates the CI builder images for tikv/rust-rocksdb presubmit jobs from CentOS 7 to Rocky Linux 8 to fix build failures caused by outdated OpenSSL (1.0.2k) on CentOS 7. The approach replaces the base image, removes redundant OpenSSL and GCC setup commands, and adjusts package installation commands accordingly. Overall, the changes are minimal, focused, and coherent with the rationale provided. The migration aligns with existing usage in tikv/tikv. The modifications appear correct and well-targeted to fix the immediate build breakage.
Code Improvements
-
Hardcoded version tags in image references
- Files:
prow-jobs/tikv/rust-rocksdb/latest-presubmits.yaml(multiple lines: ~23, ~72, ~149) - Issue: The image tags are pinned to a specific commit (
v2026.3.22-3-g892e449). This can cause maintenance overhead when the image is updated or rebuilt. - Suggestion: Consider using a floating tag like
latestor a date-based tag updated regularly, or centralize the image tag in a variable to ease future updates. For example:And document the image update policy.- image: ghcr.io/pingcap-qe/cd/builders/tikv:v2026-latest
- Files:
-
Reliance on
dnfin the container- Files:
latest-presubmits.yaml(~lines 38-43) - Issue: The script installs
clang-tools-extraviadnfifclang-formatis missing. Since the image is controlled, it’s better to ensure the required tools are pre-installed in the image to avoid CI flakiness or slowness caused by package installs during job runtime. - Suggestion: Update the builder image to consistently include
clang-formatand required tools, removing the conditional install block from the presubmit job.
- Files:
-
Remove redundant comments about GCC setup
- Files:
latest-presubmits.yaml(~lines 78-84, 149-155) - Issue: The comments and conditionals for sourcing devtoolset-8 are removed, which is good, but the leftover comments in the PR description or other docs should also be updated to avoid confusion about GCC setup.
- Files:
Best Practices
-
Add a comment about the image upgrade decision
- Files:
latest-presubmits.yaml(top of the file or near image references) - Issue: It would be helpful for future maintainers to see a brief comment explaining why the image was upgraded (OpenSSL version incompatibility).
- Suggestion:
# Updated base image to Rocky Linux 8 to fix OpenSSL version incompatibility with openssl-sys 0.9.114+
- Files:
-
Testing coverage
- Files: N/A (CI config)
- Issue: The PR does not mention if jobs were manually tested or if there are nightly runs on these images to catch regressions early.
- Suggestion: Confirm that the new image has been validated or add a temporary test presubmit job to verify all build matrix combinations pass.
Minor
- Consistent use of package manager
- Files:
latest-presubmits.yaml(~lines 38-43) - Issue: The code replaces
yum install llvm-toolset-7.0withdnf install clang-tools-extra, which is correct for Rocky Linux 8, but make sure no other places rely onyumcommands that could fail. - Suggestion: Check other scripts or jobs for similar package manager assumptions and update accordingly.
- Files:
Overall, this PR addresses the critical build breakage effectively with minimal and clear changes. Addressing the above suggestions will improve maintainability and robustness of the CI configuration.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the CI environment for rust-rocksdb by migrating to a newer Docker image and removing redundant runtime dependency installations like openssl-devel and devtoolset-8. Feedback suggests further optimizing the CI pipeline by baking clang-tools-extra directly into the Docker image instead of installing it at runtime to improve performance and reliability.
| if [ -f /opt/rh/llvm-toolset-7.0/enable ]; then | ||
| source /opt/rh/llvm-toolset-7.0/enable | ||
| fi | ||
| dnf install -y clang-tools-extra |
There was a problem hiding this comment.
According to the general rules, dependencies should be baked into the Docker image instead of being installed at runtime in CI scripts to improve performance and reliability. Since openssl-devel is already pre-installed in the new Rocky Linux 8 image, consider also baking clang-tools-extra (which provides clang-format) into the image and removing this runtime installation block.
References
- For better CI performance and reliability, bake dependencies into the Docker image instead of installing them at runtime in CI scripts.
There was a problem hiding this comment.
Pull request overview
This PR updates the tikv/rust-rocksdb Prow presubmit jobs to use a Rocky Linux 8-based builder image so CI can compile openssl-sys (which now requires OpenSSL ≥ 1.1.0), fixing the currently broken encryption feature builds on CentOS 7.
Changes:
- Switch all
tikv/rust-rocksdbpresubmit jobs toghcr.io/pingcap-qe/cd/builders/tikv:v2026.3.22-3-g892e449(Rocky Linux 8-based). - Remove CentOS 7-specific setup steps (
openssl-develinstall anddevtoolset-8enable) from test jobs. - Replace the CentOS SCL-based clang-format installation with
dnf install -y clang-tools-extrain the lint job.
openssl-sys v0.9.114 dropped support for OpenSSL < 1.1.0, which breaks the encryption feature build on CentOS 7 (OpenSSL 1.0.2k). The Rocky Linux 8 tikv builder image already ships openssl-devel 1.1.1 and a modern GCC, so no extra install steps are needed. Signed-off-by: Yishuai Li <yishuai.li@pingcap.com>
24662f3 to
1f9ce20
Compare
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary:
This PR updates the CI configuration for tikv/rust-rocksdb by migrating the builder image from CentOS 7 to Rocky Linux 8 to resolve OpenSSL version compatibility issues. The migration removes explicit installation of openssl-devel and software collections for GCC and LLVM, relying instead on the newer base image's pre-installed packages. The changes are straightforward and focused on updating the CI environment, with no changes to application code. Overall, the approach is sound and aligns with upstream decisions in tikv/tikv CI.
Code Improvements:
-
Yum vs Dnf usage consistency (prow-jobs/tikv/rust-rocksdb/latest-presubmits.yaml lines ~40 and elsewhere):
The PR replacesyum install -y llvm-toolset-7.0withdnf install -y clang-tools-extrabut leaves some references toyum installcommented out or removed. Since Rocky Linux 8 usesdnfas the default package manager, ensure all package installation commands usednfconsistently in CI scripts to avoid subtle issues.
Suggestion: Replace allyumcommands withdnfin relevant scripts or verify the base image's compatibility. -
Idempotency of
dnf installcommands (prow-jobs/tikv/rust-rocksdb/latest-presubmits.yaml lines ~40):
CI jobs should be idempotent and fail fast if dependencies are not met. The current script installsclang-tools-extraonly ifclang-formatis missing, which is good. However, consider adding-yand--setopt=install_weak_deps=Falseor--quietflags to reduce noise and avoid interactive prompts. -
Environment Variables or Setup Scripts for Toolchain (prow-jobs/tikv/rust-rocksdb/latest-presubmits.yaml lines ~80-90 & ~150-160):
The removal ofdevtoolset-8sourcing assumes the system GCC version is sufficient. Confirm that the gcc/g++ versions in the new image meet minimum requirements for building rust-rocksdb, and document this assumption in the job or README to avoid confusion.
Best Practices:
-
Documentation Update (prow-jobs/tikv/rust-rocksdb/latest-presubmits.yaml):
The PR description is excellent, but it would be helpful to add a brief comment in the YAML file near the image declarations explaining the switch and referencing this PR or the reason (OpenSSL version compatibility). This aids future maintainers in understanding why this specific builder image is used. -
Testing Coverage:
The PR affects CI infrastructure. Ensure that the changed presubmit jobs have been run successfully with the new image on multiple PRs to confirm no regressions or unexpected failures in the build, lint, or test steps. -
Consistent Image Tagging:
The new image tagv2026.3.22-3-g892e449is used in three places. Consider extracting the image tag to a single variable or comment to reduce duplication and ease future updates.
Critical Issues:
No critical issues found. The PR addresses a known and blocking build problem and makes appropriate environment updates in CI config files only.
|
/cc @lybcodes |
|
/assign @lybcodes |
|
/lgtm I reviewed the change and it looks reasonable to me. The migration from the CentOS 7 builder to Non-blocking follow-up: before merge please trigger a real
|
|
@lybcodes: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lybcodes, wuhuizuo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
Problem
openssl-sysv0.9.114 (released ~April 2026) dropped support for OpenSSL < 1.1.0. The current CentOS 7 builder image installsopenssl-develfrom the CentOS 7 repos, which provides OpenSSL 1.0.2k — now rejected at compile time with:This breaks the
encryptionfeature build for all PRs targetingtikv/rust-rocksdb(confirmed also broken on PR #849, an unrelated change).Upstream issue: tikv/rust-rocksdb#850
Fix
Switch all three jobs (
pull-rust-rocksdb-lint,pull-rust-rocksdb-test-x86,pull-rust-rocksdb-test-arm) to the same Rocky Linux 8 builder image thattikv/tikvlatest presubmits already use (v2026.3.22-3-g892e449).Rocky Linux 8 ships OpenSSL 1.1.1 and a modern GCC system-wide, so:
yum install -y openssl-develis removed (pre-installed in the image)devtoolset-8source block is removed (system GCC is sufficient)llvm-toolset-7.0SCL install is replaced withdnf install -y clang-tools-extraPrecedent
tikv/tikvlatest presubmits already run on this image (see PR #4383).