libvirt/tests: add tests to virtual_network.iface_ovs - #6877
aniket-sahu-ibmx wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughChangesThis PR adds a new libvirt test configuration and a matching Python test module for hotplugging and hot-unplugging an OVS-backed network interface. The config defines OVS bridge and guest addressing, interface attributes, and guest-check/stress variants. The test module creates or reuses the OVS bridge, attaches the interface to a running VM, verifies the host tap attachment, optionally configures the guest and pings the bridge, detaches the interface, checks live XML removal, and performs teardown. Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant run as run()
participant ovs as OVS bridge
participant virsh as virsh
participant vm as VM live XML
participant guest as Guest session
run->>ovs: setup_ovs_bridge(bridge_name, bridge_ip, bridge_netmask)
run->>virsh: attach_device(iface xml)
run->>ovs: verify_ovs_port(iface_mac, bridge_name, vm_name)
opt verify_guest_iface enabled
run->>guest: verify_guest_iface_and_ping(iface_mac)
guest->>ovs: ping bridge_ip
end
run->>virsh: detach_device(iface xml)
run->>vm: check interface removed from live XML
run->>vm: destroy VM and restore VMXML
run->>ovs: delete bridge if created by test
Related issues: None found. Related PRs: None found. Suggested labels: virtual_network, test-addition Suggested reviewers: None specified. PoemA rabbit hops onto the OVS bridge so fine, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@libvirt/tests/cfg/virtual_network/iface_ovs.cfg`:
- Around line 90-93: The netmask fields bridge_netmask and guest_netmask are set
as dotted decimals but the test harness uses them with the `ip addr add
<ip>/<netmask> dev ...` invocation which expects a CIDR prefix length (e.g. 24);
update bridge_netmask and guest_netmask in iface_ovs.cfg to use numeric prefix
lengths (e.g. "24") so the `ip addr add` calls in the test will succeed when
assigning bridge_ip and guest_ip.
- Around line 84-87: The hotplug=false variant leaves iface_attach_xml and
iface_xml uninitialized but later reuses them in the attach/detach stress loop;
move the creation of iface_attach_xml and iface_xml out of the hotplug-only
branch in libvirt/tests/src/virtual_network/iface_ovs.py (or initialize them
before the re-attach/detach loop) so they are always defined when the stress
attach/detach path runs, or alternatively guard the loop to skip
detach/re-attach when hotplug == "no".
In `@libvirt/tests/src/virtual_network/iface_ovs.py`:
- Around line 245-247: The current branch only logs ping failures but does not
fail the test; change the block that runs session.cmd_status_output("ping -c 3
-W 5 %s" % ping_dest) (see variables test_ping, ping_dest and method
session.cmd_status_output) to assert the ping succeeded or raise a test failure
when status != 0 (e.g., replace the silent log of "FAILED" with an assertion or
explicit raise including the ping_dest and status), so any failed with_ping
connectivity check will stop the test rather than just logging.
- Around line 251-257: The loop currently detaches the interface on the last
iteration when detach is true and test_qos is false, and then the shared cleanup
also attempts a detach, causing a double-detach; modify the logic so the shared
cleanup only detaches if the interface is still attached—either by setting a
boolean flag (e.g., already_detached) when virsh.detach_interface or
virsh.detach_device is called inside the loop (using detach_type, iface_mac,
iface_xml), or by changing the shared cleanup condition to skip detach when
detach is true and iteration == attach_detach_iterations - 1 and not test_qos;
ensure the flag/condition is checked before calling virsh.detach_interface or
virsh.detach_device in the shared cleanup.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ddda4938-b7c7-4bd4-8725-e5d8c36cdc09
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/iface_ovs.cfglibvirt/tests/src/virtual_network/iface_ovs.py
2ae303f to
f7ea429
Compare
There was a problem hiding this comment.
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 `@libvirt/tests/src/virtual_network/iface_ovs.py`:
- Around line 203-257: The loop always calls virsh.attach_device and later
references iface_xml (only set when attach_type == "device"), causing
UnboundLocalError and inconsistent API use across iterations; fix by computing
and preserving the chosen attach/detach mode and the corresponding prepared
payloads before the loop (use attach_type and detach_type to decide and create
iface_attach_xml or iface_xml once, or set flags like use_device_attach), then
inside the attach/detach loop call the matching API (e.g., call
virsh.attach_device with iface_attach_xml when attach_type == "device" or call
the interface-specific attach API when attach_type == "interface") and similarly
use iface_xml vs iface_attach_xml for virsh.detach_device/virsh.detach_interface
so the same mode and variables are used consistently across all
attach_detach_iterations.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c61421a-945d-4f2d-8b84-b35a48a7cc7c
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/iface_ovs.cfglibvirt/tests/src/virtual_network/iface_ovs.py
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/iface_ovs.cfg
f7ea429 to
18324b3
Compare
There was a problem hiding this comment.
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 `@libvirt/tests/src/virtual_network/iface_ovs.py`:
- Around line 212-214: The virsh.attach_interface call on line 212 (and
similarly on line 178) does not specify a MAC address parameter, causing virsh
to generate a random MAC. However, subsequent verifications reference iface_mac
which holds the original MAC from line 129, resulting in a mismatch. Fix this by
adding the --mac parameter to the virsh.attach_interface command options when
attach_type equals "interface", passing iface_mac as the value (e.g., add "--mac
%s" % iface_mac to the options string in both the attach_type == "interface"
block around line 212 and the similar attach block around line 178). This
ensures the MAC address used in the attach command matches the MAC being
verified in subsequent checks on lines 222, 226, and 241.
- Around line 251-255: The `ip addr add` command in the guest IP configuration
block fails on subsequent test iterations because the address persists from the
previous iteration when the interface is not detached. Before executing the
`session.cmd("ip addr add %s/%s dev %s"...)` command with the guest_ip and
guest_netmask values, first check if the IP address is already configured on the
interface using an `ip addr show` command, or alternatively flush existing
addresses on that interface using `ip addr flush dev %s` before adding the new
address. This prevents the "File exists" error on iterations greater than zero.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c11e961b-aebb-4376-a093-489bd6dafbd2
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/iface_ovs.cfglibvirt/tests/src/virtual_network/iface_ovs.py
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/iface_ovs.cfg
|
Test Results: |
|
I believe you are adding significant part of the needed test, the only issue is, the legacy way you unfortunately used. Please check existing code in virtual_network/attach_detach_device/attach_iface_with*. there is much simplier way how to work with iface_parameters. if you need to have it better parametrized .. you can use the approach used in virtual_network/attach_detach_device/attach_detach_nat_interface.cfg instead of a lot of new parameters that has to be processed and passed, you will add one row according cursorIDE AI I used to suggest the new approach structure (into .cfg) in .py: iface = libvirt_vmxml.create_vm_device_by_type( Second issue is, that it would be better to enlarge the existing variant attach.detach with your code. (this is again suggestion of AI ... I didn't check how the code really works .. but from brief check it looks it is right) But looking at the code overall ... it feels like you didn't check existing codes and methods and helpers either in tp-libvirt providers or avocado-vt... e.g. whole block 237->263: Check and follow the same approach as attach_detach_nat_interface.py and iface_options.py instead of raw serial session.cmd() calls. Third issue I see is more general. The code is not following best practices... At least put several part that do one logical thing to specific method. i.e. there will be just |
18324b3 to
9a69213
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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
`@libvirt/tests/src/virtual_network/attach_detach_device/attach_detach_ovs_interface.py`:
- Around line 35-40: The teardown logic is deleting an OVS bridge
unconditionally even when setup_ovs_bridge() only reused an existing bridge.
Update the test to track whether setup_ovs_bridge() actually created the bridge
(for example with a flag set when utils_net.add_ovs_bridge() runs) and only
remove it in the matching teardown path; if the bridge pre-existed, leave it
untouched. Apply the same change wherever the bridge is torn down in this test
flow so shared host OVS networking is not disrupted.
- Line 112: The `attach_detach_ovs_interface` test is parsing cfg-driven
`iface_attrs` with `eval()`, which should not be used for configuration parsing.
Update the parsing logic in this test to use `ast.literal_eval()` instead, and
make sure the module imports `ast` so `iface_attrs` is still converted from the
config string safely.
- Around line 37-40: The bridge setup commands in attach_detach_ovs_interface
use shell=True with bridge_name, bridge_ip, and bridge_netmask, which leaves
privileged ip invocation vulnerable to shell injection. Update the setup logic
in the relevant test helper to avoid shell interpolation by passing arguments to
process.run as argv, or validate/quote the inputs before executing the commands.
Keep the fix localized around the bridge setup calls that bring the interface up
and add the address.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fa0a20cb-e3e5-483f-bf3c-f71f63631847
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/attach_detach_device/attach_detach_ovs_interface.cfglibvirt/tests/src/virtual_network/attach_detach_device/attach_detach_ovs_interface.py
✅ Files skipped from review due to trivial changes (1)
- libvirt/tests/cfg/virtual_network/attach_detach_device/attach_detach_ovs_interface.cfg
|
Hi @hholoubk I have refactored the code according to your instructions using the existing utilities, and added the tests in new files under virtual_network/attach_detach_device. Although I don't think it is a good idea to enlarge the existing attach.detach variant, as the existing attach variant has hardcoded values for the interface source, and also it follows the legacy standards, as you mentioned before. Please let me know if any other changes are required. |
9a69213 to
e4f73e3
Compare
Add attach-detach tests for ovs bridge interface with multiple options, i.e. testing with pings, stress testing attach-detach with all options configurable.
e4f73e3 to
b1867bd
Compare
|
Test results after rework: guest_backuprestore Run Successfully executed |
|
@harihare keeping you in loop for this |
2 participants
Add attach-detach tests for ovs bridge interface with multiple options, i.e. testing with pings, stress testing attach-detach with all options configurable.
Summary by CodeRabbit