-
Notifications
You must be signed in to change notification settings - Fork 63
LCORE-858: Hermetic build x86_64 + ARM platform #847
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
Conversation
Signed-off-by: Haoyu Sun <hasun@redhat.com>
WalkthroughAdd ARM64 multi‑arch and hermetic build support: Tekton PipelineRuns gain arm64 platform, hermetic flags, prefetch JSON and 4h timeouts; Containerfile adds conditional hermetic venv install using local cache and copies requirements.*.txt; pyproject.toml adds a pypi index, hermetic dependency group and conflicts; RPM manifests and a UBI repo are added. Changes
Sequence Diagram(s)sequenceDiagram
participant Tekton as Tekton Pipeline
participant Builder as Container Build
participant Cache as /cachi2 (local cache)
participant Venv as uv venv
participant Indexes as Remote Indexes
rect rgb(230,245,230)
Note over Tekton,Builder: New hermetic-prefetch flow (pipeline params include hermetic + prefetch)
Tekton->>Builder: Start PipelineRun (hermetic, prefetch, arm64/x86_64)
Builder->>Cache: test -f /cachi2/cachi2.env
alt cache present
Cache-->>Builder: exists
Builder->>Builder: source /cachi2/cachi2.env
Builder->>Venv: uv venv --seed --find-links <cache> ...
Builder->>Venv: pip install requirements.$(uname -m).txt --no-index
Venv-->>Builder: deps installed from local cache
else cache absent
Cache-->>Builder: not found
Builder->>Indexes: uv sync --locked --no-dev --group llslibdev
Indexes-->>Builder: fetch packages from remote indexes
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-11-24T16:58:04.410ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (4)
requirements.hermetic.txt (1)
1-2: Hermetic uv/pip pins look good; just keep them aligned with build imageThe pinned versions are reasonable and match the Containerfile’s uv==0.8.15. Consider documenting or automating how these stay in sync with the Containerfile and any lockfiles so they don’t silently drift over time.
.tekton/lightspeed-stack-pull-request.yaml (1)
30-41: Arm64 + hermetic + prefetch wiring looks correct; clean up TODO and verify JSONThe new defaults for:
build-platforms(addinglinux-c6gd2xlarge/arm64),build-source-image: 'true',prefetch-input(rpm + pip, includingrequirements.txtandrequirements.hermetic.txt), andhermetic: 'true'are consistent with the pipelineSpec and the downstream
prefetch-dependencies/build-imagestasks.Two small follow‑ups:
- The comment on Line 30 still says “todo: add arm64” even though arm64 is already configured; it would be less confusing to update or drop it.
- Please double‑check that the
prefetch-inputJSON matches the current Cachi2 schema (in particular the expected type forallow_binary) and that both requirements files are present and used as intended..tekton/lightspeed-stack-push.yaml (1)
27-37: Push pipeline arm64/hermetic configuration matches PR pipeline; align comments and validate prefetch JSONThe added:
linux-c6gd2xlarge/arm64entry inbuild-platforms,build-source-image: 'true',prefetch-inputJSON (rpm + pip with both requirements files), andhermetic: 'true'bring the push pipeline in line with the PR pipeline and Konflux’s multi‑platform/hermetic patterns.
Minor cleanups:
- The “todo: add arm64” comment is now stale; consider updating/removing it to avoid confusion.
- As with the PR pipeline, please verify the
prefetch-inputJSON structure against the current Cachi2 docs (especially theallow_binaryfield) so the prefetch task interprets it as expected.pyproject.toml (1)
58-64: Align Pyright exclusions with coding guidelines by also excludingsrc/auth/k8s.pyCoding guidelines call for excluding
src/auth/k8s.pyfrom Pyright, while this config currently excludessrc/authentication/k8s.py. To be safe (and future‑proof against path changes), consider excluding both:[tool.pyright] exclude = [ # TODO(lucasagomes): This module was copied from road-core # service/ols/src/auth/k8s.py and currently has 58 Pyright issues. It # might need to be rewritten down the line. "src/authentication/k8s.py", + "src/auth/k8s.py", ]This keeps Pyright noise down regardless of which of those paths is present in the repo, and matches the guideline.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.tekton/lightspeed-stack-pull-request.yaml(1 hunks).tekton/lightspeed-stack-push.yaml(1 hunks)Containerfile(1 hunks)pyproject.toml(2 hunks)requirements.hermetic.txt(1 hunks)rpms.in.yaml(1 hunks)rpms.lock.yaml(1 hunks)ubi.repo(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
pyproject.toml
📄 CodeRabbit inference engine (CLAUDE.md)
pyproject.toml: Configure pylint withsource-roots = "src"
Excludesrc/auth/k8s.pyfrom pyright type checking
Files:
pyproject.toml
🧠 Learnings (2)
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Use `uv sync --group dev --group llslibdev` to install development dependencies
Applied to files:
Containerfilepyproject.toml
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Use Python package manager `uv` with `uv run` prefix for all development commands
Applied to files:
requirements.hermetic.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (5)
Containerfile (1)
25-25: Copying requirements.txt into the build context is appropriateIncluding requirements.txt alongside pyproject/uv.lock is needed for the hermetic pip install path; no issues here.
rpms.lock.yaml (1)
1-355: RPM lockfile content is coherent with UBI repos and multi-arch goalsThe lockfile cleanly captures aarch64 and x86_64 package sets with urls and repoid names that match the new UBI repo definitions. This is exactly what you want for hermetic, reproducible builds.
Going forward, just ensure this file is regenerated whenever
rpms.in.yamlor the UBI repo baselines change so that both arches stay in sync.pyproject.toml (1)
91-106: uv hermetic configuration andllslibdev-hermeticgroup are structured wellThe combination of:
- a dedicated
pypi-defaultuv index,tool.uv.sources.torchsplit by group (llslibdev→pytorch-cpu,llslibdev-hermetic→pypi-default),- a
tool.uv.conflictsrule preventingllslibdevandllslibdev-hermeticfrom being active together, and- the new
llslibdev-hermeticdependency group (with Linux‑onlytorch==2.7.1; sys_platform == 'linux')gives you a clean separation between the CPU‑wheel path and the hermetic/default‑index path while avoiding accidental mixing.
One thing to keep in mind operationally: ensure your
uv syncinvocations (locally and in CI) pick exactly one of these groups at a time (e.g.,--group llslibdevvs--group llslibdev-hermetic) so the conflict rule doesn’t surprise anyone.Also applies to: 177-220
rpms.in.yaml (1)
1-4: RPM input manifest matches build usage and multi-arch goalsDefining
gcc,jq, andpatchagainst./ubi.repofor bothx86_64andaarch64lines up with the Containerfile installs and the new lockfile.Just ensure
rpms.lock.yamlis regenerated whenever you change this package list so the lock stays faithful to this manifest.ubi.repo (1)
1-62: UBI repo configuration is consistent and secure for hermetic RPM resolutionThe repo IDs, base URLs, and GPG settings match what rpms.lock.yaml and the Containerfile expect (ubi‑9‑baseos‑rpms, ubi‑9‑appstream‑rpms, ubi‑9‑codeready‑builder‑rpms), with HTTPS and
gpgcheck=1enabled. This is a solid basis for reproducible RPM prefetching.
| # Source cachi2 environment for hermetic builds if available, otherwise use normal installation | ||
| # cachi2.env has these env vars: | ||
| # PIP_FIND_LINKS=/cachi2/output/deps/pip | ||
| # PIP_NO_INDEX=true | ||
| RUN if [ -f /cachi2/cachi2.env ]; then \ | ||
| . /cachi2/cachi2.env && uv venv --seed --no-index --find-links ${PIP_FIND_LINKS} && . .venv/bin/activate && pip install --no-index --find-links ${PIP_FIND_LINKS} -r requirements.txt; \ | ||
| else \ | ||
| uv sync --locked --no-dev --group llslibdev; \ | ||
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hermetic cachi2 path: uv venv flags are invalid and will break builds
In the hermetic branch, uv venv is called with --no-index and --find-links, but uv venv doesn’t support pip-style index flags. This will cause the RUN step to fail when /cachi2/cachi2.env is present.
You only need those flags on the pip install call; uv venv should stay simple. For example:
-RUN if [ -f /cachi2/cachi2.env ]; then \
- . /cachi2/cachi2.env && uv venv --seed --no-index --find-links ${PIP_FIND_LINKS} && . .venv/bin/activate && pip install --no-index --find-links ${PIP_FIND_LINKS} -r requirements.txt; \
- else \
- uv sync --locked --no-dev --group llslibdev; \
- fi
+RUN if [ -f /cachi2/cachi2.env ]; then \
+ . /cachi2/cachi2.env && \
+ uv venv --seed && \
+ . .venv/bin/activate && \
+ pip install --no-index --find-links "${PIP_FIND_LINKS}" -r requirements.txt; \
+ else \
+ uv sync --locked --no-dev --group llslibdev; \
+ fiThis keeps uv environment creation offline while correctly using cachi2-provided wheels for package installation.
📝 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.
| # Source cachi2 environment for hermetic builds if available, otherwise use normal installation | |
| # cachi2.env has these env vars: | |
| # PIP_FIND_LINKS=/cachi2/output/deps/pip | |
| # PIP_NO_INDEX=true | |
| RUN if [ -f /cachi2/cachi2.env ]; then \ | |
| . /cachi2/cachi2.env && uv venv --seed --no-index --find-links ${PIP_FIND_LINKS} && . .venv/bin/activate && pip install --no-index --find-links ${PIP_FIND_LINKS} -r requirements.txt; \ | |
| else \ | |
| uv sync --locked --no-dev --group llslibdev; \ | |
| fi | |
| # Source cachi2 environment for hermetic builds if available, otherwise use normal installation | |
| # cachi2.env has these env vars: | |
| # PIP_FIND_LINKS=/cachi2/output/deps/pip | |
| # PIP_NO_INDEX=true | |
| RUN if [ -f /cachi2/cachi2.env ]; then \ | |
| . /cachi2/cachi2.env && \ | |
| uv venv --seed && \ | |
| . .venv/bin/activate && \ | |
| pip install --no-index --find-links "${PIP_FIND_LINKS}" -r requirements.txt; \ | |
| else \ | |
| uv sync --locked --no-dev --group llslibdev; \ | |
| fi |
5bcb30c to
95b54dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Containerfile (1)
32-36:uv venvdoes not support--no-indexor--find-linksflags.As flagged in a previous review,
uv venvonly creates virtual environments and doesn't accept pip-style index flags. This will cause the hermetic build to fail.RUN if [ -f /cachi2/cachi2.env ]; then \ - . /cachi2/cachi2.env && uv venv --seed --no-index --find-links ${PIP_FIND_LINKS} && . .venv/bin/activate && pip install --no-index --find-links ${PIP_FIND_LINKS} -r requirements.$(uname -m).txt; \ + . /cachi2/cachi2.env && \ + uv venv --seed && \ + . .venv/bin/activate && \ + pip install --no-index --find-links "${PIP_FIND_LINKS}" -r requirements.$(uname -m).txt; \ else \ uv sync --locked --no-dev --group llslibdev; \ fi
🧹 Nitpick comments (1)
.tekton/lightspeed-stack-push.yaml (1)
27-31: Remove stale TODO comment.The TODO references adding arm64, but it's now implemented on line 31. Remove the obsolete comment:
- # todo: add arm64. refer to https://konflux.pages.redhat.com/docs/users/getting-started/multi-platform-builds.html#arm64-2 - name: build-platforms value: - linux/x86_64 - linux-c6gd2xlarge/arm64
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.tekton/lightspeed-stack-pull-request.yaml(1 hunks).tekton/lightspeed-stack-push.yaml(1 hunks)Containerfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .tekton/lightspeed-stack-pull-request.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Use `uv sync --group dev --group llslibdev` to install development dependencies
Applied to files:
Containerfile
🪛 GitHub Actions: Check image building
Containerfile
[error] 1-1: Build failed during COPY step: copier: stat: "/requirements.txt": no such file or directory. Command failed: /usr/bin/buildah bud --arch amd64 -f /home/runner/work/lightspeed-stack/lightspeed-stack/Containerfile --format oci --tls-verify=true -t lightspeed-stack:dev-latest-amd64 /home/runner/work/lightspeed-stack/lightspeed-stack. Exit code: 125.
🪛 GitHub Actions: E2E Tests
Containerfile
[error] 25-25: Build step failed: [lightspeed-stack builder 6/8] COPY ./pyproject.toml ./LICENSE ./README.md ./uv.lock ./requirements.txt ./: failed to calculate checksum of ref 25309008-d3b6-4795-9b53-5704e2dd5a06::ndcjmg2zkfikrc3l0mdudixu9: "/requirements.txt": not found
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🔇 Additional comments (3)
Containerfile (1)
28-36: Conditional hermetic build logic is well-structured.The approach of detecting
/cachi2/cachi2.envto switch between hermetic and normal builds is appropriate. Using$(uname -m)to select architecture-specific requirements files aligns well with the multi-arch strategy defined in the Tekton pipeline..tekton/lightspeed-stack-push.yaml (2)
32-37: Hermetic build and source image configuration looks correct.The additions properly enable:
- Source image building for supply chain transparency
- Hermetic mode for reproducible builds
- RPM and pip prefetching with architecture-specific requirements
This aligns with the hermetic build strategy in the Containerfile.
34-35: I was unable to verify the requirements files due to repository access limitations in the sandbox environment. However, based on the verification attempts and the constraints I'm facing, I need to categorize this review comment appropriately.Verify that all referenced requirements files (
requirements.x86_64.txt,requirements.aarch64.txt,requirements.hermetic.txt) exist in the repository.The prefetch-input references three specific requirements files in the Tekton configuration. Without access to the repository to confirm their presence or absence, manual verification is required. If these files do not exist, the prefetch-input configuration will fail at runtime.
95b54dd to
b70a885
Compare
|
/retest |
b70a885 to
2b21124
Compare
|
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.tekton/lightspeed-stack-pull-request.yaml(2 hunks).tekton/lightspeed-stack-push.yaml(2 hunks)Containerfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Containerfile
- .tekton/lightspeed-stack-pull-request.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (2)
.tekton/lightspeed-stack-push.yaml (2)
633-635: Verify pipeline and task timeouts are sufficient for multi-platform builds.Explicit timeouts of 4h (pipeline) and 2h (tasks) are now set. These values should accommodate the extended build time for multiple architectures (x86_64 + arm64) plus all SAST checks (Snyk, Coverity, ShellCheck, Unicode, ClamAV). Ensure no timeout conflicts with slower arm64 or Coverity analysis steps.
Please confirm:
- The 2h task timeout is sufficient for individual platform builds plus SAST analysis on slower arm64 hardware.
- The 4h pipeline timeout accounts for sequential SAST tasks that run after build-image-index.
- There are no timeout assumptions hard-coded in downstream Konflux tasks.
32-37: Verification inconclusive; manual confirmation needed for referenced requirements files.The hermetic build configuration in
.tekton/lightspeed-stack-push.yamlcorrectly specifies rpm and pip dependencies with the following files:
requirements.x86_64.txtrequirements.aarch64.txtrequirements.hermetic.txtUnable to verify file existence automatically. Please confirm these three files are present in the repository root and are properly formatted for pip parsing.
20023e1 to
d0cdea9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.tekton/lightspeed-stack-pull-request.yaml(2 hunks).tekton/lightspeed-stack-push.yaml(2 hunks)Containerfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .tekton/lightspeed-stack-pull-request.yaml
- Containerfile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: build-pr
- GitHub Check: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (4)
.tekton/lightspeed-stack-push.yaml (4)
32-33: Source image generation is now enabled.Enabling
build-source-image: truesupports SLSA provenance and reproducibility. Ensure the downstream source-build task is properly configured to handle the prefetch artifacts and multi-architecture context.
633-635: Timeouts account for multi-platform build overhead.The 4-hour timeout for both pipeline and tasks is reasonable for multi-platform builds. Confirm through testing that this duration is sufficient for ARM64 builds, which may require additional time compared to x86_64 builds on the available infrastructure.
36-37: Repository access unavailable for automated verification.The sandbox environment was unable to clone the repository, preventing verification of the hermetic build configuration against the Containerfile implementation. Manual verification is required to confirm:
- Whether the Containerfile contains hermetic conditional logic for network isolation
- Whether local cache and dependency installation are properly configured for hermetic builds
- Whether the hermetic=true flag in .tekton/lightspeed-stack-push.yaml aligns with actual build behavior
34-35: Unable to verify file existence due to repository access restrictions.I attempted to verify the existence of the three requirements files (
requirements.x86_64.txt,requirements.aarch64.txt,requirements.hermetic.txt) referenced in theprefetch-inputparameter, but encountered access restrictions preventing repository exploration through both local cloning and GitHub CLI methods.The
prefetch-inputparameter at lines 34-35 references three requirements files for multi-architecture and hermetic build support. These file paths should be confirmed to exist in the repository root before merging this change.This verification requires either:
- Direct repository access to list and confirm the three files are present
- Manual confirmation from the developer that these files exist and are correctly referenced
Signed-off-by: Haoyu Sun <hasun@redhat.com>
d0cdea9 to
50e828c
Compare
tisnik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
/hold |
|
Please review the PR #852 instead. It uses CPU variant of Torch package, without dependency on nvidia packages. |
|
/close |
Description
on top of the hermetic build on konflux #748, this PR addes ARM build.
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.