Add PRD for modprobe.d config file support - #1874
Conversation
✅ Deploy Preview for openshift-kmm ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Review limit reached
Next review available in: 40 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe pull request adds enhancement proposal 0005. The proposal defines modprobe.d configuration support for driver container images, including paths, validation, runtime behavior, compatibility, security, and acceptance criteria. Changesmodprobe.d configuration proposal
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
/cc @TomerNewman |
![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
🤖 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 `@docs/enhancements/0005-modprobed-config.md`:
- Around line 76-85: Update the modprobe.d lifecycle requirements to validate
the configured directory and its files before module unload, not only before
load. Require unload to stop and report failure when the directory is missing or
empty, and add an acceptance test covering both missing and empty configuration
after a worker pod restart.
- Around line 29-30: Expand the proposal’s failure-reporting requirements beyond
missing, empty, or nested directories to cover copy failures, permission errors,
destination failures, modprobe errors, and malformed configuration files. Define
the expected user-visible status or event for each path and add corresponding
acceptance tests, or narrow the stated goal so it does not promise reporting for
those failures.
- Around line 110-113: Update the NFR-3 wording and the corresponding acceptance
criteria to say “an OpenShift event” instead of “a Openshift event,” preserving
the surrounding requirements unchanged.
- Around line 72-80: Clarify FR-2 and FR-3 to define modprobe.d directive
semantics: install and remove directives replace normal module insertion or
removal rather than acting as pre-modprobe hooks. State that directive commands
continuing the operation must invoke modprobe with --ignore-install or
--ignore-remove respectively, and require KMM’s load and unload commands to omit
these options so the directives are honored.
- Around line 102-106: Update the NFR-1 security requirements to document the
trust boundary for image-provided modprobe.d install and remove commands: state
the required image provenance and authorization, or define command restrictions
and audit controls, while preserving the requirement to disclose the Privileged
worker-pod implication before deployment.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c6e10598-99a7-4e0d-95a6-d601dcf09d8d
📒 Files selected for processing (1)
docs/enhancements/0005-modprobed-config.md
| - Users are always informed when their modprobe.d configuration could not | ||
| be applied, rather than experiencing a silent failure. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make failure reporting cover every configuration application failure.
The goal requires users to be informed whenever configuration cannot be applied. NFR-3 covers only missing, empty, or nested directories. The proposal does not define status or event reporting for copy failures, permission errors, destination failures, or modprobe errors. The non-goal also allows malformed files to fail during modprobe without a KMM-reported error. Add requirements and acceptance tests for these failure paths, or narrow the goal.
Also applies to: 46-48, 110-113
🤖 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 `@docs/enhancements/0005-modprobed-config.md` around lines 29 - 30, Expand the
proposal’s failure-reporting requirements beyond missing, empty, or nested
directories to cover copy failures, permission errors, destination failures,
modprobe errors, and malformed configuration files. Define the expected
user-visible status or event for each path and add corresponding acceptance
tests, or narrow the stated goal so it does not promise reporting for those
failures.
| - **FR-2:** When a module has the modprobe.d capability enabled, the | ||
| configuration files in `/etc/modprobe.d/` must take effect before | ||
| `modprobe` is invoked to load the kernel module, so that any load-time | ||
| init sequence defined in those files runs automatically. | ||
| - **FR-3:** When a module has the modprobe.d capability enabled, the | ||
| configuration files in `/etc/modprobe.d/` must take effect before | ||
| `modprobe` is invoked to unload the kernel module, so that any de-init | ||
| sequence defined in those files runs automatically (including | ||
| sequences needed to release kernel subsystem reference counts before |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '55,95p' docs/enhancements/0005-modprobed-config.mdRepository: rh-ecosystem-edge/kernel-module-management
Length of output: 2581
🌐 Web query:
modprobe.d man page install remove directives --ignore-install --ignore-remove behavior
💡 Result:
The install and remove directives in modprobe.d(5) are configuration commands used to override the default kernel module insertion and removal behavior [1][2]. Install directive: The install modulename command instructs modprobe to execute a specified shell command instead of directly inserting the module into the kernel [1][2]. This is typically used for complex tasks, such as loading other required modules before the target module [1]. Remove directive: The remove modulename command functions similarly to the install command but is triggered when modprobe -r is executed [1][2]. --ignore-install and --ignore-remove behavior: The --ignore-install and --ignore-remove options (or -i) are used with the modprobe command to prevent it from executing these custom install and remove commands defined in the configuration files [3][4]. When either --ignore-install or --ignore-remove is used, modprobe ignores both types of commands for the module specified on the command line [3][4]. Dependent modules, however, remain subject to any install or remove commands defined for them in the configuration [3][4]. A common use case for these options is within the install command itself to avoid infinite loops [1][2]. For example, if you define an install command to load "barney" before "fred", you might use /sbin/modprobe --ignore-install fred to ensure the second modprobe call triggers the default insertion rather than re-triggering the custom install command [1][2].
Citations:
- 1: https://man.archlinux.org/man/modprobe.d.5
- 2: https://man7.org/linux/man-pages/man5/modprobe.d.5.html
- 3: https://manpages.debian.org/bookworm/kmod/modprobe.8.en.html
- 4: https://manpages.ubuntu.com/manpages/noble/man8/modprobe.8.html
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
from urllib.request import urlopen
url = "https://man7.org/linux/man-pages/man5/modprobe.d.5.html"
text = urlopen(url, timeout=10).read().decode("utf-8", "replace").lower()
checks = {
"install replaces normal insertion": "instead of inserting the module normally" in text,
"remove replaces normal removal": "instead of removing the module normally" in text,
"ignore-install exists": "--ignore-install" in text,
"ignore-remove exists": "--ignore-remove" in text,
"ignore options used to avoid recursion": "infinite loop" in text,
}
for name, result in checks.items():
print(f"{name}: {result}")
PY
rg -n -i 'install|remove|ignore-install|ignore-remove|pre-hook|hook' \
docs/enhancements/0005-modprobed-config.mdRepository: rh-ecosystem-edge/kernel-module-management
Length of output: 2903
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
import re
import ssl
from urllib.request import urlopen
url = "https://man7.org/linux/man-pages/man5/modprobe.d.5.html"
context = ssl._create_unverified_context()
html = urlopen(url, context=context, timeout=10).read().decode("utf-8", "replace")
text = re.sub(r"<[^>]+>", " ", html)
text = re.sub(r"\s+", " ", text).lower()
for term in (
"instead of inserting the module normally",
"instead of removing the module normally",
"--ignore-install",
"--ignore-remove",
):
print(f"{term}: {term in text}")
PY
sed -n '1,180p' docs/enhancements/0005-modprobed-config.mdRepository: rh-ecosystem-edge/kernel-module-management
Length of output: 9392
Define modprobe.d directive semantics for FR-2 and FR-3. install and remove directives replace normal module insertion and removal; they are not pre-modprobe hooks. Require commands that continue the operation to call modprobe --ignore-install or modprobe --ignore-remove as appropriate. Require KMM's load and unload commands not to use these options, or the directives will be bypassed.
🤖 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 `@docs/enhancements/0005-modprobed-config.md` around lines 72 - 80, Clarify
FR-2 and FR-3 to define modprobe.d directive semantics: install and remove
directives replace normal module insertion or removal rather than acting as
pre-modprobe hooks. State that directive commands continuing the operation must
invoke modprobe with --ignore-install or --ignore-remove respectively, and
require KMM’s load and unload commands to omit these options so the directives
are honored.
6b61f5d to
5935d42
Compare
Adds an enhancement doc describing the requirements for letting users supply modprobe.d configuration files in their driver container image, so KMM can apply them during kernel module load/unload. Upstream-Commit: 03a0eb3d6b03114249b6c90e1fca9d3812ac717d
5935d42 to
8bca4ae
Compare
|
/ok-to-test |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: NataliShemtov, TomerNewman 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 |
|
/retest |
1 similar comment
|
/retest |
|
/lgtm |
32e6e38
into
rh-ecosystem-edge:main
3 participants
Adds an enhancement doc describing the requirements for letting users supply modprobe.d configuration files in their driver container image, so KMM can apply them during kernel module load/unload.
This is a manual cherry pick for this upstream
Summary by CodeRabbit
modprobe.dfiles in driver container images.