fix: qnap fanspeed sensor #4590

Merged
merged 1 commit into from Oct 11, 2016

Projects

None yet

8 participants

@crcro
Contributor
crcro commented Sep 26, 2016

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

@laf
Member
laf commented Sep 26, 2016

Again looks ok to me but looking for a second opinion ( @librenms/reviewers )

@murrant
Contributor
murrant commented Sep 26, 2016

I'm not sure I like this going from one snmpwalk to many snmpget calls.
Btw, what does this PR achieve?

@crcro
Contributor
crcro commented Sep 26, 2016

qnap guys choose to change the oid names at version 4.2.0 (i think) from HdTemperature to hdTemperatureEX (all of them this was of top of my mind). so instead of poll based on oid names i poll based on numeric oid

@paulgear
Member

I too wonder what this would do from a performance and backwards compatibility perspective, but I don't have any of these devices to test against. Perhaps we could put the word out on Twitter/community/mailing list to get more feedback? There are only 3 qnap devices in https://stats.librenms.org at the moment, so it's not like we'd be breaking a lot of people's installs...

@paulgear paulgear referenced this pull request Sep 26, 2016
Merged

fix: qnap temperature sensors #4588

2 of 2 tasks complete
@crcro
Contributor
crcro commented Sep 27, 2016

the backward compatibility exists (i've tested on qnap with different os versions), they are the oid from their NAS-MIB, they only change the name values, numerical oid stayed the same

@laf laf added this to the September release milestone Sep 27, 2016
@laf
Member
laf commented Sep 27, 2016

Could we just walk the relevant tree instead so we get everything in one go?

@murrant
Contributor
murrant commented Sep 28, 2016

I actually much prefer the previous code.

@laf laf removed this from the September release milestone Sep 29, 2016
@laf
Member
laf commented Oct 3, 2016

What's the score with this one?

@f0o

Are all fans really addressable via an increment?

@crcro
Contributor
crcro commented Oct 5, 2016

Yes, they have their own oid that it's incremented (one oid for descr and one for speed).

On Oct 5, 2016, at 11:08, Daniel Preussker notifications@github.com wrote:

@f0o commented on this pull request.

Are all fans really addressable via an increment?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@f0o
f0o approved these changes Oct 5, 2016 View changes
@laf
Member
laf commented Oct 8, 2016

Can we not then walk the OIDs rather than doing single gets?

@crcro crcro rewrite for qnap fanspeeds
525acfb
@scrutinizer-notifier

The inspection completed: 3 new issues

@laf laf merged commit b908418 into librenms:master Oct 11, 2016

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zamolxe
zamolxe commented Oct 11, 2016

I think we missed a dot in the code, causing the oid concatenation with indexes to fail.

Including: /data/web/librenms/includes/discovery/sensors/fanspeeds/qnap.inc.php
QNAP: SNMP[ESC[0;36m/usr/bin/snmpbulkwalk -v2c -c COMMUNITY -OQUs -M /data/web/librenms/mibs udp:HOSTNAME:161 .1.3.6.1.4.1.24681.1.2.15.1.2ESC[0m]
enterprises.246*.1.2.1 = "System FAN 1"
enterprises.246*.1.2.2 = "System FAN 2"

SNMP[ESC[0;36m/usr/bin/snmpbulkwalk -v2c -c COMMUNITY -OQUs -M /data/web/librenms/mibs udp:HOSTNAME:161 .1.3.6.1.4.1.24681.1.2.15.1.3ESC[0m]
enterprises.246*.1.3.1 = "1298 RPM"
enterprises.246*.1.3.2 = "1318 RPM"

Discover sensor: .1.3.6.1.4.1.24681.1.2.15.1.31, 1, snmp, System FAN 1, snmp, ,
SQL[ESC[0;33mSELECT COUNT(sensor_id) FROM `sensors` WHERE `poller_type`= 'snmp' AND `sensor_class` = 'fanspeed' AND `device_id` = '12' AND sensor_type = 'snmp' AND `sensor_index` = '1'ESC[0m]
SQL[ESC[0;33mSELECT * FROM `sensors` WHERE `sensor_class` = 'fanspeed' AND `device_id` = '12' AND `sensor_type` = 'snmp' AND `sensor_index` = '1'ESC[0m]
.Discover sensor: .1.3.6.1.4.1.24681.1.2.15.1.32, 2, snmp, System FAN 2, snmp, ,
SQL[ESC[0;33mSELECT COUNT(sensor_id) FROM `sensors` WHERE `poller_type`= 'snmp' AND `sensor_class` = 'fanspeed' AND `device_id` = '12' AND sensor_type = 'snmp' AND `sensor_index` = '2'ESC[0m]
SQL[ESC[0;33mSELECT * FROM `sensors` WHERE `sensor_class` = 'fanspeed' AND `device_id` = '12' AND `sensor_type` = 'snmp' AND `sensor_index` = '2'ESC[0m]
@worton worton added a commit to worton/librenms that referenced this pull request Oct 11, 2016
@crcro @worton crcro + worton refactor: Rewrite for qnap fanspeeds (#4590) e7b315d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment