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

newdevice : Added additional sensors for APC IRRP100 Air Conditionner series #7006

Merged
merged 38 commits into from Jul 15, 2017

Conversation

Projects
None yet
7 participants
@mail4git38
Contributor

mail4git38 commented Jul 11, 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

Fix #6855

mail4git38 and others added some commits Mar 16, 2017

Update AUTHORS.md
I agree to the conditions of the Contributor Agreement contained in doc/General/Contributing.md.
Merge branch 'master' of https://github.com/librenms/librenms into is…
…sue-6161

Merge before pushing requested changes for PR 6207
Merge remote-tracking branch 'refs/remotes/librenms/master'
# Conflicts:
#	AUTHORS.md
#	includes/discovery/sensors/state/hirschmann.inc.php
Merge branch 'master' of https://github.com/librenms/librenms
Conflicts:
	AUTHORS.md
	includes/discovery/sensors/state/hirschmann.inc.php
Merge branch 'master' of https://github.com/mail4git38/librenms
Conflicts:
	AUTHORS.md
	includes/discovery/sensors/state/hirschmann.inc.php
Merge remote-tracking branch 'refs/remotes/librenms/master'
# Conflicts:
#	includes/discovery/sensors/state/hirschmann.inc.php
Merge branch 'master' of https://github.com/mail4git38/librenms
# Conflicts:
#	includes/discovery/sensors/state/hirschmann.inc.php
@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Jul 11, 2017

Thank you for submitting a PR @mail4git38! We have found the following @murrant and @Rosiak based on the history of these files to review this PR.

mention-bot commented Jul 11, 2017

Thank you for submitting a PR @mail4git38! We have found the following @murrant and @Rosiak based on the history of these files to review this PR.

@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant Jul 11, 2017

CLA assistant check
All committers have signed the CLA.

CLAassistant commented Jul 11, 2017

CLA assistant check
All committers have signed the CLA.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Jul 11, 2017

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

@laf

Thanks for the PR. You've got quite a few changes that are needed due to the change in yaml format we use but in general there's a few issues that also need to be addressed.

Show outdated Hide outdated includes/definitions/discovery/apc.yaml
- { descr: inverterSpotmode, graph: 0, value: 22, generic: 0 }
-
oid: airIRRP100UnitStatusOperateMode

This comment has been minimized.

@laf

laf Jul 11, 2017

Member

This will go for all of the new ones you've added but you should set:

index: 'airIRRP100UnitStatusOperateMode.{{ $index }}'

here.

@laf

laf Jul 11, 2017

Member

This will go for all of the new ones you've added but you should set:

index: 'airIRRP100UnitStatusOperateMode.{{ $index }}'

here.

This comment has been minimized.

@mail4git38

mail4git38 Jul 11, 2017

Contributor

The index line is now added on the missing definitions.

@mail4git38

mail4git38 Jul 11, 2017

Contributor

The index line is now added on the missing definitions.

@@ -15,3 +15,65 @@
$high_limit = 3000;
discover_sensor($valid['sensor'], 'runtime', $device, $oid, $index, $type, $descr, $divisor, '1', $low_limit, $low_limit_warn, $warn_limit, $high_limit, $current);
}
// InRow IRRP100
$oids = snmp_get($device, 'airIRRP100GroupSetpointsCoolMetric.0', '-OsqnU', 'PowerNet-MIB');

This comment has been minimized.

@laf

laf Jul 11, 2017

Member

Can these not be done in yaml?

@laf

laf Jul 11, 2017

Member

Can these not be done in yaml?

This comment has been minimized.

@mail4git38

mail4git38 Jul 11, 2017

Contributor

I decided to do this discovery in a dedicated file because we nned to know the alarm status (enable or not) on the runtime thresolds.

@mail4git38

mail4git38 Jul 11, 2017

Contributor

I decided to do this discovery in a dedicated file because we nned to know the alarm status (enable or not) on the runtime thresolds.

This comment has been minimized.

@laf

laf Jul 11, 2017

Member

Sorry I'm not sure what that means, the yaml will still record data like this does it's just abstracted away from writing code.

@laf

laf Jul 11, 2017

Member

Sorry I'm not sure what that means, the yaml will still record data like this does it's just abstracted away from writing code.

This comment has been minimized.

@mail4git38

mail4git38 Jul 12, 2017

