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 support for ArbOS #8055

Merged
merged 1 commit into from Jan 12, 2018

Conversation

Projects
None yet
4 participants
@TheMysteriousX
Copy link
Contributor

TheMysteriousX commented Jan 9, 2018

This adds basic support for PeakFlow appliances. They don't really expose anything interesting by SNMP beyond what a generic device has.

I've referred to everything as 'arbos', but I don't know if the behaviour here covers APS/TMS devices too.

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

@TheMysteriousX TheMysteriousX force-pushed the TheMysteriousX:master branch from adfa534 to aa8ed57 Jan 9, 2018

@TheMysteriousX

This comment has been minimized.

Copy link
Contributor

TheMysteriousX commented Jan 9, 2018

I don't understand the test failure - I've not done anything that should affect 3com devices.

@@ -0,0 +1,630 @@
.1.3.6.1.2.1.1.1.0 = STRING: Peakflow SP 8.3 (build HIU6) System Board Model: S2600CO Serial Number: CG22541072COI

This comment has been minimized.

@murrant

murrant Jan 9, 2018

Member

This is an snmpwalk, not an snmprec file. The format is wrong.

That is what is causing tests to fail.

See https://docs.librenms.org/Developing/os/Test-Units/

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Jan 9, 2018

You can also remove the empty file includes/definitions/discovery/arbos.yaml (unless you intend to add some sensors there)

@TheMysteriousX TheMysteriousX force-pushed the TheMysteriousX:master branch from aa8ed57 to febd6b9 Jan 9, 2018

@TheMysteriousX

This comment has been minimized.

Copy link
Contributor

TheMysteriousX commented Jan 9, 2018

thanks for the pointer on the snmprec file - not sure how I ended up with a walk capture there.

I'll remove other yaml file - there aren't any sensors exposed that I can find.

One thing I'll try and add is load average, which is exposed through their own MIB. Raw CPU information isn't exposed.

@TheMysteriousX

This comment has been minimized.

Copy link
Contributor

TheMysteriousX commented Jan 9, 2018

I can think of three ways to do this.

  • I can tack it on to the end of includes/polling/ucd-mib.inc.php (logical, as it is the same information, just exposed at a different oid)
  • I could add it into includes/polling/os/arbos.inc.php and still place it into the ucd_load rrd (may conflict with ucd.inc.php if Arbor start using standard oids)
  • I can add an additional (duplicate) load average graph definition.

Any of these preferred (or something else?).

@laf

This comment has been minimized.

Copy link
Member

laf commented Jan 9, 2018

So this is a bit tricky as ucd-mib does a lot more than just load average but I don't think I'm personally a fan of tacking it on to the end of the os polling.

Maybe be worth adding a directory includes/polling/ucd-mib/ and have the new OS file in that.

Then before where the load snmp_get is called do an is_file check for includes/polling/ucd-mib/$os.inc.php, if it exists load it, do you call for data in there and populate the variables needed for the rrd file. Then do } else { and the normal snmp call - make sense?

@murrant - Thoughts? Again this is better when it gets converted to OOP but seems an ok way to deal with this and any future devices that require similar.

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Jan 9, 2018

@laf In the processor rewrite branch, we always check for ucd mib if no other processors have been found.

@TheMysteriousX TheMysteriousX force-pushed the TheMysteriousX:master branch from febd6b9 to 0185bec Jan 10, 2018

@TheMysteriousX TheMysteriousX changed the title [WIP] Add support for ArbOS Add support for ArbOS Jan 10, 2018

@TheMysteriousX

This comment has been minimized.

Copy link
Contributor

TheMysteriousX commented Jan 10, 2018

Sounds like the best thing to do would be to wait until #8066 is merged before I add load averages in.

I've cleaned up this request, so I'm happy to submit for review, and I'll open another request with load at a later date.

@scrutinizer-notifier

This comment has been minimized.

Copy link

scrutinizer-notifier commented Jan 10, 2018

The inspection completed: No new issues

@laf

This comment has been minimized.

Copy link
Member

laf commented Jan 11, 2018

@laf In the processor rewrite branch, we always check for ucd mib if no other processors have been found.

Talking about load here rather than processors stuff so not related.

@laf laf merged commit 77e1922 into librenms:master Jan 12, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@lock

This comment has been minimized.

Copy link

lock bot commented May 16, 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 16, 2018

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