get the bonding master from slave interface - #6201
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:
WalkthroughThe change adds public Changes
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
avocado/utils/network/interfaces.py(1 hunks)
⏰ 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). (42)
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: Egg task ubuntu:22.04
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Egg task fedora:40
- GitHub Check: Egg task fedora:41
- GitHub Check: Podman spawner with 3rd party runner plugin
- GitHub Check: Egg task debian:11.0
- GitHub Check: Version task ubuntu:20.04
- GitHub Check: Fedora develop install/uninstall task
- GitHub Check: Fedora selftests
- GitHub Check: Build Package (wheel/tarball) for Python 3.9
- GitHub Check: macOS with Python 3.11
- GitHub Check: Version task ubi:8.8
- GitHub Check: Version task debian:12.4
- GitHub Check: Smokecheck on Linux with Python 3.10
- GitHub Check: Smokecheck on Linux with Python 3.11
- GitHub Check: Static checks
- GitHub Check: Code Coverage (3.11)
- GitHub Check: Analyze (python)
| def _get_bondingmaster(self, slave): | ||
| cmd = ( | ||
| f"/sys/class/net/{self.name}/master " | ||
| ) | ||
| try: | ||
| bond_master = run_command(cmd, self.host).splitlines() | ||
| return bond_master | ||
| except Exception as exc: | ||
| raise NWException( | ||
| f"Bonding master not found for " f"the interface {self.name}" | ||
| ) from exc |
There was a problem hiding this comment.
Critical: Fix the malformed command and parameter usage.
The method has several critical issues:
- Missing
catcommand: The command is malformed and will fail because it's missing thecatcommand to read the file. - Unused parameter: The
slaveparameter is accepted but never used. Based on the method name and PR objective, this should read the master of the given slave interface. - Trailing space: Unnecessary trailing space in the command string.
Apply this diff to fix the issues:
- def _get_bondingmaster(self, slave):
+ def _get_bondingmaster(self, slave):
cmd = (
- f"/sys/class/net/{self.name}/master "
+ f"cat /sys/class/net/{slave}/master"
)
try:
bond_master = run_command(cmd, self.host).splitlines()
return bond_master
except Exception as exc:
raise NWException(
- f"Bonding master not found for " f"the interface {self.name}"
+ f"Bonding master not found for the interface {slave}"
) from exc📝 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.
| def _get_bondingmaster(self, slave): | |
| cmd = ( | |
| f"/sys/class/net/{self.name}/master " | |
| ) | |
| try: | |
| bond_master = run_command(cmd, self.host).splitlines() | |
| return bond_master | |
| except Exception as exc: | |
| raise NWException( | |
| f"Bonding master not found for " f"the interface {self.name}" | |
| ) from exc | |
| def _get_bondingmaster(self, slave): | |
| cmd = ( | |
| f"cat /sys/class/net/{slave}/master" | |
| ) | |
| try: | |
| bond_master = run_command(cmd, self.host).splitlines() | |
| return bond_master | |
| except Exception as exc: | |
| raise NWException( | |
| f"Bonding master not found for the interface {slave}" | |
| ) from exc |
🤖 Prompt for AI Agents
In avocado/utils/network/interfaces.py around lines 138 to 148, the
_get_bondingmaster method has a malformed command missing the 'cat' to read the
file, an unused 'slave' parameter, and an unnecessary trailing space in the
command string. Fix this by updating the command to include 'cat' and use the
'slave' parameter instead of 'self.name' to read the master of the given slave
interface. Also, remove the trailing space from the command string to ensure it
is correctly formed.
6ccb12a to
fd99dd6
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6201 +/- ##
==========================================
- Coverage 71.74% 70.92% -0.82%
==========================================
Files 207 207
Lines 23624 23636 +12
==========================================
- Hits 16949 16765 -184
- Misses 6675 6871 +196 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
fd99dd6 to
615261f
Compare
clebergnu
left a comment
clebergnu
left a comment
There was a problem hiding this comment.
Hi @vaishnavibhat ,
Besides the comment made by CodeRabbit (the lack of an actual command to get the content of the file), it's not clear to me how this API is supposed to be consumed. Based on its name, it seems to be intended to be a private method. Can you please elaborate?
615261f to
1f11933
Compare
1f11933 to
549677f
Compare
Hi @clebergnu This method will be later used in the avocado-misc-tests to get the bond (master) name for the tests. Thank you. |
|
@clebergnu Hope Vaishnavi helped with your query to understand. |
|
Hi @PraveenPenguin @clebergnu Can you please help me with the review of this patch ? |
|
@clebergnu can you please merge this if no more review comments. this is blocking our test, as we have developed the tests using this utils. immediate help is really appreciated. |
|
Hi @vaishnavibhat I am having trouble understanding the use of a private method in a project from another project. Perhaps it is a better idea to add this private method in the avocado misc tests repo? Adding a private method meant to be used from a different project violates crucial information hiding and coupling guidelines and between making this function public and using it privately I would likely prefer using it privately where it belongs, reason being its somewhat niche use. |
The patch identifies and returns the bonding master for a slave interface. Signed-off-by: Vaishnavi Bhat <vaishnavi@linux.ibm.com>
549677f to
4f55269
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
avocado/utils/network/interfaces.py (1)
774-774: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSplit
nmclivalues by line.
nmcli -g ip4.ADDRESS device showreturns one IPv4 address per line. With multiple addresses,split(" | ")does not separate them, so the generated command passes multiple addresses to oneip addr deleteinvocation. Usesplitlines().🤖 Prompt for 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. In `@avocado/utils/network/interfaces.py` at line 774, Update the `ipaddresses` parsing in the network interface flow to use `splitlines()` on the `nmcli` output, ensuring each IPv4 address becomes a separate entry before generating deletion commands.
🤖 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 `@avocado/utils/network/interfaces.py`:
- Line 230: Quote every dynamic shell operand with shlex.quote() before
constructing commands in avocado/utils/network/interfaces.py: quote master_path
at lines 230-230 for readlink, self.name at lines 769-770 for nmcli, self.name
at lines 831-832 for nmcli connection up, and self.name at lines 836-837 for ip
link set; update the relevant command-building methods without changing their
behavior.
- Around line 233-234: Update the master-resolution method around bond_name so
it verifies the resolved master is a bonding device before returning it; return
no bond result for bridge, VRF, or other master types, using the existing
bonding-device detection mechanism.
- Around line 820-832: Update restore_from_backup() to detect the host
distribution and set distro_is_rhel9_or_later and distro_is_suse16_or_later
before computing backup_file. Ensure backup selection and the subsequent
NetworkManager versus ip link activation path use the detected flags, including
existing .nmconnection.backup files.
- Around line 834-837: Update the older-distribution branch in the interface
activation method containing self.name to use the legacy configuration
activation command ifup instead of ip link set dev ... up, so restored ifcfg
configuration—including addresses, routes, and bonding settings—is applied.
- Around line 803-811: Update the docstring for restore_from_backup() to state
that it restores the backup network configuration file to self.config_filename,
replacing the incorrect claim that it copies the file to a /sysfs path. Preserve
the existing interface bring-up and exception documentation.
---
Outside diff comments:
In `@avocado/utils/network/interfaces.py`:
- Line 774: Update the `ipaddresses` parsing in the network interface flow to
use `splitlines()` on the `nmcli` output, ensuring each IPv4 address becomes a
separate entry before generating deletion commands.
🪄 Autofix
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 Plus
Run ID: 2a13e146-8f6f-4bf0-9011-9912c3dbc7b5
📒 Files selected for processing (1)
avocado/utils/network/interfaces.py
| """Revert interface file from backup and bring the interface up. | ||
|
|
||
| This method checks if a backup version is available for given | ||
| This method checks if a backup version is available for given | ||
| interface then it copies backup file to interface file in /sysfs path. | ||
| After restoration, it automatically brings the interface up using either | ||
| NetworkManager (nmcli) for RHEL9+/SuSE16+ or ip link command for older systems. | ||
|
|
||
| :raises avocado.utils.network.exceptions.NWException: If the backup file is not available. | ||
| :raises avocado.utils.network.exceptions.NWException: If the backup file is not available | ||
| or if bringing the interface up fails. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the restore documentation.
restore_from_backup() moves the backup to self.config_filename. It does not copy a file to a /sysfs path. State that it restores the network configuration file.
🤖 Prompt for 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.
In `@avocado/utils/network/interfaces.py` around lines 803 - 811, Update the
docstring for restore_from_backup() to state that it restores the backup network
configuration file to self.config_filename, replacing the incorrect claim that
it copies the file to a /sysfs path. Preserve the existing interface bring-up
and exception documentation.
| # Bring the interface up based on network service | ||
| try: | ||
| if self.distro_is_rhel9_or_later or self.distro_is_suse16_or_later: | ||
| # Use NetworkManager for modern distributions | ||
| LOG.info(f"Bringing up interface {self.name} using NetworkManager") | ||
|
|
||
| # Reload all connections to pick up restored configuration | ||
| reload_cmd = "nmcli connection reload" | ||
| run_command(reload_cmd, self.host, sudo=True) | ||
|
|
||
| # Bring up the specific connection | ||
| up_cmd = f"nmcli connection up {self.name}" | ||
| run_command(up_cmd, self.host, sudo=True) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Detect the distribution before selecting the backup path and activation command.
A new NetworkInterface starts with both distribution flags set to False. The flags are only set by save(). Therefore, restore_from_backup() first looks for a legacy ifcfg backup and then uses ip link on RHEL 9+ and SuSE 16+, even when a .nmconnection.backup exists. Detect the distribution, set the flags, and then compute backup_file.
🤖 Prompt for 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.
In `@avocado/utils/network/interfaces.py` around lines 820 - 832, Update
restore_from_backup() to detect the host distribution and set
distro_is_rhel9_or_later and distro_is_suse16_or_later before computing
backup_file. Ensure backup selection and the subsequent NetworkManager versus ip
link activation path use the detected flags, including existing
.nmconnection.backup files.
| # Use ip link for older distributions | ||
| LOG.info(f"Bringing up interface {self.name} using ip link") | ||
| up_cmd = f"ip link set dev {self.name} up" | ||
| run_command(up_cmd, self.host, sudo=True) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Activate the restored legacy configuration.
ip link set dev <interface> up only sets the link state. It does not load the restored ifcfg-* configuration, including addresses, routes, and bonding settings. Use the legacy network configuration activation command, such as ifup, for RHEL 8 and older and legacy SuSE systems.
🤖 Prompt for 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.
In `@avocado/utils/network/interfaces.py` around lines 834 - 837, Update the
older-distribution branch in the interface activation method containing
self.name to use the legacy configuration activation command ifup instead of ip
link set dev ... up, so restored ifcfg configuration—including addresses,
routes, and bonding settings—is applied.
The patch identifies and returns the bonding master for a slave interface. Signed-off-by: Vaishnavi Bhat <vaishnavi@linux.ibm.com>
4f55269 to
06514c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
avocado/utils/network/interfaces.py (1)
719-723: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winValidate
mtubefore executing the command.Line 719 interpolates
mtudirectly into the shell command. Line 722 converts it only afterrun_command()returns. An input such as"1500;..."can execute shell syntax before validation. Convertmtuto an integer before constructingcmd, then compare the validated value.🤖 Prompt for 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. In `@avocado/utils/network/interfaces.py` around lines 719 - 723, Validate and convert mtu to an integer before constructing or executing the command in the link MTU update flow. Use that validated integer for shell-command interpolation and for the post-command comparison in the surrounding interface method, preventing unvalidated input from reaching run_command.
🧹 Nitpick comments (1)
avocado/utils/network/interfaces.py (1)
221-242: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd focused tests for the new public method.
Cover a bond master, a non-bond master returning
None, areadlinkfailure raisingNWException, and shell-quoted interface names. Mockrun_command()andis_bond()so the tests do not require a live bond.🤖 Prompt for 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. In `@avocado/utils/network/interfaces.py` around lines 221 - 242, Add focused unit tests for NetworkInterface.get_bond_master covering a valid bond master, a non-bond master returning None, run_command/readlink failures being wrapped as NWException, and shell-quoted interface names. Mock run_command and NetworkInterface.is_bond so the tests remain independent of live network bonding.
🤖 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 `@avocado/utils/network/interfaces.py`:
- Around line 221-229: Update the get_bond_master() docstring to document that
it returns None when the interface is not a bond slave or no bonding master is
found, and add the corresponding “:rtype: str or None” declaration while
preserving the existing exception documentation.
- Around line 235-237: Complete shell quoting for the remaining dynamic
network-interface operands. Update is_bond() to quote self.name in the
/proc/net/bonding command, quote self.name in both nmcli c mod save branches,
and quote ipaddr and self.name in nm_flush_ipaddr() using shlex.quote(); add the
import if needed.
---
Outside diff comments:
In `@avocado/utils/network/interfaces.py`:
- Around line 719-723: Validate and convert mtu to an integer before
constructing or executing the command in the link MTU update flow. Use that
validated integer for shell-command interpolation and for the post-command
comparison in the surrounding interface method, preventing unvalidated input
from reaching run_command.
---
Nitpick comments:
In `@avocado/utils/network/interfaces.py`:
- Around line 221-242: Add focused unit tests for
NetworkInterface.get_bond_master covering a valid bond master, a non-bond master
returning None, run_command/readlink failures being wrapped as NWException, and
shell-quoted interface names. Mock run_command and NetworkInterface.is_bond so
the tests remain independent of live network bonding.
🪄 Autofix
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 Plus
Run ID: 680ff283-6eeb-4f8a-bc67-8eac34efb309
📒 Files selected for processing (1)
avocado/utils/network/interfaces.py
The patch identifies and returns the bonding master for a slave interface. Signed-off-by: Vaishnavi Bhat <vaishnavi@linux.ibm.com>
06514c7 to
f25a3f4
Compare
The patch identifies and returns the bonding master for a slave interface. Signed-off-by: Vaishnavi Bhat <vaishnavi@linux.ibm.com>
f25a3f4 to
aa901c2
Compare
|
For the static failures: |
5 participants
The patch identifies and returns the bonding master for a slave interface.
Summary by CodeRabbit