Improve package installation for network utilities#4277
Improve package installation for network utilities#4277quanwenli wants to merge 1 commit intoavocado-framework:masterfrom
Conversation
WalkthroughAdded Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks✅ Passed checks (3 passed)
📜 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 (2)
⏰ 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). (5)
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
virttest/utils_net.py(3 hunks)virttest/utils_package.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
virttest/utils_net.py (1)
virttest/utils_package.py (2)
package_install(227-237)package_install_any(252-265)
virttest/utils_package.py (1)
virttest/utils_misc.py (2)
wait_for(557-592)wait_for(4186-4234)
🪛 GitHub Actions: CI
virttest/utils_net.py
[error] 1-1: Black formatting would reformat 2 files. Run 'black' (or your formatter) to fix code style issues.
virttest/utils_package.py
[error] 1-1: Black formatting would reformat 2 files. Run 'black' (or your formatter) to fix code style issues.
| if (not utils_package.package_install(["tmux"], session) or | ||
| not utils_package.package_install_any(["dhcp-client", "dhcpcd"], session)): | ||
| raise exceptions.TestError("Failed to install the required packages.") |
There was a problem hiding this comment.
DHCP client install vs usage mismatch; and remember to run Black
The new pattern of always installing tmux and then trying any of ["dhcp-client", "dhcpcd"] via package_install_any() improves flexibility, but there’s a subtle mismatch:
- The commands later in these functions (
create_ovs_bridge,delete_ovs_bridge,create_linux_bridge_tmux) still hard‑codedhclient(dhclient -r,dhclient {iface},pkill dhclient, etc.). - On a system where
dhcpcdis the only DHCP client and does not provide adhclientbinary,package_install_any()can succeed by installingdhcpcd, but the subsequent tmux command will still fail at runtime withdhclient: command not found.
This is a functional inconsistency and can turn an early, clear dependency failure into a later, less obvious runtime break. It would be better to either:
- Restrict
package_install_any()candidates here to packages that providedhclient, or - Make these bridge helpers detect and use the actual DHCP client binary (and its appropriate release/renew flags), similar to the
restart_guest_network()logic above that switches betweendhclientanddhcpcd.
Also, CI indicates Black would reformat this file; please run your formatter so the pipeline passes.
Also applies to: 4676-4678, 4782-4784
🧰 Tools
🪛 Ruff (0.14.7)
4633-4633: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In virttest/utils_net.py around lines 4631-4633 (also apply same change at
4676-4678 and 4782-4784): the code installs tmux and then any of
["dhcp-client","dhcpcd"] but later bridge helper functions invoke dhclient
unconditionally, causing runtime failures when dhcpcd is installed; change the
install step to only install DHCP clients that provide dhclient or update the
bridge helpers to detect which DHCP client binary is available (dhclient vs
dhcpcd) and call the appropriate release/renew commands accordingly (mirror the
logic used in restart_guest_network()), and finally run Black on the file to fix
formatting before committing.
| def package_install_any(candidates, session=None, timeout=PKG_MGR_TIMEOUT): | ||
| """ | ||
| Try to install packages from candidates in order until one succeeds. | ||
|
|
||
| :param candidates: iterable of package names to try in order | ||
| :param session: session Object | ||
| :param timeout: timeout for each install session | ||
| :return: True if any candidate installed successfully, else False | ||
| """ | ||
| for pkg in candidates: | ||
| mgr = package_manager(session, pkg) | ||
| if utils_misc.wait_for(mgr.install, timeout): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
New package_install_any() helper looks correct; consider normalizing input + run Black
The control flow (try each candidate, return on first successful install) is sound and consistent with package_install()’s use of wait_for. One small robustness nit: if a caller accidentally passes a single string instead of an iterable, this will iterate over characters rather than treat it as one package name. You can cheaply guard against that:
def package_install_any(candidates, session=None, timeout=PKG_MGR_TIMEOUT):
@@
- :return: True if any candidate installed successfully, else False
+ :return: True if any candidate installed successfully, else False
"""
- for pkg in candidates:
+ if isinstance(candidates, string_types):
+ candidates = [candidates]
+ for pkg in candidates:
mgr = package_manager(session, pkg)
if utils_misc.wait_for(mgr.install, timeout):
return True
return FalseAlso, CI reports that Black would reformat this file; please run your standard formatter so the pipeline passes.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In virttest/utils_package.py around lines 252 to 265, package_install_any() will
iterate characters if callers pass a single string; normalize the candidates
argument by detecting a str and wrapping it into a single-item iterable (e.g.,
convert a single package name string to [string]) before the loop, keep existing
behavior for real iterables, and then run Black (or your repository formatter)
on the file to satisfy CI formatting checks.
quanwenli
force-pushed
the
install-pkg
branch
2 times, most recently
from
b393304 to
84ed244
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
virttest/utils_package.py (1)
240-253: String input normalization not yet addressed.The previous review comment about guarding against a single string being passed (which would iterate over characters) has not been implemented. Please add the suggested normalization:
def package_install_any(candidates, session=None, timeout=PKG_MGR_TIMEOUT): """ Try to install the first available package from candidates. :param candidates: iterable of package names to try in order :param session: session Object :param timeout: timeout for each install session :return: True if any candidate installed successfully, else False """ + if isinstance(candidates, string_types): + candidates = [candidates] for pkg in candidates: mgr = package_manager(session, pkg) if utils_misc.wait_for(mgr.install, timeout): return True return Falsevirttest/utils_net.py (2)
4631-4648: DHCP client installation vs. command usage mismatch remains unaddressed.The previous review comment identified that installing
dhcpcd(whendhcp-clientis unavailable) will succeed, but the subsequent command on line 4647 hardcodesdhclient, which won't exist on systems usingdhcpcd. This will cause a runtime failure.Either restrict candidates to packages that provide
dhclient, or detect which DHCP client is available and use appropriate commands (similar torestart_guest_network()at lines 1707-1718).
4783-4786: Same DHCP client mismatch applies here.As noted in the previous review, installing
dhcpcdwhendhcp-clientis unavailable will succeed, but thedhclientcommand on line 4803 will fail at runtime.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
virttest/utils_net.py(3 hunks)virttest/utils_package.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
virttest/utils_package.py (1)
virttest/utils_misc.py (2)
wait_for(557-592)wait_for(4186-4234)
virttest/utils_net.py (1)
virttest/utils_package.py (2)
package_install(227-237)package_install_any(240-253)
🪛 GitHub Actions: CI
virttest/utils_net.py
[error] 4681-4681: Black formatting failed to parse the file. Cannot format: 4681:4: res = utils_misc.cmd_status_output( . 407 files would be left unchanged, 1 file would fail to reformat.
quanwenli
force-pushed
the
install-pkg
branch
from
84ed244 to
880f142
Compare
|
@qiankehan hi hhan, yanglei, As we have some discussion on weekly meeting about dhcpcd and dhclient , Could you also review this patch? |
| """ | ||
| for pkg in candidates: | ||
| mgr = package_manager(session, pkg) | ||
| if utils_misc.wait_for(mgr.install, timeout): |
There was a problem hiding this comment.
It could be merged into one line:
if utils_misc.wait_for(package_manager(session, pkg).install, timeout):There was a problem hiding this comment.
Thanks for the suggestion.
I think keeping the two-step makes the code consistent with the existing package_install().. implementation
|
@quanwenli hi wenli , you may also need to find some tp-qemu and infrastucture team member to check this patch |
|
@yanglei-rh could you help review this ? thanks a lot. |
|
@quanwenli please fix the conflicts.thx! |
utils_package: add package_install_any to try multiple candidates utils_net: update package install to support multiple dhcp clients Signed-off-by: Wenli Quan <wquan@redhat.com>
I have updated. thanks |
6 participants
utils_package: add package_install_any to try multiple candidates
utils_net: update package install to support multiple dhcp clients
ID: LIBVIRTAT-22185
Signed-off-by: Wenli Quan wquan@redhat.com
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.