-
Notifications
You must be signed in to change notification settings - Fork 336
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
T6764: Fix unhandled exception on ethtool output parsing for Xen NICs #4182
Conversation
👍 |
@@ -101,8 +101,9 @@ def __init__(self, ifname): | |||
self._features = loads(out)[0] | |||
|
|||
# Get information about NIC ring buffers | |||
out, _ = popen(f'ethtool --json --show-ring {ifname}') | |||
self._ring_buffer = loads(out)[0] | |||
out, err = popen(f'ethtool --json --show-ring {ifname}') |
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.
My only concern here: can we differentiate between an expected failure for Xen NICs and similar and an unexpected failure where --show-ring
is supposed to work? This approach may hide bugs.
I'm not against merging it as an immediate fix.
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.
It depends on what is "error" from our point of view:
- unexpected
ethtool
output - a feature is not supported by a NIC
- both
I have an internal feeling that for our purposes a reason why it failed does not matter, but input from someone deeply familiar with interface config logic is welcomed.
Not all NICs could provide ring-buffers info requested by ethtool in JSON format For example 'vif' Xen/XCP-NG interfaces Fix it
CI integration 👍 passed! Details
|
Change Summary
Not all NICs could provide ring-buffers info requested by ethtool in JSON format
For example 'vif' Xen/XCP-NG interfaces
Fix it
Types of changes
Related Task(s)
Related PR(s)
Component(s) name
ethtool
Proposed changes
How to test
XEN/XCP-ng
vif
before the fix:After the fix configuration applies without errors
Smoketest result
Checklist: