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

NFS-server #6320

Merged
merged 16 commits into from Apr 28, 2017

Conversation

Projects
None yet
6 participants
@svennd
Contributor

svennd commented Apr 2, 2017

Hey,

I rewrote the pulling of NFS stat, at current there are 3 implementations :

  1. freeBSD server polling
  2. nfs-stats (which is very limited, and I don't know where the script is)
  3. nfs-v3-stats : which uses this script (although the documentation is wrong, the oid is pointing to nfs-v3-stats)

The problem with the last option is, it depends on a script that does not always report the correct value's. Line 8 of /proc/net/rpc/nfsd on older systems (centos 6) reports nfsv2 stats, and on newer systems (centos 7, debian) nfsv3. These are similar but not exactly the same. Therefore I believe the collected data is faulty in allot of cases.

I used nfs-v3-stats as template to write nfs-server, this also makes it possible to later add nfs-client. (which is also relevant data)

So my personal opinion would be to official drop the second option and leave in nfs-v3-stats for users that trust that the data they collected are correct.

I rewrote it, but took another approach on storing the data, I let rrd do the calculation instead of doing it with a script. This means :
extend nfs-server /bin/cat /proc/net/rpc/nfsd
is enough to get it working.

I do have a few remarks tho, where your advice would be helpful

  • nfs reports tons of stats, many of those however are not used. (fh, th, ... all 0) should we store those ?
  • nfs reports allot of stats, would it be useful to split the rrd's ? This idea I like allot since v2 is being dropped and v4 is changing rapidly (see further) we could keep data even is there is an update in the data-structure later. It would also be possible to disable part of the application.
  • nfsv4ops has changed at least two times, I have seen systems of 40 value's, 59 value's and 71 value's I currently only found what the 59 where... Any help would be great. ( I assume the 59 are inc. in the 71 but I'm not sure on the order)
  • overall advice is also welcome ( be gentle ;) )

Considering I used the code of @crcro and @InsaneSplash , I would be very happy if you could help with advice of your opinions !

// addition
note : after talking to laf, I decided that splitting up would be best as currently some value's can be empty. (v2 in new systems, v4ops in old/new systems) which rrd can't handle.

Please note

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

  • Have you signed the Contributors agreement - please do NOT submit a pull request unless you have (signing the agreement in the same pull request is fine). Your commit message for signing the agreement must appear as per the docs.
  • Have you followed our code guidelines?

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

svennd added some commits Apr 1, 2017

update
just realized the pull script is a bad idea. not really portable. no
nfsv2 stats for centos 7+
fixed v4ops
removed nfs-server.sh, as only cat /proc/net/rpc/nfsd is needed as an
extend.
@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Apr 2, 2017

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

mention-bot commented Apr 2, 2017

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

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Apr 2, 2017

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

split rrd
split rrd to store and pull them seperatly
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Apr 3, 2017

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

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Apr 4, 2017

Member

@svennd You have a decent amount of style fixes needed here.
You can check with ./scripts/pre-commit.php

I think you asked in IRC, but phpcbf will be in ./vendor/bin/

Member

murrant commented Apr 4, 2017

@svennd You have a decent amount of style fixes needed here.
You can check with ./scripts/pre-commit.php

I think you asked in IRC, but phpcbf will be in ./vendor/bin/

scrutinizer-ci fix
scrutinizer-ci fix
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Apr 5, 2017

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

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Apr 5, 2017

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

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Apr 5, 2017

Member

So I guess to answer your questions:

  • nfs reports tons of stats, many of those however are not used. (fh, th, ... all 0) should we store those ?

If they are truly never used then drop them.

  • ***nfs reports allot of stats, would it be useful to split the rrd's ? This idea I like allot since v2 is being dropped and v4 is changing rapidly (see further) we could keep data even is there is an update in the data-structure later. It would also be possible to disable part of the application. ***

I'd be happy for the split.

  • nfsv4ops has changed at least two times, I have seen systems of 40 value's, 59 value's and 71 value's I currently only found what the 59 where... Any help would be great. ( I assume the 59 are inc. in the 71 but I'm not sure on the order)

This seems if we don't know how much data we are getting back. A lot of the time this is why agent/snmp scripts are written.

Member

laf commented Apr 5, 2017

So I guess to answer your questions:

  • nfs reports tons of stats, many of those however are not used. (fh, th, ... all 0) should we store those ?

If they are truly never used then drop them.

  • ***nfs reports allot of stats, would it be useful to split the rrd's ? This idea I like allot since v2 is being dropped and v4 is changing rapidly (see further) we could keep data even is there is an update in the data-structure later. It would also be possible to disable part of the application. ***

I'd be happy for the split.

  • nfsv4ops has changed at least two times, I have seen systems of 40 value's, 59 value's and 71 value's I currently only found what the 59 where... Any help would be great. ( I assume the 59 are inc. in the 71 but I'm not sure on the order)

This seems if we don't know how much data we are getting back. A lot of the time this is why agent/snmp scripts are written.

@svennd

This comment has been minimized.

Show comment
Hide comment
@svennd

svennd Apr 6, 2017

Contributor

