Rewrite broken netonix.inc.php #7189

neggles · 2017-08-21T07:13:54Z

Please let me know if there's anything I've done wrong here, I'm still new to contributing to projects like these.

Rewrote netonix.inc.php to account for all 5 states & follow coding style of other state modules.

The existing netonix.inc.php file does not correctly map all 5 possible PoE states, and doesn't match the coding style / template used in other sensor state files. There's a forum report here about this that's apparently been missed :(

The original file only allows for 3 states, Off / 24V / 48V, mapped to values 1/2/3 respectively, but there are 5 possible states for outputs;

Off	24V	48V	24VH	48VH

The 24VH/48VH modes are "High Power" mode, aka 4-pair PoE, where all 4 pairs of the cable are used to supply power. The vast majority of PoE devices do not use this; it's mostly found on wireless equipment such as Ubiquiti AirFiber radios, and if used with an incompatible device will usually fry the port & device; but these are, after all, netonix WISP switches.

I've recreated the state index as follows, and rewritten the file to follow the standard format. Please let me know if I should credit the original author; as this was written from scratch by following the documentation, no code is copied - though one line came out identical (string concat)

State	Old index	New index
Off	1	1
24V	2	2
48V	3	3
24VH	N/A	4
48VH	N/A	5

This maintains compatibility with any existing alert rules, while adding the two extra states required. The generic state of all states other than "Off" has been set to 0 (could easily be convinced to make the VH states 1/warn) while "Off" is at -1 as per convention.

I've changed the name of the state index from netonixPoeStatus to netonixPoeState as I had issues with the code not updating the state index. I'm unsure how to clean this up; as a result of my testing I actually have three separate indexes... If someone could let me know how to purge the old, broken index, that'd be great...

Pre-commit results;

Running unit tests... success
Running style check... success
Running lint check... success
Tests ok, submit away :)

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?

Testers

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

mention-bot · 2017-08-21T07:13:55Z

Thank you for submitting a PR @neg2led! We have found the following @murrant and @laf based on the history of these files to review this PR.

laf

Overall this is absolutely fine. It does look prime for switching over to the new YAML format - do you fancy doing that? If not I can provide the file for you to try.

laf · 2017-08-21T12:17:49Z

includes/discovery/sensors/state/netonix.inc.php

@@ -19,32 +19,29 @@
 *
 * @package    LibreNMS
 * @link       http://librenms.org
- * @copyright  2016 Tony Murray
- * @author     Tony Murray <murraytony@gmail.com>
+ * @copyright  2017 Andrew Holmes


You should append your copyright rather than removing the old one :)

I wasn't sure about that - but this makes more sense, and is why I asked :)

neggles · 2017-08-21T14:30:02Z

I can give YAML a try - it looks pretty simple, and is making me annoyed that I spent all this time on the PHP version.

I'm not quite sure which variables go where, though; I've gotten this far...

mib: NETONIX-SWITCH-MIB
modules:
    sensors:
        state:
            -
                oid: poeStatus
                value: poeStatus
                num_oid: .1.3.6.1.4.1.46242.5.1.2.
                descr: Port $index PoE 
                index: '{{ $index }}'
                state_name: netonixPoeState
                states:
                    - { descr: Off, graph: 0, value: 1, generic: -1 }
                    - { descr: 24V, graph: 0, value: 2, generic: 0 }
                    - { descr: 48V, graph: 0, value: 3, generic: 0 }
                    - { descr: 24VH, graph: 0, value: 4, generic: 0 }
                    - { descr: 48VH, graph: 0, value: 5, generic: 0 }

I think I've got that mostly right? Unsure which bits are configurable / which are set elsewhere...

It may be helpful to put up one of the examples on the wiki in the form of the other language; would ease translating between the two for someone less familiar with either :P

laf · 2017-08-21T15:55:35Z

Looks good, just need to use: descr: Port {{ $index }} PoE

If you remove the php discovery file git rm includes/discovery/sensors/state/netonix.inc.php

Then re-run discovery, the sensors should persist. If they do it works, if they don't then something is up :)

neggles · 2017-08-22T01:25:09Z

Hmm, this has not worked. I must have broken something... I was missing one section of the YAML, too.

Before I sit here throwing modifications back and forth, here's where I got;

