OCPBUGS-81715: Fix monitoring of udev using nsenter#600
OCPBUGS-81715: Fix monitoring of udev using nsenter#600openshift-merge-bot[bot] merged 1 commit intoopenshift:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gnufied 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughrawUdevBlockMonitor now uses a new factory, newUdevMonitorCommand(), to build the udevadm command (optionally wrapped with nsenter). It opens and scans the command's stderr in a goroutine, logs stderr lines as warnings, and synchronizes shutdown with cmd.Wait() and the scanner completion. Changes
Sequence DiagramsequenceDiagram
participant Monitor as rawUdevBlockMonitor
participant Factory as newUdevMonitorCommand
participant Lookup as exec.LookPath(nsenter)
participant Cmd as udevadm Process
participant Stderr as Stderr Scanner (goroutine)
Monitor->>Factory: Request udevadm command
Factory->>Lookup: Check nsenter
alt nsenter found
Lookup-->>Factory: path
Factory->>Cmd: return nsenter-wrapped exec.Cmd
else nsenter missing
Lookup-->>Factory: not found
Factory-->>Monitor: nil (warning)
Monitor-->>Monitor: exit without starting monitor
end
Monitor->>Cmd: Open StderrPipe() and Start()
Monitor->>Stderr: spawn scanner goroutine
Cmd-->>Stderr: write stderr lines
Stderr->>Monitor: log warnings per line
Monitor->>Cmd: defer cmd.Wait()
Cmd-->>Monitor: exit
Monitor->>Stderr: wait for scanner completion
Stderr-->>Monitor: done
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
approved
gnufied
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@gnufied: This pull request references Jira Issue OCPBUGS-81715, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
![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 the current code and only fix it if needed.
Inline comments:
In `@pkg/diskmaker/discovery/monitor.go`:
- Around line 61-77: When cmd.Start() (in the udev monitor startup around
stdout/stderr handling) fails due to nsenter runtime issues, retry
creating/starting the monitor using the direct udevadm invocation instead of
bailing out; modify newUdevMonitorCommand() or the startup logic to detect when
the returned *exec.Cmd wraps nsenter (e.g., command path or args include
"nsenter") and on Start error attempt to construct a fallback cmd that directly
runs "udevadm monitor --udev --subsystem-match=block" (or the existing
non-nsenter branch) and Start that new cmd, logging both attempts and only
returning when both fail so the caller can decide whether to disable udevEvents.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66551c15-ae94-4369-90d8-ba53af1d34c8
📒 Files selected for processing (2)
Dockerfile.diskmaker.rhel7pkg/diskmaker/discovery/monitor.go
| COPY config/manifests /manifests | ||
|
|
||
| RUN yum install -y e2fsprogs xfsprogs && yum clean all && rm -rf /var/cache/yum | ||
| RUN yum install -y e2fsprogs xfsprogs systemd-udev && yum clean all && rm -rf /var/cache/yum |
There was a problem hiding this comment.
If you're using nsenter, then udevadm does not need to be installed inside the container.
| if _, pathErr := exec.LookPath("nsenter"); pathErr == nil { | ||
| return exec.Command("nsenter", "--mount=/proc/1/ns/mnt", "--", "udevadm", "monitor", "-u", "-k", "-s", "block") | ||
| } |
There was a problem hiding this comment.
It should either discover very precisely if it's in a container or not or have a cmdline option to use stdbuf below (having nsenter as the default). The /proc/1/ns/mnt check above will pass on any recent Linux, even on the host.
There was a problem hiding this comment.
in the CRC cluster at least running without nsenter doesn't work:
udevadm monitor stderr: Running in chroot, ignoring request
W0405 18:15:35.460151 2162284 discovery.go:103] disabling udev monitoring
You are right, we probably don't need udevadm in the container then.
There was a problem hiding this comment.
Second thoughts: what's wrong with calling nsenter even on the host and for local development / debugging? It's not necessary, but it should work. Should we remove the nsenter check + stdbuf fallback below?
There was a problem hiding this comment.
I removed the check. PTAL
gnufied
force-pushed
the
fix-udev-monitoring
branch
from
a2ddd79 to
6c7841b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/diskmaker/discovery/monitor.go`:
- Around line 61-63: The current check if cmd == nil in the monitoring setup
incorrectly warns about requiring nsenter but then returns, conflicting with a
later comment about falling back to direct udevadm monitor; fix this by either
constructing a direct udevadm command when nsenter is unavailable (i.e., build
cmd to run "udevadm monitor --udev --kernel" or the equivalent used elsewhere
and continue) or change the warning/logging to clearly state that device
monitoring is being disabled and return; update the branch handling around the
cmd variable in the monitor setup (the block guarding cmd == nil and the later
fallback code) so the behavior and message are consistent.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 49671fba-ce89-4963-90d8-0d6090fedf26
📒 Files selected for processing (1)
pkg/diskmaker/discovery/monitor.go
gnufied
force-pushed
the
fix-udev-monitoring
branch
from
6c7841b to
f8dcb15
Compare
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@gnufied: This pull request references Jira Issue OCPBUGS-81715, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
@gnufied: all tests passed! 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. |
|
/lgtm |
|
/verified by @gnufied |
openshift-ci-robot
added
the
verified
|
@gnufied: This PR has been marked as verified by 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. |
|
@gnufied: Jira Issue OCPBUGS-81715: All pull requests linked via external trackers have merged: All linked pull requests have the 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. |
|
/cherry-pick release-4.22 |
|
@gnufied: new pull request created: #612 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. |
5 participants
https://redhat.atlassian.net/browse/OCPBUGS-81715
Summary by CodeRabbit