Skip to content

[VIRT] Add PCI topology stability tests - #5721

Open
SamAlber wants to merge 3 commits into
RedHatQE:mainfrom
SamAlber:pci-topology-stability-tests
Open

[VIRT] Add PCI topology stability tests#5721
SamAlber wants to merge 3 commits into
RedHatQE:mainfrom
SamAlber:pci-topology-stability-tests

Conversation

@SamAlber

@SamAlber SamAlber commented Jul 21, 2026

Copy link
Copy Markdown
Contributor
What this PR does / why we need it:

Adds test automation for PCI topology stability (CNV-81270). Verifies that
PCI device addresses remain unchanged across VM restart, migration,
snapshot/restore, and CNV upgrade by comparing sorted lspci output before
and after each operation, so failures show exactly which addresses changed.

Which issue(s) this PR fixes:
Special notes for reviewer:

STP: RedHatQE/openshift-virtualization-tests-design-docs#138

jira-ticket:

https://redhat.atlassian.net/browse/CNV-81270

Summary by CodeRabbit

Summary by CodeRabbit

  • Tests
    • Added coverage confirming PCI device addressing remains stable after VM restart, live migration, and snapshot restoration.
    • Extended virtualization upgrade testing to verify PCI topology remains unchanged before and after upgrades.
    • Added shared tooling to capture and compare VM PCI device information consistently.

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds an SSH-based PCI address utility and validates PCI address stability across VM restart, migration, snapshot restore, and virtualization upgrades.

Changes

PCI topology validation

Layer / File(s) Summary
PCI address collection
tests/virt/utils.py, tests/virt/upgrade/conftest.py
Adds get_pci_addresses, which retrieves and returns sorted guest PCI address lines. Adds a session-scoped pre-upgrade address mapping.
Lifecycle stability tests
tests/virt/node/general/test_pci_topology_stability.py
Creates a VM and compares PCI addresses after restart, migration, and snapshot restore.
Upgrade topology validation
tests/virt/upgrade/test_upgrade_virt.py
Compares each VM’s PCI addresses before and after the virtualization upgrade.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested labels: new-tests

Suggested reviewers: azhivovk, rnetser, hmeir


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (1 error)

Check name Status Explanation Resolution
Pr Template Sections ❌ Error The PR body has meaningful What, Special notes, and jira-ticket sections, but it omits the required Which issue(s) this PR fixes section. HIGH: Restore ##### Which issue(s) this PR fixes: in the PR description. It may remain empty, but all required template sections must be present.
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stp Link Required ✅ Passed The new topology test module has a module-level STP URL, and the added upgrade test function has an STP URL in its docstring. No Jira line lacks the required skip marker.
Stp Scenario Coverage ✅ Passed STP #138 defines four P0 scenarios; CNV-16326–16329 cover restart, migration, snapshot/restore, and upgrade with matching operations, assertions, and STP links.
Title check ✅ Passed The title is under 120 characters and clearly describes the added PCI topology stability tests, which are the main change.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@openshift-virtualization-qe-bot-2

Copy link
Copy Markdown
Contributor

Report bugs in Issues

Welcome! 🎉

This pull request will be automatically processed with the following features:

🔄 Automatic Actions

  • Reviewer Assignment: Reviewers are automatically assigned based on the OWNERS file in the repository root
  • Size Labeling: PR size labels (XS, S, M, L, XL, XXL) are automatically applied based on changes
  • Issue Creation: A tracking issue is created for this PR and will be closed when the PR is merged or closed
  • Branch Labeling: Branch-specific labels are applied to track the target branch
  • Auto-verification: Auto-verified users have their PRs automatically marked as verified
  • Labels: Enabled categories: branch, can-be-merged, cherry-pick, has-conflicts, hold, needs-rebase, size, verified, wip

📋 Available Commands

PR Status Management

  • /wip - Mark PR as work in progress (adds WIP: prefix to title)
  • /wip cancel - Remove work in progress status
  • /hold - Block PR merging (approvers only)
  • /hold cancel - Unblock PR merging
  • /verified - Mark PR as verified
  • /verified cancel - Remove verification status
  • /reprocess - Trigger complete PR workflow reprocessing (useful if webhook failed or configuration changed)
  • /regenerate-welcome - Regenerate this welcome message
  • /security-override - Set security check runs to pass (maintainers only)
  • /security-override cancel - Re-run security checks