Contributor

On the APC Air Conditionner, you can set an alert (number of weeks of runtime) on a few parts runtime (air filter, comrpessor,...). This alert can be enabled or not.
With the dedicated apc.inc.php runtime file, I just want to check if these alerts are enabled or not. If they are, we need to set the $high_limit value with the value (number of weeks converted in minutes) set in the Air Conditionner. If the alarm is disabled, the $high_limit is set to null.
I don't know if we can do this in the yaml file...

@mail4git38

mail4git38 Jul 12, 2017

Contributor

On the APC Air Conditionner, you can set an alert (number of weeks of runtime) on a few parts runtime (air filter, comrpessor,...). This alert can be enabled or not.
With the dedicated apc.inc.php runtime file, I just want to check if these alerts are enabled or not. If they are, we need to set the $high_limit value with the value (number of weeks converted in minutes) set in the Air Conditionner. If the alarm is disabled, the $high_limit is set to null.
I don't know if we can do this in the yaml file...

This comment has been minimized.

@laf

laf Jul 13, 2017

Member

Makes sense and no, you can't do that in yaml right now.

@laf

laf Jul 13, 2017

Member

Makes sense and no, you can't do that in yaml right now.

Show outdated Hide outdated includes/discovery/sensors/state/hirschmann.inc.php
@@ -34,10 +34,10 @@
foreach ($oid as $index => $entry) {
//Discover Sensors
discover_sensor($valid['sensor'], 'state', $device, $cur_oid.$index, 'hmPowerSupplyStatus'.$index, $state_name, 'Power Supply '.$oid[$index]['hmPSID'], '1', '1', null, null, null, null, $oid[$index]['hmPowerSupplyStatus'], 'snmp', 'hmPowerSupplyStatus'.$index);
discover_sensor($valid['sensor'], 'state', $device, $cur_oid.$index, $index, $state_name, 'Power Supply '.$oid[$index]['hmPSID'], '1', '1', null, null, null, null, $oid[$index]['hmPowerSupplyStatus'], 'snmp', $index);

This comment has been minimized.

@laf

laf Jul 11, 2017

Member

Any reason why you've changed all the index values? this will cause a loss of data and shouldn't be done anyway as it's unique per sensor class.

@laf

laf Jul 11, 2017

Member

Any reason why you've changed all the index values? this will cause a loss of data and shouldn't be done anyway as it's unique per sensor class.

This comment has been minimized.

@mail4git38

mail4git38 Jul 11, 2017

Contributor

This hirschmann file should not be part of this PR. How can I discard the commit of this file ?

@mail4git38

mail4git38 Jul 11, 2017

Contributor

This hirschmann file should not be part of this PR. How can I discard the commit of this file ?

This comment has been minimized.

@laf

laf Jul 11, 2017

Member

If you edit the files and put it back to how it was then push that commit it will remove the changes.

@laf

laf Jul 11, 2017

Member

If you edit the files and put it back to how it was then push that commit it will remove the changes.

Show outdated Hide outdated includes/polling/os/apc.inc.php
@@ -1,94 +1,20 @@
<?php
// PDU
$serial = trim(snmp_get($device, 'PowerNet-MIB::rPDUIdentSerialNumber.0', '-OQv'), '"');
$apc_serial = snmp_get_multi_oid($device, 'rPDUIdentSerialNumber.0 atsIdentSerialNumber.0 upsAdvIdentSerialNumber.0 sPDUIdentSerialNumber.0 airIRRCUnitIdentSerialNumber.0 isxModularPduIdentSerialNumber.0 airIRRP100UnitIdentSerialNumber.0 airIRRP500UnitIdentSerialNumber.0', '-OUQs', 'PowerNet-MIB');

This comment has been minimized.

@laf

laf Jul 11, 2017

Member

Have you taken this from my PR? If so, does it work ok for you?

@laf

laf Jul 11, 2017

Member

Have you taken this from my PR? If so, does it work ok for you?

This comment has been minimized.

@mail4git38

mail4git38 Jul 11, 2017

Contributor

Yes I've taken this from your PR and it seems to work fine with my Air Conditionner check. I will test it with APC PDU, Automatic Transfer Switch and Netbotz devices.

@mail4git38

mail4git38 Jul 11, 2017

Contributor

Yes I've taken this from your PR and it seems to work fine with my Air Conditionner check. I will test it with APC PDU, Automatic Transfer Switch and Netbotz devices.

This comment has been minimized.

@laf

laf Jul 11, 2017

Member

Ace.

@laf

laf Jul 11, 2017

Member

Ace.

This comment has been minimized.

@mail4git38

mail4git38 Jul 12, 2017

Contributor

I've tested you PR on an production environnement and here are the missing OID for an APC Netbotz 200 :

  • $apc_serial : you can add emsIdentSerialNumber.0
  • $apc_model : you can add emsIdentProductNumber.0
  • $apc_hardware : you can add emsIdentHardwareRev.0
  • $apc_version : you can add emsIdentFirmwareRev.0

Regards.

@mail4git38

mail4git38 Jul 12, 2017

Contributor

I've tested you PR on an production environnement and here are the missing OID for an APC Netbotz 200 :

  • $apc_serial : you can add emsIdentSerialNumber.0
  • $apc_model : you can add emsIdentProductNumber.0
  • $apc_hardware : you can add emsIdentHardwareRev.0
  • $apc_version : you can add emsIdentFirmwareRev.0

Regards.

This comment has been minimized.

@laf

laf Jul 13, 2017

Member

Ok, add those in then :)

