Adding support for payload multiple-stream.#180
Adding support for payload multiple-stream.#180ybettan wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ybettan 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 |
openshift-ci
bot
added
the
approved
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
CI is failing with @sdodson What version of OKD introduce the |
|
/retest |
|
Did you test how this works in a PR to release repo? The Also, OKD needs to work. |
ybettan
force-pushed
the
dual-stream
branch
3 times, most recently
from
8192889 to
135c837
Compare
| @@ -0,0 +1,78 @@ | |||
| FROM registry.ci.openshift.org/ocp/4.22:rhel-coreos-10 | |||
There was a problem hiding this comment.
@joepvd
There is no registry.ci.openshift.org/ocp/4.22:base-rhel10 in the release repo.
Should I use registry.ci.openshift.org/ocp/4.22:rhel-coreos-10 or should we add a new registry.ci.openshift.org/ocp/4.22:base-rhel10 image in the CI registry?
There was a problem hiding this comment.
TODO: Replace the base image.
The base-rhel10 should be available in the next few days:
https://redhat-internal.slack.com/archives/CBN38N3MW/p1772993158695559?thread_ts=1772982114.642989&cid=CBN38N3MW
ybettan
force-pushed
the
dual-stream
branch
3 times, most recently
from
231919e to
32feeaa
Compare
I have now because I split the Dockerfile. This is the chicken and egg problem though. CI fill fail here because the is no |
ybettan
force-pushed
the
dual-stream
branch
3 times, most recently
from
b1f13ca to
fcad0e1
Compare
WalkthroughAdds RHEL 10 support and expands runtime/kernel metadata: introduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Dockerfile.rhel10 (3)
59-59: Minor inconsistency in cache cleanup.The RHEL9
Dockerfileadditionally runsrm -rf /var/cache/dnf/*afterdnf clean all. Consider aligning for consistency, thoughdnf clean allalone is typically sufficient.♻️ Proposed fix for consistency
-RUN dnf clean all +RUN dnf clean all && rm -rf /var/cache/dnf/*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile.rhel10` at line 59, The Dockerfile.rhel10 currently runs "dnf clean all" but omits the extra cache purge used in the RHEL9 Dockerfile; update the RUN instruction that contains "dnf clean all" to also remove the dnf cache (e.g., append an rm -rf /var/cache/dnf/* after dnf clean all) so the cache cleanup is consistent with the RHEL9 Dockerfile.
72-75: Pre-existing concern: RT kernel metadata on non-x86_64 architectures.On architectures other than x86_64,
kernel-rt-coreis not installed (see lines 19-25), sorpm -q kernel-rt-corewill fail or return an error message. This could result in an incorrectRT_KERNEL_VERSIONvalue in the JSON.This is a pre-existing pattern from the RHEL9
Dockerfile, so no action is required in this PR, but worth noting for future improvement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile.rhel10` around lines 72 - 75, The RT kernel query can fail on non-x86_64 arches because kernel-rt-core isn't installed; update the RUN step that sets INSTALLED_RT_KERNEL (and the JSON write to /etc/driver-toolkit-release.json) to handle missing packages gracefully by detecting rpm -q kernel-rt-core failure and setting INSTALLED_RT_KERNEL to an explicit empty/null value or "N/A" instead of propagating the error; ensure the logic around INSTALLED_KERNEL and INSTALLED_RT_KERNEL uses conditional checks (e.g., test rpm -q kernel-rt-core exit status) before composing the JSON so RT_KERNEL_VERSION is accurate on architectures that lack kernel-rt-core.
6-7: Minor inconsistency with RHEL9 Dockerfile's dnf configuration approach.The RHEL9
Dockerfileusesecho >> /etc/dnf/dnf.confwhile this usesdnf config-manager. Both achieve the same result. Thednf config-managerapproach here is arguably cleaner; consider aligning both files to use the same method for maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile.rhel10` around lines 6 - 7, The dnf configuration step in Dockerfile.rhel10 uses "dnf config-manager" while the RHEL9 Dockerfile appends settings to /etc/dnf/dnf.conf; make them consistent by replacing the "dnf config-manager --best --setopt=install_weak_deps=False --save" usage in the RUN that sets RHEL_VERSION with an explicit append to /etc/dnf/dnf.conf (e.g., echo "install_weak_deps=False" >> /etc/dnf/dnf.conf), keeping the existing RHEL_VERSION detection logic (RHEL_VERSION, sourcing /etc/os-release) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile.rhel10`:
- Around line 39-40: The RUN instruction installing mandatory driver-container
packages is missing autoconf, automake, and libtool; update the RUN dnf -y
install elfutils-libelf-devel kmod binutils kabi-dw glibc invocation in
Dockerfile.rhel10 to include autoconf, automake, and libtool so the RHEL10
package list matches the RHEL9 Dockerfile and driver container builds remain
consistent.
---
Nitpick comments:
In `@Dockerfile.rhel10`:
- Line 59: The Dockerfile.rhel10 currently runs "dnf clean all" but omits the
extra cache purge used in the RHEL9 Dockerfile; update the RUN instruction that
contains "dnf clean all" to also remove the dnf cache (e.g., append an rm -rf
/var/cache/dnf/* after dnf clean all) so the cache cleanup is consistent with
the RHEL9 Dockerfile.
- Around line 72-75: The RT kernel query can fail on non-x86_64 arches because
kernel-rt-core isn't installed; update the RUN step that sets
INSTALLED_RT_KERNEL (and the JSON write to /etc/driver-toolkit-release.json) to
handle missing packages gracefully by detecting rpm -q kernel-rt-core failure
and setting INSTALLED_RT_KERNEL to an explicit empty/null value or "N/A" instead
of propagating the error; ensure the logic around INSTALLED_KERNEL and
INSTALLED_RT_KERNEL uses conditional checks (e.g., test rpm -q kernel-rt-core
exit status) before composing the JSON so RT_KERNEL_VERSION is accurate on
architectures that lack kernel-rt-core.
- Around line 6-7: The dnf configuration step in Dockerfile.rhel10 uses "dnf
config-manager" while the RHEL9 Dockerfile appends settings to
/etc/dnf/dnf.conf; make them consistent by replacing the "dnf config-manager
--best --setopt=install_weak_deps=False --save" usage in the RUN that sets
RHEL_VERSION with an explicit append to /etc/dnf/dnf.conf (e.g., echo
"install_weak_deps=False" >> /etc/dnf/dnf.conf), keeping the existing
RHEL_VERSION detection logic (RHEL_VERSION, sourcing /etc/os-release) intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a7ce7e2b-de73-46ed-98df-cd2ac87f1d58
📒 Files selected for processing (3)
DockerfileDockerfile.rhel10manifests/image-references-rhel10
The multiple-stream effort will contain multiple images considered as `machine-os` in the payload as well as multiple `driver-toolkit` images. This commit is adding some labels to the container such as the `rhel-stream` and `kernel-version`. In addition it adds a reference to the `rhel-coreos-10` image in order to specify to the payload that DTK depends on the `rhel-coreos-10` image. The Dockerfile has been split into 2 Dockerfile, one for each rhel release. In the future, it may extend to 3 Dockerfile including rhel for nvidia. Signed-off-by: Yoni Bettan <yonibettan@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Around line 61-62: The current blanket replacement using
s/rhel-coreos/stream-coreos/g is too broad and rewrites tags like
rhel-coreos-10; update the replacement to target only the exact tag name (or the
tag suffix) so only SCOS manifests are changed: adjust the sed invocation (the
s/.../.../g command) to match whole-word or delimiter-aware occurrences (for
example use a word-boundary or capture the trailing colon/line-end such as
s/\brhel-coreos\b/stream-coreos/g or s/rhel-coreos(:|$)/stream-coreos\1/g) and
apply it only to the SCOS manifest(s) (e.g.,
manifests/01-openshift-imagestream.yaml or the SCOS-specific entries in
manifests/image-references) instead of globally replacing across all manifests.
In `@Dockerfile.rhel10`:
- Around line 73-75: The RUN layer unconditionally queries kernel-rt-core into
INSTALLED_RT_KERNEL causing empty/incorrect RT metadata on non-x86_64 builds;
modify the RUN block to detect architecture (e.g., via uname -m or
/etc/os-release arch) and only query rpm -q --qf
"%{VERSION}-%{RELEASE}.%{ARCH}+rt" kernel-rt-core when arch == "x86_64",
otherwise set INSTALLED_RT_KERNEL to an explicit empty/null value; ensure the
final echo writing /etc/driver-toolkit-release.json includes the conditional
value of INSTALLED_RT_KERNEL so the file accurately reflects whether RT kernel
metadata exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8099f538-68a8-407c-ba4b-dfbbf39ca586
📒 Files selected for processing (3)
DockerfileDockerfile.rhel10manifests/image-references-rhel10
🚧 Files skipped from review as they are similar to previous changes (1)
- manifests/image-references-rhel10
|
/retest |
|
@ybettan: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
4 participants
The multiple-stream effort will contain multiple images considered as
machine-osin the payload as well as multipledriver-toolkitimages.This commit is adding some labels to the container such as the
rhel-streamandkernel-version.In addition it adds a reference to the
rhel-coreos-10image in order to specify to the payload that DTK depends on therhel-coreos-10image./assign @jlebon
/cc @dustymabe @sdodson @travier @joepvd
Summary by CodeRabbit
New Features
Infrastructure