OCPNODE-4536: Migrate OCP-55486 check no MountVolume.SetUp failed error in cronjob events#31189
OCPNODE-4536: Migrate OCP-55486 check no MountVolume.SetUp failed error in cronjob events#31189BhargaviGudi wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
@BhargaviGudi: No Jira issue with key OCP-55486 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. |
WalkthroughAdds a Ginkgo E2E that polls CronJob namespaces' events for the regex-matched MountVolume.SetUp failed ... object ... not registered message and documents the test in the node README (OCP-55486). ChangesCronJob Volume Mount Error E2E Test
🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels: 🚥 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)
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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BhargaviGudi 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 |
BhargaviGudi
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@BhargaviGudi: No Jira issue with key OCP-55486 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. |
|
@BhargaviGudi: No Jira issue with key OCP-55486 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 180-186: The test is parsing human-readable `oc get events` table
output (variable events) which can truncate/wrap messages and break the regex;
replace that with a structured query for the event .message field and run the
regex against the returned message(s). Update the call using
oc.AsAdmin().WithoutNamespace().Run("get").Args("events", "-n", ns, "-o",
"jsonpath={range .items[*]}{.message}{\"\\n\"}{end}") (or equivalent jsonpath)
to capture raw .message text, then apply the existing regexp (errorPattern) and
check matches as before; change references to the `events` variable and keep the
same errorPattern and match logic (len(matches) > 0) so behavior is preserved.
- Around line 174-195: The poll callback currently returns true on the first
clean read which ends polling early; change the logic so the callback returns
false,nil when no matches are found (to continue polling until timeout) and
return a non-nil error (e.g. fmt.Errorf("Found MountVolume.SetUp failed for
volume in namespace %s: %v", ns, matches[0])) when matches are found to fail
fast; update the post-poll assertion around the wait.Poll call so that
wait.ErrWaitTimeout is treated as the successful outcome (i.e. assert err ==
wait.ErrWaitTimeout or treat nil as failure and ErrWaitTimeout as pass) —
reference symbols: wait.Poll, errorPattern, matches, the anonymous poll
function, and the o.Expect(...) after wait.Poll.
🪄 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: d11c7e9c-8c0e-4212-82f1-a8761b36e0ff
📒 Files selected for processing (2)
test/extended/node/README.mdtest/extended/node/node_e2e/node.go
BhargaviGudi
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@BhargaviGudi: This pull request references OCPNODE-4536 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 "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. |
|
Scheduling required tests: |
BhargaviGudi
force-pushed
the
migrate-55486
branch
from
cf7004b to
7e902fe
Compare
openshift-ci
Bot
added
the
ready-for-human-review
|
Scheduling required tests: |
|
/pipeline required |
|
Scheduling required tests: |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 7e902fe
New tests seen in this PR at sha: 7e902fe
|
|
/retest-required |
|
/verified by @BhargaviGudi - the test case was executed on the local cluster and passed successfully. |
openshift-ci-robot
added
the
verified
|
@BhargaviGudi: 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. |
BhargaviGudi
force-pushed
the
migrate-55486
branch
from
7e902fe to
cc8158d
Compare
openshift-ci-robot
removed
the
verified
|
@BhargaviGudi: This pull request references OCPNODE-4536 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 "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. |
|
Scheduling required tests: |
|
/verified by @BhargaviGudi - the test case was executed on the local cluster and passed successfully. |
BhargaviGudi
changed the title
openshift-ci-robot
added
the
verified
|
@BhargaviGudi: 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. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/retest-required |
|
/cc @bitoku |
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: cc8158d
|
|
/test e2e-aws-ovn-microshift-serial |
… events Migrates test from openshift-tests-private to origin. Test validates that cronjob events do not contain the error: "MountVolume.SetUp failed for volume ... object ... not registered" This is a regression test for a bug where volume mounting in cronjobs could fail with an error about unregistered objects. The test: 1. Gets all cronjob namespaces in the cluster 2. Retrieves events from each cronjob namespace 3. Checks for the error pattern using regex 4. Fails if any cronjob events contain the mount error Updates: - Add test to test/extended/node/node_e2e/node.go - Add regexp import for pattern matching - Document test in test/extended/node/README.md
BhargaviGudi
force-pushed
the
migrate-55486
branch
from
cc8158d to
84fa429
Compare
openshift-ci-robot
removed
the
verified
|
@BhargaviGudi: This pull request references OCPNODE-4536 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 "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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/extended/node/node_e2e/node.go (1)
202-203: ⚡ Quick winUse typed error comparison instead of string matching.
Checking for "timed out" in the error message is fragile.
wait.Pollreturns the sentinel errorwait.ErrWaitTimeouton timeout, so compare against that directly for more robust and idiomatic error handling.♻️ Proposed fix
- // Expect timeout (no errors found during the full polling window) - o.Expect(err).To(o.HaveOccurred()) - o.Expect(err.Error()).To(o.ContainSubstring("timed out")) + // Expect timeout (no errors found during the full polling window) + o.Expect(err).To(o.Equal(wait.ErrWaitTimeout))🤖 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` around lines 202 - 203, The test currently asserts the timeout by checking the error string; instead use a typed comparison against wait.ErrWaitTimeout (or errors.Is(err, wait.ErrWaitTimeout) to handle wrapped errors). Replace the o.Expect(err.Error()).To(o.ContainSubstring("timed out")) check with an assertion that the error equals or matches wait.ErrWaitTimeout (for example use errors.Is(err, wait.ErrWaitTimeout) and assert true), and add the "errors" import if it is not present.
🤖 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.
Nitpick comments:
In `@test/extended/node/node_e2e/node.go`:
- Around line 202-203: The test currently asserts the timeout by checking the
error string; instead use a typed comparison against wait.ErrWaitTimeout (or
errors.Is(err, wait.ErrWaitTimeout) to handle wrapped errors). Replace the
o.Expect(err.Error()).To(o.ContainSubstring("timed out")) check with an
assertion that the error equals or matches wait.ErrWaitTimeout (for example use
errors.Is(err, wait.ErrWaitTimeout) and assert true), and add the "errors"
import if it is not present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 91537fef-5f1f-47a8-a47c-1123c004aee2
📒 Files selected for processing (2)
test/extended/node/README.mdtest/extended/node/node_e2e/node.go
✅ Files skipped from review due to trivial changes (1)
- test/extended/node/README.md
|
Scheduling required tests: |
|
@BhargaviGudi: 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. |
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 84fa429
|
4 participants
Description
Migrates test case OCP-55486 from openshift-tests-private to origin repository.
Test Overview
This test validates that cronjob events do not contain volume mount errors related to unregistered objects. It checks for the error pattern:
MountVolume.SetUp failed for volume ... object ... not registeredTest Steps
Changes
test/extended/node/node_e2e/node.goregexpimport for pattern matchingtest/extended/node/README.mdRelated
Testing
Summary by CodeRabbit
Tests
Documentation