-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
feature: HPE Comware Processor Discovery #6029
Conversation
Auto-Deploy finished, Test PR at http://6029.ci.librenms.org or https://6029.ci.librenms.org |
Auto-Deploy finished, Test PR at http://6029.ci.librenms.org or https://6029.ci.librenms.org |
@Rosiak I was wondering if we could use the entPhysicalData from the database, soooo, I re-wrote this. Do you like this better?
|
if ($index == $procindex) { | ||
$cur_oid = '.1.3.6.1.4.1.25506.2.6.1.1.1.1.6.'; | ||
|
||
discover_processor($valid['processor'], $device, $cur_oid . $procindex, $procindex, 'comware', 'Slot ' . $x, '1', $value['h3cEntityExtCpuUsage']); |
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 be hh3cEntityExtCpuUsage
Perfectly fine with that @murrant ! |
Auto-Deploy finished, Test PR at http://6029.ci.librenms.org or https://6029.ci.librenms.org |
lol, I'm against that personally as per the other PR which did the same. Doing it this way requires entity-physical discovery module to be enabled and have run before this (admittedly after the second run it won't matter if entity-physical runs first or not). However the dependancy on another module imho is a no no as you're restricting users who have sensors enabled because they want to see them but not entity-physical. Other discovery modules which require entityphysical info do another walk afaik - which if we can get the updated snmp Class finished and merged won't be an issue as the results will be cached. |
@laf I specifically made a fallback so this doesn't rely on the entity-physical module, only takes advantage of it if it exists. |
Yeah I didn't realise that until you pointed it out as I'd only read the reply on email where it said you rewritten it to pull from DB. As you pointed out in irc, this still does mean that someone could have stale data if entity-physical was disabled 6 months ago as an example. It's not a huge concern but does still swing me more to why bother checking the DB if we can't be sure when that data was from. |
Auto-Deploy finished, Test PR at http://6029.ci.librenms.org or https://6029.ci.librenms.org |
The inspection completed: No new issues |
As it's only disco job, I'm not concerned regarding the "extra" snmp req as this not that "expensive" at this point. |
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