mib: NETONIX-SWITCH-MIB
modules:
    sensors:
        state:
            data:
                -
                    oid: poeStatusTable
                    value: poeStatus
                    num_oid: .1.3.6.1.4.1.46242.5.1.2.
                    descr: Port {{ $index }} PoE
                    index: '{{ $index }}'
                    state_name: netonixPoeState
                    states:
                        - { descr: Off, graph: 0, value: 1, generic: -1 }
                        - { descr: 24V, graph: 0, value: 2, generic: 0 }
                        - { descr: 48V, graph: 0, value: 3, generic: 0 }
                        - { descr: 24VH, graph: 0, value: 4, generic: 0 }
                        - { descr: 48VH, graph: 0, value: 5, generic: 0 }

This shows up when it caches the SNMP;

Modules status: Global+ OS  Device
#### Load disco module sensors ####
SNMP[/usr/bin/snmpbulkwalk -v2c -c COMMUNITY -OeQUs -Ih -m NETONIX-SWITCH-MIB -M /opt/librenms/mibs:/opt/librenms/mibs/netonix udp:HOSTNAME:161 poeStatusTable]
poeStatus.1 = "48VH"
poeStatus.2 = "Off"
poeStatus.3 = "24V"
poeStatus.4 = "24V"
poeStatus.5 = "24V"
poeStatus.6 = "Off"
poeStatus.7 = "48V"
poeStatus.8 = "48V"
poeStatus.9 = "48V"
poeStatus.10 = "Off"
poeStatus.11 = "Off"
poeStatus.12 = "Off"
poeStatus.13 = "Off"
poeStatus.14 = "Off"

But when it gets to the actual sensor discovery, this is all I get;

State: SQL[SELECT * FROM sensors AS S, devices AS D WHERE S.sensor_class='state' AND S.device_id = D.device_id AND D.device_id = '25' AND S.poller_type = 'snmp']

I must be missing something about how this is meant to work; is there a definition-of-the-definitions I can look at anywhere?

For reference, here's what the table looks like in iReasoning MIB Browser. The table itself is .1.3.6.1.4.1.46242.5. and port statuses are found at .1.3.6.1.4.1.46242.5.1.2.portnumber

