Fix wrongly set expr_driver for vfio_vfio variant #6617

hholoubk · 2025-10-17T11:32:33Z

The vfio_vfio variant uses the generic vfio-pci driver but was incorrectly expecting mlx5_vfio_pci driver. This caused false test failures where the test reported 'Get incorrect driver vfio-pci, it should be mlx5_vfio_pci' even though the actual driver was correct.

Changes:

Add expr_driver = 'vfio-pci' for the vfio_vfio variant
Move test_pf parameter to x86_64 section only (not needed for aarch64)

AI assisted code and commit. Human reviewed.

Summary by CodeRabbit

Chores
- Scoped the platform-specific test interface setting to the x86_64 variant.
- Added original-driver markers to specific test variants for clearer driver attribution.
- Introduced an expected-driver expression parameter to the VFIO variant to make driver expectations explicit; no behavioral changes to other variants or defaults.

The vfio_vfio variant uses the generic vfio-pci driver but was incorrectly expecting mlx5_vfio_pci driver. This caused false test failures where the test reported 'Get incorrect driver vfio-pci, it should be mlx5_vfio_pci' even though the actual driver was correct. Changes: - Add expr_driver = 'vfio-pci' for the vfio_vfio variant - Move test_pf parameter to x86_64 section only (not needed for aarch64) AI assisted code and commit. Human reviewed.

coderabbitai · 2025-10-17T11:34:17Z

Walkthrough

Moved test_pf into the x86_64 variant and added original_driver / expr_driver attributes to specific variants in the SR-IOV VFIO test config; the test script now reads an original_driver parameter with a default.

Changes

Cohort / File(s)	Summary
SR-IOV VFIO config `libvirt/tests/cfg/sriov/plug_unplug/sriov_attach_detach_device_vfio_variant_driver.cfg`	Moved `test_pf` from top-level into the `x86_64` variant; added `original_driver = "mlx5_core"` to `mlx5_vfio`; added `expr_driver = "vfio-pci"` and `original_driver = "igbvf"` to `vfio_vfio`.
Test parsing logic `libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device_vfio_variant_driver.py`	Added retrieval of `original_driver` from parameters in `parse_iface_dict` with default `"mlx5_core"` and used it when verifying post-detach driver state.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Runner as Test runner
    participant CFG as Variant config file
    participant Parser as parse_iface_dict()
    participant VMTest as VFIO attach/detach logic

    note right of CFG #E8F8E8: Config variants include\nx86_64 (now has `test_pf`),\nmlx5_vfio, vfio_vfio

    Runner->>CFG: load variant (e.g., x86_64 / mlx5_vfio / vfio_vfio)
    CFG-->>Runner: return variant data (includes test_pf, expr_driver, original_driver)
    Runner->>Parser: parse_iface_dict(variant data)
    Parser->>Parser: read original_driver (default "mlx5_core")
    Parser-->>Runner: iface dict with driver fields
    Runner->>VMTest: run attach/detach using iface dict
    VMTest->>VMTest: verify driver post-detach using original_driver
    VMTest-->>Runner: test result (pass/fail)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 I nibbled through configs tonight,

Moved a PF so everything's right,
Named drivers tidy, one by one,
Tests hop forward, task's nearly done,
✨ small changes, a rabbit's fun.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title "Fix wrongly set expr_driver for vfio_vfio variant" directly addresses the primary objective of this pull request. According to the PR description, the main issue is that the vfio_vfio variant was incorrectly expecting the mlx5_vfio_pci driver when it should expect vfio-pci, and the primary fix is adding expr_driver = 'vfio-pci' for the vfio_vfio variant. The title is specific, mentioning both the affected variant and the parameter being fixed, making it clear what the change addresses. While the PR includes secondary changes (moving test_pf, adding original_driver variables), these are not required to be included in the title; the title appropriately focuses on the main issue being resolved.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b698f0d and 7348419.

📒 Files selected for processing (1)

libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device_vfio_variant_driver.py (2 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

GitHub Check: Python 3.11
GitHub Check: Python 3.12
GitHub Check: Python 3.8
GitHub Check: Python 3.9

🔇 Additional comments (2)

libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device_vfio_variant_driver.py (2)

45-45: LGTM! Parameterization enables variant-specific driver configuration.

The addition of original_driver parameter with a sensible default correctly addresses the past review comment about the unused variable. This change enables different VFIO variants to specify their own original driver, fixing the false failure for the vfio_vfio variant.

103-103: Excellent fix! Resolves the hardcoded driver issue.

This change directly implements the fix requested in the past review comment. By using the original_driver parameter instead of hardcoding "mlx5_core", the code now correctly verifies driver restoration for all VFIO variants, including vfio_vfio which expects "igbvf" or other drivers.

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 and usage tips.}