@laf thanks for your answers! Well the file tells you how much value's you are to receive : its the first value of the line. The issue is that I can't find what the value's mean. Its a bit more work to find out... if someone knew it would be useful

Contributor

svennd commented Apr 6, 2017

@laf thanks for your answers! Well the file tells you how much value's you are to receive : its the first value of the line. The issue is that I can't find what the value's mean. Its a bit more work to find out... if someone knew it would be useful

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Apr 6, 2017

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

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Apr 6, 2017

Member

rrd is a fixed format though so you need to know how many entries you are going to insert - it has to be predictable really.

Member

laf commented Apr 6, 2017

rrd is a fixed format though so you need to know how many entries you are going to insert - it has to be predictable really.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Apr 7, 2017

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

@svennd

This comment has been minimized.

Show comment
Hide comment
@svennd

svennd Apr 7, 2017

Contributor

please see https://community.librenms.org/t/nfs-server/975

Code is not working.

Contributor

svennd commented Apr 7, 2017

please see https://community.librenms.org/t/nfs-server/975

Code is not working.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Apr 13, 2017

Member

Fair few travis issues that need fixing :)

https://travis-ci.org/librenms/librenms/builds/219620508

Member

laf commented Apr 13, 2017

Fair few travis issues that need fixing :)

https://travis-ci.org/librenms/librenms/builds/219620508

@svennd

This comment has been minimized.

Show comment
Hide comment
@svennd

svennd Apr 13, 2017

Contributor

Yeah, but white-spaces/tabs/enters won't fix the code.

Contributor

svennd commented Apr 13, 2017

Yeah, but white-spaces/tabs/enters won't fix the code.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Apr 13, 2017

Member

What do you mean won't fix the code? They will fix travis, are you having other issues?

Member

laf commented Apr 13, 2017

What do you mean won't fix the code? They will fix travis, are you having other issues?

applied reviews murrant suggestions
applied reviews murrant suggestions
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Apr 13, 2017

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

@svennd

This comment has been minimized.

Show comment
Hide comment
@svennd

svennd Apr 13, 2017

Contributor

https://community.librenms.org/t/nfs-server/975

When the "normal" poller runs it generates device/A.rrd instead of the device/app-nfs-server-default.rrd. the app-nfs-server-proc[2,3,4,4ops].rrd aren't generated either. While manually running
./poller.php -r -f -m applications -h device -d

Shows the correct rrd creation/update.

Contributor

svennd commented Apr 13, 2017

https://community.librenms.org/t/nfs-server/975

When the "normal" poller runs it generates device/A.rrd instead of the device/app-nfs-server-default.rrd. the app-nfs-server-proc[2,3,4,4ops].rrd aren't generated either. While manually running
./poller.php -r -f -m applications -h device -d

Shows the correct rrd creation/update.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Apr 13, 2017

Member

@svennd you are setting up rrdname incorrectly. It is trying to use Array, but only uses the first A. You can only have one rrd per data_update() call.

Member

murrant commented Apr 13, 2017

@svennd you are setting up rrdname incorrectly. It is trying to use Array, but only uses the first A. You can only have one rrd per data_update() call.

dirty scope
dirty scope was the issue
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Apr 14, 2017

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

svennd added some commits Apr 14, 2017

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Apr 14, 2017

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

@svennd

This comment has been minimized.

Show comment
Hide comment
@svennd

svennd Apr 14, 2017

Contributor

Ok, the code works and I made it PSR2 valid (?). It's running here now for a few hours and pulling data in correctly. Perhaps It needs some more documentation.

Contributor

svennd commented Apr 14, 2017

Ok, the code works and I made it PSR2 valid (?). It's running here now for a few hours and pulling data in correctly. Perhaps It needs some more documentation.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Apr 14, 2017

Member

yeah, updating the docs would be good :-)

Member

murrant commented Apr 14, 2017

yeah, updating the docs would be good :-)

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Apr 14, 2017

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

@svennd svennd referenced this pull request Apr 16, 2017

Closed

agree to contributor agreement #6437

2 of 2 tasks complete
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Apr 17, 2017

Member

Needs the contributor PR fixing and the order for the Applications.md (it should be alphabetical) + the other comments below.

Member

laf commented Apr 17, 2017

Needs the contributor PR fixing and the order for the Applications.md (it should be alphabetical) + the other comments below.

@laf

Needs all the is_file() and $rrd_filename updates doing.

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Apr 18, 2017

The inspection completed: 676 Issues, 20 Patches

scrutinizer-notifier commented Apr 18, 2017

The inspection completed: 676 Issues, 20 Patches

@laf

laf approved these changes Apr 18, 2017

@laf

This comment has been minimized.

Show comment
Hide comment
@laf
Member

laf commented Apr 18, 2017

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Apr 27, 2017

Member

@librenms/reviewers I'm merging this in tomorrow unless anyone has any objections.

Member

laf commented Apr 27, 2017

@librenms/reviewers I'm merging this in tomorrow unless anyone has any objections.

@laf laf merged commit 0211c58 into librenms:master Apr 28, 2017

2 of 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 not signed yet.
Details
@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 18, 2018

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

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

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