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

Add support for Infinera-Coriant Groove #9843

Merged
merged 2 commits into from Mar 4, 2019

Conversation

Projects
None yet
3 participants
@nickhilliard
Copy link
Contributor

commented Feb 18, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2019

Hello
Thanx for your PR. Here are a few questions:

Bye
PipoCanaja

@nickhilliard

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

Why did you code most of the sensors instead of using YAML ?

Couple of reasons: the interface name is not presented in snmp - this needs to be reconstructed in code. I tried using yaml, and can get the information off the box, but can't create meaningful labels for the UI. Is there a way of using $pre_cache[] array data in yaml? If so, this might solve that problem.

Also, these devices don't have any of the usual IF-MIB contents. All these interface instances need to be reconstructed from several other entity tables in the coriant mib.

Could you improve the test data, the snmprec is quite empty for the moment, and the json is not there at all: https://docs.librenms.org/Developing/os/Test-Units/

ok, will take a look at that.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2019

You can actually precache data in yaml file and use other fields to build descriptions. You can look at includes/definitions/discovery/vrp.yaml which precaches entPhysicalName to group each SFP rx and tx power, for instance.
It is clearly not as powerful as coding, so if needed, you are perfectly correct to code instead of Yaml.

@nickhilliard nickhilliard force-pushed the nickhilliard:master branch from 0461f4a to 26496a9 Feb 20, 2019

Add support for Infinera Groove
    - added CORIANT-GROOVE-MIB
    - added sensors
        - client side traffic counters
        - line side snr / dbm / chromatic dispersion
        - chassis fanspeed / temperature
        - card states
    - added test data
        - snmprec
        - json

@nickhilliard nickhilliard force-pushed the nickhilliard:master branch from e81f76c to 00006af Feb 20, 2019

@nickhilliard

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

i looked at vrp.yaml for some ideas, but it wasn't as flexible as needed. There was an additional complication because the network interface indexes were non numeric, which meant that this needed to be simulated in code.

If the yaml processing code is looked at in future, it would be great to see coded pre-cache data available in the yaml configs - if this happened, a good chunk of this code could be replaced by yaml.

otherwise, I've updated the test data and this is running clean. I also did a rename + logo change because Coriant got bought out by Infinera. i.e. no further substantial changes planned on this side for this pull request.

@TheGreatDoc

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

@nickhilliard There is already pre-cache for yaml if that is what you mean

@PipoCanaja PipoCanaja added this to the 1.50 milestone Mar 4, 2019

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

Looks pretty good then. @TheGreatDoc, what do you think ?

@TheGreatDoc
Copy link
Contributor

left a comment

If you dont mind to move those snmp_get to a multi_get will be ok to merge

$hardware = snmp_get($device, '.1.3.6.1.4.1.42229.1.2.2.1.3.0', '-OQv', '+CORIANT-GROOVE-MIB', 'infinera-groove');
// CORIANT-GROOVE-MIB::softwareloadSwloadVersion.1
$version = snmp_get($device, '.1.3.6.1.4.1.42229.1.2.9.2.1.1.3.1', '-OQv', '+CORIANT-GROOVE-MIB', 'infinera-groove');

This comment has been minimized.

Copy link
@TheGreatDoc

TheGreatDoc Mar 4, 2019

Contributor

Can you move this to a snmp_get_multi?

@nickhilliard

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

ok, moved to snmp_get_multi().

@TheGreatDoc
Copy link
Contributor

left a comment

LGTM

@TheGreatDoc TheGreatDoc changed the title Add support for Coriant Groove Add support for Infinera-Coriant Groove Mar 4, 2019

@TheGreatDoc TheGreatDoc merged commit 09caced into librenms:master Mar 4, 2019

5 of 6 checks passed

codeclimate Code Climate encountered an error attempting to analyze this pull request.
Details
Inspection Summary
Details
Node: analysis
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details

@nickhilliard nickhilliard referenced this pull request Mar 5, 2019

Closed

New Device: Coriant Groove G30 #9655

3 of 3 tasks complete

funzoneq added a commit to funzoneq/librenms that referenced this pull request Apr 30, 2019

@lock lock bot locked as resolved and limited conversation to collaborators May 3, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.