NO-JIRA:feat(docs): add RAID test plan#2861
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@qJkee: This pull request explicitly references no 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qJkee The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
approved
|
Caution Review failedAn error occurred during the review process. Please try again later. WalkthroughAdds a comprehensive manual test plan for LVMS RAID on OpenShift, covering setup, valid and invalid configurations, immutability, day-2 operations, failure recovery, observability, guardrails, cleanup, and execution guidance. ChangesLVMS RAID validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (14 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@docs/testing/raid-test-plan.md`:
- Around line 764-770: Update the RAID reconciliation interval assertions in
Test 6A.3 and the referenced checks to expect a 30-second periodic requeue for
RAID/Dynamic configurations. Add or revise the static-configuration assertions
to require no periodic requeue when explicit paths are configured, keeping each
expected outcome aligned with the configuration type.
- Around line 1260-1272: Update the Test Summary table in the RAID test plan so
Phase 6 lists 34 tests, and update the overall Total from 82 to 94; leave the
other phase counts and descriptions unchanged.
- Around line 739-745: Update the four output-only fenced blocks in the testing
plan, including the blocks near the RAID metrics and the referenced sections, by
adding the text language identifier to each opening fence. Preserve all enclosed
output unchanged and ensure every affected fence satisfies markdownlint MD040.
- Around line 159-166: Add an app=raid-test label to the raid-test-pod metadata
so the existing oc delete pods -l app=raid-test cleanup selector matches it and
removes the pod, preserving the current pod name and test behavior.
- Around line 1190-1203: Update the “Delete All Test Resources” cleanup commands
to target only PVCs and LVMClusters created by the RAID test, using dedicated
resource names or test-specific labels. Ensure every namespace-scoped command,
including PVC deletion, waits, and related cleanup, explicitly passes -n "$NS"
and preserves the existing finalizer wait behavior without deleting unrelated
workloads.
- Around line 1040-1067: Update the RAID sync throttling steps around the node
debug commands to capture the node’s original speed_limit_max and
speed_limit_min values before changing them, then restore those captured values
during cleanup. Use trap/finally-style cleanup so restoration runs whether
verification succeeds or fails, including the corresponding procedure at the
additional sync-throttling section.
- Around line 110-133: Make the RAID test-plan YAML examples explicitly render
shell variables before application. Update the snippets around the LVMCluster
definitions to document using envsubst or an equivalent shell heredoc, or
replace $NODE and $LOOP* with clearly defined placeholders and rendering
instructions; preserve the intended node selector and device paths.
- Around line 589-591: Update the RAID test-plan patches at the shown and
referenced locations to avoid merge-patching partial deviceClasses objects. Use
JSON Patch to modify only the nested raidConfig/deviceSelector fields, or
include every required device-class field such as fstype, default, and
nodeSelector while preserving existing configuration.
- Around line 883-901: Update Test 6C.1 to recover the detached loop device
without overwriting its existing backing file; remove the in-place dd recreation
and reuse the original loop image or an explicitly preserved replacement. Update
Test 6C.2 so the intended loop-device path is expanded or passed into the bash
command rather than remaining literal inside single quotes, ensuring vgextend
--restoremissing targets the recovered device.
🪄 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: 1a659fdc-1bfe-451d-b419-dfdb7ffd51e6
📒 Files selected for processing (1)
docs/testing/raid-test-plan.md
|
@qJkee: 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. |
|
|
||
| - OpenShift cluster with latest LVMS operator deployed | ||
| - `oc` CLI authenticated with cluster-admin | ||
| - One worker node with enough disk space for 10 x 1GB backing files (~10GB in /var/tmp) |
There was a problem hiding this comment.
When you say one worker node, is the target here SNO clusters ?
|
|
||
| ```bash | ||
| export NODE="<worker-node-name>" | ||
| export NS="openshift-storage" # or wherever LVMS is deployed |
There was a problem hiding this comment.
Let us set this to openshift-lvm-storage as starting 4.20 we always deploy it on openshift-lvm-storage.
|
|
||
| **Expected**: Alert rule present with `for: 30m` and `lvms_raid_sync_in_progress == 1`. | ||
|
|
||
| ### Test 6F.3: Multiple LVs — minSyncPercent Picks Slowest |
There was a problem hiding this comment.
This depends on what kind of disks we are using and this might be out of scope for the testing of this feature in lvms right ?
|
|
||
| **Expected**: No entry for the RAID device class StorageClass. Only thin-pool DCs get VolumeSnapshotClasses. | ||
|
|
||
| ### Test 7.3: StorageClass is Thick Provisioned |
There was a problem hiding this comment.
This test is really not required, as we have a test above to verify that no thinpool related params are allowed right ?
|
|
||
| **Expected**: No thin-pool-related parameters. Should look like a standard thick StorageClass. | ||
|
|
||
| ### Test 7.4: Device Removal Blocked When Below Minimum |
There was a problem hiding this comment.
I think this was covered as well in the above section, if i am not wrong.
|
|
||
| ### Test 7.7: Dynamic Discovery Ignored for RAID | ||
|
|
||
| If deviceDiscoveryPolicy were to matter, extra unassigned loop devices might get auto-added to the RAID VG. |
There was a problem hiding this comment.
I see the tile and verify are misleading here, worth checking to make sure they align fine.
| dd if=/dev/zero of=/var/tmp/raid-test-loop${i}.img bs=1M count=1024 status=none | ||
| LOOP=$(losetup --find --show /var/tmp/raid-test-loop${i}.img) | ||
| echo "Created: $LOOP -> /var/tmp/raid-test-loop${i}.img" | ||
| done |
There was a problem hiding this comment.
should we use actual disks here instead of loop devices ?
| @@ -0,0 +1,1283 @@ | |||
| # RAID Feature — Comprehensive Test Plan | |||
|
|
|||
| Manual testing plan for the LVMS RAID feature on a live OpenShift cluster. | |||
There was a problem hiding this comment.
Thanks for putting this together. One question — what is the plan for these tests? Are they intended to stay as a manual test plan, or is the goal to eventually automate them as e2e tests ? If automation is planned, should QE be involved in writing or reviewing the test automation?
| | 6. Failure & Recovery | 34 | Degraded/Failed states, metrics at every transition, sync monitoring, multi-LV minSyncPercent | | ||
| | 7. Guardrails | 7 | Snapshot rejection, thick SC, finalizers, force-wipe, discovery isolation | | ||
| | 8. Cleanup | 5 | Ordered teardown, orphan check | | ||
| | **Total** | **94** | | |
There was a problem hiding this comment.
Do not see any test related to upgrade ?
|
|
||
| ```bash | ||
| # Pattern for each test: | ||
| oc apply -f bad-cr.yaml 2>&1 |
There was a problem hiding this comment.
where is this file or contents of the file ?
| 4. Write different data to each | ||
| 5. **Verify**: All 3 PVCs provisioned, all data readable, `lvs` shows 3 RAID LVs in same VG | ||
|
|
||
| ### Test 5.4: Pod Reschedule — Data Persistence |
There was a problem hiding this comment.
what is the difference between this and # Test 2.12: raid1 ext4 — Data Integrity Across Pod Reschedule ? can we just have one and remove the other one ?
3 participants
Summary by CodeRabbit