-
Notifications
You must be signed in to change notification settings - Fork 361
get the bonding master from slave interface #6201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughA new private method Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (3.3.7)avocado/utils/network/interfaces.pyNote ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (52)
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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
avocado/utils/network/interfaces.py
Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 68.06% 68.04% -0.03%
==========================================
Files 205 205
Lines 22504 22511 +7
==========================================
Hits 15317 15317
- Misses 7187 7194 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The patch identifies and returns the bonding master for a slave interface. Signed-off-by: Vaishnavi Bhat <vaishnavi@linux.vnet.ibm.com>
fd99dd6 to
615261f
Compare
clebergnu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
The patch identifies and returns the bonding master for a slave interface.
Summary by CodeRabbit