What am I doing wrong? :(

laf · 2017-08-22T08:32:59Z

Are the states still in the webui? If so, that output looks ok, it will do a mysql query to check the sensor exists and if it does it will continue on without doing anything else - that's expected as the php file would have discovered the sensors and now the yaml will just carry on.

If they've dropped then yes, something is wrong and we'd need to see the debug output - or if you want we can just merge this in as is for now?

neggles · 2017-08-22T08:47:23Z

The "state" sensor section has evaporated entirely from the webUI - and the first time I ran discovery with the .yaml in place, it gave me a nice slab of -s next to the sensor output to show it deleting them - it's getting far enough to cache the OID, but not far enough to generate the sensor defs :(

I'm happy to merge it in as-is - but I would like to work out why YAML isn't working...

laf · 2017-08-22T08:52:42Z

@neg2led pastebin the output of ./discover.php -h HOSTNAME -d -m sensors

neggles · 2017-08-22T08:59:03Z

Sure thing; it's here

laf · 2017-08-22T09:14:33Z

I think it's because we don't get a numeric value back :/

What does this show:

snmpget -UQnt -v2c -c$COMMUNITY $HOSTNAME .1.3.6.1.4.1.46242.5.1.2.1

neggles · 2017-08-22T13:41:01Z

Indeed, each port index just returns the status string;

librenms@orwell:~$ snmpget -OUQnt -v2c -c$COMMUNITY $HOSTNAME .1.3.6.1.4.1.46242.5.1.2.1
.1.3.6.1.4.1.46242.5.1.2.1 = "48VH"

laf · 2017-08-22T13:51:43Z

So I assume this never worked then? It will need a custom poller file writing as well to translate the description to a numerical value.

neggles · 2017-08-22T14:00:54Z

The original file in the master branch never worked, no - the SNMPget commands used an invalid OID that never loaded. But the file I've added here works - is that weird?

With this file in place, and the yaml removed, when I run discovery they show up as you can see here

And when I run the poller, they poll, though it looks kind of odd

laf · 2017-08-22T14:14:00Z

Let's just go with your php version :)

laf · 2017-08-22T14:14:23Z

So if you can update the copyright we can merge in.

murrant

.

murrant · 2017-08-24T04:17:02Z

Hmm, the original file worked for me. However, it looks like they added the 24VH and 48VH after that code was developed.

Renaming the state loses the history, is that required to add 24VH and 48VH?

@laf the state sensor code will map string values to numeric values.

not sure

neggles · 2017-08-24T05:28:24Z

I made no changes to the install other than this file to make this work - should I have to add a MIB to my conf? The commands used by libre specify alternate MIB dirs anyway, no?

This system is an otherwise-blank Ubuntu 16.04 system with nginx/mariadb that does nothing but run libre - built with the instructions on the main site - would you not expect that to work without modification to snmp.conf?

With the existing discovery module, discovery goes like this - As you can see, the discovery bulkwalk goes fine, but when it goes to insert the sensors into the database, the SNMP OID it uses has the .1.3.6.1.4. section repeated at the start;

State: SNMP[/usr/bin/snmpbulkwalk -v2c -c COMMUNITY -OQUs -M /opt/librenms/mibs:/opt/librenms/mibs/netonix udp:HOSTNAME:161 .1.3.6.1.4.1.46242.5.1.2]
iso.*.1.46*.1 = "48VH"

<snip>

SQL[INSERT INTO `sensors` (`poller_type`,`sensor_class`,`device_id`,`sensor_oid`,`sensor_index`,`sensor_type`,`sensor_descr`,`sensor_divisor`,`sensor_multiplier`)  VALUES ('snmp','state','25','.1.3.6.1.4.1.3.6.1.4.1.46242.5.1.2.1','1','netonixPoeStatus','Port 1 PoE','1','1')]

So when we get to running the poller it tries to pull data from an OID that doesn't exist, and fails miserably;

SQL[SELECT * FROM `sensors` WHERE `sensor_class` = 'state' AND `device_id` = '25']
SNMP[/usr/bin/snmpget -v2c -c COMMUNITY -OUQnt -M /opt/librenms/mibs:/opt/librenms/mibs/netonix udp:HOSTNAME:161 .1.3.6.1.4.1.3.6.1.4.1.46242.5.1.2.1 .1.3.6.1.4.1.3.6.1.4.1.46242.5.1.2.2 <etc>]
.*.*.**.1 = No Such Object available on this agent at this OID

<snip>

Checking (snmp) state Port 1 PoE...
SQL[SELECT `state_value`
                        FROM `state_translations` LEFT JOIN `sensors_to_state_indexes`
                        ON `state_translations`.`state_index_id` = `sensors_to_state_indexes`.`state_index_id`
                        WHERE `sensors_to_state_indexes`.`sensor_id` = '181'
                        AND `state_translations`.`state_descr` LIKE '']
State value of  is

When run with this file in place discovery looks quite a bit happier;

SQL[INSERT INTO `sensors` (`poller_type`,`sensor_class`,`device_id`,`sensor_oid`,`sensor_index`,`sensor_type`,`sensor_descr`,`sensor_divisor`,`sensor_multiplier`,`sensor_current`,`entPhysicalIndex`)  VALUES ('snmp','state','25','.1.3.6.1.4.1.46242.5.1.2.1','1','netonixPoeState','Port 1 PoE','1','1','48VH','1')]

And the poller works;

SNMP[/usr/bin/snmpget -v2c -c COMMUNITY -OUQnt -M /opt/librenms/mibs:/opt/librenms/mibs/netonix udp:HOSTNAME:161 .1.3.6.1.4.1.46242.5.1.2.1 <snip>]
.*.4.1.46*.1 = "48VH"

<snip>

Checking (snmp) state Port 1 PoE...
SQL[SELECT `state_value`
                        FROM `state_translations` LEFT JOIN `sensors_to_state_indexes`
                        ON `state_translations`.`state_index_id` = `sensors_to_state_indexes`.`state_index_id`
                        WHERE `sensors_to_state_indexes`.`sensor_id` = '195'
                        AND `state_translations`.`state_descr` LIKE '48VH']
State value of 48VH is 5

While stressing that I'm still struggling to wrap my head around PHP arrays, the difference seems to be due to this bit of code in the original (lines 27/28);

librenms/includes/discovery/sensors/state/netonix.inc.php

Lines 27 to 28 in 30c1b79

    
           $temp = snmpwalk_cache_multi_oid($device, '.1.3.6.1.4.1.46242.5.1.2', array()); 
        
           $cur_oid = '.1.3.6.1.4.1.';

When you get to the end and it creates the actual sensors, it lands up concatenating $cur_oid with $index - which at this point is ".3.6.1.4.1.46242.5.1.2.portnumber", apparently?

librenms/includes/discovery/sensors/state/netonix.inc.php

Line 65 in 30c1b79

    
           discover_sensor($valid['sensor'], 'state', $device, $cur_oid.$index, $id, $state_name, $descr, '1', '1', null, null, null, null, $current);

Which at this point spits out the string we see above in discovery, likely because

librenms/includes/discovery/sensors/state/netonix.inc.php

Line 27 in 30c1b79

$temp = snmpwalk_cache_multi_oid($device, '.1.3.6.1.4.1.46242.5.1.2', array());

should look more like this as per the doco;

https://github.com/neg2led/librenms/blob/86e69edb4431794d1b4fcdfebb9257cdc0fed7bd/includes/discovery/sensors/state/netonix.inc.php#L30

This change alone is actually enough to fix the original file - save for the fact that 24VH and 48VH are missing from the state table - but I rewrote the rest of it to match the docu style so that I actually understood what I was doing

Sorry for the wall of text! I hope this makes sense.

If I had to guess, I'd say it works on your system because you have the MIB installed globally - so it gets translated to the text form in snmpwalk's output even though no MIB is specified in the actual command/routine?

neggles · 2017-08-24T05:40:46Z

RE: renaming the state - it shouldn't be necessary, but currently the discovery file checks for a state map with $NAME & creates it if it doesn't exist; so if it already exists, nothing is done, and the two new states aren't added to the table.

librenms/includes/discovery/sensors/state/netonix.inc.php

Line 33 in f2c69f3

$state_index_id = create_state_index($state_name);

librenms/includes/discovery/sensors/state/netonix.inc.php

Line 42 in f2c69f3

if ($state_index_id !== null) {

L33 creates a state table for state $NAME
L42 only runs if L36 actually created something

So if $NAME already exists, no creation code. It seems like it should be fairly simple to add some sanity-checking (check how many states are in the table and recreate if != 5?) but I don't have enough confidence in my PHP-fu to know where to start (for fear of trashing my DB)

murrant · 2017-08-24T12:17:35Z

@neg2led I never said it doesn't work for you, just that it was working for devices that don't have 24VH and 48VH.

I worked a bit last night and fixed the yaml code for this. I am changing the code so it will update the translation states too.

murrant · 2017-08-24T15:14:25Z

@neg2led @laf I just sent my PR to improve yaml state handling which will allow yaml to be used for this. Also, you won't have to rename the state and lose existing data.

#7221

laf · 2017-08-24T20:59:55Z

@neg2led Can you test murrants PR with the yaml file you created?

neggles · 2017-08-25T00:18:47Z

@murrant sounds great to me - much more elegant than my fix :)

I've applied the changes from #7221 to my system and added this file

mib: NETONIX-SWITCH-MIB
modules:
    sensors:
        state:
            data:
                -
                    oid: poeStatusTable
                    value: poeStatus
                    num_oid: .1.3.6.1.4.1.46242.5.1.2.
                    descr: Port {{ $index }} PoE
                    index: '{{ $index }}'
                    state_name: netonixPoeStatus
                    states:
                        - { descr: Off, graph: 0, value: 1, generic: -1 }
                        - { descr: 24V, graph: 0, value: 2, generic: 0 }
                        - { descr: 48V, graph: 0, value: 3, generic: 0 }
                        - { descr: 24VH, graph: 0, value: 4, generic: 0 }
                        - { descr: 48VH, graph: 0, value: 5, generic: 0 }

as includes/definitions/discovery/netonix.yaml, and sure enough, it works great :) discovery looks like this and polling looks like this

