Refactor SR-IOV VFIO test to use dynamic driver verification #6629
Conversation
WalkthroughThe SR-IOV test config was refactored: global Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Host as "Host / VF PCI"
participant Libvirt as "Libvirt / Guest"
Test->>Host: Query current driver (if not recorded)
Host-->>Test: Return driver
Note right of Test #D3E4CD: Store in original_drivers
Test->>Libvirt: Attach hostdev to guest
Libvirt-->>Test: Attachment result
alt expr_driver provided
Test->>Libvirt: Validate VF driver == expr_driver
Libvirt-->>Test: Validation result
else
Test->>Libvirt: Validate VF bound to vfio
Libvirt-->>Test: Validation result
end
Test->>Libvirt: Detach hostdev from guest
Libvirt-->>Test: Detach result
Test->>Host: Query current driver (post-detach)
Host-->>Test: Return driver
Test->>Test: Compare with original_drivers
alt match
Note right of Test #D3E4CD: Restoration confirmed
else
Note right of Test #F8D7DA: Restoration mismatch reported
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
|
(.libvirt-ci-venv-ci-runtest-SQR4mv) [root@nvidia-grace-hopper-07 libvirt-ci-latest-venv]# bin/avocado run --vt-type libvirt --vt-omit-data-loss --vt-machine-type arm64-pci --vt-connect-uri qemu:///system sriov.plug_unplug.vfio_variant_driver |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device_vfio_variant_driver.py (3)
25-31: Bug: parse_iface_dict uses free var vf_pci; ignores its parameterThis can capture the outer loop variable by accident and is fragile. Use the function parameter consistently.
Apply this diff:
-def parse_iface_dict(pf_pci): +def parse_iface_dict(pci_id): @@ - vf_pci_addr = utils_sriov.pci_to_addr(vf_pci) + vf_pci_addr = utils_sriov.pci_to_addr(pci_id)Also ensure the call site remains
parse_iface_dict(vf_pci); only the local param name changes.
35-37: Typo: "manged" → "managed"Key name is misspelled; attribute won’t apply.
- if managed: - iface_dict.update({"manged": managed}) + if managed: + iface_dict.update({"managed": managed})
90-93: Bug: ping condition invertedYou fail the test when ping succeeds. Invert the condition.
- if utils_test.ping(ping_dest, count=3, timeout=5, session=vm_session): - test.fail("Failed to ping %s." % ping_dest) + if not utils_test.ping(ping_dest, count=3, timeout=5, session=vm_session): + test.fail("Failed to ping %s." % ping_dest)
🧹 Nitpick comments (4)
libvirt/tests/cfg/sriov/plug_unplug/sriov_attach_detach_device_vfio_variant_driver.cfg (1)
8-9: Avoid duplicating test_pf; consider arch‑specific blocks onlytest_pf is set globally (Line 5) and again under x86_64. Prefer defining per-arch values only and dropping the global to avoid mismatches on aarch64. Please confirm the correct PF name for aarch64 or make it parameterizable.
libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device_vfio_variant_driver.py (3)
26-33: Replace eval with ast.literal_eval for safety and predictabilityAvoid executing arbitrary code from params. literal_eval suffices for dicts.
+import ast @@ - iface_dict = eval(params.get('iface_dict', '{}')) + iface_dict = ast.literal_eval(params.get('iface_dict', '{}')) @@ - iface_dict = eval(params.get('hostdev_dict', '{}')) + iface_dict = ast.literal_eval(params.get('hostdev_dict', '{}')) @@ - driver_dict = eval(params.get("driver_dict", "{}")) + driver_dict = ast.literal_eval(params.get("driver_dict", "{}"))
44-44: Optional expr_driver retrieval looks rightNo default assumed now; matches cfg variants. If empty string is possible from params, consider normalizing:
expr_driver = params.get("expr_driver") or None.
111-116: Rebind can be async; add short poll before asserting restorationA brief poll avoids flakes after detach.
- for idx in range(iface_number): - vf_pci = utils_sriov.get_vf_pci_id(pf_pci, idx) - original_driver = original_drivers[vf_pci] - libvirt_vfio.check_vfio_pci(vf_pci, exp_driver=original_driver) - test.log.debug(f"Verify: Original driver {original_driver} restored for {vf_pci} - PASS") + for idx in range(iface_number): + vf_pci = utils_sriov.get_vf_pci_id(pf_pci, idx) + expected = original_drivers[vf_pci] + for _ in range(10): # wait up to ~10s + if libvirt_vfio.get_pci_driver(vf_pci) == expected: + break + time.sleep(1) + else: + test.fail(f"Original driver {expected} not restored for {vf_pci}") + test.log.debug(f"Verify: Original driver {expected} restored for {vf_pci} - PASS")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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(5 hunks)
🔇 Additional comments (3)
libvirt/tests/cfg/sriov/plug_unplug/sriov_attach_detach_device_vfio_variant_driver.cfg (1)
12-12: Good move: scope expr_driver to mlx5_vfio variantScoping expr_driver to the mlx5_vfio variant matches the new conditional logic in the test. Please confirm that vfio_vfio intentionally omits expr_driver and relies on the generic VFIO check.
libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device_vfio_variant_driver.py (2)
84-87: Conditional VFIO check: confirm default pathWhen expr_driver is absent, you call check_vfio_pci(vf_pci) without exp_driver. Please confirm the helper’s default expectation (e.g., vfio_pci) aligns with the intended variant behavior.
54-54: Verify handling ofNonereturn fromget_pci_driverThe code at lines 70–74 stores whatever
libvirt_vfio.get_pci_driver(vf_pci)returns without checking forNone. If the function can returnNonewhen a VF has no bound driver, this value is then passed tocheck_vfio_pci(..., exp_driver=original_driver)at line 115.Action required: Consult the
virttest.utils_libvirt.libvirt_vfiomodule documentation or source to confirm:
- Does
get_pci_driver()returnNonefor unbound drivers?- Does
check_vfio_pci()safely handleNoneas theexp_driverparameter?If both answers are "yes," add defensive
Nonechecks when storing/retrieving fromoriginal_drivers. Otherwise, document the assumption that drivers are always bound during test execution.
- Remove hardcoded driver values and improve config organization - Refactor SRIOV VFIO test to use dynamic driver verification - Simplify SR-IOV VFIO driver check logic by removing unnecessary conditionals - The check_vfio_pci function can handle None values for exp_driver parameter - Reduces code complexity and improves maintainability Signed-off-by: hholoubk <[email protected]> AI assisted code and commit. Human reviewed.
hholoubk
force-pushed
the
XXX-538-sriov-vfio-get-driver
branch
from
b0e7864 to
3624234
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device_vfio_variant_driver.py (2)
19-41: Fix wrong variable usage and 'managed' typo in parse_iface_dict.The function ignores its parameter and reads the free var vf_pci; also sets key "manged". These are brittle and user-visible bugs.
Apply:
-def parse_iface_dict(pf_pci): +def parse_iface_dict(vf_pci_param): @@ - vf_pci_addr = utils_sriov.pci_to_addr(vf_pci) + vf_pci_addr = utils_sriov.pci_to_addr(vf_pci_param) @@ - if managed: - iface_dict.update({"manged": managed}) + if managed: + iface_dict.update({"managed": managed})
86-90: Fix condition to check status code from utils_test.ping return tuple.The code treats the return tuple as a boolean; in Python, non-empty tuples are always truthy. Extract and check the status code instead:
status, output = utils_test.ping(ping_dest, count=3, timeout=5, session=vm_session) if status: test.fail("Failed to ping %s." % ping_dest)
🧹 Nitpick comments (4)
libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device_vfio_variant_driver.py (4)
44-45: Gate VFIO model check when expr_driver is not provided.With top-level expr_driver removed, None may reach check_vfio_pci. Guard to avoid unintended assertions.
- libvirt_vfio.check_vfio_pci(vf_pci, exp_driver=expr_driver) + if expr_driver: + libvirt_vfio.check_vfio_pci(vf_pci, exp_driver=expr_driver) + else: + test.log.debug("expr_driver not set; skipping VFIO model check")Please confirm libvirt_vfio.check_vfio_pci explicitly supports exp_driver=None; if it does, we can keep the single call.
Also applies to: 84-85
108-113: Make restore check resilient to async rebind.Driver rebind can lag detach. Add a short retry/poll to reduce flakes.
- for idx in range(iface_number): - vf_pci = utils_sriov.get_vf_pci_id(pf_pci, idx) - original_driver = original_drivers[vf_pci] - libvirt_vfio.check_vfio_pci(vf_pci, exp_driver=original_driver) + for idx in range(iface_number): + vf_pci = utils_sriov.get_vf_pci_id(pf_pci, idx) + original_driver = original_drivers[vf_pci] + for _ in range(30): + try: + libvirt_vfio.check_vfio_pci(vf_pci, exp_driver=original_driver) + break + except Exception: + time.sleep(1) + else: + test.fail(f"Driver {original_driver} not restored for {vf_pci} within timeout")
27-33: Safer parsing for driver_dict; keep eval where templates require variables.iface_dict/hostdev_dict templates reference variables (e.g., vf_pci_addr, mac_addr) and need eval. driver_dict is a pure literal—use literal_eval.
+import ast @@ - driver_dict = eval(params.get("driver_dict", "{}")) + driver_dict = ast.literal_eval(params.get("driver_dict", "{}"))
82-83: Harden nested dict access to avoid TypeError when driver is missing.Avoid None["driver_attr"] crash.
- if act_hostdev_dict.get("driver")["driver_attr"].get("name") != "vfio": + drv_name = (((act_hostdev_dict.get("driver") or {}).get("driver_attr") or {}).get("name")) + if drv_name != "vfio": test.fail("The hostdev driver is not set to VFIO.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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(4 hunks)
🔇 Additional comments (5)
libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device_vfio_variant_driver.py (2)
54-55: LGTM: per-VF original driver map.Local dict initialization is clear and scoped correctly.
70-74: Good capture of original driver; add guard for empty/unknown.If get_pci_driver returns empty/unknown, log and still proceed to avoid KeyError later.
Optionally:
- if vf_pci not in original_drivers: - original_driver = libvirt_vfio.get_pci_driver(vf_pci) + if vf_pci not in original_drivers: + original_driver = libvirt_vfio.get_pci_driver(vf_pci) or "unknown" original_drivers[vf_pci] = original_driverlibvirt/tests/cfg/sriov/plug_unplug/sriov_attach_detach_device_vfio_variant_driver.cfg (3)
8-9: Platform override present only for x86_64; confirm PF name on aarch64.aarch64 often uses different PF names. Ensure default test_pf is valid there, or add an aarch64: block.
12-13: Variant-scoped expr_driver is consistent; ensure model matches.expr_driver="mlx5_vfio_pci" aligns with driver_dict.model="mlx5_vfio_pci". Looks good; just keep them in sync.
16-17: Default variant with empty driver_dict is fine; coordinate with code gating.This relies on expr_driver being optional in code. Please land the gating change (or confirm check_vfio_pci handles None).
hholoubk
changed the title
1 participant
Depends on
avocado-framework/avocado-vt#4255
Summary by CodeRabbit