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

Os netagent2 3phase support #9175

Merged
merged 9 commits into from Oct 17, 2018

Conversation

Projects
None yet
4 participants
@sippe2
Contributor

sippe2 commented Sep 9, 2018

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

@sippe2

This comment has been minimized.

Contributor

sippe2 commented Sep 9, 2018

I think Travis CI and codeclimate hates me... :)

@murrant

This comment has been minimized.

Member

murrant commented Sep 10, 2018

That sysObjectID is already used by powerwalker. (aka perhaps there should not have been two OS in the first place)

@murrant

This comment has been minimized.

Member

murrant commented Sep 10, 2018

If there does need to be two, use this for detection (delete sysDescr)

    -
      sysObjectID:
        - .1.3.6.1.4.1.935
      sysObjectID_except:
        - .1.3.6.1.4.1.935.10
@sippe2

This comment has been minimized.

Contributor

sippe2 commented Sep 11, 2018

screenshot 2018-09-11 at 11 09 43

Unfortunately looks like that there is a lot of devices under that device and identical oid. I'll try to find out how those can be separated...

Actually there is same kind of issue under carel. There is also a lot of devices under same oid. This time issue is a bit more tricky because that pCOWeb snmp adapter gives possibility to change oid by our self under webskin.

What you suggest?

@murrant

This comment has been minimized.

Member

murrant commented Sep 13, 2018

Fun, good luck, let us know if you need any help.

@sippe2

This comment has been minimized.

Contributor

sippe2 commented Sep 14, 2018

If someone can send snmprec files... it might help a lot. Emerson, SEC and Phoenixtec are in my mind...

@sippe2

This comment has been minimized.

Contributor

sippe2 commented Sep 17, 2018

Some state elements still need to be written:

upsThreePhaseUPSStatusInverterOperating.0
upsThreePhaseUPSStatusACStatus.0
upsThreePhaseUPSStatusManualBypassBreaker.0
upsThreePhaseDCandRectifierStatusRecOperating.0
upsThreePhaseDCandRectifierStatusChargeStatus.0
upsThreePhaseDCandRectifierStatusBatteryStatus.0
upsThreePhaseDCandRectifierStatusInAndOut.0
upsThreePhaseDCandRectifierStatusRecRotError.0
upsThreePhaseFaultStatusShortCircuit.0
upsThreePhaseUPSStaticSwitchMode.0
upsThreePhaseUPSStatusBypassFreqFail.0

@laf

We don't have any existing test data for these so I'm unsure if it will break anything.

Couple of comments in the code, other than that it looks good to me. Thanks for the contribution.

@@ -23,6 +23,9 @@
* @author Tony Murray <murraytony@gmail.com>
*/
// This functionality moved to file includes/definitions/discovery/netagent2.inc.php

This comment has been minimized.

@laf

laf Sep 19, 2018

Member

Just delete this file. It's in git history anyway.

- { descr: noBatteryNeedsReplacing, graph: 0, value: 1, generic: 0 }
- { descr: batteryNeedsReplacing, graph: 0, value: 2, generic: 2 }
-
oid: upsBaseOutputStatus

This comment has been minimized.

@laf

laf Sep 19, 2018

Member

This was the existing sensor moved from php, because the index and state name has changed then people will lose data, you should keep them the same here for backwards compatibility (unless the previous way was broke).

sippe2 and others added some commits Sep 23, 2018

@laf

laf approved these changes Oct 13, 2018

LGTM

@laf laf added this to the 1.45 milestone Oct 13, 2018

@laf laf added the Device 🖥 label Oct 13, 2018

@laf laf merged commit 7d5035d into librenms:master Oct 17, 2018

2 of 3 checks passed

codeclimate 11 issues to fix
Details
WIP ready for review
Details
license/cla Contributor License Agreement is signed.
Details

murrant added a commit that referenced this pull request Oct 18, 2018

@trs80

This comment has been minimized.

Contributor

trs80 commented Oct 20, 2018

My battery voltage showing as 0 now, although .1.3.6.1.4.1.935.1.1.1.2.2.2.0 still returns 22 and I can't see anything in the diff that would make it not query that. Single-phase system.

@sippe2

This comment has been minimized.

Contributor

sippe2 commented Oct 20, 2018

Intresting... What happens if you try to discovery your ups again? What kind of ups and netagent2 -version you have? Can you send the full snmp tree? If you get real value from oid .1.3.6.1.4.1.935.1.1.1.2.2.2.0 there should not to be any reason why you can't see the voltage graphics...

@sippe2

This comment has been minimized.

Contributor

sippe2 commented Oct 20, 2018

Everything else works nicely?

@murrant

This comment has been minimized.

Member

murrant commented Oct 28, 2018

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

https://community.librenms.org/t/v1-45-release-changelog-october-2018/6016/1

@trs80

This comment has been minimized.

Contributor

trs80 commented Nov 16, 2018

The sensor was missing the OID, I found why:

diff --git a/includes/discovery/sensors/voltage/netagent2.inc.php b/includes/discovery/sensors/voltage/netagent2.inc.php
index 5700a02..0d1c570 100644
--- a/includes/discovery/sensors/voltage/netagent2.inc.php
+++ b/includes/discovery/sensors/voltage/netagent2.inc.php
@@ -389,7 +389,7 @@ if ($out_phaseNum == '3') {
 // BATTERY Voltage
 # Set divisor and limit ranges 1 phase UPS systems
 if ($in_phaseNum == '1') {
-    $battery_voltage_oid = '.1.3.6.1.4.1.935.1.1.1.2.2.2.0';
+    $battery_voltage1_oid = '.1.3.6.1.4.1.935.1.1.1.2.2.2.0';
     $battery_voltage1 = snmp_get($device, $battery_voltage_oid, '-Oqv');
     $limit = $bat_1phase_limit;
     $warnlimit = $bat_1phase_warnlimit;

Do you need me to make a branch to merge?

@sippe2

This comment has been minimized.

Contributor

sippe2 commented Nov 17, 2018

Please feel free to make the fix branch if you have time to make it. Nice work with the bug hunt. Thank you.

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