[Storage] Remove skip_block_volumemode from archive import test#5099
Conversation
Archive import only supports Filesystem volume mode, so explicitly set it on the DV instead of skipping Block volume mode at runtime. Remove the unused skip_block_volumemode_scope_module fixture. Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: Jenia Peimer <jpeimer@redhat.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughFixtures now accept and propagate a per-test volume_mode for HTTP DataVolume imports; tests specify FILE for archive imports; running pod fixture reads volumeMode from the created DV PVC; test file-counting helper filters ls output before asserting. ChangesHTTP Import Volume Mode Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot-4
requested review from
ema-aka-young and
kshvaika
openshift-virtualization-qe-bot-4
requested review from
Ahmad-Hafe,
josemacassan and
kgoldbla
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/storage/cdi_import/test_import_http.py`:
- Line 135: The test test_successful_import_secure_archive forces the DataVolume
to Filesystem but running_pod_with_dv_pvc still uses the storage-class default
volume_mode when calling create_pod_for_pvc, causing consumer/producer mismatch;
update the test so the pod creation uses the actual PVC/DV volume_mode (e.g.,
retrieve the DataVolume or resulting PVC volumeMode after create_dv and pass
that explicit value into create_pod_for_pvc) or explicitly pass
volume_mode='Filesystem' into create_pod_for_pvc to ensure create_dv and
create_pod_for_pvc agree on the PVC mode.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8f770483-a447-4e23-90a1-070b8ba67e5f
📒 Files selected for processing (3)
tests/storage/cdi_import/conftest.pytests/storage/cdi_import/test_import_http.pytests/storage/conftest.py
💤 Files with no reviewable changes (1)
- tests/storage/conftest.py
Read volume mode from the actual PVC spec instead of the storage class matrix, ensuring the pod matches the DV's effective volume mode (e.g., Filesystem for archive imports) regardless of storage class defaults. Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: Jenia Peimer <jpeimer@redhat.com>
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5099 published |
|
Verification failed for PR #5099 (image: openshift-virtualization-tests:pr-5099). |
Block volume mode PVCs create a lost+found directory that should not be counted when asserting the number of files in a pod. Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: Jenia Peimer <jpeimer@redhat.com>
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Output rules: Your deliverable: Test Execution Plan
Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL — Verification rules for smoke and gating decisions: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Find smoke and gating marked tests
echo "=== SMOKE TESTS ==="
rg -l '`@pytest.mark.smoke`' tests/ 2>/dev/null
echo ""
echo "=== GATING TESTS ==="
rg -l '`@pytest.mark.gating`' tests/ 2>/dev/null
echo ""
echo "=== assert_num_files_in_pod callers ==="
rg -n 'assert_num_files_in_pod' tests/ --type py -C2
echo ""
echo "=== running_pod_with_dv_pvc callers ==="
rg -n 'running_pod_with_dv_pvc' tests/ --type py -C2
echo ""
echo "=== dv_from_http_import callers ==="
rg -n 'dv_from_http_import' tests/ --type py -C2
echo ""
echo "=== PR diff to find first changed line ==="
git diff HEAD~2..HEAD -- tests/storage/cdi_import/conftest.py | head -60Length of output: 12064 🏁 Script executed: #!/bin/bash
# Get the actual diff
git log --oneline -5
echo "---"
git diff HEAD~1..HEAD -- tests/storage/cdi_import/conftest.py | head -80
echo "=== test_import_http markers ==="
rg -n '`@pytest.mark`\.' tests/storage/cdi_import/test_import_http.py
echo "=== conftest.py full relevant section ==="
grep -n '' tests/storage/cdi_import/conftest.py | head -120Length of output: 5422 🏁 Script executed: #!/bin/bash
# Get PR details and diff to find the right commit SHA and line for inline comment
gh pr view 5099 --json commits,headRefOid,baseRefOid | jq '{headRefOid, baseRefOid, commits: [.commits[] | {oid, messageHeadline}]}'
echo "==="
# Get files changed in PR
gh pr diff 5099 -- tests/storage/cdi_import/conftest.py 2>/dev/null | head -60Length of output: 1078 🏁 Script executed: #!/bin/bash
# Get the full PR diff
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5099/files \
--jq '.[] | select(.filename | contains("cdi_import/conftest")) | {filename, patch: .patch[0:500]}'
echo "==="
# Get the raw diff between base and head
git fetch origin main b1117fc37b9d071233f9d43518c46c1102404c4a 2>/dev/null || true
git diff b1117fc37b9d071233f9d43518c46c1102404c4a 0455bd443f42fe66e53787e8465c661df9bc0ee0 -- tests/storage/cdi_import/conftest.py 2>/dev/null | head -80Length of output: 2371 🏁 Script executed: #!/bin/bash
# Post inline review comment on the first changed line of tests/storage/cdi_import/conftest.py
BODY="**Test Execution Plan**
**Run smoke tests: False** — No smoke-marked tests use the modified fixtures or \`assert_num_files_in_pod\`.
**Run gating tests: True** — \`dv_from_http_import\` fixture now forwards \`volume_mode\` to \`create_dv\`; two gating tests depend on it directly:
- \`test_successful_import_image\` → \`dv_from_http_import\` (fixture changed: \`volume_mode=request.param.get(\"volume_mode\")\` added to \`create_dv\` call)
- \`test_successful_import_secure_image\` → \`dv_from_http_import\` (same path)
**Affected tests to run:**
- \`tests/storage/cdi_import/test_import_http.py::test_successful_import_image\` (gating)
- \`tests/storage/cdi_import/test_import_http.py::test_successful_import_secure_image\` (gating)
- \`tests/storage/cdi_import/test_import_http.py::test_successful_import_secure_archive\` — fixture chain changed (\`running_pod_with_dv_pvc\` now reads \`volumeMode\` from actual PVC spec; \`assert_num_files_in_pod\` now filters \`lost+found\`)"
gh api \
repos/RedHatQE/openshift-virtualization-tests/pulls/5099/comments \
-X POST \
-f body="$BODY" \
-f commit_id="0455bd443f42fe66e53787e8465c661df9bc0ee0" \
-f path="tests/storage/cdi_import/conftest.py" \
-f side="RIGHT" \
-F line=95Length of output: 5174 |
|
/lgtm |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4985. Overlapping filestests/storage/conftest.py |
|
/lgtm |
|
polarion check was missing, ran it manually: @rnetser FYI |
|
/approve |
rnetser
added
tox:verify-tc-requirement-polarion:passed
|
/approve |
|
Successfully removed PR tag: quay.io/openshift-cnv/openshift-virtualization-tests:pr-5099. |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:latest published |
10 participants
What this PR does / why we need it:
Filesystemvolume mode, so explicitly set it on the DV instead of skipping Block volume mode at runtimeskip_block_volumemode_scope_modulefixture.running_pod_with_dv_pvcfixtureAssisted-by: Claude noreply@anthropic.com
Which issue(s) this PR fixes:
Remove programmatic skip
Special notes for reviewer:
jira-ticket:
Summary by CodeRabbit