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
Equallogic add disk status #6497
Conversation
contained in doc/General/Contributing.md.
…statuses as individual sensors.
Auto-Deploy finished, Test PR at http://6497.ci.librenms.org or https://6497.ci.librenms.org |
$member_id = $split_oid[(count($split_oid) - 2)]; | ||
$num_index = $member_id.'.'.$disk_index; | ||
$oid = $base_oid.$num_index; | ||
$extra = snmp_get_multi($device, $oid, '-OQne', 'EQLDISK-MIB', 'equallogic'); |
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.
You probably just want snmp_get here.
d_echo($extra); | ||
if (!empty($extra)) { | ||
list($foid,$pstatus) = explode(' = ', $extra, 2); | ||
$index = (100 + $index); |
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's probably better to use eqlDiskStatus.'.'.$disk_index here (need to also update create_sensor_to_state() as well. It will just stop the hack with adding 100 to the index).
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.
Should we do something similar to the power supply fetching as well?
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.
You can't change existing code as it will break graphs for people (rrd filename is based on the index).
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.
good point
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.
Can you give me a little more guidance here?
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.
Yeah I'd try $index = 'eqlDiskStatus.'.$disk_index;
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.
Ok; if we reassign $index
then which var in create_sensor_to_state()
do we need to update, the first or second one? I'm assuming one needs to be the actual index ID, and the other needs to be the more deliberate one? Should I just rename that to $disk_index
in that case?
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.
Actually you might not need to, give it a try.
d_echo($oids_disks."\n"); | ||
|
||
/* | ||
.1.3.6.1.4.1.12740.3.1.1.1.5.1.329840783.1 = RZ1wTmZ00980 |
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.
May as well remove this data.
|
||
if (!empty($oids_disks)) { | ||
/* | ||
eqlDiskStatus |
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.
May as well remove this as well.
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.
You mean remove the wrapping if
statement?
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.
No, the /* */ data - don't always copy what people have done before you :D
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.
👍 In that same vein, I'm also cleaning up all the $variable1 stuff and naming things appropriately. I got a little overzealous this morning I guess and didn't bother going in and cleaning things up like I normally would have.
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.
Can you unset() any variables you created so we don't hog memory (especially the snmp walk one)
is it OK to unset all the variables in a single |
Single unset is fine :)
|
Nice PR! |
I don't know what to do about the sorting; the OIDs are processed in order; the view is doing any sorting that happens. That seems like a separate bug to me relating to that view. (natural vs strict alphanumeric sorting) Not sure what happened to the status labels. They were still working on my side after the last batch of changes but i will double check this morning and see. |
Its still doing state translation for me...but it could be that they were already mapped previously. Here's the requested output:
|
Think I found it. Commit inbound. |
Auto-Deploy finished, Test PR at http://6497.ci.librenms.org or https://6497.ci.librenms.org |
I'll test the new changes at work tomorrow and report back. |
Auto-Deploy finished, Test PR at http://6497.ci.librenms.org or https://6497.ci.librenms.org |
The inspection completed: 1 new issues |
Thanks for adding this support :) |
I plan on adding additional things to accommodate our needs in the near future. Hopefully going to look at expanding snmp traps too. |
Removed the test PR, ran daily.sh then reran discovery and polling. Still not getting images as disk states and just 1 / 2's. |
Aaaand never mind. Just saw #6512. |
Yeah, you were correct with having to remove the translations though :) we are working out some quirks in 6512 |
This thread has been automatically locked since there has not been any recent activity after it was closed. |
DO NOT DELETE THIS TEXT
Please note
Testers
If you would like to test this pull request then please run:
./scripts/github-apply <pr_id>
, i.e./scripts/github-apply 5926
Adds individual disk statues to EqualLogic arrays. Should allow alerting for individual drive status changes.