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

BGP Polling: Add error code management #11424

Merged
merged 3 commits into from Apr 25, 2020
Merged

Conversation

kedare
Copy link
Contributor

@kedare kedare commented Apr 15, 2020

DO NOT DELETE THIS TEXT

Poll BGP errors codes from devices (Tested on IOS XR, JunOS, Arista EOS)
Display them on the routing pages

First line of status is generated from the error codes and subcodes (if supported)
The second line is that the device return as error text (if supported)

Screenshot 2020-04-15 at 11 47 35

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?
  • 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.

@kedare
Copy link
Contributor Author

kedare commented Apr 15, 2020

I am adding the missing test data

@kedare kedare force-pushed the bgp-last-error branch 2 times, most recently from cd05517 to dd1042f Compare April 15, 2020 15:16
@Jellyfrog Jellyfrog added Polling Needs Tests 🦄 https://docs.librenms.org/Developing/os/Test-Units/ labels Apr 15, 2020
@label-actions
Copy link

label-actions bot commented Apr 15, 2020

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

@kedare
Copy link
Contributor Author

kedare commented Apr 16, 2020

I am still adding test data, I suppose I can just add blank data like most of the other values ? (new fields to Null) as I can't add real test data.

@kedare kedare force-pushed the bgp-last-error branch 4 times, most recently from 6778586 to 7e36787 Compare April 16, 2020 10:20
@kedare
Copy link
Contributor Author

kedare commented Apr 16, 2020

Tests passed, I can't do much for the code climate issues however ?

includes/functions.php Outdated Show resolved Hide resolved
includes/functions.php Outdated Show resolved Hide resolved
@murrant
Copy link
Member

murrant commented Apr 21, 2020

Looks good, just some improvements.

It might be a good idea to include some updated test data with the new fields. They seem to all be null.

@kedare
Copy link
Contributor Author

kedare commented Apr 22, 2020

For the test data I could potentially add some for Arista, JunOS, and IOS-XR, but I will have to add it by hand on the test data, it's not very clear how to do that on the snmp test data ?

@kedare kedare changed the title BGP Polling: Add error code management [WIP] BGP Polling: Add error code management Apr 22, 2020
@kedare kedare changed the title [WIP] BGP Polling: Add error code management BGP Polling: Add error code management Apr 22, 2020
@kedare kedare requested a review from murrant April 22, 2020 09:36
@murrant
Copy link
Member

murrant commented Apr 23, 2020

Inserting additional data into existing snmprec files can be tricky. If you do it by hand, make sure the oid is always increasing.

@kedare
Copy link
Contributor Author

kedare commented Apr 23, 2020

Maybe it would be more interesting to let someone that has a lab environment to add new test data ? I don't have access to non-production environment and I am not allowed to dump data from production for the tests (for the risk of leaking data)

@murrant
Copy link
Member

murrant commented Apr 24, 2020

It would be good to have at least one with test data.

You could manually add the oids, but that seems a little tedious.

@zombah
Copy link
Contributor

zombah commented Apr 24, 2020

@kedare @murrant There is exist repo with snmpsim test data, i didn't checked it myself, but maybe it contain something useful for cases like this - https://github.com/etingof/snmpsim-data/tree/master/data/network/router

@kedare
Copy link
Contributor Author

kedare commented Apr 24, 2020

I added some test data, let me know if this is enough

@murrant murrant removed the Needs Tests 🦄 https://docs.librenms.org/Developing/os/Test-Units/ label Apr 25, 2020
@murrant murrant merged commit fc39109 into librenms:master Apr 25, 2020
@murrant
Copy link
Member

murrant commented Apr 25, 2020

Thanks @kedare , nice work :)

@murrant
Copy link
Member

murrant commented Apr 28, 2020

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

https://community.librenms.org/t/v1-63-release-changelog-april-2020/11828/1

@murrant
Copy link
Member

murrant commented May 5, 2020

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

https://community.librenms.org/t/v1-63-high-cpu-on-arista-switches-with-bgp-peers-module-enabled/11947/4

LEV82 pushed a commit to LEV82/librenms that referenced this pull request May 14, 2020
* BGP Polling: Add error code polling

* Rework describe_bgp_error_code and fix bgp error fields migration

* Add real test data for IOS-XR and Arista EOS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants