fix: use architecture-aware RHEL preference in snapshot fixtures#4866
Conversation
|
Warning Review limit reached
More reviews will be available in 59 minutes and 58 seconds. 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 We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdd a session-scoped ChangesLatest RHEL Fixture Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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-3
requested review from
Ahmad-Hafe,
acinko-rh,
dalia-frank,
ema-aka-young,
josemacassan and
rnetser
openshift-virtualization-qe-bot-3
requested review from
dshchedr,
jpeimer and
vsibirsk
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
/lgtm |
|
Clean rebase detected — no code changes compared to previous head ( |
openshift-virtualization-qe-bot
removed
the
tox:verify-tc-requirement-polarion:passed
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
|
Clean rebase detected — no code changes compared to previous head ( |
carterpewpew
force-pushed
the
fix/use-arch-aware-rhel-preference-for-snapshot-fixtures
branch
from
3b7e4a8 to
7e532ce
Compare
|
Clean rebase detected — no code changes compared to previous head ( |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
Clean rebase detected — no code changes compared to previous head ( |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4856. Overlapping filestests/conftest.py |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4896. Overlapping filestests/conftest.py |
|
/approve |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4934. Overlapping filestests/conftest.py |
|
/approve |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4985. Overlapping filestests/conftest.py |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5099. Overlapping filestests/storage/conftest.py |
|
/verified |
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
Use these formats:
Real test commands (MANDATORY when changes affect session/runtime code): When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
Example output for a session-startup fix: 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
echo "=== Tests using rhel_vm_for_snapshot ==="
rg -rn "rhel_vm_for_snapshot" tests/ --include="*.py" -l
echo ""
echo "=== Tests using rhel_vm_for_snapshot_with_content ==="
rg -rn "rhel_vm_for_snapshot_with_content" tests/ --include="*.py" -l
echo ""
echo "=== Tests using latest_rhel_data_source ==="
rg -rn "latest_rhel_data_source" tests/ --include="*.py" -l
echo ""
echo "=== Smoke-marked tests in storage ==="
rg -rn "`@pytest.mark.smoke`" tests/storage/ --include="*.py" -l
echo ""
echo "=== Gating-marked tests in storage ==="
rg -rn "`@pytest.mark.gating`" tests/storage/ --include="*.py" -l
echo ""
echo "=== All smoke-marked test files ==="
rg -l "`@pytest.mark.smoke`" tests/ --include="*.py"
echo ""
echo "=== All gating-marked test files ==="
rg -l "`@pytest.mark.gating`" tests/ --include="*.py"
echo ""
echo "=== Storage snapshot test files ==="
fd "test_.*snapshot" tests/storage/ --extension py
echo ""
echo "=== VM export test files ==="
fd "test_" tests/storage/vm_export/ --extension py
echo ""
echo "=== Smoke/gating in snapshot test files ==="
rg -l "`@pytest.mark.smoke`|`@pytest.mark.gating`" tests/storage/ --include="*snapshot*"Length of output: 5486 🏁 Script executed: #!/bin/bash
echo "=== Tests using rhel_vm_for_snapshot ==="
rg -l "rhel_vm_for_snapshot" tests/
echo ""
echo "=== Tests using rhel_vm_for_snapshot_with_content ==="
rg -l "rhel_vm_for_snapshot_with_content" tests/
echo ""
echo "=== Tests using latest_rhel_data_source ==="
rg -l "latest_rhel_data_source" tests/
echo ""
echo "=== Smoke marks in snapshot test files ==="
rg "`@pytest.mark.smoke`" tests/storage/snapshots/test_snapshots.py tests/storage/golden_image/test_cached_snapshots.py tests/storage/vm_export/test_vm_export.py || echo "No smoke marks found"
echo ""
echo "=== Gating marks in snapshot test files ==="
rg "`@pytest.mark.gating`" tests/storage/snapshots/test_snapshots.py tests/storage/golden_image/test_cached_snapshots.py tests/storage/vm_export/test_vm_export.py || echo "No gating marks found"
echo ""
echo "=== All smoke-marked files ==="
rg -l "`@pytest.mark.smoke`" tests/
echo ""
echo "=== All gating-marked files ==="
rg -l "`@pytest.mark.gating`" tests/
echo ""
echo "=== Snapshot test file functions using rhel_vm_for_snapshot ==="
rg -n "rhel_vm_for_snapshot" tests/storage/snapshots/test_snapshots.py tests/storage/golden_image/test_cached_snapshots.py || echo "Not found in those files"
echo ""
echo "=== VM export test functions using rhel_vm_for_snapshot_with_content ==="
rg -n "rhel_vm_for_snapshot_with_content" tests/storage/vm_export/test_vm_export.py || echo "Not found"Length of output: 9331 🏁 Script executed: #!/bin/bash
# Get PR info to find diff position for inline comment
gh pr view 4866 --repo RedHatQE/openshift-virtualization-tests --json files
echo ""
echo "=== Get PR diff to find line numbers ==="
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/4866/files --jq '.[] | {filename: .filename, patch: .patch}' | head -100
echo ""
echo "=== Get latest commit SHA for inline comment ==="
gh pr view 4866 --repo RedHatQE/openshift-virtualization-tests --json headRefOid
echo ""
echo "=== Check infrastructure instance_types conftest for latest_rhel_data_source usage ==="
rg -n "latest_rhel_data_source" tests/infrastructure/instance_types/conftest.py || echo "Not found"
echo ""
echo "=== Check test_rhel_os.py for latest_rhel_data_source usage ==="
rg -n "latest_rhel_data_source" tests/infrastructure/instance_types/supported_os/test_rhel_os.py || echo "Not found in test file directly"
echo ""
echo "=== Check vhostmd test ==="
rg -n "latest_rhel_data_source" tests/infrastructure/vhostmd/test_vhostmd.py || echo "Not found"
echo ""
echo "=== Check gating in instance_types tests ==="
rg -n "`@pytest.mark.gating`" tests/infrastructure/instance_types/ || echo "None"Length of output: 8151 🏁 Script executed: #!/bin/bash
echo "=== Does test_rhel_os.py use rhel_vm_for_dedicated_cpu? ==="
rg -n "rhel_vm_for_dedicated_cpu" tests/infrastructure/instance_types/
echo ""
echo "=== Does vhostmd test have smoke or gating marks? ==="
rg -n "`@pytest.mark.smoke`|`@pytest.mark.gating`" tests/infrastructure/vhostmd/test_vhostmd.py || echo "No smoke/gating in vhostmd"
echo ""
echo "=== First changed line number in tests/conftest.py (from diff) ==="
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/4866/files --jq '.[] | select(.filename=="tests/conftest.py") | .patch' | head -5Length of output: 1305 🏁 Script executed: #!/bin/bash
# Post inline review comment on tests/conftest.py at the first changed line (position 5)
BODY="## Test Execution Plan
**Run smoke tests: False**
No verified dependency path from any smoke-marked test to the modified fixtures. \`tests/infrastructure/instance_types/supported_os/test_rhel_os.py\` (smoke) does not use \`rhel_vm_for_dedicated_cpu\` or any other fixture that consumes \`latest_rhel_data_source\`.
**Run gating tests: True**
- \`tests/storage/vm_export/test_vm_export.py\` (gating) → \`rhel_vm_for_snapshot_with_content\` (tests/storage/vm_export/conftest.py) → \`latest_rhel_data_source\` (scope changed: module→session, moved to tests/conftest.py)
- \`tests/infrastructure/instance_types/test_common_vm_instancetype.py\` (gating) → \`rhel_vm_for_dedicated_cpu\` (tests/infrastructure/instance_types/conftest.py) → \`latest_rhel_data_source\`
**Affected tests to run**
| Test file | Marker | Changed fixture dependency |
|---|---|---|
| \`tests/storage/snapshots/test_snapshots.py\` | — | \`rhel_vm_for_snapshot\` → \`latest_rhel_data_source\` |
| \`tests/storage/vm_export/test_vm_export.py\` | gating | \`rhel_vm_for_snapshot_with_content\` → \`latest_rhel_data_source\` |
| \`tests/infrastructure/instance_types/test_common_vm_instancetype.py\` | gating | \`rhel_vm_for_dedicated_cpu\` → \`latest_rhel_data_source\` |
| \`tests/infrastructure/vhostmd/test_vhostmd.py\` | — | \`latest_rhel_data_source\` (direct use; scope was module, now session) |
**Real tests (cluster required)**
These fixtures run at session/collection time. Verify on real clusters:
_Error path (the fix — run on s390x cluster):_
\`\`\`
pytest tests/storage/snapshots/test_snapshots.py --collect-only
pytest tests/storage/vm_export/test_vm_export.py --collect-only
\`\`\`
Expected: session collects without error; VMs boot using RHEL9 preference (no SecureBoot/EFI requirement).
_Happy path (regression — run on amd64/arm64 cluster):_
\`\`\`
pytest tests/storage/snapshots/test_snapshots.py -k test_snapshot
pytest tests/storage/vm_export/test_vm_export.py -m gating
\`\`\`
Expected: RHEL10 preference resolved, VMs boot normally, snapshot/export tests pass."
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/4866/reviews \
--method POST \
--field commit_id="09f94f1bf475dcce7de6c613eadbdbba4d7b7cd6" \
--field event="COMMENT" \
--field body="" \
--field "comments[][path]=tests/conftest.py" \
--field "comments[][position]=5" \
--field "comments[][body]=$BODY"Length of output: 4280 |
|
Clean rebase detected — no code changes compared to previous head ( |
The rhel_vm_for_snapshot and rhel_vm_for_snapshot_with_content fixtures hardcode RHEL10_PREFERENCE which enables SecureBoot. On s390x, SecureBoot is unsupported (no EFI OVMF ROMs), causing VM boot failures. Replace hardcoded RHEL10 preference and data source with dynamic lookup via py_config["latest_instance_type_rhel_os_dict"], which resolves to RHEL9 on s390x (no SecureBoot) and RHEL10 on amd64/arm64. Add latest_rhel_data_source_scope_session fixture to tests/conftest.py that dynamically selects the data source based on architecture. Signed-off-by: Jathavedhan M <jathavedhan.m@ibm.com> Co-authored-by: Alexander Mamani Yucra <alexander.mamani@ibm.com>
|
Clean rebase detected — no code changes compared to previous head ( |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4808. Overlapping filestests/conftest.py |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5167. Overlapping filestests/conftest.py |
11 participants
Short description: Use architecture-aware RHEL preference in snapshot and vmexport fixtures
More details:
The
rhel_vm_for_snapshotandrhel_vm_for_snapshot_with_contentfixtures hardcodeRHEL10_PREFERENCE, which enables SecureBoot. On s390x, SecureBoot is unsupported (no EFI OVMF ROMs), causing VM boot failures. The codebase already handles this via per-architecture global configs -- these fixtures were bypassing that.What this PR does / why we need it:
py_config["latest_instance_type_rhel_os_dict"]latest_rhel_data_source_scope_sessionfixture totests/conftest.pyalongside the existing rhel9/rhel10 data source fixturesWhich issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket: NONE
Summary by CodeRabbit