Improve virtual_network provider's public function#6845
Improve virtual_network provider's public function#6845Yingshun merged 1 commit intoautotest:masterfrom
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:
WalkthroughAdds compile_netperf_pkg(params, env, address) in provider/virtual_network/netperf_base.py, importing re, avocado.core.exceptions, and avocado.utils.process. The function resolves the build target (localhost, a VM from params["vms"], or dotted IPv4 when remote_server=yes), determines install paths and expected artifacts (src/netserver, src/netperf), checks idempotently for existing artifacts (session.cmd_status for remote/VM, os.path.exists for localhost), copies the archive to remote targets if needed, selects decompression command by .tar.bz2/.tar.gz, derives the build triplet from arch (remote via session.cmd_output, local via process.run), runs autogen.sh, ./configure --build/--prefix, and make, and returns (nserver_path, nperf_path). In provider/virtual_network/network_base.py, check_throughput() accepts netserver_cmd and netperf_cmd and derives the pkill target from the server command; cancel_if_ovs_bridge() and setup_ovs_bridge_attrs() use the full params['netdst'] to resolve the bridge manager, raise TestError when not found, detect OpenVSwitch by the bridge object's class name, and set iface_attrs['source'] = {'bridge': netdst}. Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
provider/virtual_network/netperf_base.py (2)
155-161: Incomplete docstring: missing parameter documentation.The docstring documents
addressand return value but omitsparamsandenvparameters.📝 Suggested docstring improvement
def compile_netperf_pkg(params, env, address): """ Compile netperf source package + :param params: Test parameters dictionary + :param env: Test environment object :param address: localhost, vm name, or ip address :return: netserver_path, netperf_path """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider/virtual_network/netperf_base.py` around lines 155 - 161, The docstring for compile_netperf_pkg is incomplete: it documents only address and the return but omits params and env; update the function docstring to document all parameters (params, env, address) and the return values (netserver_path, netperf_path), including brief types/expected values and purpose for params and env, so anyone reading compile_netperf_pkg can understand what each argument represents and what the function returns.
212-212: Unnecessaryshell=Truefor simple command.The
archcommand doesn't require shell interpretation. Removingshell=Trueis cleaner and avoids static analysis warnings.♻️ Suggested fix
- arch = process.run("arch", shell=True).stdout_text.strip() + arch = process.run("arch").stdout_text.strip()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider/virtual_network/netperf_base.py` at line 212, The call that captures architecture uses process.run("arch", shell=True) — remove shell=True and invoke the command without a shell (e.g., process.run(["arch"]) or process.run("arch", shell=False)) so the command is executed directly; update the assignment to arch = process.run(["arch"]).stdout_text.strip() (or equivalent) to avoid unnecessary shell usage and static-analysis warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@provider/virtual_network/netperf_base.py`:
- Around line 155-161: The docstring for compile_netperf_pkg is incomplete: it
documents only address and the return but omits params and env; update the
function docstring to document all parameters (params, env, address) and the
return values (netserver_path, netperf_path), including brief types/expected
values and purpose for params and env, so anyone reading compile_netperf_pkg can
understand what each argument represents and what the function returns.
- Line 212: The call that captures architecture uses process.run("arch",
shell=True) — remove shell=True and invoke the command without a shell (e.g.,
process.run(["arch"]) or process.run("arch", shell=False)) so the command is
executed directly; update the assignment to arch =
process.run(["arch"]).stdout_text.strip() (or equivalent) to avoid unnecessary
shell usage and static-analysis warnings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ad20a54-161a-45f1-b62b-b6904e4f9fe8
📒 Files selected for processing (2)
provider/virtual_network/netperf_base.pyprovider/virtual_network/network_base.py
yanglei-rh
force-pushed
the
LIBVIRTAT-22400
branch
from
ddbe7ef to
53f80b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
provider/virtual_network/netperf_base.py (2)
217-217: Build type mapping may be incomplete for x86_64.The mapping only explicitly handles
aarch64. For other architectures likex86_64, the raw arch value is used as the--buildargument. While this often works, the canonical triplet for x86_64 is typicallyx86_64-pc-linux-gnuorx86_64-unknown-linux-gnu.If configure complains about unrecognized architectures in the future, consider expanding the mapping:
- build_type = {"aarch64": "aarch64-unknown-linux-gnu"}.get(arch, arch) + build_type_map = { + "aarch64": "aarch64-unknown-linux-gnu", + "x86_64": "x86_64-unknown-linux-gnu", + } + build_type = build_type_map.get(arch, arch)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider/virtual_network/netperf_base.py` at line 217, The build_type assignment currently maps only "aarch64" and otherwise uses the raw arch, which can lead to non-canonical triplets for x86_64; update the mapping used to compute build_type (the dict assigned to build_type in netperf_base.py) to include "x86_64" -> "x86_64-unknown-linux-gnu" (or "x86_64-pc-linux-gnu") so the --build argument uses a canonical triplet, and keep the fallback to raw arch for any other values.
174-187: Potentially confusing behavior when IP is passed withoutremote_server=True.When
addressmatches an IP butparams.get_boolean("remote_server", False)returnsFalse, the function setssession = Noneand proceeds to compile locally instead of on the remote server. This silent fallback could be unexpected for callers who pass an IP address assuming remote compilation.Consider either:
- Raising an error when an IP is provided without
remote_server=True, or- Treating any IP address as implying remote compilation
♻️ Option 1: Error on ambiguous input
elif re.match(r"^(?:\d{1,3}\.){3}\d{1,3}$", address): if params.get_boolean("remote_server", False): session = remote.remote_login( "ssh", params.get("server_ip", address), "22", params.get("server_user"), params.get("server_pwd"), r'[$#%]' ) else: - session = None + raise exceptions.TestError( + f"IP address '{address}' provided but 'remote_server' param is not set. " + "Set remote_server=yes to compile on remote server, or use 'localhost'." + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider/virtual_network/netperf_base.py` around lines 174 - 187, When address matches an IP but params.get_boolean("remote_server", False) is False, the code silently falls back to local compilation; change this to reject ambiguous input by raising an explicit error (e.g., ValueError/RuntimeError) when an IP is provided without remote_server=True so callers must opt-in to remote compilation. Update the branch that currently sets session = None (the block using re.match(..., address), params.get_boolean("remote_server", False), remote.remote_login, install_path, target_ip, user, pwd) to raise a clear exception with guidance (e.g., "IP address provided; set remote_server=True to compile remotely or provide a hostname for local compile") instead of continuing silently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@provider/virtual_network/netperf_base.py`:
- Around line 170-189: The code in netperf_base.py creates sessions via
vm.wait_for_login() and remote.remote_login() but never closes them; update the
logic around session creation (the branches that set session and install_path)
to ensure sessions are properly closed by either (a) wrapping subsequent
session-use code in a try/finally where finally calls session.close() if session
is not None, or (b) change the API to return the session to the caller so the
caller is responsible for closing it — modify the blocks that set session
(vm.wait_for_login() and remote.remote_login()) and any code that uses session
accordingly to guarantee session.close() is invoked on all execution paths.
---
Nitpick comments:
In `@provider/virtual_network/netperf_base.py`:
- Line 217: The build_type assignment currently maps only "aarch64" and
otherwise uses the raw arch, which can lead to non-canonical triplets for
x86_64; update the mapping used to compute build_type (the dict assigned to
build_type in netperf_base.py) to include "x86_64" -> "x86_64-unknown-linux-gnu"
(or "x86_64-pc-linux-gnu") so the --build argument uses a canonical triplet, and
keep the fallback to raw arch for any other values.
- Around line 174-187: When address matches an IP but
params.get_boolean("remote_server", False) is False, the code silently falls
back to local compilation; change this to reject ambiguous input by raising an
explicit error (e.g., ValueError/RuntimeError) when an IP is provided without
remote_server=True so callers must opt-in to remote compilation. Update the
branch that currently sets session = None (the block using re.match(...,
address), params.get_boolean("remote_server", False), remote.remote_login,
install_path, target_ip, user, pwd) to raise a clear exception with guidance
(e.g., "IP address provided; set remote_server=True to compile remotely or
provide a hostname for local compile") instead of continuing silently.
🪄 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: c1373552-7810-4e18-94bf-07482c9628a3
📒 Files selected for processing (2)
provider/virtual_network/netperf_base.pyprovider/virtual_network/network_base.py
🚧 Files skipped from review as they are similar to previous changes (1)
- provider/virtual_network/network_base.py
yanglei-rh
force-pushed
the
LIBVIRTAT-22400
branch
from
53f80b1 to
ab49031
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
provider/virtual_network/netperf_base.py (1)
205-208:⚠️ Potential issue | 🟠 MajorClose the session on skip and failure paths too.
Line 208 returns before Line 249, and any exception in the copy/build path skips the close as well. That still leaks VM/SSH sessions across repeated tests.
Suggested fix
- if session: - if session.cmd_status(f"test -f {nperf_path}") == 0: - LOG_JOB.info(f"Netperf already compiled on {address}, skipping.") - return nserver_path, nperf_path - - ... - - LOG_JOB.info(f"Netperf compilation completed on {address}") - if session: - session.close() - return nserver_path, nperf_path + try: + if session and session.cmd_status(f"test -f {nperf_path}") == 0: + LOG_JOB.info(f"Netperf already compiled on {address}, skipping.") + return nserver_path, nperf_path + + ... + + LOG_JOB.info(f"Netperf compilation completed on {address}") + return nserver_path, nperf_path + finally: + if session: + session.close()Also applies to: 247-249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider/virtual_network/netperf_base.py` around lines 205 - 208, The early return when the remote netperf binary exists and any exceptions during copy/build leak the SSH session; wrap the logic that uses the session (the block that checks session.cmd_status(...), the copy/build sequence around nperf_path/nserver_path) in a try/finally (or use a context manager) and call session.close() in the finally so the session is always closed before returning or re-raising; specifically ensure session.close() is invoked before returning from the check that returns nserver_path, nperf_path and also in the exception paths after attempts to copy/build.
🧹 Nitpick comments (1)
provider/virtual_network/network_base.py (1)
782-784: Consider the error handling pattern inconsistency, but note thatnetdstis a guaranteed prerequisite in tp-libvirt tests.The same helper is wrapped in
except process.CmdErrorincancel_if_ovs_bridge()(line 769), but not here at line 782. However, tp-libvirt test configurations always setnetdstas a prerequisite for testing, meaning it will be a valid bridge name. The None check and defensive error message are therefore not expected to be triggered in normal operation.If adding the
try-exceptfor consistency across the codebase, ensure it aligns with the broader pattern incancel_if_ovs_bridge(). Otherwise, the current code is acceptable given the test configuration constraints.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider/virtual_network/network_base.py` around lines 782 - 784, Call to utils_net.find_bridge_manager(netdst) should follow the same error-handling pattern as cancel_if_ovs_bridge: wrap the find_bridge_manager(netdst) invocation in a try/except process.CmdError block (catch process.CmdError) and handle the exception the same way cancel_if_ovs_bridge does (e.g., cancel or log/raise consistent error), rather than only checking for None and raising TestError; reference the existing cancel_if_ovs_bridge implementation and the symbols utils_net.find_bridge_manager, cancel_if_ovs_bridge, process.CmdError, and exceptions.TestError to locate and copy the handling behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@provider/virtual_network/netperf_base.py`:
- Around line 176-183: The remote-IP branch currently passes
params.get("server_ip", address) into the remote.remote_login call, which
prefers params["server_ip"] over the supplied address; change it so the explicit
address argument is honored (i.e., pass address when it was selected, falling
back to params.get("server_ip") only if address is empty/None) — update the
remote.remote_login calls (the session assignment) and the other similar
occurrence around the second call (line ~190) to use address (or address or
params.get("server_ip")) instead of params.get("server_ip", address).
In `@provider/virtual_network/network_base.py`:
- Line 453: The pkill call uses os.path.basename(netserver_cmd) which fails when
netserver_cmd contains flags; import shlex and use shlex.split(netserver_cmd)[0]
to get the executable token, then call os.path.basename on that token before
passing to serv_runner (i.e., change the pkill target derivation in the code
around serv_runner and netserver_cmd to parse the command first and then
basename the first token).
---
Duplicate comments:
In `@provider/virtual_network/netperf_base.py`:
- Around line 205-208: The early return when the remote netperf binary exists
and any exceptions during copy/build leak the SSH session; wrap the logic that
uses the session (the block that checks session.cmd_status(...), the copy/build
sequence around nperf_path/nserver_path) in a try/finally (or use a context
manager) and call session.close() in the finally so the session is always closed
before returning or re-raising; specifically ensure session.close() is invoked
before returning from the check that returns nserver_path, nperf_path and also
in the exception paths after attempts to copy/build.
---
Nitpick comments:
In `@provider/virtual_network/network_base.py`:
- Around line 782-784: Call to utils_net.find_bridge_manager(netdst) should
follow the same error-handling pattern as cancel_if_ovs_bridge: wrap the
find_bridge_manager(netdst) invocation in a try/except process.CmdError block
(catch process.CmdError) and handle the exception the same way
cancel_if_ovs_bridge does (e.g., cancel or log/raise consistent error), rather
than only checking for None and raising TestError; reference the existing
cancel_if_ovs_bridge implementation and the symbols
utils_net.find_bridge_manager, cancel_if_ovs_bridge, process.CmdError, and
exceptions.TestError to locate and copy the handling behavior.
🪄 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: b7a620bb-604e-47ae-9527-43b3233e8ed5
📒 Files selected for processing (2)
provider/virtual_network/netperf_base.pyprovider/virtual_network/network_base.py
yanglei-rh
force-pushed
the
LIBVIRTAT-22400
branch
from
ab49031 to
218567b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
provider/virtual_network/network_base.py (1)
453-454:⚠️ Potential issue | 🔴 CriticalAdd the missing
shleximport.Line 453 uses
shlex.split(...), but this module never importsshlex, socheck_throughput()will fail withNameErroron the first invocation.Suggested fix
import logging import math import os import re +import shlex import shutil import timeRun this to confirm the missing import:
#!/bin/bash set -e rg -n '^(import shlex|from shlex import)' provider/virtual_network/network_base.py || true rg -n 'shlex\.split\(' provider/virtual_network/network_base.pyExpected: the first search returns no match, and the second reports the
shlex.split(call at Line 453.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider/virtual_network/network_base.py` around lines 453 - 454, The code calls shlex.split(...) in check_throughput() (creating netserver_bin then calling serv_runner), but shlex is not imported; add "import shlex" near the other module imports at the top of provider/virtual_network/network_base.py so shlex.split in check_throughput() resolves correctly and avoids NameError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@provider/virtual_network/netperf_base.py`:
- Around line 249-252: The current finally block in the function returns
nserver_path and nperf_path, which suppresses exceptions from copy_files_to(),
session.cmd(), or make; modify the function so that finally only performs
cleanup (e.g., closing session via session.close() if session) and remove the
return from the finally block, placing the return of nserver_path, nperf_path
after the try/except/finally so exceptions can propagate; ensure the code still
closes session in the finally and that the return occurs only on successful
compilation/build (i.e., after the block that calls copy_files_to(),
session.cmd(), and make).
---
Duplicate comments:
In `@provider/virtual_network/network_base.py`:
- Around line 453-454: The code calls shlex.split(...) in check_throughput()
(creating netserver_bin then calling serv_runner), but shlex is not imported;
add "import shlex" near the other module imports at the top of
provider/virtual_network/network_base.py so shlex.split in check_throughput()
resolves correctly and avoids NameError.
🪄 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: b9050ace-037c-48a3-9a89-325c50ccbbb1
📒 Files selected for processing (2)
provider/virtual_network/netperf_base.pyprovider/virtual_network/network_base.py
yanglei-rh
force-pushed
the
LIBVIRTAT-22400
branch
2 times, most recently
from
d0c298e to
4bb2765
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@provider/virtual_network/netperf_base.py`:
- Line 204: There is a blank line in netperf_base.py that contains trailing
whitespace causing the CI failure; open provider/virtual_network/netperf_base.py
(module netperf_base) and remove the trailing spaces on the empty line (the
blank line near the reported region) so the line is truly empty, then
save/commit the change.
🪄 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: 5bdd51fa-5d4c-4994-aa8b-8bebfd2e4d55
📒 Files selected for processing (2)
provider/virtual_network/netperf_base.pyprovider/virtual_network/network_base.py
yanglei-rh
force-pushed
the
LIBVIRTAT-22400
branch
from
4bb2765 to
990a339
Compare
|
@yiqianwei Please help review this patch, thanks. |
yanglei-rh
force-pushed
the
LIBVIRTAT-22400
branch
from
990a339 to
f760ffd
Compare
|
Test pass. |
yanglei-rh
force-pushed
the
LIBVIRTAT-22400
branch
from
f760ffd to
4f840d1
Compare
yanglei-rh
force-pushed
the
LIBVIRTAT-22400
branch
2 times, most recently
from
b7ebec1 to
fc4a76a
Compare
yanglei-rh
force-pushed
the
LIBVIRTAT-22400
branch
from
fc4a76a to
561d739
Compare
yanglei-rh
force-pushed
the
LIBVIRTAT-22400
branch
2 times, most recently
from
a4c63ff to
6e1b6fd
Compare
1.Add netperf compile function in netperf_base to support some function test need to capture output 2.Improve network_base.check_throughput can accept netperf's absolute paths 3.Improve netdst obtain method in network_base 4.Improve ovs bridge's judgement method Assisted-by: Gemini AI Signed-off-by: Lei Yang <leiyang@redhat.com>
yanglei-rh
force-pushed
the
LIBVIRTAT-22400
branch
from
6e1b6fd to
750b657
Compare
|
Test pass. |
3 participants
1.Add netperf compile function in netperf_base to support some function test need to capture output
2.Improve network_base.check_throughput can accept netperf's absolute paths
3.Improve netdst obtain method in network_base
4.Improve ovs bridge's judgement method
ID: 22400
Assisted-by: Gemini AI
Signed-off-by: Lei Yang leiyang@redhat.com
Summary by CodeRabbit
New Features
Improvements
Bug Fixes