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

Reduce discovery snmp load of Cisco VTP vlans module #10510

Merged
merged 2 commits into from Aug 14, 2019

Conversation

@PipoCanaja
Copy link
Contributor

commented Aug 9, 2019

Current 'vlans' discovery does load the complete vtpVlanEntry and dot1dStpPortEntry table even if it only uses very few OIDs in them.
This patch reduces the snmp requests to only the necessary OIDs -> quicker vlan discovery and less CPU pressure on the device.

Some nexus 55xx devices have a buggy SNMP implementation and get killed (CPU goes high, and forwarding can even be impacted).

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 10510
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.

@PipoCanaja PipoCanaja changed the title Reduce discovery snmp load of Cisco VTP vlan discovery Reduce discovery snmp load of Cisco VTP vlans module Aug 9, 2019

@PipoCanaja PipoCanaja self-assigned this Aug 9, 2019

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

Tested OK on Nexus 5596, Cat 6500, Cat 2960x. On Nexus, load on CPU is now back to more acceptable values, even with the buggy firmware. And most importantly, the discovery now succeed everytime instead of failing more or less badly.

Ready for merge for me, @librenms/reviewers

@murrant
Copy link
Member

left a comment

LGTM

@murrant

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

@PipoCanaja do we have any ios vlans test data?

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@murrant Good question. I'll check. If no, then I'll start a test device with 3 VLANs to collect the data.

@murrant

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

@PipoCanaja please capture the data without this patch applied then add it here so we can verify it doesn't break anything ;)

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@murrant snmpsim data is already collected (snmpsim/ios_2960X.snmprec. But the JSON was not generated. Should I open a PR to generate the JSON, we merge it first, and then we run tests again here to confirm nothing is broken ?

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

Test added here on 'master':
#10519

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

Test added here on 'master':
#10519

Restarted tests here just now.

@PipoCanaja PipoCanaja merged commit 67376e2 into librenms:master Aug 14, 2019

6 checks passed

Inspection Summary
Details
Node: analysis
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
codeclimate 1 fixed issue
Details
license/cla Contributor License Agreement is signed.
Details
@murrant

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

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

https://community.librenms.org/t/v1-55-release-changelog-august-2019/9428/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.