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
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe ChangesDevice Discovery Policy Validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)test/integration/qe_tests/lvms.goast-grep timed out on this file 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.62% +1.58%
==========================================
Files 53 54 +1
Lines 4033 4225 +192
==========================================
+ Hits 2139 2308 +169
- Misses 1723 1734 +11
- Partials 171 183 +12 🚀 New features to boost your workflow:
|
coderabbitai
Bot
added
the
ready-for-human-review
7b83924 to
8d84f64
Compare
openshift-ci
Bot
added
size/M
|
/test e2e-aws-single-node |
1 similar comment
|
/test e2e-aws-single-node |
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| defer func() { | ||
| deleteLVMClusterSafely(newLVMClusterName, lvmsNamespace, deviceClassName) | ||
| }() |
There was a problem hiding this comment.
we need the defer here, any reason you removed this ?
- Step 4 creates test-lvmcluster-89594
- The defer that would clean it up is now gone
- If steps 5-8 fail (assertion, timeout, anything), the test panics out
- The outer defer at step 3 (lines 5800-5805) tries to restore the original LVMCluster — but LVMS only allows one LVMCluster. The restore fails because test-lvmcluster-89594 still exists
- Result: the test cluster is leaked, the original LVMCluster is not restored, and all subsequent tests in this file fail
| excludedReasons, err := getExcludedDeviceReasons(deviceClassName) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| o.Expect(excludedReasons).To(o.ContainElement(o.ContainSubstring("excluded"))) | ||
| o.Expect(len(excludedReasons)).To(o.BeNumerically(">", 0), "Excluded devices should have reasons") |
There was a problem hiding this comment.
Step 7 already asserts len(excludedDevices) > 0. Step 8's purpose is to verify that excluded devices have clear status messages (per the g.By text). The new assertion only checks the
list is non-empty — it no longer validates the content of the reason strings at all. This makes step 8 a near-duplicate of step 7 with no additional value.
Either restore content validation or pick a more appropriate substring if "excluded" was too brittle:
o.Expect(excludedReasons).To(o.ContainElement(o.Not(o.BeEmpty())), "Excluded devices should have non-empty reason strings")
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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`:
- Around line 5912-5916: Update the VG preservation assertions around
getVGDevices in the Dynamic transition and the additionally referenced
transition block to compare canonicalized device identities, not just lengths.
Capture and canonicalize the original and post-transition device lists, then
assert the lists contain the same devices while retaining the existing error
checks and preservation expectations.
- Around line 5862-5867: Update the exec.Command calls in the Eventually
callbacks around the lvmvolumegroup checks and the additional locations to use
context-aware execution with a short per-invocation timeout. Create a
context.Context and cancel function for each command, pass it to
exec.CommandContext, and ensure cancellation occurs after the command completes
or times out so CombinedOutput cannot block indefinitely.
- Around line 5858-5870: The verification after deleteLVMClusterSafely must
confirm physical VG removal from every worker disk, not merely that
LVMVolumeGroup resources are gone. Replace or extend the oc get lvmvolumegroup
polling with node-side LVM state checks for all relevant workers and devices,
and wait until no VG remains before proceeding to recreate the cluster.
🪄 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: 2c3a85ac-302d-4f3f-8d93-11a254f5b1fd
📒 Files selected for processing (1)
test/integration/qe_tests/lvms.go
|
/retest-required |
coderabbitai
Bot
removed
the
ready-for-human-review
|
@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. |
|
/lgtm |
3 participants
…rable
Summary by CodeRabbit