The test was hardcoded to expect 'mlx5_core' as the original driver after detaching VFIO devices, but different systems use different original drivers (e.g., 'igbvf' on Intel systems). Changes: - Add original_driver parameter to config variants - Update test script to use configurable original_driver instead of hardcoded 'mlx5_core' - Set original_driver = 'igbvf' for vfio_vfio variant to match Intel systems - Set original_driver = 'mlx5_core' for mlx5_vfio variant for Mellanox systems This fixes the error: 'Get incorrect driver igbvf, it should be mlx5_core' AI assisted code and commit. Human reviewed.

coderabbitai

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c60b796 and b698f0d.

📒 Files selected for processing (2)

libvirt/tests/cfg/sriov/plug_unplug/sriov_attach_detach_device_vfio_variant_driver.cfg (1 hunks)
libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device_vfio_variant_driver.py (1 hunks)

🧰 Additional context used

🪛 Ruff (0.14.0)

libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device_vfio_variant_driver.py

45-45: Local variable original_driver is assigned to but never used

Remove assignment to unused variable original_driver

(F841)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

GitHub Check: Python 3.11
GitHub Check: Python 3.9
GitHub Check: Python 3.12
GitHub Check: Python 3.8

🔇 Additional comments (2)

libvirt/tests/cfg/sriov/plug_unplug/sriov_attach_detach_device_vfio_variant_driver.cfg (2)

12-12: LGTM - original_driver correctly set for mlx5_vfio variant.

This properly specifies the original driver that should be restored after detaching the VF device.

7-9: No issues found with aarch64 compatibility.

The original review concern is incorrect. While test_pf is now defined only for x86_64 in the config, the Python code in sriov_vfio.py (line 67) has a default value: test_pf = params.get("test_pf", "ens3f0np0")

This means aarch64 will receive the same default value ("ens3f0np0") even when test_pf is not defined in the config. Both architectures will have identical behavior, and the is_linked() function receives a valid string parameter on both platforms.

coderabbitai · 2025-10-20T06:30:56Z

libvirt/tests/cfg/sriov/plug_unplug/sriov_attach_detach_device_vfio_variant_driver.cfg

+            expr_driver = "vfio-pci"
+            original_driver = "igbvf"


⚠️ Potential issue | 🟠 Major

Good fix for vfio_vfio expected driver, but Python code needs update.

Line 15 correctly fixes the issue described in the PR objectives - the vfio_vfio variant now expects the correct "vfio-pci" driver instead of inheriting the "mlx5_vfio_pci" default from line 4.

However, line 16 sets original_driver = "igbvf" but the Python code at line 103 in sriov_attach_detach_device_vfio_variant_driver.py hardcodes exp_driver="mlx5_core" when checking the driver after detachment. This will cause the vfio_vfio variant to fail when verifying driver restoration.

🤖 Prompt for AI Agents

In libvirt/tests/cfg/sriov/plug_unplug/sriov_attach_detach_device_vfio_variant_driver.cfg around lines 15–16 and in libvirt/tests/sriov/sriov_attach_detach_device_vfio_variant_driver.py at line 103, the config sets expr_driver="vfio-pci" and original_driver="igbvf" but the Python test still hardcodes exp_driver="mlx5_core"; update the test to use the config's original_driver (or read the device's pre-detach driver) instead of the hardcoded "mlx5_core" so the post-detach verification checks exp_driver == original_driver (or the fetched original driver) and thus matches the variant's configured original_driver.

libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device_vfio_variant_driver.py

The test was using hardcoded "mlx5_core" for the post-detach driver check instead of the configurable original_driver parameter. This caused failures on systems using different original drivers (e.g., igbvf on Intel systems). Change the hardcoded "mlx5_core" to use the original_driver parameter that is configured per variant in the test configuration. AI assisted code and commit. Human reviewed.

hholoubk · 2025-10-24T11:35:19Z

replaced with #6629

coderabbitai bot reviewed Oct 20, 2025

View reviewed changes

hholoubk closed this Oct 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix wrongly set expr_driver for vfio_vfio variant #6617

Fix wrongly set expr_driver for vfio_vfio variant #6617

Uh oh!

hholoubk commented Oct 17, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Oct 17, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Oct 20, 2025

Uh oh!

Uh oh!

hholoubk commented Oct 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Fix wrongly set expr_driver for vfio_vfio variant #6617

Fix wrongly set expr_driver for vfio_vfio variant #6617

Uh oh!

Conversation

hholoubk commented Oct 17, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hholoubk commented Oct 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

hholoubk commented Oct 17, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 17, 2025 •

edited

Loading