OCPBUGS-83580: Skip dev fuse test on runc runtime#31140
OCPBUGS-83580: Skip dev fuse test on runc runtime#31140Chandan9112 wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
jira/valid-reference
|
@Chandan9112: This pull request references Jira Issue OCPBUGS-83580, 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe node e2e test's skip logic now detects CRI-O's default runtime by running ChangesTest Skip Logic Refinement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 (1)
test/extended/node/node_e2e/node.go (1)
123-123: ⚡ Quick win
awkpattern matches commented-outdefault_runtimelines
/default_runtime/matches any line containing that string, including commented-out lines such as# default_runtime = "runc". This would cause the test to skip incorrectly on a crun cluster that has a commented-out runc line in its config.Anchor the pattern to non-comment lines:
♻️ Proposed fix
- "crio status config 2>/dev/null | awk -F'\"' '/default_runtime/{print $2}'") + "crio status config 2>/dev/null | awk -F'\"' '/^[[:space:]]*default_runtime[[:space:]]*=/{print $2}'")🤖 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/extended/node/node_e2e/node.go` at line 123, The awk pattern in the crio command currently matches any line containing "default_runtime" including commented lines; update the pattern string in node.go that contains "crio status config 2>/dev/null | awk -F'\"' '/default_runtime/{print $2}'" to anchor to non-commented lines by using a regex that requires the line to start (optionally after whitespace) with default_runtime, e.g. change the awk match portion to /^[[:space:]]*default_runtime/ (so the full command becomes something like "crio status config 2>/dev/null | awk -F'\"' '/^[[:space:]]*default_runtime/{print $2}'") to avoid matching commented-out lines.
🤖 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/extended/node/node_e2e/node.go`:
- Around line 122-127: The runtime detection swallows stderr and hides failures;
change the ExecOnNodeWithChroot invocation (the call assigning runtime) to not
redirect stderr to /dev/null so ExecOnNodeWithChroot can surface command errors
(i.e., remove "2>/dev/null" from the command string passed to
ExecOnNodeWithChroot), and add an explicit guard after the call: if err != nil
or strings.TrimSpace(runtime) == "" then fail/skip with a clear message (use the
existing o.Expect(err).NotTo(...) or g.Skip with a diagnostic) before checking
for "runc" so a missing/failed crio detection is surfaced instead of silently
proceeding.
---
Nitpick comments:
In `@test/extended/node/node_e2e/node.go`:
- Line 123: The awk pattern in the crio command currently matches any line
containing "default_runtime" including commented lines; update the pattern
string in node.go that contains "crio status config 2>/dev/null | awk -F'\"'
'/default_runtime/{print $2}'" to anchor to non-commented lines by using a regex
that requires the line to start (optionally after whitespace) with
default_runtime, e.g. change the awk match portion to
/^[[:space:]]*default_runtime/ (so the full command becomes something like "crio
status config 2>/dev/null | awk -F'\"' '/^[[:space:]]*default_runtime/{print
$2}'") to avoid matching commented-out lines.
🪄 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: e5ebacfd-8b08-499c-a323-00e97e3f5014
📒 Files selected for processing (1)
test/extended/node/node_e2e/node.go
Chandan9112
force-pushed
the
fix-devfuse-runc-skip
branch
from
e46c62a to
e12f8e8
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Scheduling required tests: |
Chandan9112
force-pushed
the
fix-devfuse-runc-skip
branch
from
e12f8e8 to
dfb8aa2
Compare
|
Scheduling required tests: |
| "crio status config 2>/dev/null | awk -F'\"' '/default_runtime/{print $2}'") | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| runtime = strings.TrimSpace(runtime) | ||
| if runtime == "" { |
There was a problem hiding this comment.
This seems backwards. If we can't determine the default runtime, at best it would be crun, so we should skip, at worst the cluster is in a weird state and we shouldn't be testing on it, so again skip.
There was a problem hiding this comment.
Pretty sure default_runtime should always be set. Someone correct me if I am wrong.
There was a problem hiding this comment.
This seems backwards. If we can't determine the default runtime, at best it would be crun, so we should skip, at worst the cluster is in a weird state and we shouldn't be testing on it, so again skip.
@cpmeadors, corrected in the latest commit.
There was a problem hiding this comment.
If default_runtime should always be set, it should fail, shouldn't it?
and I think it should be set.
There was a problem hiding this comment.
It falls back to crun if empty, so it should be set.
https://github.com/cri-o/cri-o/blob/1b111f660d863a72b32f01d978d41f663f3a2815/pkg/config/config.go#L1123
There was a problem hiding this comment.
@bitoku, @cpmeadors ., since CRI-O falls back to crun when empty, should I remove the empty check. If default_runtime is unset, the test will run as expected on crun.
There was a problem hiding this comment.
That seems right. Skip only if default_runtime is set to runc.
There was a problem hiding this comment.
+1 from me. just a small correction, it shouldn't be empty. if it's empty cri-o set it crun explicitly instead of treating it as crun silently.
There was a problem hiding this comment.
Modified the code in latest commit. Please feel free to add if anything needed.
Chandan9112
force-pushed
the
fix-devfuse-runc-skip
branch
from
dfb8aa2 to
ab6efb4
Compare
openshift-merge-bot
Bot
added
the
ready-for-human-review
|
Scheduling required tests: |
The io.kubernetes.cri-o.Devices annotation is only supported by crun. Detect the actual runtime via crio status config on the node and skip when runc is in use. This reliably handles both platform-default and explicitly configured runc environments.
Chandan9112
force-pushed
the
fix-devfuse-runc-skip
branch
from
ab6efb4 to
e386c75
Compare
|
/retest |
|
/pipeline required |
|
Scheduling required tests: |
|
/test images |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BhargaviGudi, Chandan9112 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/retest |
1 similar comment
|
/retest |
|
@Chandan9112: 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. |
5 participants
The io.kubernetes.cri-o.Devices annotation is only supported by crun. This will detect the actual runtime via crio status config on the node and skip when runc is there.
Bug
OCPBUGS-83580
Follow-up PR
#PR
Fix
Added a runtime check before test execution: query crio config on a worker node to detect the default runtime
Skip the test with a clear message when runc is the default runtime, since the /dev/fuse device annotation is not applicable
Testing
Tested on a fresh OCP 4.17 GCP cluster where the default runtime is runc.
cc: @ngopalak-redhat
Summary by CodeRabbit