What's the next step here - should I close this PR and create a new one to add the yaml (maybe after 7221 is merged?)

murrant · 2017-08-25T00:32:53Z

You can just change this PR to revert the other changes and include the yaml file. We will merge it after the other PR. (which needs some more testing to make sure it doesn't break other sensors)

My hint is git reset --hard upstream/master (assuming you have upstream remote defined as in our git docs).

neggles · 2017-08-25T00:56:06Z

I... have made an error. Goddamnit, Git. Lemme fix that...

murrant · 2017-08-25T00:57:18Z

no worries :)

scrutinizer-notifier · 2017-08-25T01:03:55Z

The inspection completed: No new issues

neggles · 2017-08-25T01:06:34Z

Alright, I made enough of a hash of this (and closed it?!) that I'm just going again. I tried to be smart, and it did not pay off...

See #7224

lock · 2018-05-17T12:35:52Z

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

laf requested changes Aug 21, 2017

View reviewed changes

laf approved these changes Aug 22, 2017

View reviewed changes

laf requested a review from murrant August 22, 2017 14:19

laf approved these changes Aug 22, 2017

View reviewed changes

murrant previously requested changes Aug 24, 2017

View reviewed changes

neggles closed this Aug 25, 2017

neggles deleted the patch-1 branch August 25, 2017 01:06

