Skip to content
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

Added SFP sensor discovery for Procurve devices #8746

Merged
merged 4 commits into from May 24, 2018

Conversation

angryp
Copy link
Contributor

@angryp angryp commented May 21, 2018

Procurve devices have SPF ports. If those are filled with SPF modules with DOM support, sensor data will be available through SNMP. Tested on J9775A 2530-48G switches.
Please bear with my coding. I'm far from programming and adjusted discoveries from Comware as a sample. Let me know if something should be adjusted.

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

Copy link
Member

@laf laf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a few comments in line. We normally would want this in yaml format but as you access some other data that would be silly to snmpwalk for now then we can leave it for now and convert this later when we can cache snmp data.

You have two blank lines in each of your new files, can you remove the second so only one line exists.

Also, we need test data for this: https://docs.librenms.org/#Developing/os/Test-Units/#example-workflow

$entPhysicalIndex_measured = 'ports';
foreach ($dbquery as $dbindex => $dbresult) {
$descr = makeshortif($dbresult['ifDescr']) . ' Port Bias Current';
discover_sensor($valid['sensor'], 'current', $device, $oid, 'bias-' . $index, 'procurve', $descr, $divisor, $multiplier, $limit_low, $warn_limit_low, $warn_limit, $limit, $current, 'snmp', $entPhysicalIndex, $entPhysicalIndex_measured);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change 'bias-' . $index for "hpicfXcvrBias.$index"

$entPhysicalIndex_measured = 'ports';
foreach ($dbquery as $dbindex => $dbresult) {
$descr = makeshortif($dbresult['ifDescr']) . ' Port Receive Power';
discover_sensor($valid['sensor'], 'dbm', $device, $oid, 'rx-' . $index, 'procurve', $descr, $divisor, $multiplier, $limit_low, $warn_limit_low, $warn_limit, $limit, $current, 'snmp', $entPhysicalIndex, $entPhysicalIndex_measured);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change 'rx-' . $index for "hpicfXcvrRxPower.$index"

$entPhysicalIndex_measured = 'ports';
foreach ($dbquery as $dbindex => $dbresult) {
$descr = makeshortif($dbresult['ifDescr']) . ' Port Transmit Power';
discover_sensor($valid['sensor'], 'dbm', $device, $oid, 'tx-' . $index, 'procurve', $descr, $divisor, $multiplier, $limit_low, $warn_limit_low, $warn_limit, $limit, $current, 'snmp', $entPhysicalIndex, $entPhysicalIndex_measured);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change 'tx-' . $index for "hpicfXcvrTxPower.$index"

'temperature',
$device,
$cur_oid . $tempindex,
'temp-' . $tempindex,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change 'temp-' . $tempindex for "hpicfXcvrTemp.$tempindex"

$entPhysicalIndex_measured = 'ports';
foreach ($dbquery as $dbindex => $dbresult) {
$descr = makeshortif($dbresult['ifDescr']) . ' Port Supply Voltage';
discover_sensor($valid['sensor'], 'voltage', $device, $oid, 'volt-' . $index, 'procurve', $descr, $divisor, $multiplier, $limit_low, $warn_limit_low, $warn_limit, $limit, $current, 'snmp', $entPhysicalIndex, $entPhysicalIndex_measured);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change 'volt-' . $index for "hpicfXcvrVoltage.$index"

@angryp
Copy link
Contributor Author

angryp commented May 22, 2018

That was painful, but I think the changes have been pushed in the end.

As per the tests, I followed "Additional module support or test data" part. Snmprec file has been populated, but the following steps were faulty - snmpsim threw out a ton of errors on "save-test-data" step and "pre-commit" step succeeded at first run, but then started to fail.
Pastebin output: https://pastebin.com/ddEzvBqL

@laf
Copy link
Member

laf commented May 23, 2018

I've added the json test data on this, should be good once tests passes.

@kkrumm1 kkrumm1 added Enhancement Device 🖥️ New or added device support labels May 24, 2018
@laf laf merged commit 1d4f662 into librenms:master May 24, 2018
angryp added a commit to angryp/librenms that referenced this pull request Jun 21, 2018
@angryp angryp mentioned this pull request Jun 21, 2018
1 task
laf pushed a commit that referenced this pull request Jun 21, 2018
This is a fix for #8746, which added support of SFP module discovery on Procurve devices. I noticed a lot of false positive alerts for current bias when we launched this in production, so looked once more through MIB/code. 

MIB data: 
hpicfXcvrBias - Tx bias current in microamps
hpicfXcvrBiasHiAlarm - Transceiver bias high alarm threshold limit in microamps
hpicfXcvrBiasLoAlarm - Transceiver bias low alarm threshold limit in microamps

Sensor discovery sample: 
hpicfXcvrBias.49 = 23254
hpicfXcvrBiasHiAlarm.49 = 100000
hpicfXcvrBiasLoAlarm.49 = 0
hpicfXcvrBiasHiWarn.49 = 90000
hpicfXcvrBiasLoWarn.49 = 100

So yes, my mistake was to use 10^-7 instead of 10^-6 divisor for these fields. Fix is in this pull request. Sorry for the inconveniences caused. 

DO NOT DELETE THIS TEXT

#### Please note

> Please read this information carefully. You can run `./scripts/pre-commit.php` to check your code before submitting.

- [ ] Have you followed our [code guidelines?](http://docs.librenms.org/Developing/Code-Guidelines/)

#### Testers

If you would like to test this pull request then please run: `./scripts/github-apply <pr_id>`, i.e `./scripts/github-apply 5926`
mattie47 pushed a commit to mattie47/librenms that referenced this pull request Jul 2, 2018
* Added SFP sensor discovery for Procurve devices

* Added SFP sensor discovery for Procurve devices

* Added json test data
mattie47 pushed a commit to mattie47/librenms that referenced this pull request Jul 2, 2018
This is a fix for librenms#8746, which added support of SFP module discovery on Procurve devices. I noticed a lot of false positive alerts for current bias when we launched this in production, so looked once more through MIB/code. 

MIB data: 
hpicfXcvrBias - Tx bias current in microamps
hpicfXcvrBiasHiAlarm - Transceiver bias high alarm threshold limit in microamps
hpicfXcvrBiasLoAlarm - Transceiver bias low alarm threshold limit in microamps

Sensor discovery sample: 
hpicfXcvrBias.49 = 23254
hpicfXcvrBiasHiAlarm.49 = 100000
hpicfXcvrBiasLoAlarm.49 = 0
hpicfXcvrBiasHiWarn.49 = 90000
hpicfXcvrBiasLoWarn.49 = 100

So yes, my mistake was to use 10^-7 instead of 10^-6 divisor for these fields. Fix is in this pull request. Sorry for the inconveniences caused. 

DO NOT DELETE THIS TEXT

#### Please note

> Please read this information carefully. You can run `./scripts/pre-commit.php` to check your code before submitting.

- [ ] Have you followed our [code guidelines?](http://docs.librenms.org/Developing/Code-Guidelines/)

#### Testers

If you would like to test this pull request then please run: `./scripts/github-apply <pr_id>`, i.e `./scripts/github-apply 5926`
@lock lock bot locked as resolved and limited conversation to collaborators Jul 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Device 🖥️ New or added device support Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants