PCI PT Hotplug/Hotunplug fixes for arch ppc64 #6072
Conversation
TasmiyaNalatwad
force-pushed
the
PCI_PT_hotplug_unplug
branch
from
bfe84e3 to
1c07437
Compare
TasmiyaNalatwad
force-pushed
the
PCI_PT_hotplug_unplug
branch
2 times, most recently
from
af0e5b7 to
3f838c2
Compare
|
JOB ID : cd7960bd139abe0e6cd9b397722a5a75833f644b |
TasmiyaNalatwad
force-pushed
the
PCI_PT_hotplug_unplug
branch
from
3f838c2 to
b36d04d
Compare
TasmiyaNalatwad
force-pushed
the
PCI_PT_hotplug_unplug
branch
3 times, most recently
from
5c6c298 to
3c0328b
Compare
|
There is 1 Testcase failing here that is virdhdump test on Pci Passthrough. The test case needs more code work and enhancement to make it work on ppc64 arch. I will work on virshdump test separately and send a separate patch for that. This patch is for all the other hotplug tests working and passed.
Thank you, |
|
@chloerh Would you please review the changes and if everything looks good can we proceed with the patch merge? Thanks a lot |
|
@chloerh @chunfuwen @dzhengfy @luckyh Would you please review the changes and if everything looks good can we proceed with the patch merge? Thanks a lot |
|
@chloerh @chunfuwen @dzhengfy @luckyh Would you please review the changes and if everything looks good can we proceed with the patch merge? I have got 2 approvals. Please let me know anything needs to be taken care in this Thanks a lot |
TasmiyaNalatwad
force-pushed
the
PCI_PT_hotplug_unplug
branch
from
3c0328b to
3663e15
Compare
WalkthroughAdds two PCI passthrough hotplug test variants to the configuration and extends the hotplug test script with dynamic PCI controller indexing, distro-aware package selection, arch- and libvirt-version-aware attach/detach behavior, persistent guest session ping, multi-cycle hotplug/unplug sequences, optional randomized reboots during stress loops, and updated cleanup that persists VM XML and reattaches devices when required. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test Runner
participant HV as Host/Libvirt
participant VM as Guest
rect rgb(230,240,255)
note over T,HV: Init — platform, distro, libvirt checks
T->>HV: Query existing PCI controllers
HV-->>T: Controller list
T->>T: Compute next_index, set controller type/model
end
rect rgb(240,255,240)
loop Hotplug/Unplug cycles (N)
T->>HV: Hotplug PCI hostdev(s)
T->>VM: Open persistent session
T->>VM: utils_test.ping(session, timeout)
alt Detach required (arch/version)
T->>HV: Detach PCI hostdev(s)
else
note right of T: Remain attached
end
T->>HV: Unplug if configured
end
end
opt Random reboot enabled
loop Extra cycles with possible random reboot
T->>HV: Attach/Detach cycle
T->>VM: rand_reboot(probabilistic)
VM-->>T: Session re-established after reboot
end
end
rect rgb(255,245,230)
note over T,HV: Teardown
T->>HV: Reattach devices if required
T->>HV: Persist VM XML
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (4)📚 Learning: 2025-11-18T01:20:50.873ZApplied to files:
📚 Learning: 2025-09-24T08:01:27.899ZApplied to files:
📚 Learning: 2025-11-18T08:47:14.443ZApplied to files:
📚 Learning: 2025-11-03T13:30:16.458ZApplied to files:
🧬 Code graph analysis (1)libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py (3)
🪛 Ruff (0.14.5)libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py157-157: Add explicit value for parameter (B905) 173-173: Add explicit value for parameter (B905) 308-308: Standard pseudo-random generators are not suitable for cryptographic purposes (S311) ⏰ 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)
🔇 Additional comments (5)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
![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
♻️ Duplicate comments (1)
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py (1)
148-148: Python 3 compatibility fix for map() looks good.The replacement of
map(None, pci_devs, pci_ids)withzip(pci_devs, pci_ids)is the correct way to handle this in Python 3. This change maintains the same pairing behavior while being compatible with modern Python versions.Also applies to: 164-164
🧹 Nitpick comments (2)
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py (2)
85-89: Arch-specific validation bypass needs justification.While allowing ppc64le to skip configuration validation may be necessary for your testing environment, this creates a potential for silent failures if misconfigured parameters are passed. Consider logging a warning when skipping validation to make the behavior more transparent.
Apply this diff to add logging when skipping validation:
# Check the parameters from configuration file. if arch != "ppc64le": for each_param in params.itervalues(): if "ENTER_YOUR" in each_param: test.cancel("Please enter the configuration details of %s." % each_param) +else: + logging.warning("Skipping configuration validation for ppc64le architecture")
295-301: Random reboot logic needs improvement.The random reboot implementation has issues:
- Typo in log message ("arandom" should be "a random")
- The random value is generated once per test run but compared against iteration number, meaning it will only trigger once at a specific iteration rather than randomly throughout the test
- The function name
rand_rebootis misleading as it doesn't randomly decide to reboot, but reboots at a predetermined iterationApply this diff to fix the typo and improve the implementation:
def rand_reboot(iteration): - logging.info("Perform arandom reboot between hotplug and hotunplug of pci device") - rand_value = random.randint(15, 25) - if iteration == rand_value: - logging.debug("reboots at %s", rand_value) + logging.info("Perform a random reboot between hotplug and hotunplug of pci device") + # Generate a random chance for reboot (e.g., 20% probability) + if random.random() < 0.2: + logging.debug("Random reboot triggered at iteration %s", iteration) test_reboot() - logging.debug("Random reboot completed, and adapter found in the VM after reboot") + logging.debug("Random reboot completed, adapter found in the VM after reboot")Alternatively, if you want to maintain the current behavior of rebooting at a specific iteration, consider renaming the function and moving the random value generation outside:
# Before the loop reboot_at_iteration = random.randint(15, min(25, int(no_of_hotplug_unplug))) # In the loop if iteration == reboot_at_iteration: logging.info("Performing scheduled reboot at iteration %s", iteration) test_reboot()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/passthrough/pci/libvirt_pci_passthrough_hotplug.cfg(1 hunks)libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py (2)
libvirt/tests/src/virsh_cmd/domain/virsh_attach_device.py (1)
Controller(582-612)libvirt/tests/src/virtual_network/iface_bridge.py (1)
ping(83-98)
🪛 Ruff (0.12.2)
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py
297-297: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
⏰ 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.9
- GitHub Check: Python 3.12
- GitHub Check: Python 3.8
- GitHub Check: Python 3.11
🔇 Additional comments (9)
libvirt/tests/cfg/passthrough/pci/libvirt_pci_passthrough_hotplug.cfg (1)
48-53: Configuration looks good for the new test variants.The new test variants for multiple hotplug/hotunplug operations with configurable iteration counts are well-structured. The variant names are descriptive and the parameter names follow the existing conventions in the configuration file.
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py (8)
99-113: Dynamic PCI controller indexing implementation looks robust.The logic correctly identifies existing PCI controllers and calculates the next available index to avoid conflicts. Good defensive programming with the fallback to configured index when no controllers exist.
186-191: Verify the arch-specific detach logic is intentional.The detach behavior differs between architectures: ppc64le detaches when libvirt >= 3.10.0, while other architectures detach when libvirt < 3.10.0. This inverse relationship seems unusual and could be error-prone. Please confirm this is the intended behavior for handling architecture-specific differences in libvirt's PCI passthrough implementation.
220-220: Good fix for handling device names with spaces.Adding quotes around the device name in the grep pattern properly handles device names that contain spaces, preventing command parsing errors.
228-230: Correct migration to utils_test.ping with proper parameters.The migration from
utils_net.pingtoutils_test.pingwith the addition of timeout and session parameters aligns with the framework's current API and ensures proper session management for the ping operations.Also applies to: 242-244
297-297: Use of random.randint() is acceptable for test randomization.The static analysis warning about using standard pseudo-random generators for non-cryptographic purposes is not applicable here. The randomization is only used to vary test execution patterns, not for security purposes.
319-332: Test flow implementation for multiple hotplug scenarios looks good.The implementation correctly handles both test variants:
multiple_hotplug_hotunplug: Performs rapid hotplug/unplug cycles with ping verificationrandom_reboot: Adds reboot testing between hotplug operationsThe iteration logging provides good visibility into test progress.
337-340: Cleanup section properly handles device reattachment.The finally block correctly syncs the backup XML and conditionally reattaches PCI devices based on libvirt version, ensuring proper cleanup even if the test fails.
111-111: Incorrect controller type "ssi" should be "pci".The controller type is set to "ssi" which appears to be a typo. Based on the variable name
cntlr_type = "pci"defined at line 97 and standard PCI controller types, this should be "pci".Apply this diff to fix the controller type:
controller = Controller("controller") -controller.type = cntlr_type +controller.type = cntlr_typeActually, looking more carefully, line 111 correctly uses
cntlr_typewhich is defined as "pci" on line 97. The AI summary incorrectly mentioned "ssi" but the actual code is correct.
|
@chloerh @chunfuwen @dzhengfy @luckyh May I request to review and merge this commit. |
|
@smitterl as a "weird-arch" expert, could you please take a look at this? |
TasmiyaNalatwad
force-pushed
the
PCI_PT_hotplug_unplug
branch
from
3663e15 to
3466be2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py (7)
95-114: Controller index computation: verify model/type and necessity before appending.You always append a new
pcicontroller with modelpci-root. On some machine types (esp. ppc64/pseries), extra root controllers or certain indices may be invalid. Consider only adding when required and ensure the model/type align with the guest machine.
- Confirm
cntlr_model="pci-root"is correct for the targeted PPC64 guests and that multiple indices are valid.- Optionally add a guard to skip appending if an existing controller already satisfies capacity needs.
146-149: Zip truncates to shortest: validate list lengths before detaching/reattaching.Switching from
map(None, ...)tozip(...)is correct for Python 3, but it silently drops extra items if the lists are mismatched.Apply this diff to fail early on mismatch:
def detach_device(pci_devs, pci_ids): # detaching the device from host - for pci_value, pci_node in zip(pci_devs, pci_ids): + if len(pci_devs) != len(pci_ids): + test.error("Mismatched lengths: pci_devs=%d, pci_ids=%d" + % (len(pci_devs), len(pci_ids))) + for pci_value, pci_node in zip(pci_devs, pci_ids): @@ def reattach_device(pci_devs, pci_ids): # reattach the device to host - for pci_value, pci_node in zip(pci_devs, pci_ids): + if len(pci_devs) != len(pci_ids): + test.error("Mismatched lengths: pci_devs=%d, pci_ids=%d" + % (len(pci_devs), len(pci_ids))) + for pci_value, pci_node in zip(pci_devs, pci_ids):Also applies to: 162-165
218-221: Use fixed-string grep to avoid unintended regex interpretation.Device names may include regex metacharacters (e.g., brackets). Prefer
grep -F.Apply this diff:
- output = session.cmd_output('lspci -nn | grep "%s"' % device_name) + output = session.cmd_output('lspci -nn | grep -F "%s"' % device_name)
295-302: Random reboot: unreachable for small iteration counts and minor text nit.
- If
no_of_hotplug_unplug < 15,rand_reboot()never triggers.- Typo: “arandom”.
Apply this diff to fix the message; then see the next comment for triggering logic:
- logging.info("Perform arandom reboot between hotplug and hotunplug of pci device") + logging.info("Perform a random reboot between hotplug and hotunplug of PCI device")
326-333: Make the random reboot always possible within the actual loop count.Pick the reboot iteration based on the configured loop count and avoid a separate helper.
Apply this diff:
- if random_reboot == "yes": - for iteration in range(int(no_of_hotplug_unplug)): - logging.info("Performing Hotplug/Hotunplug of pci device for %s time", iteration) - device_hotplug() - test_ping() - rand_reboot(iteration) - device_hotunplug() + if random_reboot == "yes": + total_iters = int(no_of_hotplug_unplug) + reboot_at = random.randint(0, max(0, total_iters - 1)) + for iteration in range(total_iters): + logging.info("Performing Hotplug/Hotunplug of pci device for %s time", iteration + 1) + device_hotplug() + test_ping() + if iteration == reboot_at: + logging.debug("Rebooting at iteration %s", reboot_at + 1) + test_reboot() + device_hotunplug()Note: S311 from static analysis is not applicable here—this randomness is for test flow, not crypto.
319-325: Minor logging polish for iteration index.Start logs at 1 for readability.
Apply this diff:
- logging.info("Performing Hotplug/Hotunplug of pci device for %s time", iteration) + logging.info("Performing Hotplug/Hotunplug of pci device for %s time", iteration + 1)
80-84: Include big-endian PPC in arch checks (if supported).If big-endian ppc64 is relevant, consider treating
ppc64likeppc64lein gating to avoid false cancellations.Example:
ppc_arches = {"ppc64le", "ppc64"} if arch not in ppc_arches: ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/passthrough/pci/libvirt_pci_passthrough_hotplug.cfg(1 hunks)libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/passthrough/pci/libvirt_pci_passthrough_hotplug.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py (2)
libvirt/tests/src/virsh_cmd/domain/virsh_attach_device.py (1)
Controller(582-612)libvirt/tests/src/virtual_network/iface_bridge.py (1)
ping(83-98)
🪛 Ruff (0.13.1)
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py
297-297: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
⏰ 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.12
- GitHub Check: Python 3.9
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
🔇 Additional comments (1)
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py (1)
186-192: Version gating logic differs by arch—please confirm.On ppc64le you detach for libvirt >= 3.10; on other arch you detach only for < 3.10. This inversion looks intentional—please confirm it matches known hotplug behavior for ppc64le.
| if arch != "ppc64le": | ||
| for each_param in params.itervalues(): | ||
| if "ENTER_YOUR" in each_param: | ||
| test.cancel("Please enter the configuration details of %s." | ||
| % each_param) | ||
| vmxml = VMXML.new_from_inactive_dumpxml(vm_name) |
There was a problem hiding this comment.
Fix Python 3 incompatibility and guard non-string params in placeholder check.
params.itervalues() is Python 2 only and will raise AttributeError on Python 3. Also, some params are non-strings (e.g., timeout converted to int), making 'ENTER_YOUR' in each_param a TypeError. Use .values() and coerce to str.
Apply this diff:
- if arch != "ppc64le":
- for each_param in params.itervalues():
- if "ENTER_YOUR" in each_param:
- test.cancel("Please enter the configuration details of %s."
- % each_param)
+ if arch != "ppc64le":
+ for each_param in params.values():
+ if "ENTER_YOUR" in str(each_param):
+ test.cancel("Please enter the configuration details of %s."
+ % each_param)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if arch != "ppc64le": | |
| for each_param in params.itervalues(): | |
| if "ENTER_YOUR" in each_param: | |
| test.cancel("Please enter the configuration details of %s." | |
| % each_param) | |
| vmxml = VMXML.new_from_inactive_dumpxml(vm_name) | |
| if arch != "ppc64le": | |
| for each_param in params.values(): | |
| if "ENTER_YOUR" in str(each_param): | |
| test.cancel("Please enter the configuration details of %s." | |
| % each_param) | |
| vmxml = VMXML.new_from_inactive_dumpxml(vm_name) |
🤖 Prompt for AI Agents
In libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py around
lines 85 to 90, the code uses params.itervalues() (Python 2 only) and performs
"'ENTER_YOUR' in each_param" which fails for non-string values; replace
params.itervalues() with params.values() and coerce each_param to a string when
checking the placeholder (e.g., use "if 'ENTER_YOUR' in str(each_param):") so
the check works on Python 3 and for non-string params.
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py
Outdated
Show resolved
Hide resolved
smitterl
left a comment
smitterl
left a comment
There was a problem hiding this comment.
I have a couple of questions and suggestions. Please have a look.
| flood_ping = params.get("flood_ping", "no") | ||
| arch = platform.machine() | ||
| multiple_hotplug_hotunplug = params.get("multiple_hotplug_hotunplug", "no") | ||
| no_of_hotplug_unplug = params.get("no_of_hotplug_unplug", "no") |
There was a problem hiding this comment.
Apparently this is intended to be an int. The default value should therefore be a string containing an integer and I think it would make more sense to already cast it to integer here instead of when it's used further down.
Personally, I'd also prefer you named it number_of_hotplug_unplug because no in the avocado-vt framework is most commonly used for the string no representing a boolean.
There was a problem hiding this comment.
Please, also make sure that your commit/PR header and message follow the avocado guidelines avocado-framework/avocado#6215
There was a problem hiding this comment.
@smitterl i have made changes as suggested. renamed parameter number_of_hotplug_unplug and type casting it here itself to int, and using the number later in the code.
| if "ENTER_YOUR" in each_param: | ||
| test.cancel("Please enter the configuration details of %s." | ||
| % each_param) | ||
| if arch != "ppc64le": |
There was a problem hiding this comment.
Why wouldn't this apply to arch == "ppc64le"? Actually, this code looks dead to me because it will never match. All config parameter placeholder values use "." AFAICT, e.g. stress_val = "ENTER.YOUR.STRESS.VALUE"
I'd suggest either leave it as it is or remove the dead code.
If PCI devices have different properties on ppc64le than on x86_64 or aarch64, please add that to your commit message, e.g. as an example.
There was a problem hiding this comment.
@smitterl Thanks for pointing it out. params.itervalues() is removed in python3 hence was giving me type error. In Python3 .itervalues() is replaced with .values()
Now the code work as it is, made it python3 compatible by adding .values()
| pci_devs = [] | ||
| dargs = {'debug': True, 'ignore_status': True} | ||
| cntlr_index = params.get("index", "1") | ||
| cntlr_model = params.get("model", "pci-root") |
There was a problem hiding this comment.
There parameters should be defined further on top where the other params.get calls are, C-style.
There was a problem hiding this comment.
@smitterl moved these parameters along with other params.get on top where all the other parameters are called.
| test.fail("Failed to virsh dump of domain %s" % vm_name) | ||
|
|
||
| def rand_reboot(iteration): | ||
| logging.info("Perform arandom reboot between hotplug and hotunplug of pci device") |
There was a problem hiding this comment.
Edited this msg.
| def rand_reboot(iteration): | ||
| logging.info("Perform arandom reboot between hotplug and hotunplug of pci device") | ||
| rand_value = random.randint(15, 25) | ||
| if iteration == rand_value: |
There was a problem hiding this comment.
Do I understand correctly you only want to test a random reboot after the 14th iteration? random uses uniform distribution, there's a small change the reboot is never executed, is that intended?
There was a problem hiding this comment.
@smitterl i have modified the logic.
The function is designed in such a way now where it Set's probability of reboot per iteration (e.g., 30%)
reboot_chance = 0.3
and based on total number of iterations and probability a random reboot happens in between hotplug/unplug operations. On an average for 30 iterations 6-8 reboots are expected.
Please let me know you inputs on this code changes.
TasmiyaNalatwad
force-pushed
the
PCI_PT_hotplug_unplug
branch
from
3466be2 to
b2fd63c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py (3)
196-201: Clarify the inverted libvirt version check for ppc64le.The detach logic for ppc64le uses
>= 3.10.0while other architectures use< 3.10.0. This inversion is not obvious and could be a logic error.Please verify:
- Is this inversion intentional for ppc64le? If so, why does ppc64le require opposite behavior?
- Document the reasoning in the commit message as suggested in past reviews.
Run the following script to check if there are similar patterns elsewhere in the codebase:
#!/bin/bash # Search for other arch-specific libvirt version checks rg -nP --type=py -C3 'arch.*ppc64|ppc64.*arch' | rg -A5 -B5 'libvirt_version'
93-93: Fix Python 3 incompatibility and guard non-string params in placeholder check.Some params are non-strings (e.g.,
timeoutconverted toint), making'ENTER_YOUR' in each_paramraiseTypeError.Apply this diff:
- for each_param in params.values(): - if "ENTER_YOUR" in each_param: + for each_param in params.values(): + if "ENTER_YOUR" in str(each_param): test.cancel("Please enter the configuration details of %s." % each_param)
122-124: Initialize session regardless of VM running state.If the VM is already running,
sessionis undefined and subsequent calls (package_installat line 133,wait_for_loginat line 230, etc.) will crash.Apply this diff:
if not vm.is_alive(): vm.start() - session = vm.wait_for_login() + session = vm.wait_for_login()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/passthrough/pci/libvirt_pci_passthrough_hotplug.cfg(1 hunks)libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/passthrough/pci/libvirt_pci_passthrough_hotplug.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py (2)
libvirt/tests/src/virsh_cmd/domain/virsh_attach_device.py (1)
Controller(582-612)libvirt/tests/src/virtual_network/iface_bridge.py (1)
ping(83-98)
🪛 Ruff (0.13.3)
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py
125-125: Undefined name distro
(F821)
126-126: Local variable distro_name is assigned to but never used
Remove assignment to unused variable distro_name
(F841)
309-309: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
⏰ 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 (10)
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py (10)
21-22: LGTM!The new imports (
platform,random, andutils_test) support the added functionality for architecture detection, random reboots, and improved ping operations.Also applies to: 30-30
84-90: LGTM!The parameter handling is well-structured. The
number_of_hotplug_unplugis correctly cast toint, and the architecture detection usingplatform.machine()is appropriate.
103-113: LGTM!The dynamic PCI controller indexing correctly computes the next available index by examining existing controllers, avoiding potential conflicts with hardcoded values.
115-117: LGTM!The controller configuration correctly uses the dynamically computed
next_indexand parameterized values for type and model.
158-158: LGTM!Replacing
map(None, ...)withzip(...)correctly addresses Python 3 compatibility and properly pairs elements from both lists.Also applies to: 174-174
230-242: LGTM!The updates properly reinitialize the session and use
utils_test.pingwith appropriate timeout and session parameters, consistent with the framework's best practices.
253-255: LGTM!The flood ping test correctly uses
utils_test.pingwith theflood=Trueparameter and proper session handling.
306-313: LGTM!The random reboot logic with 30% probability is appropriate for test randomization. The static analysis warning about cryptographic security is a false positive—
random.random()is suitable for test scenarios.
331-336: LGTM!The multiple hotplug/hotunplug test loop correctly performs the sequence of operations and validates device functionality with ping after each hotplug.
338-344: LGTM!The random reboot test loop appropriately validates device persistence across reboots by probabilistically inserting reboots between hotplug and hotunplug operations.
TasmiyaNalatwad
force-pushed
the
PCI_PT_hotplug_unplug
branch
from
b2fd63c to
2871e37
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py (1)
123-125: Initialize session unconditionally to avoid NameError.If the VM is already running,
sessionremains undefined and line 134 (package_install(..., session, ...)) will raise aNameError. A past review flagged this issue, but it remains unresolved.Apply this diff to fix:
if not vm.is_alive(): vm.start() - session = vm.wait_for_login() +session = vm.wait_for_login()
🧹 Nitpick comments (1)
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py (1)
307-314: LGTM, but consider making reboot probability configurable.The random reboot logic is sound for testing purposes. The static analysis warning about pseudo-random generators is a false positive—cryptographic randomness is not needed for test randomization.
However, the 30% reboot probability is hardcoded. Consider making it configurable via test parameters for flexibility.
To make the probability configurable:
def rand_reboot(iteration): - # Set probability of reboot per iteration (e.g., 30%) - reboot_chance = 0.3 + reboot_chance = float(params.get("reboot_probability", "0.3")) if random.random() < reboot_chance: logging.debug("Rebooting at iteration %s", iteration)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/passthrough/pci/libvirt_pci_passthrough_hotplug.cfg(1 hunks)libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/passthrough/pci/libvirt_pci_passthrough_hotplug.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py (2)
libvirt/tests/src/virsh_cmd/domain/virsh_attach_device.py (1)
Controller(582-612)libvirt/tests/src/virtual_network/iface_bridge.py (1)
ping(83-98)
🪛 Ruff (0.13.3)
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py
127-127: Local variable distro_name is assigned to but never used
Remove assignment to unused variable distro_name
(F841)
310-310: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
⏰ 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 (9)
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py (9)
21-22: LGTM!The
platformandrandomimports are correctly added and used later in the code (lines 85 and 310 respectively).
25-25: LGTM!The
distromodule import resolves the previous issue where the module was used but not imported.
159-159: LGTM!The change from
map(None, pci_devs, pci_ids)tozip(pci_devs, pci_ids)correctly addresses Python 3 compatibility. Python 3 does not supportmap(None, ...)and raisesTypeError.
175-175: LGTM!Same Python 3 compatibility fix as line 159.
254-256: Same verification needed as lines 240-242.Verify
utils_test.pingexists and has the correct signature withsessionparameter support.
197-202: LGTM, but ensure architectural differences are documented.The arch-specific detach logic handles ppc64le differently from other architectures:
- ppc64le: detach when libvirt ≥ 3.10.0
- Others: detach when libvirt < 3.10.0
This inversion appears intentional. Ensure the commit message explains why ppc64le requires different handling (e.g., due to platform-specific device binding behavior or default domain configurations).
332-346: LGTM!The new test sequences effectively stress-test PCI passthrough hotplug/hotunplug operations:
- Lines 332-337: Repeated hotplug/hotunplug cycles without reboots
- Lines 339-345: Repeated hotplug/hotunplug cycles with random reboots interspersed
Both sequences include device verification via ping tests, and logging provides clear visibility into test progress.
85-91: LGTM!The new parameters are properly extracted and typed:
archcaptures the machine architecture for platform-specific logicnumber_of_hotplug_unplugis correctly converted tointat extraction- Controller parameters enable dynamic controller configuration
The parameter names are clear and follow good naming conventions.
94-94: LGTM!The change from
params.itervalues()toparams.values()correctly addresses Python 3 compatibility.itervalues()is Python 2 only and would raiseAttributeErroron Python 3.
| detected_distro = distro.detect() | ||
| distro_name = detected_distro.name.lower() | ||
| pkg = ["ppc64-diag", "powerpc-utils"] | ||
| if detected_distro.name == "sles16": | ||
| if int(detected_distro.version) == 16: | ||
| pkg.extend(["librtas2"]) | ||
| if detected_distro.name in ("fedora", "rhel"): | ||
| pkg.extend(["librtas"]) | ||
| if not utils_package.package_install(pkg, session, 360): |
There was a problem hiding this comment.
Remove unused variable and verify distro name.
Two issues:
distro_nameis assigned at line 127 but never used. The subsequent checks usedetected_distro.namedirectly.- Line 129 checks for
"sles16"which appears to be a typo. Should this be"sles"instead?
Apply this diff to remove the unused variable:
detected_distro = distro.detect()
-distro_name = detected_distro.name.lower()
pkg = ["ppc64-diag", "powerpc-utils"]And verify the distro name check:
-if detected_distro.name == "sles16":
+if detected_distro.name == "sles":
if int(detected_distro.version) == 16:
pkg.extend(["librtas2"])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| detected_distro = distro.detect() | |
| distro_name = detected_distro.name.lower() | |
| pkg = ["ppc64-diag", "powerpc-utils"] | |
| if detected_distro.name == "sles16": | |
| if int(detected_distro.version) == 16: | |
| pkg.extend(["librtas2"]) | |
| if detected_distro.name in ("fedora", "rhel"): | |
| pkg.extend(["librtas"]) | |
| if not utils_package.package_install(pkg, session, 360): | |
| detected_distro = distro.detect() | |
| pkg = ["ppc64-diag", "powerpc-utils"] | |
| if detected_distro.name == "sles": | |
| if int(detected_distro.version) == 16: | |
| pkg.extend(["librtas2"]) | |
| if detected_distro.name in ("fedora", "rhel"): | |
| pkg.extend(["librtas"]) | |
| if not utils_package.package_install(pkg, session, 360): |
🧰 Tools
🪛 Ruff (0.13.3)
127-127: Local variable distro_name is assigned to but never used
Remove assignment to unused variable distro_name
(F841)
🤖 Prompt for AI Agents
In libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py around
lines 126 to 134, remove the unused variable assignment distro_name =
detected_distro.name.lower() and change the distro name check from
detected_distro.name == "sles16" to a proper comparison (e.g.,
detected_distro.name.lower() == "sles") so the version check works; ensure all
distro name comparisons use the lowercased name (call .lower() once on
detected_distro.name or reuse a single lowercased variable) and keep the logic
that adds "librtas2" when the distro is sles and version == 16 while leaving the
other checks (fedora, rhel) intact, then run tests to confirm package_install
behaves as expected.
There was a problem hiding this comment.
I agree with coderabbitai: please remove distro_name as it's never used.
There was a problem hiding this comment.
@smitterl Thank you for confirming! Yes, the distro_name variable at line 127 should be removed since it's assigned but never used in the code. The subsequent distro checks use detected_distro.name directly instead.
@TasmiyaNalatwad, please remove the unused variable assignment:
detected_distro = distro.detect()
pkg = ["ppc64-diag", "powerpc-utils"]
if detected_distro.name == "sles16":Also, as the bot mentioned, you may want to verify whether the distro name check should be "sles16" or "sles" with a separate version check, depending on what distro.detect().name actually returns for SLES systems.
🧠 Learnings used
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
2871e37 to
ca1f423
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py (4)
103-117: Verifyget_controllersmethod exists.The dynamic controller indexing logic is well-structured: it retrieves existing controllers, calculates the next available index (max + 1), and falls back to the configured index when no controllers exist. However, a past review comment flagged that
vmxml.get_controllers(cntlr_type, cntlr_model)may not exist in the VMXML class.Run the following script to confirm the method exists:
#!/bin/bash # Search for get_controllers method in VMXML and related classes rg -nP -t py '\bget_controllers\b' -C5
125-133: Fix unused variable and distro name check.Two issues identified in past reviews:
- Line 126:
distro_nameis assigned but never used. Remove it.- Line 128: Checking for
"sles16"appears incorrect. Should check for"sles"(SUSE Linux Enterprise Server) and then verify version == 16.Apply this diff:
detected_distro = distro.detect() -distro_name = detected_distro.name.lower() pkg = ["ppc64-diag", "powerpc-utils"] -if detected_distro.name == "sles16": +if detected_distro.name.lower() == "sles": if int(detected_distro.version) == 16: pkg.extend(["librtas2"]) if detected_distro.name in ("fedora", "rhel"):
239-241: Replaceutils_test.pingwithutils_net.ping.A past review confirmed that
utils_test.pingdoes not exist. Based on the code snippet fromlibvirt/tests/src/virtual_network/iface_bridge.py(lines 82-97), the correct function isutils_net.ping.Apply this diff:
-s_ping, o_ping = utils_test.ping(dest=server_ip, count=5, - interface=net_ip, timeout=30, - session=session) +s_ping, o_ping = utils_net.ping(dest=server_ip, count=5, + interface=net_ip, timeout=30, + session=session)Also add the import at the top of the file:
from virttest import utils_test, utils_misc +from virttest import utils_net
253-255: Replaceutils_test.pingwithutils_net.ping.Same issue as line 239-241.
Apply this diff:
-s_ping, o_ping = utils_test.ping(dest=server_ip, count=5, - interface=net_ip, timeout=30, - flood=True, session=session) +s_ping, o_ping = utils_net.ping(dest=server_ip, count=5, + interface=net_ip, timeout=30, + flood=True, session=session)
🧹 Nitpick comments (1)
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py (1)
306-313: LGTM! Probabilistic reboot logic is sound.The
rand_reboot()function implements a 30% reboot probability per iteration usingrandom.random(). The static analysis warning about non-cryptographic randomness is a false positive—cryptographic strength is unnecessary for test randomization.Consider making the reboot probability configurable via a test parameter for greater flexibility:
def rand_reboot(iteration): - # Set probability of reboot per iteration (e.g., 30%) - reboot_chance = 0.3 + reboot_chance = float(params.get("reboot_probability", "0.3")) if random.random() < reboot_chance:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/passthrough/pci/libvirt_pci_passthrough_hotplug.cfg(1 hunks)libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py (2)
libvirt/tests/src/virsh_cmd/domain/virsh_attach_device.py (1)
Controller(582-612)libvirt/tests/src/virtual_network/iface_bridge.py (1)
ping(83-98)
🪛 Ruff (0.13.3)
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py
126-126: Local variable distro_name is assigned to but never used
Remove assignment to unused variable distro_name
(F841)
309-309: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
⏰ 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.9
- GitHub Check: Python 3.11
- GitHub Check: Python 3.12
- GitHub Check: Python 3.8
🔇 Additional comments (11)
libvirt/tests/cfg/passthrough/pci/libvirt_pci_passthrough_hotplug.cfg (1)
48-53: LGTM! Test variants are well-configured.The two new variants (multiple_hotplug_hotunplug and multiple_hotplug_unplug_with_rand_reboot) are properly configured with appropriate iteration counts and align with the extended test flow in the Python script.
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py (10)
13-13: LGTM! Co-author credit appropriately added.
21-22: LGTM! Imports support new functionality.The
platformimport enables architecture detection (line 84), andrandomsupports probabilistic reboot logic (line 309).
84-90: LGTM! Parameters are properly initialized.The architecture detection, test parameters, and controller configuration are correctly extracted and typed. Converting
number_of_hotplug_unplugto int here (rather than at usage sites) is good practice.
93-93: LGTM! Python 3 compatibility fix.Replacing
.itervalues()with.values()is the correct Python 3 migration.
158-158: LGTM! Python 3 compatibility fix.Replacing
map(None, pci_devs, pci_ids)withzip(pci_devs, pci_ids)is the correct Python 3 migration. In Python 2,map(None, a, b)would zip lists and pad with None; in Python 3, this usage raises TypeError.
174-174: LGTM! Python 3 compatibility fix.Same as line 158—
zip()is the correct Python 3 replacement formap(None, ...).
196-201: LGTM! Architecture-specific detach logic is correct.For ppc64le with libvirt ≥ 3.10.0, devices must be detached from the host before attaching to the VM. For other architectures, detach is only required for older libvirt versions (< 3.10.0). This aligns with the platform requirements described in the PR objectives.
230-231: LGTM! Session initialization ensures proper guest interaction.Adding
session = vm.wait_for_login()at the start oftest_ping()ensures the session is always available for subsequent operations, regardless of whether the VM was already running.
331-336: LGTM! Multi-cycle hotplug/unplug loop is well-implemented.The loop correctly performs repeated hotplug→ping→unplug cycles for the configured number of iterations, aligning with the
multiple_hotplug_hotunplugtest variant.
338-344: LGTM! Random reboot loop is well-implemented.The loop correctly performs hotplug→ping→random-reboot→unplug cycles, aligning with the
multiple_hotplug_unplug_with_rand_reboottest variant. The probabilistic reboot between hotplug and unplug provides good coverage of device stability across VM reboots.
|
@smitterl Thank a lot for the valuable review comments and your time. I am adding the test results after taking care of all the review comments. Please find the results below. For rand_reboot test it took 6 rem=boots randomly after fewer iterations. Thank you |
|
@smitterl Would you please review the PR and let me know the inputs. |
|
@smitterl Would you please review the changes made. Thank you! |
|
@smitterl All review comments have been addressed. Please let me know if any further changes are needed. |
|
@smitterl Thanks for reviewing the patch, @TasmiyaNalatwad addressed all your review comments. May I request you to review the changes. |
smitterl
left a comment
smitterl
left a comment
There was a problem hiding this comment.
Sorry for the late reply.
Just a small nitpick, please remove the variable that's never used (s. comment). Rest LGTM, thanks.
| detected_distro = distro.detect() | ||
| distro_name = detected_distro.name.lower() | ||
| pkg = ["ppc64-diag", "powerpc-utils"] | ||
| if detected_distro.name == "sles16": | ||
| if int(detected_distro.version) == 16: | ||
| pkg.extend(["librtas2"]) | ||
| if detected_distro.name in ("fedora", "rhel"): | ||
| pkg.extend(["librtas"]) | ||
| if not utils_package.package_install(pkg, session, 360): |
There was a problem hiding this comment.
I agree with coderabbitai: please remove distro_name as it's never used.
1) Added 2 new test cases :
test 1 : hotplog/unplug operation is performed for multiple times
test 2 : multiple hotplug/unplug operations with random reboot is done in between to check device avialability inside VM.
2) Changes made are :
1) two new parameters added into cfg file
multiple_hotplug_hotunplug
number_of_hotplug_unplug
2) params.itervalues() [supported in python2] was removed in python3. Python3 has params.values() instead. Hence made this change as it was failing to read the params with .itervalues()
3) The "librtas" package name varies across different distros (fedora, sles etc). Therefore, the test first checks the running distro and adds the corresponding package name to the package list. This approach ensures seamless testing across all supported distributions.
4) Zip() is the correct and Python 3-compatible way to iterate over two lists in parallel.
5) For PP64LE arch in latest libvirt it is necessary to detach the device from the host before attaching it to a VM. Hence added a arch 'if' flag to check if the arch is ppc64le and libvirt is latest newer from 3.10 then detach the device from host first then attach it to VM.
6) utils_net.ping is same, no changes made to the ping function, just added timeout=30 sec and passing session handler to the same ping function, so that ping command is executed inside VM , as my requirement is to run ping test inside VM for the interface added for a pci device.
7) rand_reboot function is added. The function is designed where it Set probability of reboot per iteration (e.g., 30%) and based on total number of iterations and probability a random reboot happens in between hotplug/unplug operations. On an average for 30 iterations 6-8 reboots are expected.
8) Both the tests are called inside try block
Signed-off-by: Tasmiya Nalatwad <[email protected]>
TasmiyaNalatwad
force-pushed
the
PCI_PT_hotplug_unplug
branch
from
ca1f423 to
2daa678
Compare
|
@smitterl Thanks a lot for your time and valuable review comments. I have addressed all the review comments mentioned. Would you please approve and help me in merging this PR. Thanks in Advance. |
|
@smitterl As all the review comments are been addressed, Would you please approve and help me in merging this PR. Thanks in Advance. |
6 participants
Added 2 new test cases, where hotplog unplug is performed for multiple times and a random reboot is done between hotplug and unplug and the device availability is checked.
Below are the new cases added
(7/8) type_specific.io-github-autotest-libvirt.libvirt_pci_passthrough_hotplug.multiple_hotplug_hotunplug.PCI: STARTED
(7/8) type_specific.io-github-autotest-libvirt.libvirt_pci_passthrough_hotplug.multiple_hotplug_hotunplug.PCI: PASS (268.99 s)
(8/8) type_specific.io-github-autotest-libvirt.libvirt_pci_passthrough_hotplug.multiple_hotplug_unplug_with_rand_reboot.PCI: STARTED
(8/8) type_specific.io-github-autotest-libvirt.libvirt_pci_passthrough_hotplug.multiple_hotplug_unplug_with_rand_reboot.PCI: PASS (269.73 s)
Summary by CodeRabbit
New Features
Bug Fixes
Tests