@laf

laf Jul 13, 2017

Member

Ok, add those in then :)

Updated apc.yaml
@laf I modified the yaml file according to your comment. I forgot the index line in a few definitions...
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Jul 11, 2017

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

@mail4git38

Thanks @laf for your feedbacks. Here are my comments.

Show outdated Hide outdated includes/definitions/discovery/apc.yaml
- { descr: inverterSpotmode, graph: 0, value: 22, generic: 0 }
-
oid: airIRRP100UnitStatusOperateMode

This comment has been minimized.

@mail4git38

mail4git38 Jul 11, 2017

Contributor

The index line is now added on the missing definitions.

@mail4git38

mail4git38 Jul 11, 2017

Contributor

The index line is now added on the missing definitions.

@@ -15,3 +15,65 @@
$high_limit = 3000;
discover_sensor($valid['sensor'], 'runtime', $device, $oid, $index, $type, $descr, $divisor, '1', $low_limit, $low_limit_warn, $warn_limit, $high_limit, $current);
}
// InRow IRRP100
$oids = snmp_get($device, 'airIRRP100GroupSetpointsCoolMetric.0', '-OsqnU', 'PowerNet-MIB');

This comment has been minimized.

@mail4git38

mail4git38 Jul 11, 2017

Contributor

I decided to do this discovery in a dedicated file because we nned to know the alarm status (enable or not) on the runtime thresolds.

@mail4git38

mail4git38 Jul 11, 2017

Contributor

I decided to do this discovery in a dedicated file because we nned to know the alarm status (enable or not) on the runtime thresolds.

