USHIFT-7408: Use rich dep for kernel-modules-extra to support kernel-rt - #7116
USHIFT-7408: Use rich dep for kernel-modules-extra to support kernel-rt#7116agullon wants to merge 2 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@agullon: This pull request references USHIFT-7408 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThe RPM now selects standard or RT kernel module dependencies. A new RHEL 9.8 source-tuned image blueprint and presubmit scenario define the image contents, VM lifecycle, boot readiness check, and ordered Robot test execution. ChangesSource-tuned presubmit
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: agullon 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
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packaging/rpm/microshift.spec`:
- Around line 87-90: Update the RPM dependency near the kernel module
requirement so it is variant-aware: regular kernel installations must require
kernel-modules-extra, while RT installations must require
kernel-rt-modules-extra. Replace the current broad OR dependency with the
appropriate conditional or package metadata mechanism, preserving the RHEL 9
no-op behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1bd429ef-78c9-4d9b-9043-d4d74206d387
📒 Files selected for processing (2)
packaging/rpm/microshift.spectest/image-blueprints/layer2-presubmit/group1/rhel98-source-tuned.toml
4e5996a to
2bc8f29
Compare
…kernel-rt RHEL 10 moved iptables compat kernel modules (nft_compat, xt_CT) from kernel-modules-core to kernel-modules-extra. Use conditional rich dependencies so that each variant is gated on its own kernel-core package — installing MicroShift on a kernel-rt system does not pull in the regular kernel (and vice-versa). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Re-enable the tuned ostree blueprint and the low-latency presubmit scenario that were disabled due to the missing kernel-rt package. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
2bc8f29 to
0f8740e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/scenarios/presubmits/el98-src`@low-latency.sh:
- Around line 1-4: Update the shell prologue in the presubmit script to use the
mandated #!/usr/bin/bash shebang and add set -euo pipefail immediately
afterward, before the environment exports.
- Around line 27-29: Update the timeout branch in the VM-waiting function to
return failure instead of calling exit, so control returns to the sourced
scenario harness and allows scenario_remove_vms to run. Preserve the timeout
message and failure status.
- Line 20: In the presubmit scenario, update the start_time declaration to
separate the readonly local declaration from the date command assignment,
preserving the existing timestamp value while allowing command-substitution
failures to surface and avoiding ShellCheck SC2155.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ca11ef40-f5b5-4398-89c4-4f6fd57ae50a
📒 Files selected for processing (3)
packaging/rpm/microshift.spectest/image-blueprints/layer2-presubmit/group1/rhel98-source-tuned.tomltest/scenarios/presubmits/el98-src@low-latency.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- packaging/rpm/microshift.spec
- test/image-blueprints/layer2-presubmit/group1/rhel98-source-tuned.toml
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/scenarios/presubmits/el98-src`@low-latency.sh:
- Around line 1-4: Update the shell prologue in the presubmit script to use the
mandated #!/usr/bin/bash shebang and add set -euo pipefail immediately
afterward, before the environment exports.
- Around line 27-29: Update the timeout branch in the VM-waiting function to
return failure instead of calling exit, so control returns to the sourced
scenario harness and allows scenario_remove_vms to run. Preserve the timeout
message and failure status.
- Line 20: In the presubmit scenario, update the start_time declaration to
separate the readonly local declaration from the date command assignment,
preserving the existing timestamp value while allowing command-substitution
failures to surface and avoiding ShellCheck SC2155.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ca11ef40-f5b5-4398-89c4-4f6fd57ae50a
📒 Files selected for processing (3)
packaging/rpm/microshift.spectest/image-blueprints/layer2-presubmit/group1/rhel98-source-tuned.tomltest/scenarios/presubmits/el98-src@low-latency.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- packaging/rpm/microshift.spec
- test/image-blueprints/layer2-presubmit/group1/rhel98-source-tuned.toml
🛑 Comments failed to post (3)
test/scenarios/presubmits/el98-src@low-latency.sh (3)
1-4: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add the required shell prologue.
Use the mandated shebang and strict-mode flags; otherwise setup failures can be masked by subsequent commands.
Proposed fix
-#!/bin/bash +#!/usr/bin/bash +set -euo pipefailAs per coding guidelines, shell scripts must use the
#!/usr/bin/bashshebang andset -euo pipefail.📝 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.#!/usr/bin/bash set -euo pipefail export SKIP_GREENBOOT=true export TEST_RANDOMIZATION=none🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/scenarios/presubmits/el98-src`@low-latency.sh around lines 1 - 4, Update the shell prologue in the presubmit script to use the mandated #!/usr/bin/bash shebang and add set -euo pipefail immediately afterward, before the environment exports.Source: Coding guidelines
20-20: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/bash set -euo pipefail sed -n '1,120p' 'test/scenarios/presubmits/el98-src@low-latency.sh' | cat -nRepository: openshift/microshift
Length of output: 1739
🏁 Script executed:
#!/usr/bin/bash set -euo pipefail bash --version | head -n 1 printf '\ncase1\n' bash -c 'set -e; f(){ local -r x=$(false); echo "after local:$?"; }; f; echo "survived"' || echo "died" printf '\ncase2\n' bash -c 'set -e; f(){ local x; x=$(false); echo "after assign:$?"; }; f; echo "survived"' || echo "died" printf '\ncase3\n' bash -c 'set -e; f(){ local -r x=$(date +%s); echo "after local date:$? value=$x"; }; f; echo "survived"' || echo "died"Repository: openshift/microshift
Length of output: 314
Split the declaration from the assignment.
local -r start_time=$(date +%s)can hide a failed substitution underset -eand triggers ShellCheck SC2155; declare first, then assign.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/scenarios/presubmits/el98-src`@low-latency.sh at line 20, In the presubmit scenario, update the start_time declaration to separate the readonly local declaration from the date command assignment, preserving the existing timestamp value while allowing command-substitution failures to surface and avoiding ShellCheck SC2155.Source: Coding guidelines
27-29: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return from the function instead of exiting the sourced harness.
exit 1can terminatescenario.shbeforescenario_remove_vmsruns, leaving the test VM behind after a timeout. Return the failure and let the scenario harness own cleanup and process termination.- exit 1 + return 1📝 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.if [ $(( $(date +%s) - start_time )) -gt 60 ]; then echo "Timed out waiting for VM having 2 boots" return 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/scenarios/presubmits/el98-src`@low-latency.sh around lines 27 - 29, Update the timeout branch in the VM-waiting function to return failure instead of calling exit, so control returns to the sourced scenario harness and allows scenario_remove_vms to run. Preserve the timeout message and failure status.
|
@agullon: The following tests 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. |
|
/test e2e-aws-tests |
3 participants
Summary
Requires: kernel-modules-extratoRequires: (kernel-modules-extra or kernel-rt-modules-extra)so that the dependency resolver picks whichever variant matches the installed kernelrhel98-source-tunedostree blueprint that was disabled due to the missingkernel-rtpackageel98-src@low-latencypresubmit scenario that was disabled due to the missingkernel-rtpackageProblem
The plain
Requires: kernel-modules-extrapulls inkernel-core(regular kernel) even whenkernel-rtis the intended kernel. This causesrpm-ostree compose postprocessto fail withMultiple vmlinuz-because bothvmlinuz-*+rtandvmlinuz-*are present in the boot directory.Fix
RPM rich dependencies (
or) let the solver pickkernel-modules-extrawhen the regular kernel is installed, orkernel-rt-modules-extrawhenkernel-rtis installed. Neither pulls in the wrong kernel.kernel-corekernel-modules-extrakernel-modules-core)kernel-rt-corekernel-rt-modules-extrakernel-corekernel-modules-extrakernel-rt-corekernel-rt-modules-extraTest plan
e2e-aws-testspasses (tuned ostree image builds withkernel-rtwithout dual-kernel failure)e2e-aws-tests-armpasses (nokernel-rton ARM, regular path works)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
rhel-9.8-microshift-source-tuned).x86_64.Bug Fixes