neggles mentioned this pull request Aug 25, 2017

Netonix PoE State Sensor Fix (YAML) #7224

Merged

1 task

neggles restored the patch-1 branch August 27, 2017 00:50

neggles deleted the patch-1 branch August 27, 2017 00:50

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite broken netonix.inc.php #7189

Rewrite broken netonix.inc.php #7189

neggles commented Aug 21, 2017

mention-bot commented Aug 21, 2017

laf left a comment

laf Aug 21, 2017

neggles Aug 21, 2017

neggles commented Aug 21, 2017 •

edited

Loading

laf commented Aug 21, 2017

neggles commented Aug 22, 2017

laf commented Aug 22, 2017

neggles commented Aug 22, 2017

laf commented Aug 22, 2017

neggles commented Aug 22, 2017

laf commented Aug 22, 2017

neggles commented Aug 22, 2017

laf commented Aug 22, 2017

neggles commented Aug 22, 2017

laf commented Aug 22, 2017

laf commented Aug 22, 2017

murrant left a comment •

edited

Loading

murrant commented Aug 24, 2017 •

edited

Loading

neggles commented Aug 24, 2017 •

edited

Loading

neggles commented Aug 24, 2017

murrant commented Aug 24, 2017

murrant commented Aug 24, 2017

laf commented Aug 24, 2017

neggles commented Aug 25, 2017

murrant commented Aug 25, 2017

neggles commented Aug 25, 2017

murrant commented Aug 25, 2017

scrutinizer-notifier commented Aug 25, 2017

neggles commented Aug 25, 2017 •

edited

Loading

lock bot commented May 17, 2018

Rewrite broken netonix.inc.php #7189

Rewrite broken netonix.inc.php #7189

Conversation

neggles commented Aug 21, 2017

Please note

Testers

mention-bot commented Aug 21, 2017

laf left a comment

Choose a reason for hiding this comment

laf Aug 21, 2017

Choose a reason for hiding this comment

neggles Aug 21, 2017

Choose a reason for hiding this comment

neggles commented Aug 21, 2017 • edited Loading

laf commented Aug 21, 2017

neggles commented Aug 22, 2017

laf commented Aug 22, 2017

neggles commented Aug 22, 2017

laf commented Aug 22, 2017

neggles commented Aug 22, 2017

laf commented Aug 22, 2017

neggles commented Aug 22, 2017

laf commented Aug 22, 2017

neggles commented Aug 22, 2017

laf commented Aug 22, 2017

laf commented Aug 22, 2017

murrant left a comment • edited Loading

Choose a reason for hiding this comment

murrant commented Aug 24, 2017 • edited Loading

neggles commented Aug 24, 2017 • edited Loading

neggles commented Aug 24, 2017

murrant commented Aug 24, 2017

murrant commented Aug 24, 2017

laf commented Aug 24, 2017

neggles commented Aug 25, 2017

murrant commented Aug 25, 2017

neggles commented Aug 25, 2017

murrant commented Aug 25, 2017

scrutinizer-notifier commented Aug 25, 2017

neggles commented Aug 25, 2017 • edited Loading

lock bot commented May 17, 2018

neggles commented Aug 21, 2017 •

edited

Loading

murrant left a comment •

edited

Loading

murrant commented Aug 24, 2017 •

edited

Loading

neggles commented Aug 24, 2017 •

edited

Loading

neggles commented Aug 25, 2017 •

edited

Loading