Show outdated Hide outdated includes/discovery/sensors/state/hirschmann.inc.php
@@ -34,10 +34,10 @@
foreach ($oid as $index => $entry) {
//Discover Sensors
discover_sensor($valid['sensor'], 'state', $device, $cur_oid.$index, 'hmPowerSupplyStatus'.$index, $state_name, 'Power Supply '.$oid[$index]['hmPSID'], '1', '1', null, null, null, null, $oid[$index]['hmPowerSupplyStatus'], 'snmp', 'hmPowerSupplyStatus'.$index);
discover_sensor($valid['sensor'], 'state', $device, $cur_oid.$index, $index, $state_name, 'Power Supply '.$oid[$index]['hmPSID'], '1', '1', null, null, null, null, $oid[$index]['hmPowerSupplyStatus'], 'snmp', $index);

This comment has been minimized.

@mail4git38

mail4git38 Jul 11, 2017

Contributor

This hirschmann file should not be part of this PR. How can I discard the commit of this file ?

@mail4git38

mail4git38 Jul 11, 2017

Contributor

This hirschmann file should not be part of this PR. How can I discard the commit of this file ?

Show outdated Hide outdated includes/polling/os/apc.inc.php
@@ -1,94 +1,20 @@
<?php
// PDU
$serial = trim(snmp_get($device, 'PowerNet-MIB::rPDUIdentSerialNumber.0', '-OQv'), '"');
$apc_serial = snmp_get_multi_oid($device, 'rPDUIdentSerialNumber.0 atsIdentSerialNumber.0 upsAdvIdentSerialNumber.0 sPDUIdentSerialNumber.0 airIRRCUnitIdentSerialNumber.0 isxModularPduIdentSerialNumber.0 airIRRP100UnitIdentSerialNumber.0 airIRRP500UnitIdentSerialNumber.0', '-OUQs', 'PowerNet-MIB');

This comment has been minimized.

@mail4git38

mail4git38 Jul 11, 2017

Contributor

Yes I've taken this from your PR and it seems to work fine with my Air Conditionner check. I will test it with APC PDU, Automatic Transfer Switch and Netbotz devices.

@mail4git38

mail4git38 Jul 11, 2017

Contributor

Yes I've taken this from your PR and it seems to work fine with my Air Conditionner check. I will test it with APC PDU, Automatic Transfer Switch and Netbotz devices.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Jul 12, 2017

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

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Jul 12, 2017

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

@laf

@mail4git38 You still need to fix the formatting of the yaml file, this now needs data as part of it's structure. It's in the yaml file you edited but you've removed it.

I've made some comments within files again that need to be looked at.

Show outdated Hide outdated includes/discovery/sensors/runtime/apc.inc.php
// airIRRP100UnitRunHoursAirFilter
$index = 0;
$cur_oid = '.1.3.6.1.4.1.318.1.1.13.3.3.1.2.3.1.';
$current = snmp_get($device, 'PowerNet-MIB::airIRRP100UnitRunHoursAirFilter.'.$index, '-Oqv');

This comment has been minimized.

@laf

laf Jul 13, 2017

Member

Please put the PowerNet-MIB at the end of the snmp_get call like:

$current = snmp_get($device, 'airIRRP100UnitRunHoursAirFilter.'.$index, '-Oqv', 'PowerNet-MIB');

Needs to be done for all the other calls as well.

@laf

laf Jul 13, 2017

Member

Please put the PowerNet-MIB at the end of the snmp_get call like:

$current = snmp_get($device, 'airIRRP100UnitRunHoursAirFilter.'.$index, '-Oqv', 'PowerNet-MIB');

Needs to be done for all the other calls as well.

Show outdated Hide outdated includes/discovery/sensors/runtime/apc.inc.php
$service_interval = snmp_get($device, 'PowerNet-MIB::airIRRP100UnitServiceIntervalAirFilter.'.$index, '-Oqv');
$alarm_status= snmp_get($device, 'PowerNet-MIB::airIRRP100UnitServiceIntervalAirFilterAlarm.'.$index, '-Oqv');
$multiplier = 60;
$current = ($current * 60);

This comment has been minimized.

@laf

laf Jul 13, 2017

Member

You can just do $current * $multiplier in all of these so that changing $multiplier will be all that's needed (I know it won't change).

@laf

laf Jul 13, 2017

Member

You can just do $current * $multiplier in all of these so that changing $multiplier will be all that's needed (I know it won't change).

mail4git38 added some commits Jul 13, 2017

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Jul 13, 2017

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

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Jul 13, 2017

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

Updated apc.inc.php after @laf requests
Modified $current calculation, including $multiplier in place of static value and added 'PowerNet-MIB' as an argument of the snmp_get function (detached from OID writing)
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Jul 13, 2017

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

@laf laf referenced this pull request Jul 14, 2017

Merged

newdevice: Added sensor metrics for APC IRRP 100/500 devices #7024

1 of 1 task complete

@murrant murrant closed this Jul 14, 2017

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jul 14, 2017

Member

Think this has more in than mine.

Member

laf commented Jul 14, 2017

Think this has more in than mine.

@laf laf reopened this Jul 14, 2017

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jul 14, 2017

Member

Oh, sorry :-)

Member

murrant commented Jul 14, 2017

Oh, sorry :-)

laf added some commits Jul 15, 2017

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Jul 15, 2017

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

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Jul 15, 2017

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

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Jul 15, 2017

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

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Jul 15, 2017

The inspection completed: No new issues

scrutinizer-notifier commented Jul 15, 2017

The inspection completed: No new issues

@laf

laf approved these changes Jul 15, 2017

@laf laf merged commit 579bfd4 into librenms:master Jul 15, 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

@mail4git38 mail4git38 deleted the mail4git38:issue-6855 branch Jul 16, 2017

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

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

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