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

new device: ADVA FSP150CC and FSP3000R7 Series #6696

Merged
merged 7 commits into from Jun 2, 2017

Conversation

Projects
None yet
4 participants
@czilian
Contributor

czilian commented May 21, 2017

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

czilian added some commits May 20, 2017

ADVA device support
- sensors for temperature, voltage, current and dbm
- ADVA mibs for FSP150CC and FSP3000R7
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented May 21, 2017

Auto-Deploy finished, Test PR at http://6696.ci.librenms.org or https://6696.ci.librenms.org

@czilian

This comment has been minimized.

Show comment
Hide comment
@czilian

czilian May 21, 2017

Contributor

ADVA device support

/images/os/adva.svg - created a squarish logo
includes/definitions/adva.yaml - detects anything from ADVA
includes/polling/os/adva.inc.php - device specifics
tests/snmpsim/adva.snmprec - EGX as one type to detect
mibs/adva - ADVA specific MIBs but also some generic which are currently not in the MIBs root (maybe can move those from ADVA one level up later)

Sensor detection with two sections each for FSP150CC OS and FSP3000R7 OS:
discovery/sensors/pre-cache/adva.inc.php
discovery/sensors/current/adva.inc.php
discovery/sensors/dbm/adva.inc.php
discovery/sensors/temperature/adva.inc.php
discovery/sensors/voltage/adva.inc.php

Contributor

czilian commented May 21, 2017

ADVA device support

/images/os/adva.svg - created a squarish logo
includes/definitions/adva.yaml - detects anything from ADVA
includes/polling/os/adva.inc.php - device specifics
tests/snmpsim/adva.snmprec - EGX as one type to detect
mibs/adva - ADVA specific MIBs but also some generic which are currently not in the MIBs root (maybe can move those from ADVA one level up later)

Sensor detection with two sections each for FSP150CC OS and FSP3000R7 OS:
discovery/sensors/pre-cache/adva.inc.php
discovery/sensors/current/adva.inc.php
discovery/sensors/dbm/adva.inc.php
discovery/sensors/temperature/adva.inc.php
discovery/sensors/voltage/adva.inc.php

@laf

Thanks for submitting this again. Certainly a lot cleaner :)

I've put some comments in, feel free to jump on irc to discuss if you need to.

Show outdated Hide outdated includes/definitions/adva.yaml
@@ -0,0 +1,12 @@
os: adva

This comment has been minimized.

@laf

laf May 21, 2017

Member

I feel this is too generic. The os name should be based after the firmware/software the devices run, like IOS for Cisco switches.

If they don't have a name then at worst the os here should be something like adva-fsp.

@laf

laf May 21, 2017

Member

I feel this is too generic. The os name should be based after the firmware/software the devices run, like IOS for Cisco switches.

If they don't have a name then at worst the os here should be something like adva-fsp.

This comment has been minimized.

@czilian

czilian May 21, 2017

Contributor

Thx for going through. These are two OS. Hence, splitting it into two yaml. All the sysObjectID checks in the other files will then also disappear. The OS names are unfortunately not catchy. Called it adva_fsp150 and adva_fsp3kr7 now.

@czilian

czilian May 21, 2017

Contributor

Thx for going through. These are two OS. Hence, splitting it into two yaml. All the sysObjectID checks in the other files will then also disappear. The OS names are unfortunately not catchy. Called it adva_fsp150 and adva_fsp3kr7 now.

