Automate OCP-89594-Verify LVMCluster deviceDiscoveryPolicy is configu…#2758
Automate OCP-89594-Verify LVMCluster deviceDiscoveryPolicy is configu…#2758mmakwana30 wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 41 minutes. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughTest helpers now apply LVMCluster manifests with server-side fallback, and disruptive integration tests recreate saved clusters through the manifest helper. The deviceDiscoveryPolicy scenario also updates runtime transition checks and excluded-device assertions. ChangesLVMS manifest recreation and policy transitions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 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 |
openshift-ci
Bot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mmakwana30 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/integration/qe_tests/lvms_utils.go (1)
803-817: 🗄️ Data Integrity & Integration | 🟠 MajorSanitize exported JSON fields before applying
Raw server-side fields (specifically
status,uid,resourceVersion, andmanagedFields) passed in the manifest cause conflicts or validation errors, even with the fallback client-side apply. The input JSON must be stripped of these server-owned keys before execution.🤖 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/integration/qe_tests/lvms_utils.go` around lines 803 - 817, The createLVMClusterFromManifest flow still passes exported JSON with server-owned fields through both apply paths, so sanitize the manifest before calling oc apply. In createLVMClusterFromManifest, strip status, uid, resourceVersion, and managedFields from the input manifest first, then use the cleaned manifest for both the server-side and fallback client-side apply commands so the function no longer feeds invalid server-managed data to oc.
🧹 Nitpick comments (1)
test/integration/qe_tests/lvms_utils.go (1)
806-812: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound both
oc applycalls with a timeout.Use
exec.CommandContextfor the SSA attempt and fallback so a stuck CLI/API call does not hang the serial disruptive suite.As per path instructions,
**/*.go:context.Context for cancellation and timeouts.🤖 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/integration/qe_tests/lvms_utils.go` around lines 806 - 812, Both oc apply invocations in the apply-manifest flow are unbounded and can hang the serial suite; update the logic around the existing exec.Command calls to use exec.CommandContext with a context that has a timeout for both the server-side apply attempt and the fallback apply. Make sure the context is created in the surrounding helper before calling the apply commands, and that both command paths in this function use the same cancellation/timeout handling so a stuck CLI or API call cannot block the run.Source: Path instructions
🤖 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/integration/qe_tests/lvms.go`:
- Line 564: Handle the restore calls in the cleanup defers so their errors are
not silently discarded; update each createLVMClusterFromManifest(...) use in the
affected deferred cleanup blocks to capture the returned error and either log it
with context or fail cleanup explicitly, following the same pattern across the
repeated restore sites in qe_tests/lvms.go.
- Around line 5859-5866: The VG cleanup check in the eventual assertion
currently treats any `oc get` error as success, which can mask transient
failures. Update the retry logic around the `exec.Command("oc", "get",
"lvmvolumegroup", ...)` call so that command errors are logged and return false
to keep retrying, and only return true when the `CombinedOutput` succeeds and
the trimmed output is actually empty. Keep the behavior localized to the
`Eventually` block in `lvms.go`.
- Line 5832: The test setup for createLVMClusterFromManifest should register
failure-path cleanup for test-lvmcluster-89594 as soon as newLVMClusterName and
deviceClassName are initialized. Add an immediate defer in this setup block to
delete the temporary LVMCluster on failure before any later assertions run, so
the existing restore defer in the surrounding test does not leave the temporary
resource behind.
---
Outside diff comments:
In `@test/integration/qe_tests/lvms_utils.go`:
- Around line 803-817: The createLVMClusterFromManifest flow still passes
exported JSON with server-owned fields through both apply paths, so sanitize the
manifest before calling oc apply. In createLVMClusterFromManifest, strip status,
uid, resourceVersion, and managedFields from the input manifest first, then use
the cleaned manifest for both the server-side and fallback client-side apply
commands so the function no longer feeds invalid server-managed data to oc.
---
Nitpick comments:
In `@test/integration/qe_tests/lvms_utils.go`:
- Around line 806-812: Both oc apply invocations in the apply-manifest flow are
unbounded and can hang the serial suite; update the logic around the existing
exec.Command calls to use exec.CommandContext with a context that has a timeout
for both the server-side apply attempt and the fallback apply. Make sure the
context is created in the surrounding helper before calling the apply commands,
and that both command paths in this function use the same cancellation/timeout
handling so a stuck CLI or API call cannot block the run.
🪄 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: 0e870db2-2132-482f-9ae3-da9a4672d852
📒 Files selected for processing (2)
test/integration/qe_tests/lvms.gotest/integration/qe_tests/lvms_utils.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2758 +/- ##
==========================================
+ Coverage 53.03% 54.14% +1.11%
==========================================
Files 53 54 +1
Lines 4033 4170 +137
==========================================
+ Hits 2139 2258 +119
- Misses 1723 1730 +7
- Partials 171 182 +11 🚀 New features to boost your workflow:
|
coderabbitai
Bot
added
the
ready-for-human-review
openshift-ci
Bot
added
size/M
|
/test e2e-aws-single-node |
1 similar comment
|
/test e2e-aws-single-node |
|
@mmakwana30: 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. |
2 participants
…rable
Summary by CodeRabbit