Review & Approval

  • /lgtm - Approve changes (looks good to me)
  • /approve - Approve PR (approvers only)
  • /assign-reviewers - Assign reviewers based on OWNERS file
  • /assign-reviewer @username - Assign specific reviewer
  • /check-can-merge - Check if PR meets merge requirements

Testing & Validation

  • /retest tox - Run Python test suite with tox
  • /retest build-container - Rebuild and test container image
  • /retest verify-bugs-are-open - verify-bugs-are-open
  • /retest all - Run all available tests

Container Operations

  • /build-and-push-container - Build and push container image (tagged with PR number)
    • Supports additional build arguments: /build-and-push-container --build-arg KEY=value

Cherry-pick Operations

  • /cherry-pick <branch> - Schedule cherry-pick to target branch when PR is merged
    • Multiple branches: /cherry-pick branch1 branch2 branch3
  • /cherry-pick-retry <branch> - Retry a failed cherry-pick (merged PRs only)

Branch Management

  • /rebase - Rebase this PR branch onto its base branch

Label Management

  • /<label-name> - Add a label to the PR
  • /<label-name> cancel - Remove a label from the PR

✅ Merge Requirements

This PR will be automatically approved when the following conditions are met:

  1. Approval: /approve from at least one approver
  2. LGTM Count: Minimum 2 /lgtm from reviewers
  3. Status Checks: All required status checks must pass
  4. No Blockers: No wip, hold, has-conflicts labels and PR must be mergeable (no conflicts)
  5. Verified: PR must be marked as verified

📊 Review Process

Approvers and Reviewers

Approvers:

  • dshchedr
  • myakove
  • rnetser
  • vsibirsk

Reviewers:

  • RoniKishner
  • SamAlber
  • akri3i
  • dshchedr
  • hmeir
  • rlobillo
  • rnetser
  • vsibirsk
Available Labels
  • hold
  • verified
  • wip
  • lgtm
  • approve
AI Features
  • Cherry-Pick Conflict Resolution: Enabled (claude/claude-opus-4-6-1m)
Security Checks
  • Suspicious Path Detection: Monitors paths: .claude/, .vscode/, .cursor/, .devcontainer/, .pi/, .github/workflows/, .github/actions/
  • Committer Identity Check: Verifies last committer matches PR author
  • Mandatory: Security checks block merge (use /security-override to bypass — maintainers only)

💡 Tips

  • WIP Status: Use /wip when your PR is not ready for review
  • Verification: The verified label is removed on new commits unless the push is detected as a clean rebase
  • Cherry-picking: Cherry-pick labels are processed when the PR is merged
  • Container Builds: Container images are automatically tagged with the PR number
  • Permission Levels: Some commands require approver permissions
  • Auto-verified Users: Certain users have automatic verification and merge privileges

📌 Additional Information

Custom Commands:

  • /test-plan — Triggers CodeRabbit to analyze the PR's changed files and post a test execution plan
  • /rerun-smoke — Adds retest-smoke label on the PR to trigger smoke tests execution

For more information, please refer to the project documentation or contact the maintainers.

@openshift-virtualization-qe-bot

Copy link
Copy Markdown

