-
Notifications
You must be signed in to change notification settings - Fork 546
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
[infiniband] collect info from all active ports of all IB devices #1262
[infiniband] collect info from all active ports of all IB devices #1262
Conversation
Thanks Hongang Li for initial version of the patch. |
I'm not really in favour of this approach if the root cause is a deficiency in the tools that we are calling. If this is confusing to users of |
I dont know if/how confusing the behaviour is, or if it is worth fallback behaviour when not providing options. See e.g. https://linux.die.net/man/8/infiniband-diags and "Multiple port/Multiple CA support:" paragraph. It makes sense to collect that info from all active ports - it is up to discussion if |
sos/plugins/infiniband.py
Outdated
s = open(IB_SYS_DIR + ca + "/ports/" + port + "/state") | ||
state = s.readline() | ||
s.close() | ||
except: |
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.
s
is not closed in the event of an exception in s.readline()
.
sos/plugins/infiniband.py
Outdated
p = open(IB_SYS_DIR + ca + "/ports/" + port + | ||
"/link_layer") | ||
link_layer = p.readline() | ||
p.close() |
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.
p
is not closed in the event of an exception in p.readline()
.
It just seems like a bad design choice in these tools: pretty much all system level tools of this kind that I can think of defaults to printing information for all devices when no option is given:
What I really dislike here is the need to poke around If there's no choice but to work around this in |
(but the exception handling in the current proposed patch does need to be fixed). |
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.
Let me take a look at this tomorrow: I don't think we'll see changes in the upstream IB tools given how longstanding this behaviour is (although I still think that it is a defect: it's crazy to not give users a simple way to list the devices and ports on their machine).
currently just the very first active port is checked Resolves: sosreport#1262 Signed-off-by: Pavel Moravec <[email protected]>
40de592
to
b5b9965
Compare
currently just the very first active port is checked Resolves: sosreport#1262 Signed-off-by: Pavel Moravec <[email protected]> Signed-off-by: Bryn M. Reeves <[email protected]>
currently just the very first active port is checked
Resolves: #1262
Signed-off-by: Pavel Moravec [email protected]
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines