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

Add TiMOS temperature and power supply state sensors. #6657

Merged
merged 7 commits into from May 22, 2017

Conversation

Projects
None yet
5 participants
@peelman
Contributor

peelman commented May 16, 2017

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

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

peelman added some commits May 16, 2017

Discover TiMOS device serial number
Pull proper hardware OID instead of parsing sysDescr
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
Show outdated Hide outdated includes/discovery/sensors/state/timos.inc.php
@@ -0,0 +1,79 @@
<?php
$power_supply_table = end(snmpwalk_cache_numerical_oid($device, 'tmnxChassisPowerSupplyTable', $power_supply_table = array(), 'TIMETRA-CHASSIS-MIB', 'aos', '-OQUsn'));

This comment has been minimized.

@laf

laf May 16, 2017

Member

What's going on here?

@laf

laf May 16, 2017

Member

What's going on here?

This comment has been minimized.

@peelman

peelman May 17, 2017

Contributor

This can probably be refactored; it was being used to verify what was being grabbed, but then I ended up reusing the table for the population. We can probably replace it with a direct snmp_get in the loop below.

@peelman

peelman May 17, 2017

Contributor

This can probably be refactored; it was being used to verify what was being grabbed, but then I ended up reusing the table for the population. We can probably replace it with a direct snmp_get in the loop below.

This comment has been minimized.

@peelman

peelman May 18, 2017

Contributor

@laf while looking at this this morning, wouldn't it really be more efficient to just grab the table rather than issuing X number of individual snmp_get's on each device?

@peelman

peelman May 18, 2017

Contributor

@laf while looking at this this morning, wouldn't it really be more efficient to just grab the table rather than issuing X number of individual snmp_get's on each device?

Show outdated Hide outdated includes/discovery/sensors/state/timos.inc.php
);
foreach ($power_supply_oids as $data) {
$full_oid = $base_oid . "." . $data['sub_oid'];

This comment has been minimized.

@laf

laf May 16, 2017

Member

You should just add the trailing . to $base_oid to make this easier to read.

@laf

laf May 16, 2017

Member

You should just add the trailing . to $base_oid to make this easier to read.

This comment has been minimized.

@peelman

peelman May 17, 2017

Contributor

good call.

@peelman

peelman May 17, 2017

Contributor

good call.

Show outdated Hide outdated includes/discovery/sensors/temperature/timos.inc.php
@@ -0,0 +1,30 @@
<?php
if ($device['os'] === 'timos') {

This comment has been minimized.

@laf

laf May 16, 2017

Member

You don't need this check

@laf

laf May 16, 2017

Member

You don't need this check

This comment has been minimized.

@peelman

peelman May 17, 2017

Contributor

This is another one of those where its in so many things and in the examples, its hard to know what is necessary and what isn't.

@peelman

peelman May 17, 2017

Contributor

This is another one of those where its in so many things and in the examples, its hard to know what is necessary and what isn't.

Show outdated Hide outdated includes/discovery/sensors/temperature/timos.inc.php
<?php
if ($device['os'] === 'timos') {
$oids = snmp_walk($device, 'tmnxHwID', '-Osqn', 'TIMETRA-CHASSIS-MIB', 'aos');

This comment has been minimized.

@laf

laf May 16, 2017

Member

Does one of the snmpwalk_* functions not do what you want here?

@laf

laf May 16, 2017

Member

Does one of the snmpwalk_* functions not do what you want here?

This comment has been minimized.

@peelman

peelman May 17, 2017

Contributor

Quite possibly; but the context and use cases of the derivatives aren't made very clear.

@peelman

peelman May 17, 2017

Contributor

Quite possibly; but the context and use cases of the derivatives aren't made very clear.

This comment has been minimized.

@peelman

peelman May 18, 2017

Contributor

I think I got it, after a little tinkering and poking around at some of the other includes. It does result in a much cleaner, simpler bit of code. 👍

@peelman

peelman May 18, 2017

Contributor

I think I got it, after a little tinkering and poking around at some of the other includes. It does result in a much cleaner, simpler bit of code. 👍

Show outdated Hide outdated includes/polling/os/timos.inc.php
//All rights reserved. All use subject to applicable license agreements.
//Built on Thu Jan 5 11:01:16 IST 2017 by builder in /home/builder/9.0B1/R3/panos/main
$chassis_type_name_array = snmpwalk_cache_oid($device, 'tmnxChassisTypeName', $a = array(), 'TIMETRA-CHASSIS-MIB', 'aos', '-OQUs');
$hardware = end(reset($chassis_type_name_array));

This comment has been minimized.

@laf

laf May 16, 2017

Member

Why is this being done, how big is the snmpwalk?

@laf

laf May 16, 2017

Member

Why is this being done, how big is the snmpwalk?

This comment has been minimized.

@peelman

peelman May 17, 2017

Contributor

To fetch the actual chassis model and type. without resorting to a regex of the sysDescr (which wasn't working on v9 of SROS, but was on v8)

I almost used tmnxChassisTypeDescription to populate $features but decided against it.

On a SAS-K the walk returns two nodes:

snmpwalk -M /usr/share/snmp/mibs/:Code/public_projects/librenms/mibs:Code/SNMP-mibs/nokia -m +TIMETRA-PORT-MIB -v2c -c public -OQun 10.1.86.31 tmnxChassisTypeName
.1.3.6.1.4.1.6527.3.1.2.2.1.6.1.2.20 = "7210 SAS-K 2F2T1C-1"
.1.3.6.1.4.1.6527.3.1.2.2.1.6.1.2.36 = "7210 VC"

If they ever get our SAS-T's powered, I'll test against those too; but I don't expect them to return a lot of additional stuff, based on the documentation in the MIB.

The .20 and .36 endpoints are not readily identifiable through another direct get that I can find, so a narrow walk and reading the first object in the array seemed like the best path forward.

@peelman

peelman May 17, 2017

Contributor

To fetch the actual chassis model and type. without resorting to a regex of the sysDescr (which wasn't working on v9 of SROS, but was on v8)

I almost used tmnxChassisTypeDescription to populate $features but decided against it.

On a SAS-K the walk returns two nodes:

snmpwalk -M /usr/share/snmp/mibs/:Code/public_projects/librenms/mibs:Code/SNMP-mibs/nokia -m +TIMETRA-PORT-MIB -v2c -c public -OQun 10.1.86.31 tmnxChassisTypeName
.1.3.6.1.4.1.6527.3.1.2.2.1.6.1.2.20 = "7210 SAS-K 2F2T1C-1"
.1.3.6.1.4.1.6527.3.1.2.2.1.6.1.2.36 = "7210 VC"

If they ever get our SAS-T's powered, I'll test against those too; but I don't expect them to return a lot of additional stuff, based on the documentation in the MIB.

The .20 and .36 endpoints are not readily identifiable through another direct get that I can find, so a narrow walk and reading the first object in the array seemed like the best path forward.

Show outdated Hide outdated includes/polling/os/timos.inc.php
if ($matches['hardware']) {
$hardware = $matches['hardware'];
$props = snmpwalk_cache_numerical_oid($device, 'tmnxHwEntry.7', $props = array(), 'TIMETRA-CHASSIS-MIB', 'aos', '-OQne');

This comment has been minimized.

@laf

laf May 16, 2017

Member

How big is this snmpwalk?

@laf

laf May 16, 2017

Member

How big is this snmpwalk?

This comment has been minimized.

@peelman

peelman May 17, 2017

Contributor

Relatively small, but will vary depending on the device. On a SAS-K:

snmpwalk -M /usr/share/snmp/mibs/:Code/public_projects/librenms/mibs:Code/SNMP-mibs/nokia -m +TIMETRA-CHASSIS-MIB -v2c -c public -OQun 10.1.86.31 tmnxHwEntry.7
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.50331649 = physChassis
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.83886081 = powerSupply
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.134217729 = ioModule
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.150994977 = cpmModule
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.167772162 = fabricModule
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.184549633 = mdaModule
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.201327105 = flashDiskModule
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.201327106 = flashDiskModule

Again, I can find no way to otherwise identify those IDs and I don't yet have a broad enough pool of devices to test against to see if there is any significant consistency to them. So walking them and looking for the one identified as "3" (PhysicalChassis/Chassis, depends on the MIB version which is returned) was the best solution I could concoct.

@peelman

peelman May 17, 2017

Contributor

Relatively small, but will vary depending on the device. On a SAS-K:

snmpwalk -M /usr/share/snmp/mibs/:Code/public_projects/librenms/mibs:Code/SNMP-mibs/nokia -m +TIMETRA-CHASSIS-MIB -v2c -c public -OQun 10.1.86.31 tmnxHwEntry.7
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.50331649 = physChassis
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.83886081 = powerSupply
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.134217729 = ioModule
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.150994977 = cpmModule
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.167772162 = fabricModule
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.184549633 = mdaModule
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.201327105 = flashDiskModule
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.201327106 = flashDiskModule

Again, I can find no way to otherwise identify those IDs and I don't yet have a broad enough pool of devices to test against to see if there is any significant consistency to them. So walking them and looking for the one identified as "3" (PhysicalChassis/Chassis, depends on the MIB version which is returned) was the best solution I could concoct.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier May 18, 2017

The inspection completed: No new issues

The inspection completed: No new issues

@peelman

This comment has been minimized.

Show comment
Hide comment
@peelman

peelman May 18, 2017

Contributor

What say you now @laf?

Contributor

peelman commented May 18, 2017

What say you now @laf?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 19, 2017

Member

The two biggest things for me in this is us doing snmpwalks in polling for two simple items. What data do they provide back?

Member

laf commented May 19, 2017

The two biggest things for me in this is us doing snmpwalks in polling for two simple items. What data do they provide back?

@peelman

This comment has been minimized.

Show comment
Hide comment
@peelman

peelman May 22, 2017

Contributor

$chassis_type_name_array

08:35:49 npeelman@npeelman-mini:[~] > snmpwalk -M /usr/share/snmp/mibs/:Code/public_projects/librenms/mibs:Code/SNMP-mibs/nokia -m +TIMETRA-PORT-MIB -v2c -c hobbit7 -OQun 10.1.86.31 tmnxChassisTypeName
.1.3.6.1.4.1.6527.3.1.2.2.1.6.1.2.20 = "7210 SAS-K 2F2T1C-1"
.1.3.6.1.4.1.6527.3.1.2.2.1.6.1.2.36 = "7210 VC"

From the MIB:

tmnxChassisTypeName     OBJECT-TYPE
    SYNTAX      TNamedItemOrEmpty
    MAX-ACCESS  read-only
    STATUS      current
    DESCRIPTION
        "The administrative name that identifies this type of Alcatel
         7x50 SR series chassis model.  This name string may be used in 
         CLI commands to specify a particular chassis model type."
    ::= { tmnxChassisTypeEntry 2 }

They do not specify what comes after the .2; so there isn't a way I am comfortable with saying that it will always be .20 as the endpoint containing the chassis type.

$props

08:37:44 npeelman@npeelman-mini:[~] > snmpwalk -M /usr/share/snmp/mibs/:Code/public_projects/librenms/mibs:Code/SNMP-mibs/nokia -m +TIMETRA-PORT-MIB -v2c -c hobbit7 -OQun 10.1.86.31 tmnxHwEntry.7
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.50331649 = physChassis
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.83886081 = powerSupply
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.134217729 = ioModule
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.150994977 = cpmModule
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.167772162 = fabricModule
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.184549633 = mdaModule
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.201327105 = flashDiskModule
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.201327106 = flashDiskModule

I have walked the entire tree offered by our devices (SAS-K and SAS-T. There is no simpler, faster, or less intensive way to find 50331649; if I could guarantee that 50331649 would always be the physChassis node, I would just skip this and call it directly. And if that's the path you want to take, I will make that change and any bugs discovered down the line can be dealt with on a case-by-case basis. But this seemed to be the best compromise between longevity, simplicity, and speed.

Contributor

peelman commented May 22, 2017

$chassis_type_name_array

08:35:49 npeelman@npeelman-mini:[~] > snmpwalk -M /usr/share/snmp/mibs/:Code/public_projects/librenms/mibs:Code/SNMP-mibs/nokia -m +TIMETRA-PORT-MIB -v2c -c hobbit7 -OQun 10.1.86.31 tmnxChassisTypeName
.1.3.6.1.4.1.6527.3.1.2.2.1.6.1.2.20 = "7210 SAS-K 2F2T1C-1"
.1.3.6.1.4.1.6527.3.1.2.2.1.6.1.2.36 = "7210 VC"

From the MIB:

tmnxChassisTypeName     OBJECT-TYPE
    SYNTAX      TNamedItemOrEmpty
    MAX-ACCESS  read-only
    STATUS      current
    DESCRIPTION
        "The administrative name that identifies this type of Alcatel
         7x50 SR series chassis model.  This name string may be used in 
         CLI commands to specify a particular chassis model type."
    ::= { tmnxChassisTypeEntry 2 }

They do not specify what comes after the .2; so there isn't a way I am comfortable with saying that it will always be .20 as the endpoint containing the chassis type.

$props

08:37:44 npeelman@npeelman-mini:[~] > snmpwalk -M /usr/share/snmp/mibs/:Code/public_projects/librenms/mibs:Code/SNMP-mibs/nokia -m +TIMETRA-PORT-MIB -v2c -c hobbit7 -OQun 10.1.86.31 tmnxHwEntry.7
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.50331649 = physChassis
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.83886081 = powerSupply
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.134217729 = ioModule
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.150994977 = cpmModule
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.167772162 = fabricModule
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.184549633 = mdaModule
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.201327105 = flashDiskModule
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.7.1.201327106 = flashDiskModule

I have walked the entire tree offered by our devices (SAS-K and SAS-T. There is no simpler, faster, or less intensive way to find 50331649; if I could guarantee that 50331649 would always be the physChassis node, I would just skip this and call it directly. And if that's the path you want to take, I will make that change and any bugs discovered down the line can be dealt with on a case-by-case basis. But this seemed to be the best compromise between longevity, simplicity, and speed.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 22, 2017

Member

What else is in 1.3.6.1.4.1.6527.3.1.2.2.1.8.1.5.1?

Member

laf commented May 22, 2017

What else is in 1.3.6.1.4.1.6527.3.1.2.2.1.8.1.5.1?

@peelman

This comment has been minimized.

Show comment
Hide comment
@peelman

peelman May 22, 2017

Contributor

It returns a table with serial numbers for all components (including empty strings for those without serial numbers).

09:27:27 npeelman@npeelman-mini:[~] > ] > snmpwalk -M /usr/share/snmp/mibs/:Code/public_prts/librenms/mibs:Code/SNMP-mibs/nokia -m +TIMETRA-PORT-MIB -v2c -c hobbit7 -OQun 10.1.86.31 .1.3.6.1.4.1.6527.3.1.2.2.1.8.1.5.1
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.5.1.50331649 = NS1530C0361
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.5.1.83886081 = 
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.5.1.134217729 = NS1530C0361
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.5.1.150994977 = NS1530C0361
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.5.1.167772162 = 
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.5.1.184549633 = NS1530C0361
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.5.1.201327105 = 
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.5.1.201327106 = 
Contributor

peelman commented May 22, 2017

It returns a table with serial numbers for all components (including empty strings for those without serial numbers).

09:27:27 npeelman@npeelman-mini:[~] > ] > snmpwalk -M /usr/share/snmp/mibs/:Code/public_prts/librenms/mibs:Code/SNMP-mibs/nokia -m +TIMETRA-PORT-MIB -v2c -c hobbit7 -OQun 10.1.86.31 .1.3.6.1.4.1.6527.3.1.2.2.1.8.1.5.1
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.5.1.50331649 = NS1530C0361
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.5.1.83886081 = 
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.5.1.134217729 = NS1530C0361
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.5.1.150994977 = NS1530C0361
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.5.1.167772162 = 
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.5.1.184549633 = NS1530C0361
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.5.1.201327105 = 
.1.3.6.1.4.1.6527.3.1.2.2.1.8.1.5.1.201327106 = 
@laf

laf approved these changes May 22, 2017

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 22, 2017

Member

I'm not 100% happy about merging this but I guess we'll just see if it has any impact for people.

Polling should be as lite as possible so walking unnecessary trees and even single OIDs should always be avoided.

Member

laf commented May 22, 2017

I'm not 100% happy about merging this but I guess we'll just see if it has any impact for people.

Polling should be as lite as possible so walking unnecessary trees and even single OIDs should always be avoided.

@laf laf merged commit 77c5b1e into librenms:master May 22, 2017

3 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 22, 2017

Member

Thanks for contributing the changes though :)

Member

laf commented May 22, 2017

Thanks for contributing the changes though :)

@peelman

This comment has been minimized.

Show comment
Hide comment
@peelman

peelman May 22, 2017

Contributor

I don't disagree. Were it up to me, setting the properties of the device like $hardware, $features, $version, etc. would be done in Discovery anyway, not at every polling interval.

Contributor

peelman commented May 22, 2017

I don't disagree. Were it up to me, setting the properties of the device like $hardware, $features, $version, etc. would be done in Discovery anyway, not at every polling interval.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 22, 2017

Member

@peelman That's certainly something that can be moved and I'd be happy about it. We would have to check that nothing else relied on it but it shouldn't.

Things like that change so infrequently anyway - @murrant?

Member

laf commented May 22, 2017

@peelman That's certainly something that can be moved and I'd be happy about it. We would have to check that nothing else relied on it but it shouldn't.

Things like that change so infrequently anyway - @murrant?

@peelman

This comment has been minimized.

Show comment
Hide comment
@peelman

peelman May 22, 2017

Contributor

Would it be as simple as moving it from polling/os to discovery/os? Or are there other moving pieces there?

Contributor

peelman commented May 22, 2017

Would it be as simple as moving it from polling/os to discovery/os? Or are there other moving pieces there?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 22, 2017

Member

It's not as simple as that no :(

Some OS polling does other things including basing the checks on group which won't have been set first time around.

Member

laf commented May 22, 2017

It's not as simple as that no :(

Some OS polling does other things including basing the checks on group which won't have been set first time around.

@peelman peelman deleted the peelman:feature/timos-sensors branch May 23, 2017

@peelman

This comment has been minimized.

Show comment
Hide comment
@peelman

peelman May 23, 2017

Contributor

Well crap.

Contributor

peelman commented May 23, 2017

Well crap.

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 18, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

lock bot commented May 18, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 18, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.