D/S test tox -e verify-tc-requirement-polarion failed: cnv-tests-tox-executor/30785

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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/virt/node/general/test_pci_topology_stability.py`:
- Line 76: The new tests lack required STP/RFE/Jira traceability documentation.
Add a docstring containing the appropriate traceability URL to
TestPCITopologyStability or the module docstring in
tests/virt/node/general/test_pci_topology_stability.py:76-76, and add one to
test_pci_topology_after_upgrade in
tests/virt/upgrade/test_upgrade_virt.py:313-319 unless TestUpgradeVirt already
provides it.
- Line 27: Update the module-level pytestmark declaration to include
pytest.mark.special_infra alongside rwx_default_storage and
data_collector_scope, preserving the existing markers.
🪄 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: c4a4cefe-031c-4db8-b954-cd1dd80df5cf

📥 Commits

Reviewing files that changed from the base of the PR and between 30f2a62 and 48e1a1d.

📒 Files selected for processing (4)
  • tests/virt/node/general/test_pci_topology_stability.py
  • tests/virt/upgrade/conftest.py
  • tests/virt/upgrade/test_upgrade_virt.py
  • tests/virt/utils.py
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

Comment thread tests/virt/node/general/test_pci_topology_stability.py
Comment thread tests/virt/node/general/test_pci_topology_stability.py
@openshift-virtualization-qe-bot

Copy link
Copy Markdown

D/S test tox -e verify-tc-requirement-polarion failed: cnv-tests-tox-executor/30793

@openshift-virtualization-qe-bot

Copy link
Copy Markdown

D/S test tox -e verify-tc-requirement-polarion failed: cnv-tests-tox-executor/30843

@openshift-virtualization-qe-bot

Copy link
Copy Markdown

D/S test tox -e verify-tc-requirement-polarion failed: cnv-tests-tox-executor/30844

Verify that PCI device addresses remain unchanged across VM restart,
migration, snapshot/restore, and CNV upgrade.

- Add get_pci_fingerprint() to tests/virt/utils.py (md5 of sorted
  lspci)
- Add tests/virt/node/general/test_pci_topology_stability.py with
  restart, migration, and snapshot/restore tests
- Add PCI topology check to upgrade tests (before/after fingerprint
  comparison on all upgrade VMs)

Signed-off-by: Samuel Albershtein <salbersh@redhat.com>
Assisted-by: Claude <noreply@anthropic.com>
@openshift-virtualization-qe-bot

Copy link
Copy Markdown

D/S test tox -e verify-tc-requirement-polarion failed: cnv-tests-tox-executor/31064

@openshift-virtualization-qe-bot-3

Copy link
Copy Markdown
Contributor

/retest all

Auto-triggered: Files in this PR were modified by merged PR #5823.

Overlapping files

tests/virt/upgrade/test_upgrade_virt.py

@openshift-virtualization-qe-bot-3

Copy link
Copy Markdown
Contributor

/retest all

Auto-triggered: Files in this PR were modified by merged PR #5777.

Overlapping files

tests/virt/upgrade/conftest.py
tests/virt/utils.py

@openshift-virtualization-qe-bot

Copy link
Copy Markdown

D/S test tox -e verify-tc-requirement-polarion failed: cnv-tests-tox-executor/31363

Add the arm64 marker to enable collection on aarch64 platforms.
@openshift-virtualization-qe-bot

Copy link
Copy Markdown

D/S test tox -e verify-tc-requirement-polarion failed: cnv-tests-tox-executor/31365

dshchedr
dshchedr previously approved these changes Aug 5, 2026
@openshift-virtualization-qe-bot-3

Copy link
Copy Markdown
Contributor

/retest all

Auto-triggered: Files in this PR were modified by merged PR #5893.

Overlapping files

tests/virt/utils.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/virt/utils.py`:
- Around line 418-420: Update the helper returning lspci results to sort the
split PCI address lines before logging and returning them; preserve the existing
VM context in LOGGER.info and return the deterministically sorted list.
🪄 Autofix

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 Plus

Run ID: aaf2bd4d-2b9f-44db-a936-20043ae7d3a7

📥 Commits

Reviewing files that changed from the base of the PR and between 62ce51f and 4531c47.

📒 Files selected for processing (4)
  • tests/virt/node/general/test_pci_topology_stability.py
  • tests/virt/upgrade/conftest.py
  • tests/virt/upgrade/test_upgrade_virt.py
  • tests/virt/utils.py
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

Comment thread tests/virt/utils.py
@openshift-virtualization-qe-bot

Copy link
Copy Markdown

D/S test tox -e verify-tc-requirement-polarion failed: cnv-tests-tox-executor/31552

Replace get_pci_fingerprint (MD5 hash of BDF addresses) with
get_pci_addresses (lspci output) so assertion failures show
which device addresses actually changed instead of two opaque hashes.

Update PCI topology stability tests and upgrade fixtures accordingly.

Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Samuel Albershtein <salbersh@redhat.com>
@openshift-virtualization-qe-bot-6

Copy link
Copy Markdown

Clean rebase detected — no code changes compared to previous head (4531c47).
The following labels were preserved: commented-coderabbitai[bot], commented-SamAlber, changes-requested-coderabbitai[bot].

@openshift-virtualization-qe-bot

Copy link
Copy Markdown

D/S test tox -e verify-tc-requirement-polarion failed: cnv-tests-tox-executor/31583

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants