[draft]virttest/utils_net: Fix get_network_cfg_file() for the distro over RHEL9 #4222
Conversation
📝 WalkthroughWalkthroughAdds distro version retrieval across utils and VM APIs, then updates network configuration path selection to use distro name/version (including RHEL/CentOS 9+ NetworkManager paths). Introduces utils_misc.get_distro_version, BaseVM.get_distro_version, and refactors utils_net to source distro info from host or VM session. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
virttest/utils_test/libvirt.py (1)
1440-1441: Keep supported checkpoints list consistent with implementationThe docstring and code support a "state" checkpoint, but support_check omits it, leading to a misleading error message.
Suggested snippet near Line 1440:
support_check = ["exists", "mac", "ip", "ping", "state"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
virttest/utils_test/libvirt.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
virttest/utils_test/libvirt.py (1)
virttest/remote.py (1)
run(529-558)
🪛 GitHub Actions: CI
virttest/utils_test/libvirt.py
[error] 1440-1442: Black formatting check failed during 'make check'. 1 file would be reformatted (virttest/utils_test/libvirt.py). Run 'black' to format the code.
virttest/utils_test/libvirt.py
Outdated
| check_pass = not process.run("ip addr show dev {0}".format(iface_name), | ||
| ignore_status=True).exit_status |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Functional change LGTM; fix Black failure and improve robustness by avoiding shell string
The switch to using ip addr is correct and aligns with the PR objective. However, the current multi-line call is triggering the Black formatter failure, and passing a single string to process.run is less robust than an argv list.
Refactor as below to:
- satisfy Black,
- avoid quoting issues by passing argv list,
- add a helpful debug log.
- check_pass = not process.run("ip addr show dev {0}".format(iface_name),
- ignore_status=True).exit_status
+ # Determine interface existence via ip(8) exit status
+ result = process.run(
+ ["ip", "addr", "show", "dev", iface_name],
+ ignore_status=True,
+ )
+ check_pass = result.exit_status == 0
+ if not check_pass:
+ LOG.debug("Interface %s not found by 'ip addr show'", iface_name)📝 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.
| check_pass = not process.run("ip addr show dev {0}".format(iface_name), | |
| ignore_status=True).exit_status | |
| # Determine interface existence via ip(8) exit status | |
| result = process.run( | |
| ["ip", "addr", "show", "dev", iface_name], | |
| ignore_status=True, | |
| ) | |
| check_pass = result.exit_status == 0 | |
| if not check_pass: | |
| LOG.debug("Interface %s not found by 'ip addr show'", iface_name) |
🤖 Prompt for AI Agents
In virttest/utils_test/libvirt.py around lines 1445-1446, the call passing a
single formatted shell string to process.run is causing Black formatting
failures and is brittle; change it to pass an argv list (e.g.
["ip","addr","show","dev", iface_name]) to avoid shell quoting issues and
satisfy Black, and add a concise debug log before or after the call that
includes the iface_name and the command/result so failures are easier to
diagnose; keep ignore_status=True and use the process.run return value to set
check_pass as before.
Signed-off-by: Han Han <[email protected]>
It cannot get the host distribution name from platform.platform().lower(). And for rhel9 and the later distributions[1], the net configration files are stored to /etc/NetworkManager/system-connections/, not /etc/sysconfig/network-scripts/. [1]: https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/10/html/considerations_in_adopting_rhel_10/networking Signed-off-by: Han Han <[email protected]>
qiankehan
changed the title
|
v2 update: Fix the function get_network_cfg_file() instead of check_iface() |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (3)
virttest/utils_misc.py(1 hunks)virttest/utils_net.py(1 hunks)virttest/virt_vm.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
virttest/virt_vm.py (3)
virttest/utils_test/__init__.py (1)
session_handler(993-1007)virttest/utils_misc.py (1)
get_distro_version(4469-4488)virttest/utils_netperf.py (1)
session(122-124)
virttest/utils_net.py (2)
virttest/virt_vm.py (2)
get_distro(717-721)get_distro_version(724-728)virttest/utils_misc.py (2)
get_distro(4447-4466)get_distro_version(4469-4488)
virttest/utils_misc.py (3)
virttest/virt_vm.py (1)
get_distro_version(724-728)virttest/remote.py (1)
cmd_status_output(718-727)virttest/utils_libvirt/libvirt_keywrap.py (1)
cmd_status_output(54-72)
🪛 GitHub Actions: CI
virttest/utils_net.py
[error] 1-1: make check failed due to Black formatting: 1 file would be reformatted (virttest/utils_net.py). Run 'black virttest/utils_net.py' to fix code style issues.
🪛 Ruff (0.12.2)
virttest/utils_misc.py
4488-4488: return inside finally blocks cause exceptions to be silenced
(B012)
🔇 Additional comments (2)
virttest/virt_vm.py (1)
723-729: LGTM: mirrors get_distro() pattern correctlyThe new method is consistent with get_distro(), reuses session management, and keeps the public API tidy.
virttest/utils_net.py (1)
1819-1835: CI: Black formatting failure on this fileGitHub Actions reports Black would reformat virttest/utils_net.py. Please run:
- black virttest/utils_net.py
to satisfy style checks.
| def get_distro_version(session=None): | ||
| """ | ||
| Get distribution version of the Host/Guest/Remote Host | ||
| :param session: ShellSession object of VM or remote host | ||
| :return: distribution name of type str | ||
| """ | ||
| if not session: | ||
| return distro.detect().version | ||
| else: | ||
| distro_version = "" | ||
| cmd = "cat /etc/os-release | grep '^VERSION_ID='" | ||
| try: | ||
| status, output = session.cmd_status_output(cmd, timeout=300) | ||
| if status: | ||
| LOG.debug("Unable to get the distro version: %s" % output) | ||
| else: | ||
| distro_version = output.split("=")[1].strip() | ||
| finally: | ||
| return distro_version | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix: avoid return in finally, sanitize VERSION_ID, and correct docstring
- Returning inside finally silences exceptions (Ruff B012). Also, VERSION_ID often comes quoted; downstream code casting to numbers will fail without stripping quotes. The docstring’s return description says “distribution name” but this function returns the version.
Apply this refactor:
-def get_distro_version(session=None):
- """
- Get distribution version of the Host/Guest/Remote Host
-
- :param session: ShellSession object of VM or remote host
- :return: distribution name of type str
- """
- if not session:
- return distro.detect().version
- else:
- distro_version = ""
- cmd = "cat /etc/os-release | grep '^VERSION_ID='"
- try:
- status, output = session.cmd_status_output(cmd, timeout=300)
- if status:
- LOG.debug("Unable to get the distro version: %s" % output)
- else:
- distro_version = output.split("=")[1].strip()
- finally:
- return distro_version
+def get_distro_version(session=None):
+ """
+ Get distribution version of the Host/Guest/Remote Host.
+
+ :param session: ShellSession object of VM or remote host
+ :return: distribution version as a string
+ """
+ if not session:
+ return distro.detect().version
+ # Strip quotes from VERSION_ID to make it safe to parse/compare downstream
+ cmd = r"awk -F= '/^VERSION_ID=/{gsub(/\"/,\"\",$2); print $2}' /etc/os-release"
+ status, output = session.cmd_status_output(cmd, timeout=300)
+ if status:
+ LOG.debug("Unable to get the distro version: %s", output)
+ return ""
+ return output.strip()This preserves original behavior, removes the finally-return smell, and yields a sanitized version string suitable for numeric comparisons.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.12.2)
4488-4488: return inside finally blocks cause exceptions to be silenced
(B012)
🤖 Prompt for AI Agents
In virttest/utils_misc.py around lines 4469–4489, remove the return from the
finally block and update the docstring to state the function returns the
distribution version (not name); run the session command inside a try/except (no
return in finally), if cmd_status_output returns non-zero log the output and
return empty string, otherwise extract the right-hand side of VERSION_ID, strip
surrounding whitespace and any surrounding single/double quotes (so "20.04" ->
20.04), and return that sanitized version string; allow unexpected exceptions to
propagate (do not swallow them).
| distro_name = utils_misc.get_distro() | ||
| distro_version = utils_misc.get_distro_version() | ||
| if vm: | ||
| distro = vm.get_distro().lower() | ||
| else: | ||
| distro = platform.platform().lower() | ||
| distro_name = vm.get_distro().lower() | ||
| distro_version = vm.get_distro_version() | ||
| iface_cfg_file = "" | ||
| if "ubuntu" in distro: | ||
| if "ubuntu" in distro_name: | ||
| iface_cfg_file = "/etc/network/interfaces" | ||
| elif "suse" in distro: | ||
| elif "suse" in distro_name: | ||
| iface_cfg_file = "/etc/sysconfig/network/ifcfg-%s" % (iface_name) | ||
| # Net configs are stored to /etc/NetworkManager/system-connections/{iface_name}.nmconnection | ||
| # since rhel/centos 9 | ||
| elif distro_name in ["rhel", "centos"] and float(distro_version) >= 9: | ||
| iface_cfg_file = "/etc/NetworkManager/system-connections/{0}.nmconnection".format(iface_name) | ||
| else: | ||
| iface_cfg_file = "/etc/sysconfig/network-scripts/" | ||
| iface_cfg_file += "ifcfg-%s" % (iface_name) | ||
| iface_cfg_file = "/etc/sysconfig/network-scripts/ifcfg-{0}".format(iface_name) | ||
| return iface_cfg_file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
RHEL/CentOS 9+ detection is brittle (quotes/case) and may misroute to legacy path
- distro_name for host is not lower-cased; “ubuntu”/“suse” checks can miss (e.g., “Ubuntu”).
- get_distro_version() and VM method can return quoted strings (e.g., '"9.3"'), and this code uses float(distro_version), which will raise ValueError.
- For RHEL/CentOS, you’re using equality against ["rhel", "centos"]; if distro_name is '"rhel"' (quoted), the test fails and falls back to network-scripts.
Refactor to normalize name/version and compare on a parsed major version:
- distro_name = utils_misc.get_distro()
- distro_version = utils_misc.get_distro_version()
+ distro_name = utils_misc.get_distro().lower()
+ distro_version = utils_misc.get_distro_version()
if vm:
- distro_name = vm.get_distro().lower()
- distro_version = vm.get_distro_version()
+ distro_name = vm.get_distro().lower()
+ distro_version = vm.get_distro_version()
+ # Normalize values (strip quotes/whitespace) and extract major version
+ distro_name = distro_name.strip().strip('"').strip("'")
+ _ver_str = (distro_version or "").strip().strip('"').strip("'")
+ major_version = 0
+ m = re.match(r"^(\d+)", _ver_str)
+ if m:
+ major_version = int(m.group(1))
@@
- elif distro_name in ["rhel", "centos"] and float(distro_version) >= 9:
- iface_cfg_file = "/etc/NetworkManager/system-connections/{0}.nmconnection".format(iface_name)
+ elif any(n in distro_name for n in ("rhel", "centos")) and major_version >= 9:
+ iface_cfg_file = "/etc/NetworkManager/system-connections/{0}.nmconnection".format(
+ iface_name
+ )This fixes:
- Case sensitivity for host distro_name checks.
- Quoted VERSION_ID issues.
- Correct branch selection for RHEL/CentOS 9+ due to sanitized equality.
Note: NM connection filenames may differ from the iface name when connection IDs are customized, but using iface_name is a reasonable default consistent with existing behavior.
Committable suggestion skipped: line range outside the PR's diff.
qiankehan
changed the title
1 participant
From RHEL10, the ifcfg files in /etc/sysconfig/network-scripts/ has been removed1. Use the
ip addr show dev <dev_name>command for the iface existing check.Summary by CodeRabbit