Fix missing PoE port graphs for Cisco Catalyst 9K #11698

ajsiersema · 2020-05-25T19:44:05Z

The PoE port graphs are missing on Cisco Catalyst 9K devices.
Polling code assumes interfaces named *Ethernet[SLOT#]/[SUBSLOT#]/[PORT#] will have corresponding entries in the SNMP table, e.g. cpeExtStuff.3.0.1 for GigabitEthernet3/0/1
However, Cisco decided to omit the 0 of the SUBSLOT, the SNMP entry is thus cpeExtStuff.3.1

This pull requests registers both variations in order to match both the Cat 9K devices, and any devices conforming to the existing assumption with regard to interface name<->SNMP name relation.
The change will not break existing behavior, but only adds the Cat 9K case.
AFAIK there are no ios/iosxe devices that have both
GigabitEthernet[1-9]+/0/[0-9]+ i.e. slot/subslot/port
as well as
GigabitEthernet[1-9]+/[0-9]+ i.e. slot/port
All the slots with subslots start at 1, with possibly a single OOB interface GigabitEthernet0/0 (in slot 0.)

Tested on IOS-XE 16.9.x

DO NOT DELETE THE UNDERLYING TEXT

Please note

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

[ X] Have you followed our code guidelines?
If my Pull Request does some changes/fixes/enhancements in the WebUI, I have inserted a screenshot of it.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

ajsiersema · 2020-05-25T20:08:47Z

Fixing codeclimate "Avoid deeply nested control flow statements" requires extensive code rewrite.
The same codeclimate issue popped up in other PR's, where the consensus seems to be that we can ignore this message in these cases.

label-actions · 2020-05-25T22:38:36Z

Please add test data so we can ensure your change is not broken in the future.

Read the docs to find out how: https://docs.librenms.org/Developing/os/Test-Units

murrant · 2020-05-25T22:43:13Z

includes/polling/ports.inc.php

+                    devices that do include the subslot (for subslot == 0)
+                    */
+                    $iface_parts = explode('/', $matches[1]);
+                    while (1) {


while (1) needs to change, you could honestly update the regex above and drop most of this code.

I left the regexp from the original code as we still need to match ethernet interfaces and capture the slot[/subslot]/port part (to be able replace '/' by '.' later.)
Capturing repeated subgroups in PHP's PCRE would make for a hard to read regexp. Maybe that was your intention ? I'm not quite sure what you meant by 'update the regexp'.

We also still need to add both slot.subslot.port and slot.port to $port_ent_to_if, i.e. two assignments (at least). Otherwise we'd break the poller for non-cat9K devices.
We don't want to strip the subslot element if it's non-zero, i.e. we need to check that part of the interface notation.
Maybe your assumption is that there will always be either slot/subslot/port or slot/port interfaces, and not e.g. slot/subslot/subsubslot/port. The PR code doesn't make any assumptions.

The while (1) is in effect a do..while, preventing a duplication of the assignment statements.
I'll see if a variation on that theme with a for loop or some such is less offensive to the eyes :)

if (preg_match('/^[a-z]+ethernet(\d+)\/(\d+)(?:\/(\d+))?$/i', $if_descr['ifDescr'], $matches)) { port_ent_to_if[$matches[1] . '.' . ($matches[3] ?: $matches[2])] = ['portIfIndex' => $if_index]; }

Should be it. Specifically only matches XethernetX/X or XethernetX/X/X

If cpeExtPsePortEntry can be indexed differently, this code is probably slightly wrong.

Should be it. Specifically only matches XethernetX/X or XethernetX/X/X
If cpeExtPsePortEntry can be indexed differently, this code is probably slightly wrong.

That's indeed the catch: I fully expect there to be an existing ios(xe) device that indexes XethernetA/B/C in cpeExtPsePortEntry with a.b.c. even with B == 0. Breaking the poller is bad, hence the code to add both A.B.C. and A.C to $port_ent_to_if, for arbitrary 'depths' of interface names.

However, if we're working with the assumption that there will only ever be XethernetX/X or XethernetX/X/X (i.e. two or three parts notation), we can refactor the PR as two assignment statements, like so:

if (preg_match('/^\w+ethernet(\d+)\/(\d+)(?:\/(\d+))?$/i', $if_descr['ifDescr'], $matches)) { foreach (array(0=>0) + ($matches[3] ? array(1=>2):array()) as $idx) { unset($matches[$idx]); $port_ent_to_if[implode('.', $matches)] = ['portIfIndex' => $if_index]; } }

What do you think ?

Why do you expect that? Generally the table indexes are fixed. I don't have any sample data to l look at so I can't confirm. (btw you should add test data to this PR)

why the loop?
just add two entries into the $port_ent_to_if array

Added tests. I've check with ./lnms dev:check -o iosxe_c9400 , they seem to pass.
Not sure about the 2nd ASA device in the JSON, did snmpsim inject it ?

codeclimate once again complains about "Avoid deeply nested control flow statements". Can't be fixed without drastic rewriting all of the code.

Node analysis timed out "The command produced no output for 180 seconds. You can increase this timeout by defining the "idle_timeout" modifier.". Not something I can fix :)

…ller-ports-poe-fails-on-cisco-cat9k rebase

murrant · 2020-06-05T01:44:00Z

Understood, you aren't expected to fix the underlying code issues, just hopefully not make it worse ;)

murrant · 2020-06-05T01:45:56Z

Also, enable_ports_poe is false by default, so I'm not sure if your code is being tested.

Checking the MIBs... CISCO-POWER-ETHERNET-EXT-MIB: AUGMENTS { pethPsePortEntry } POWER-ETHERNET-MIB: INDEX { pethPsePortGroupIndex , pethPsePortIndex } This means cpeExtPsePortEntry is always indexed by exactly 2 values. No point in populating slot.subslot.port

ajsiersema

The simplified code will also strip subslot != 0.
I currently don't have any devices with e.g. Gi1/1/1, I haven't been able to verify if Cisco always strips the subslot. Hence my reasoning for storing both. You already mentioned you didn't expect any issues.
Thanks again for your effort

murrant · 2020-07-03T04:42:47Z

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/v1-65-release-changelog-june-2020/12687/1

murrant · 2020-07-03T04:43:46Z

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/v1-65-release-changelog-june-2020/12687/1

Fix missing PoE port graphs for Cisco Catalyst 9K

d3bd252

murrant added the Needs Tests 🦄 https://docs.librenms.org/Developing/os/Test-Units/ label May 25, 2020

murrant added the Device 🖥️ New or added device support label May 25, 2020

murrant reviewed May 25, 2020

View reviewed changes

ajsiersema added 2 commits June 2, 2020 15:08

Merge branch 'master' of https://github.com/librenms/librenms into po…

6cc2e13

…ller-ports-poe-fails-on-cisco-cat9k rebase

Reworked code: 2 assignments, added tests

ea09ef4

murrant removed the Needs Tests 🦄 https://docs.librenms.org/Developing/os/Test-Units/ label Jun 5, 2020

Simplify code

f0b2665

Checking the MIBs... CISCO-POWER-ETHERNET-EXT-MIB: AUGMENTS { pethPsePortEntry } POWER-ETHERNET-MIB: INDEX { pethPsePortGroupIndex , pethPsePortIndex } This means cpeExtPsePortEntry is always indexed by exactly 2 values. No point in populating slot.subslot.port

murrant approved these changes Jun 5, 2020

View reviewed changes

murrant merged commit cd362b7 into librenms:master Jun 5, 2020

ajsiersema commented Jun 5, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix missing PoE port graphs for Cisco Catalyst 9K #11698

Fix missing PoE port graphs for Cisco Catalyst 9K #11698

ajsiersema commented May 25, 2020 •

edited

ajsiersema commented May 25, 2020

label-actions bot commented May 25, 2020

murrant May 25, 2020

ajsiersema May 26, 2020

ajsiersema May 26, 2020

murrant May 26, 2020 •

edited

ajsiersema May 26, 2020

murrant May 27, 2020 •

edited

murrant May 27, 2020

ajsiersema Jun 2, 2020

ajsiersema Jun 4, 2020

murrant commented Jun 5, 2020

murrant commented Jun 5, 2020

ajsiersema left a comment

murrant commented Jul 3, 2020

murrant commented Jul 3, 2020

Fix missing PoE port graphs for Cisco Catalyst 9K #11698

Fix missing PoE port graphs for Cisco Catalyst 9K #11698

Conversation

ajsiersema commented May 25, 2020 • edited

Please note

Testers

ajsiersema commented May 25, 2020

label-actions bot commented May 25, 2020

murrant May 25, 2020

Choose a reason for hiding this comment

ajsiersema May 26, 2020

Choose a reason for hiding this comment

ajsiersema May 26, 2020

Choose a reason for hiding this comment

murrant May 26, 2020 • edited

Choose a reason for hiding this comment

ajsiersema May 26, 2020

Choose a reason for hiding this comment

murrant May 27, 2020 • edited

Choose a reason for hiding this comment

murrant May 27, 2020

Choose a reason for hiding this comment

ajsiersema Jun 2, 2020

Choose a reason for hiding this comment

ajsiersema Jun 4, 2020

Choose a reason for hiding this comment

murrant commented Jun 5, 2020

murrant commented Jun 5, 2020

ajsiersema left a comment

Choose a reason for hiding this comment

murrant commented Jul 3, 2020

murrant commented Jul 3, 2020

ajsiersema commented May 25, 2020 •

edited

murrant May 26, 2020 •

edited

murrant May 27, 2020 •

edited