Show outdated Hide outdated includes/discovery/sensors/current/adva.inc.php
// ***** Sensors for ADVA FSP150EG-X Chassis
// ******************************************
if (starts_with($device['sysObjectID'], 'enterprises.2544.1.12.1.1')) {

This comment has been minimized.

@laf

laf May 21, 2017

Member

If this is to restrict the check to this OS then you can remove this if check as we only run sensors with the same filename as the os.

@laf

laf May 21, 2017

Member

If this is to restrict the check to this OS then you can remove this if check as we only run sensors with the same filename as the os.

This comment has been minimized.

@laf

laf May 21, 2017

Member

Looking at your other code it looks like it's to only perform the checks for specific devices.

Are you sure $device['sysObjectID'] isn't a full numerical string like .1.3.x.x.x.x?

@laf

laf May 21, 2017

Member

Looking at your other code it looks like it's to only perform the checks for specific devices.

Are you sure $device['sysObjectID'] isn't a full numerical string like .1.3.x.x.x.x?

This comment has been minimized.

@czilian

czilian May 21, 2017

Contributor

gone.

@czilian

czilian May 21, 2017

Contributor

gone.

Show outdated Hide outdated includes/discovery/sensors/current/adva.inc.php
if (starts_with($device['sysObjectID'], 'enterprises.2544.1.12.1.1')) {
// Define Sensors and Limits
$sensors = array

This comment has been minimized.

@laf

laf May 21, 2017

Member

Please rename this variable, it's most likely going to conflict somewhere.

@laf

laf May 21, 2017

Member

Please rename this variable, it's most likely going to conflict somewhere.

This comment has been minimized.

@czilian

czilian May 21, 2017

Contributor

gone.

@czilian

czilian May 21, 2017

Contributor

gone.

Show outdated Hide outdated includes/discovery/sensors/current/adva.inc.php
'high_warn_limit' => 25,
'high_limit' => 30));
foreach (array_keys($pre_cache['fsp150']) as $index1) {

This comment has been minimized.

@laf

laf May 21, 2017

Member

Is this double foreach needed?

@laf

laf May 21, 2017

Member

Is this double foreach needed?

This comment has been minimized.

@czilian

czilian May 21, 2017

Contributor

Not for only one sensor, true. However, suggest to leave it still like this because then one can easily add another sensor into the $sensor array.

@czilian

czilian May 21, 2017

Contributor

Not for only one sensor, true. However, suggest to leave it still like this because then one can easily add another sensor into the $sensor array.

Show outdated Hide outdated includes/discovery/sensors/current/adva.inc.php
foreach (array_keys($pre_cache['fsp3kr7_Card']) as $index) {
//AC PSU Limits
if ($pre_cache['fsp3kr7_Card'][$index]['eqptPhysInstValuePsuAmpere']) {
if ($pre_cache['fsp3kr7_Card'][$index]['eqptPhysInstValuePsuAmpere'] > 1000) {

This comment has been minimized.

@laf

laf May 21, 2017

Member

Combine this if check with the one above.

@laf

laf May 21, 2017

Member

Combine this if check with the one above.

This comment has been minimized.

@czilian

czilian May 21, 2017

Contributor

Sorry, this was unfinished and forgotten business. Wanted to distinguish between AC/DC PSUs to set the correct limits. Since I kicked out the hardcoded limits, this went away as well.

@czilian

czilian May 21, 2017

Contributor

Sorry, this was unfinished and forgotten business. Wanted to distinguish between AC/DC PSUs to set the correct limits. Since I kicked out the hardcoded limits, this went away as well.

Show outdated Hide outdated includes/polling/os/adva.inc.php
// *********** FSP150 Devices
if (starts_with($device['sysObjectID'], 'enterprises.2544.1.12.1.1')) {
$version = 'FSP150 SW V'.trim(snmp_get($device, "entPhysicalSoftwareRev.1", "-OQv", "ADVA-MIB"), '"');

This comment has been minimized.

@laf

laf May 21, 2017

Member

Not sure we need to prepend FSP150 SW V here.

@laf

laf May 21, 2017

Member

Not sure we need to prepend FSP150 SW V here.

This comment has been minimized.

@czilian

czilian May 21, 2017

Contributor

No. Took it out.

@czilian

czilian May 21, 2017

Contributor

No. Took it out.

Show outdated Hide outdated includes/polling/os/adva.inc.php
$hardware = 'ADVA '.trim(snmp_get($device, "entPhysicalName.1", "-OQv", "ADVA-MIB"), '"')
.' V'.trim(snmp_get($device, "entPhysicalHardwareRev.1", "-OQv", "ADVA-MIB"), '"');
$serial = trim(snmp_get($device, "entPhysicalSerialNum.1", "-OQv", "ADVA-MIB"), '"');
$features = ''; //search for PTP info in MIB

This comment has been minimized.

@laf

laf May 21, 2017

Member

You don't need to set this if you aren't populating it.

@laf

laf May 21, 2017

Member

You don't need to set this if you aren't populating it.

This comment has been minimized.

@czilian

czilian May 21, 2017

Contributor

Unfinished business. Took it out for now. Want to pull license based features for which I still need to search for the OID

@czilian

czilian May 21, 2017

Contributor

Unfinished business. Took it out for now. Want to pull license based features for which I still need to search for the OID

Show outdated Hide outdated includes/polling/os/adva.inc.php
// ********** FSP3000 R7 Devices
if (starts_with($device['sysObjectID'], 'enterprises.2544.1.11.1.1')) {
$version = 'FSP3000R7 SW V'.trim(snmp_get($device, "swVersionActiveApplSw.100737280", "-OQv", "ADVA-MIB"), '"');

This comment has been minimized.

@laf

laf May 21, 2017

Member

Same comments as above.

@laf

laf May 21, 2017

Member

Same comments as above.

Show outdated Hide outdated mibs/adva/8021ae.mib
--
-- *****************************************************************
IEEE8021-SECY-MIB DEFINITIONS ::= BEGIN

This comment has been minimized.

@laf

laf May 21, 2017

Member

This mib already exists: ``mibs/IEEE8021-SECY-MIB`.

I expect it's the same for a lot of the others, please remove the duplicates.

@laf

laf May 21, 2017

Member

This mib already exists: ``mibs/IEEE8021-SECY-MIB`.

I expect it's the same for a lot of the others, please remove the duplicates.

This comment has been minimized.

@czilian

czilian May 21, 2017

Contributor

done.

@czilian

czilian May 21, 2017

Contributor

done.

Show outdated Hide outdated tests/snmpsim/adva.snmprec
@@ -0,0 +1,2 @@
1.3.6.1.2.1.1.1.0|4|FSP150EG-X

This comment has been minimized.

@laf

laf May 21, 2017

Member

Is this the full sysDescr value?

@laf

laf May 21, 2017

Member

Is this the full sysDescr value?

This comment has been minimized.

@czilian

czilian May 21, 2017

Contributor

For this one yes. But added sim-files for all types of ADVA boxes I have now.

@czilian

czilian May 21, 2017

Contributor

For this one yes. But added sim-files for all types of ADVA boxes I have now.

OS for FSP150 and FSP3000R7 now in two seperate OS definitions
- split the files
- removed sysObjectId check for each
- removed hardcoded sensor limits
- removed more generic MIBs
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented May 22, 2017

Auto-Deploy finished, Test PR at http://6696.ci.librenms.org or https://6696.ci.librenms.org

@czilian

This comment has been minimized.

Show comment
Hide comment
@czilian

czilian May 22, 2017

Contributor

@laf
OS for FSP150 and FSP3000R7 now in two seperate OS definitions

  • split the files
  • removed sysObjectId check for each
  • removed hardcoded sensor limits
  • removed more generic MIBs
Contributor

czilian commented May 22, 2017

@laf
OS for FSP150 and FSP3000R7 now in two seperate OS definitions

  • split the files
  • removed sysObjectId check for each
  • removed hardcoded sensor limits
  • removed more generic MIBs
@laf

Some changes needed but it's definitely getting closer.

Show outdated Hide outdated includes/definitions/adva_fsp150.yaml
@@ -0,0 +1,13 @@
os: adva_fsp150
text: ADVA_FSP150CC

This comment has been minimized.

@laf

laf May 22, 2017

Member

These are display texts so you should do them as human readable like Adva FSP150CC

@laf

laf May 22, 2017

Member

These are display texts so you should do them as human readable like Adva FSP150CC

This comment has been minimized.

@czilian

czilian May 23, 2017

Contributor

thx @laf. Remarks all understood and accepted. Only regarding the HW release I consider this as useful for the Ops people to immediately see which generation is in use. Will be back...

@czilian

czilian May 23, 2017

Contributor

thx @laf. Remarks all understood and accepted. Only regarding the HW release I consider this as useful for the Ops people to immediately see which generation is in use. Will be back...

Show outdated Hide outdated html/images/os/tplink.svg
<svg id="Layer_1" data-name="Layer 1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 210.66 251.52"><defs><style>.cls-1{fill:#4accd5;}.cls-2{isolation:isolate;}</style></defs><title>tplink</title><path class="cls-1" d="M63.63,207.44a11.67,11.67,0,0,1-7.39-6l-.88-1.69L55.17,178,55,156.19,33,156l-22-.19-1.73-1a13.52,13.52,0,0,1-4.74-4.9c-.61-1.12-.67-2.76-.67-20.36V110.36h97v97.25l-18,.06c-9.93,0-18.56-.07-19.18-.22Z" transform="translate(-0.41 -2.66)"/><path class="cls-1" d="M116.65,154.43c-.11-.28-.15-10-.1-21.54l.1-21,10.72-.23c11.07-.24,12.26-.39,17.42-2.18,13-4.52,21.77-18.57,20.8-33.45-1-15.36-10.07-26.36-24.4-29.56a45.63,45.63,0,0,0-14.82,0,33.8,33.8,0,0,0-25.93,25c-.47,1.87-.66,4.89-.8,12.86-.16,9.34-.25,10.42-.8,10.44-3.44.09-42-.09-42.1-.2-.33-.33,0-22.77.34-25.73a76,76,0,0,1,41.76-58.2,74.41,74.41,0,0,1,34.69-8c12.68,0,23.37,2.24,33.84,7.23C188.52,20,202.63,38.34,207.22,61.77c5.29,27-2.85,53.56-22.09,71.92a75.68,75.68,0,0,1-40,20.09c-3.66.66-6.44.82-16.35,1s-12,.1-12.13-.33Z" transform="translate(-0.41 -2.66)"/><g class="cls-2"><path d="M34.62,218.67a10.81,10.81,0,0,1-.09,1.47,3.59,3.59,0,0,1-.26,1,1.33,1.33,0,0,1-.43.55,1,1,0,0,1-.57.18H22.08v31.07a.76.76,0,0,1-.22.54,1.64,1.64,0,0,1-.74.39,8.38,8.38,0,0,1-1.4.24q-.88.09-2.19.09t-2.19-.09a8.38,8.38,0,0,1-1.4-.24,1.63,1.63,0,0,1-.74-.39.76.76,0,0,1-.22-.54V221.86H1.76a1,1,0,0,1-.59-.18,1.39,1.39,0,0,1-.41-.55,3.52,3.52,0,0,1-.26-1,10.73,10.73,0,0,1-.09-1.47,11.31,11.31,0,0,1,.09-1.52,3.54,3.54,0,0,1,.26-1,1.26,1.26,0,0,1,.41-.54,1,1,0,0,1,.59-.16H33.27a1,1,0,0,1,.57.16,1.21,1.21,0,0,1,.43.54,3.61,3.61,0,0,1,.26,1A11.39,11.39,0,0,1,34.62,218.67Z" transform="translate(-0.41 -2.66)"/></g><g class="cls-2"><path d="M70.52,227.16a13.39,13.39,0,0,1-1.17,5.75,11.26,11.26,0,0,1-3.42,4.21,15.91,15.91,0,0,1-5.51,2.61,29.32,29.32,0,0,1-7.69.89H49v12.3a.76.76,0,0,1-.22.54,1.63,1.63,0,0,1-.74.39,8.35,8.35,0,0,1-1.38.24q-.86.09-2.21.09t-2.19-.09a8.38,8.38,0,0,1-1.4-.24,1.5,1.5,0,0,1-.73-.39.8.8,0,0,1-.21-.54V218.26a2.56,2.56,0,0,1,.85-2.1,3.38,3.38,0,0,1,2.23-.7H53.52q1.59,0,3,.1T60,216A16.86,16.86,0,0,1,64,217.28a12,12,0,0,1,3.51,2.34,9.53,9.53,0,0,1,2.21,3.31A11.31,11.31,0,0,1,70.52,227.16Zm-9.5.57a6.14,6.14,0,0,0-.83-3.34,5.31,5.31,0,0,0-2-1.94,7.5,7.5,0,0,0-2.54-.79,22.44,22.44,0,0,0-2.75-.16H49v13.08h4.08a11.29,11.29,0,0,0,3.65-.51,6.36,6.36,0,0,0,2.4-1.42,6,6,0,0,0,1.42-2.17A7.66,7.66,0,0,0,61,227.73Z" transform="translate(-0.41 -2.66)"/></g><g class="cls-2"><path d="M90.77,239a5.4,5.4,0,0,1-.38,2.41,1.28,1.28,0,0,1-1.21.68H74.81a1.31,1.31,0,0,1-1.24-.7,5.41,5.41,0,0,1-.38-2.4,5.22,5.22,0,0,1,.38-2.35,1.31,1.31,0,0,1,1.24-.68H89.18a1.55,1.55,0,0,1,.71.15,1.24,1.24,0,0,1,.5.51,2.69,2.69,0,0,1,.29,1A10.24,10.24,0,0,1,90.77,239Z" transform="translate(-0.41 -2.66)"/><path d="M122.24,250.72a11,11,0,0,1-.09,1.5,4.06,4.06,0,0,1-.26,1,1.31,1.31,0,0,1-.43.58,1,1,0,0,1-.6.18H100.37a3.11,3.11,0,0,1-1.92-.58,2.21,2.21,0,0,1-.78-1.89v-35a.76.76,0,0,1,.22-.54,1.64,1.64,0,0,1,.74-.39,8.18,8.18,0,0,1,1.4-.24q.88-.09,2.19-.09a22,22,0,0,1,2.21.09,8.15,8.15,0,0,1,1.38.24,1.65,1.65,0,0,1,.74.39.76.76,0,0,1,.22.54v31h14.06a1.11,1.11,0,0,1,.6.16,1.22,1.22,0,0,1,.43.54,3.6,3.6,0,0,1,.26,1A11,11,0,0,1,122.24,250.72Z" transform="translate(-0.41 -2.66)"/><path d="M136.44,217.42a3.84,3.84,0,0,1-1.07,3.13q-1.07.86-4,.86t-4-.83a3.72,3.72,0,0,1-1-3,3.92,3.92,0,0,1,1.05-3.14q1.05-.88,4-.88t3.94.85A3.73,3.73,0,0,1,136.44,217.42ZM135.79,253a.74.74,0,0,1-.21.52,1.59,1.59,0,0,1-.71.37,7,7,0,0,1-1.33.22q-.83.07-2.11.07t-2.11-.07a7,7,0,0,1-1.33-.22,1.59,1.59,0,0,1-.71-.37.75.75,0,0,1-.21-.52V226.48a.75.75,0,0,1,.21-.52,1.69,1.69,0,0,1,.71-.39,6.8,6.8,0,0,1,1.33-.25,25,25,0,0,1,4.21,0,6.8,6.8,0,0,1,1.33.25,1.69,1.69,0,0,1,.71.39.74.74,0,0,1,.21.52Z" transform="translate(-0.41 -2.66)"/><path d="M173.9,253a.74.74,0,0,1-.21.52,1.58,1.58,0,0,1-.69.37,6.7,6.7,0,0,1-1.33.22q-.85.07-2.09.07c-.85,0-1.56,0-2.12-.07a6.7,6.7,0,0,1-1.33-.22,1.57,1.57,0,0,1-.69-.37.74.74,0,0,1-.21-.52V237.83a11.09,11.09,0,0,0-.33-3,5.6,5.6,0,0,0-1-1.89,4.1,4.1,0,0,0-1.61-1.22,5.69,5.69,0,0,0-2.3-.43,6.36,6.36,0,0,0-3.35,1,17.12,17.12,0,0,0-3.52,3V253a.74.74,0,0,1-.21.52,1.59,1.59,0,0,1-.71.37,7,7,0,0,1-1.33.22q-.83.07-2.11.07t-2.11-.07a7,7,0,0,1-1.33-.22,1.59,1.59,0,0,1-.71-.37.75.75,0,0,1-.21-.52V226.42a.82.82,0,0,1,.17-.52,1.28,1.28,0,0,1,.62-.37,5.62,5.62,0,0,1,1.16-.22,17.47,17.47,0,0,1,1.78-.07q1.1,0,1.83.07a4.69,4.69,0,0,1,1.12.22,1.26,1.26,0,0,1,.57.37.83.83,0,0,1,.17.52v3.07a19.43,19.43,0,0,1,5.2-3.57,13.28,13.28,0,0,1,5.51-1.19,13.9,13.9,0,0,1,5.3.89,9.15,9.15,0,0,1,3.49,2.43,9.29,9.29,0,0,1,1.92,3.59,18.14,18.14,0,0,1,.59,4.95Z" transform="translate(-0.41 -2.66)"/><path d="M211.07,253a.78.78,0,0,1-.19.52,1.36,1.36,0,0,1-.69.36,8.23,8.23,0,0,1-1.36.21c-.58,0-1.33.07-2.25.07s-1.71,0-2.3-.06a9.35,9.35,0,0,1-1.45-.19,2,2,0,0,1-1.43-1l-10.26-14V253a.74.74,0,0,1-.21.52,1.58,1.58,0,0,1-.71.37,7,7,0,0,1-1.33.22q-.83.07-2.11.07t-2.11-.07a7,7,0,0,1-1.33-.22,1.58,1.58,0,0,1-.71-.37.75.75,0,0,1-.21-.52V213.82a.8.8,0,0,1,.21-.54,1.57,1.57,0,0,1,.71-.4,6.66,6.66,0,0,1,1.33-.25,24.77,24.77,0,0,1,4.22,0,6.66,6.66,0,0,1,1.33.25,1.58,1.58,0,0,1,.71.4.8.8,0,0,1,.21.54v23.32l9.12-10.57a3.5,3.5,0,0,1,.62-.63,2.52,2.52,0,0,1,.92-.42,8.71,8.71,0,0,1,1.42-.22q.85-.07,2.16-.07t2.16.07a8,8,0,0,1,1.42.22,1.62,1.62,0,0,1,.74.37.78.78,0,0,1,.21.55,2.09,2.09,0,0,1-.26,1,5,5,0,0,1-.78,1.07l-9,8.82,10.43,13.91a6.4,6.4,0,0,1,.6,1A2,2,0,0,1,211.07,253Z" transform="translate(-0.41 -2.66)"/></g></svg>

This comment has been minimized.

@laf

laf May 22, 2017

Member

Can you revert this file as it shouldn't be part of this PR.

@laf

laf May 22, 2017

Member

Can you revert this file as it shouldn't be part of this PR.

Show outdated Hide outdated html/images/os/adva.svg
@@ -0,0 +1 @@
<svg id="Layer_1" data-name="Layer 1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 328.07 318.86"><defs><style>.cls-1{fill:none;}.cls-2{fill:#e80707;}.cls-3{fill:#3950a3;stroke:#305e84;stroke-miterlimit:10;}.cls-4{fill:#004588;}.cls-5{fill:#28e87e;}</style></defs><title>adva</title><line class="cls-1" x1="287.28" y1="281.75" x2="322.32" y2="281.75"/><line class="cls-1" x1="289.03" y1="288.25" x2="326.32" y2="288.25"/><path class="cls-1" d="M328.32,309.5" transform="translate(-2)"/><line class="cls-1" x1="328.07" y1="309.5" x2="301.32" y2="282.75"/><path class="cls-2" d="M75.53,40.57A106.34,106.34,0,0,0,52.79,98.65h64.63a43.93,43.93,0,0,1,3.82-10.25Z" transform="translate(-2)"/><path class="cls-2" d="M159.28,0a106.29,106.29,0,0,0-66.5,23.26l45.33,47.43a44,44,0,0,1,65.14,28h62.52A106.82,106.82,0,0,0,159.28,0Z" transform="translate(-2)"/><path class="cls-2" d="M122,130.3a48.34,48.34,0,0,1-3.07-7H53.82a116.68,116.68,0,0,0,23.42,54.63Z" transform="translate(-2)"/><path class="cls-2" d="M201.76,123.28c-6.17,17.54-22.36,30.09-41.42,30.09a42.54,42.54,0,0,1-20.92-5.51L94.94,194a103.87,103.87,0,0,0,64.34,22.37c53.2,0,97.29-40.31,105.46-93.07Z" transform="translate(-2)"/><path class="cls-3" d="M212,327.17" transform="translate(-2)"/><path class="cls-3" d="M251,249.26" transform="translate(-2)"/><path class="cls-4" d="M38.15,250.94H63.46L81,298.56H12.84Zm-.7,39.72H57.59l-8.53-24.27Z" transform="translate(-2)"/><polygon class="cls-4" points="30.14 304.37 7.83 304.37 0 318.7 23.14 318.7 30.14 304.37"/><polygon class="cls-4" points="80.77 304.37 58.47 304.37 62.1 318.7 85.94 318.7 80.77 304.37"/><path class="cls-3" d="M192.24,324.81" transform="translate(-2)"/><path class="cls-3" d="M165.85,248" transform="translate(-2)"/><path class="cls-4" d="M269.77,250.94h25.31l17.49,47.62H244.46Zm-.7,39.72h20.14l-8.53-24.27Z" transform="translate(-2)"/><polygon class="cls-4" points="261.76 304.37 239.45 304.37 231.62 318.7 254.76 318.7 261.76 304.37"/><polygon class="cls-4" points="312.39 304.37 290.09 304.37 293.72 318.7 317.56 318.7 312.39 304.37"/><polygon class="cls-4" points="183.89 303.81 220.74 303.81 213.26 318.86 189.08 318.86 183.89 303.81"/><polygon class="cls-4" points="225.53 250.94 247.19 250.94 222.96 298.86 204.05 298.86 225.53 250.94"/><polygon class="cls-4" points="182.21 298.86 202.19 298.86 187.56 250.94 166.21 250.94 182.21 298.86"/><path class="cls-4" d="M165.85,277.16a25.94,25.94,0,0,0-5-15.39c-8.21-11.32-22.9-10.84-28.15-10.84H97.25v47.62H118V265.83H132.7c10,0,11.72,12.42,11.64,16.26-.21,11.33-4.09,16.47-4.09,16.47h23.18S166.13,286.77,165.85,277.16Z" transform="translate(-2)"/><path class="cls-1" d="M163.44,298.56" transform="translate(-2)"/><path class="cls-1" d="M132.7,250.94c0,13.29,14.29,24,31.94,24" transform="translate(-2)"/><path class="cls-4" d="M134.1,318.86H97.25V304.37h62.9S149.91,318.86,134.1,318.86Z" transform="translate(-2)"/><path class="cls-1" d="M134.1,304.37" transform="translate(-2)"/><path class="cls-1" d="M160.15,304.37" transform="translate(-2)"/><path class="cls-1" d="M134.1,304.37" transform="translate(-2)"/><path class="cls-5" d="M169.77,314.16" transform="translate(-2)"/></svg>

This comment has been minimized.

@laf

laf May 22, 2017

Member

OS logos should be Y x Y (square), this is slightly different in width. Can you stretch it out?

@laf

laf May 22, 2017

Member

OS logos should be Y x Y (square), this is slightly different in width. Can you stretch it out?

Show outdated Hide outdated includes/discovery/sensors/current/adva_fsp150.inc.php
if ($pre_cache['fsp150'][$index1][$sensor_name]) {
$descr = $pre_cache['fsp150'][$index1]['slotCardUnitName']." [#".$pre_cache['fsp150'][$index1]['slotIndex']."]";
$current = $pre_cache['fsp150'][$index1][$entry];
$sensorType = 'advafsp150';

This comment has been minimized.

@laf

laf May 22, 2017

Member

This should match the OS name so: adva_fsp150

@laf

laf May 22, 2017

Member

This should match the OS name so: adva_fsp150

Show outdated Hide outdated includes/discovery/sensors/current/adva_fsp150.inc.php
'current',
$device,
$oid,
$index1,

This comment has been minimized.

@laf

laf May 22, 2017

Member

You should replace this with "psuOutputCurrent.$index1",

@laf

laf May 22, 2017

Member

You should replace this with "psuOutputCurrent.$index1",

Show outdated Hide outdated includes/discovery/sensors/voltage/adva_fsp3kr7.inc.php
$slotnum = $index;
$descr = strtoupper($pre_cache['fsp3kr7_Card'][$index]['entityEqptAidString'])." Input";
$current = $pre_cache['fsp3kr7_Card'][$index]['eqptPhysInstValuePsuVoltInp'];
$sensorType = 'advafsp3kr7';

This comment has been minimized.

@laf

laf May 22, 2017

Member

Should be the os name.

@laf

laf May 22, 2017

Member

Should be the os name.

Show outdated Hide outdated includes/discovery/sensors/voltage/adva_fsp3kr7.inc.php
'voltage',
$device,
$oid,
$index,

This comment has been minimized.

@laf

laf May 22, 2017

Member

Should be "eqptPhysInstValuePsuVoltInp.$index",

@laf

laf May 22, 2017

Member

Should be "eqptPhysInstValuePsuVoltInp.$index",

Show outdated Hide outdated includes/discovery/sensors/voltage/adva_fsp3kr7.inc.php
$descr,
$divisor,
$multiplier,
$low_limit,

This comment has been minimized.

@laf

laf May 22, 2017

Member

Should be set to null if you haven't defined them.

@laf

laf May 22, 2017

Member

Should be set to null if you haven't defined them.

// *********** FSP150 Devices
$version = trim(snmp_get($device, "entPhysicalSoftwareRev.1", "-OQv", "ADVA-MIB"), '"');
$hardware = trim(snmp_get($device, "entPhysicalName.1", "-OQv", "ADVA-MIB"), '"')
.' V'.trim(snmp_get($device, "entPhysicalHardwareRev.1", "-OQv", "ADVA-MIB"), '"');

This comment has been minimized.

@laf

laf May 22, 2017

Member

I'm not sure we should store the hardware rev

@laf

laf May 22, 2017

Member

I'm not sure we should store the hardware rev

// ********** FSP3000 R7 Devices
$version = trim(snmp_get($device, "swVersionActiveApplSw.100737280", "-OQv", "ADVA-MIB"), '"');
$hardware = trim(snmp_get($device, "inventoryUnitName.33619968", "-OQv", "ADVA-MIB"), '"')
.' V'.trim(snmp_get($device, "inventoryHardwareRev.33619968", "-OQv", "ADVA-MIB"), '"');

This comment has been minimized.

@laf

laf May 22, 2017

Member

I'm not sure we should store the hardware rev

@laf

laf May 22, 2017

Member

I'm not sure we should store the hardware rev

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented May 23, 2017

Auto-Deploy finished, Test PR at http://6696.ci.librenms.org or https://6696.ci.librenms.org

@czilian

This comment has been minimized.

Show comment
Hide comment
@czilian

czilian May 23, 2017

Contributor

@laf Greetings. The variables related stuff should be all done. For the RRD filename I rather want to use the more meaningful definition as you can see here.
2017-05-23 1
The adva mib was searching for 2 LLDP mibs. Hence, put the one into the ADVA mib folder.
2017-05-23
Regarding the OS logos:
1.) the "improved" tplink.svg sneaked into this branch. Problem is I don't know how to get back your original one. Tried checkout -- filename but this just deleted mine. Tried now by copying your original version into this branch.
2.) ADVA logo. Not sure why the logo is not 100% because it is an export from an AI art-board 320x320.

Contributor

czilian commented May 23, 2017

@laf Greetings. The variables related stuff should be all done. For the RRD filename I rather want to use the more meaningful definition as you can see here.
2017-05-23 1
The adva mib was searching for 2 LLDP mibs. Hence, put the one into the ADVA mib folder.
2017-05-23
Regarding the OS logos:
1.) the "improved" tplink.svg sneaked into this branch. Problem is I don't know how to get back your original one. Tried checkout -- filename but this just deleted mine. Tried now by copying your original version into this branch.
2.) ADVA logo. Not sure why the logo is not 100% because it is an export from an AI art-board 320x320.

@czilian

This comment has been minimized.

Show comment
Hide comment
@czilian

czilian May 29, 2017

Contributor

Anything you still want me to change?

Contributor

czilian commented May 29, 2017

Anything you still want me to change?

@laf

I've put some comments in for a few files but the same comments are applicable to most of your other files.

LLDP-V2-TC-MIB and LLDP-V2-MIB need moving to mibs/ dir and that will fix that error.

After this, should be good. Thanks again :)

Show outdated Hide outdated includes/discovery/sensors/current/adva_fsp150.inc.php
$sensor_name = $entry['sensor_name'];
if ($pre_cache['adva_fsp150'][$index][$sensor_name]) {
$oid = $entry['sensor_oid'].".".$index;
$rrd_filename = $pre_cache['adva_fsp150'][$index]['slotCardUnitName']."-".$pre_cache['adva_fsp150'][$index]['slotIndex'];

This comment has been minimized.

@laf

laf May 31, 2017

Member

I really think this is over complicating things. $rrd_filename should be named $index anyway as that's what the variable is but the setting of how it is is overkill and should be simplified to psuOutputCurrent.$index

This can be looked up in the db if more info is needed to work out what that index references.

@laf

laf May 31, 2017

Member

I really think this is over complicating things. $rrd_filename should be named $index anyway as that's what the variable is but the setting of how it is is overkill and should be simplified to psuOutputCurrent.$index

This can be looked up in the db if more info is needed to work out what that index references.

Show outdated Hide outdated includes/discovery/sensors/current/adva_fsp150.inc.php
$oid = $entry['sensor_oid'].".".$index;
$rrd_filename = $pre_cache['adva_fsp150'][$index]['slotCardUnitName']."-".$pre_cache['adva_fsp150'][$index]['slotIndex'];
$descr = $pre_cache['adva_fsp150'][$index]['slotCardUnitName']." [#".$pre_cache['adva_fsp150'][$index]['slotIndex']."]";
$current = $pre_cache['adva_fsp150'][$index][$entry];

This comment has been minimized.

@laf

laf May 31, 2017

Member

This needs to be divided by $divisor otherwise high/low values will be wrong.

@laf

laf May 31, 2017

Member

This needs to be divided by $divisor otherwise high/low values will be wrong.

Show outdated Hide outdated includes/discovery/sensors/current/adva_fsp150.inc.php
$device,
$oid,
$rrd_filename,
$sensorType,

This comment has been minimized.

@laf

laf May 31, 2017

Member

You should just put 'adva_fsp150' here

@laf

laf May 31, 2017

Member

You should just put 'adva_fsp150' here

Show outdated Hide outdated includes/discovery/sensors/current/adva_fsp3kr7.inc.php
$device,
$oid,
$rrd_filename,
$sensorType,

This comment has been minimized.

@laf

laf May 31, 2017

Member

You should just put 'adva_fsp3kr7' here.

@laf

laf May 31, 2017

Member

You should just put 'adva_fsp3kr7' here.

Show outdated Hide outdated includes/discovery/sensors/current/adva_fsp3kr7.inc.php
if ($pre_cache['adva_fsp3kr7_Card'][$index]['eqptPhysInstValuePsuAmpere']) {
$oid = '.1.3.6.1.4.1.2544.1.11.11.1.2.1.1.1.6.'.$index;
$descr = strtoupper($pre_cache['adva_fsp3kr7_Card'][$index]['entityEqptAidString'])." Input";
$rrd_filename = $descr;

This comment has been minimized.

@laf

laf May 31, 2017

Member

Same as the previous file, rrd_filename should be index.

@laf

laf May 31, 2017

Member

Same as the previous file, rrd_filename should be index.

Show outdated Hide outdated includes/discovery/sensors/current/adva_fsp3kr7.inc.php
$oid = '.1.3.6.1.4.1.2544.1.11.11.1.2.1.1.1.6.'.$index;
$descr = strtoupper($pre_cache['adva_fsp3kr7_Card'][$index]['entityEqptAidString'])." Input";
$rrd_filename = $descr;
$current = $pre_cache['adva_fsp3kr7_Card'][$index]['eqptPhysInstValuePsuAmpere'];

This comment has been minimized.

@laf

laf May 31, 2017

Member

Needs dividing by $divisor

@laf

laf May 31, 2017

Member

Needs dividing by $divisor

- corrected 'current' readings by adding $devisor
- changed rrdfile names to sensorname+$index
- moved LLDP-V2 mibs to main mib directory
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Jun 2, 2017

Auto-Deploy finished, Test PR at http://6696.ci.librenms.org or https://6696.ci.librenms.org

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Jun 2, 2017

The inspection completed: No new issues

scrutinizer-notifier commented Jun 2, 2017

The inspection completed: No new issues

@czilian

I hope this complies now with your code philosophy. But I do agree it should be consistent throughout all NEs.

@laf

laf approved these changes Jun 2, 2017

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jun 2, 2017

Member

image

Member

laf commented Jun 2, 2017

image

@laf laf merged commit f784228 into librenms:master Jun 2, 2017

3 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@czilian czilian deleted the czilian:ADVA-support-V1 branch Apr 24, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2018

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