Add a collector for ZFS, currently focussed on ARC stats. #213

problame · 2016-03-12T14:27:08Z

It is tested on FreeBSD 10.2-RELEASE and Linux (ZFS on Linux 0.6.5.4).
It does not require root privileges to run on either of the above systems.

On FreeBSD, Solaris, etc. ZFS metrics are exposed through sysctls.
ZFS on Linux exposes the same metrics through procfs /proc/spl/....

In addition to sysctl metrics, computed metrics are exposed by
the collector, which are based on several sysctl values.
There is some conditional logic involved in computing these metrics
which cannot be easily mapped to PromQL.

Not all 92 ARC sysctls are exposed right now but this can be changed
with one additional LOC each.

Questions about behavior:

What should happen if -collectors.enabled zfs is set but the stats cannot be found (e.g. because the kernel module is not loaded).
Currently node_exporter terminates with an error message.
If the behavior described above is not desired, should the collector determine at runtime whether data is available?

2.1. Should it expose no metrics if no data is available?
2.2. Could the zfs collector be enabled by default if it does nothing on a non-ZFS system?

It is tested on FreeBSD 10.2-RELEASE and Linux (ZFS on Linux 0.6.5.4). On FreeBSD, Solaris, etc. ZFS metrics are exposed through sysctls. ZFS on Linux exposes the same metrics through procfs `/proc/spl/...`. In addition to sysctl metrics, 'computed metrics' are exposed by the collector, which are based on several sysctl values. There is some conditional logic involved in computing these metrics which cannot be easily mapped to PromQL. Not all 92 ARC sysctls are exposed right now but this can be changed with one additional LOC each.

brian-brazil · 2016-03-12T21:29:05Z

collector/zfs.go

+	fetchedMetrics := make(map[zfsSysctl]prometheus.Gauge)
+
+	insertGauge := func(sysctl, name string) {
+		fetchedMetrics[zfsSysctl(sysctl)] = prometheus.NewGauge(


You should be using ConstMetrics rather than creating Gauges. Gauges are the like are for direction instrumentation, whereas a custom collector like this should use ConstMetrics.

brian-brazil · 2016-03-12T21:35:37Z

The README will also need updating.

problame · 2016-03-12T21:50:49Z

What was causing go vet to fail in the Travis CI build?

brian-brazil · 2016-03-12T21:54:57Z

Looks like a false positive.

Use ConstMetric with UntypedValue. Handle nozfs build flag. Remove computed values.

collector now does not expose metrics if stats cannot be fetched.

problame · 2016-03-13T00:20:54Z

Please review the above commits. Will squash them together as soon as you confirm all issues mentioned have been addressed.

Side note: Novice users will have trouble making sense of / interpreting ZFS statistics. Should we include a link to some resources, e.g. the implementation of zfs-stats?

(It is a popular tool on FreeBSD for getting a quick overview. Currently, node_exporter exposes a subset of zfs-stat's metrics, some of which are computed).

brian-brazil · 2016-03-13T08:16:50Z

collector/zfs.go

+//------------------------------------------------------------------------------
+
+type zfsMetric struct {
+	subsystem zfsSubsystemName // The Prometheus subsystem name


Comments should end in periods

brian-brazil · 2016-03-13T08:28:32Z

Side note: Novice users will have trouble making sense of / interpreting ZFS statistics. Should we include a link to some resources,

Links to reference docs are good. As someone who uses ZFS on linux, the zfs-stats source doesn't seem to add anything that's not already obvious from the names.

brian-brazil · 2016-03-13T08:55:35Z

collector/zfs_linux.go

+	return zfsErrorValue, fmt.Errorf("sysctl '%s' found")
+}
+
+func (p *zfsMetricProvider) prepareUpdateArcstats(zfsArcstatsProcpath string) (err error) {


This would benefit from a unittest

mdlayher · 2016-03-28T03:53:25Z

Neat, did not realize this work was happening. @eliothedeman and I had previously started work on our own ZFS exporter, but integrating the information into node_exporter makes sense to me too.

We expose metrics using go-zfs, but as has been stated, many of these do require root on Linux. I run ZoL and would be very interested in making these stats accessible on Linux as well.

What would be an appropriate way to go about doing this? It seems that creating some sort of "ZFS helper" binary which can independently run as root and spit out metrics for the textfile collector could work.

mdlayher · 2016-03-28T03:59:25Z

collector/zfs_linux.go

+func (c *zfsCollector) zfsAvailable() (err error) {
+	file, err := c.openArcstatsFile()
+	if err != nil {
+		file.Close()


File needs to be closed in the err == nil case as well.

That's new to me. Do you have any links on this one?

Check out this blog: http://blog.golang.org/defer-panic-and-recover.

You could also just use os.Stat() to check for the /proc file instead of actually opening it.

And actually you only need to close the file when the opening was successful. When there was an error, you won't even get a valid *File back, but nil. So the code above would actually crash the entire node exporter with a nil pointer dereference panic in case the file can't be read.

brian-brazil · 2016-03-28T07:41:19Z

What would be an appropriate way to go about doing this? It seems that creating some sort of "ZFS helper" binary which can independently run as root and spit out metrics for the textfile collector could work.

Has someone asked ZoL why all the stats we're interested in require root? No other filesystem requires that that I'm aware of.

problame · 2016-03-28T08:56:28Z

@brian-brazil See here and here (search for delegation).

mdlayher · 2016-03-28T15:16:28Z

Filed an issue with ZoL: openzfs/zfs#4468.

juliusv · 2016-03-28T17:38:20Z

collector/zfs.go

@@ -0,0 +1,109 @@
+package collector


Please include the same copyright/license header here as in all the other Node Exporter files. Same for the other Go files here.

discordianfish · 2016-09-13T13:02:45Z

@problame @mdlayher It looks like delegation will be supported in 0.7.0. Maybe we can revive this PR?

mdlayher · 2016-09-13T14:09:27Z

I'm definitely in favor. 👍

problame · 2016-09-13T16:18:20Z

@discordianfish regarding the requirement for superuser privileges to run the zfs binary: even if it is dropped, the Prometheus policy of not shelling out prohibits using this functionality.

Remember that the part about parsing /proc/spl/kstat/zfs/arcstats (Linux) or alternatively sysctl (FreeBSD) works without superuser privileges on both platforms.

Only the per-dataset / per-pool metrics cannot be gathered without either

shelling out to zfs(8) (will work on ZoL 0.7, works on FreeBSD) (prohibited by Prometheus policy)
using libzfs (reimplementing zfs(8) functionality, relying on an unstable interface)

About the current state of this patch:

In the Linux-part, the cleanups mention by @brian-brazil probably need to be done. Should work as non-root for now.
In the FreeBSD-part, we violate the shelling-out policy right now, so we'd need to use the FreeBSD sysctl(2) API to first dicover and then read all the arcstats properties.

I am short on time right now and won't touch the FreeBSD side in the foreseeable future (sysctl(2) is rather tedious in comparison to some string processing of procfs pseudo-files).

Feel free to strip the FreeBSD-part and try to upstream the currently existing Linux-part, however ;)

cbstewart9393 · 2016-11-21T13:37:13Z

@problame My team at HPE would like to see the Linux part of this get upstreamed. Our thoughts are to do exactly what you said: get rid of the FreeBSD stuff and do the cleanups on the Linux stuff that @brian-brazil has mentioned. Would you be okay with these changes being made to try to get this PR accepted? I can start a new branch if it would be better to preserve the FreeBSD stuff on this one, or I can commit directly to this branch.
Anything else that needs to be done, aside from removing FreeBSD support and making the changes that have been requested via reviews in this PR?

problame · 2016-11-21T20:30:52Z

@cbstewart9393 It doesn't look like I'm going to have the time to implement the FreeBSD part for this any time soon.
Hence, I would be glad to see this PR modified / merged following the procedure you described.

However, I would suggest to keep the implementation easily extensible to non-Linux OSes, as is now.

discordianfish · 2016-12-21T11:43:46Z

Going to close this in favor of #369

brian-brazil reviewed Mar 12, 2016
View reviewed changes

Incorporate Feedback from brian-brazil.

538183f

Use ConstMetric with UntypedValue. Handle nozfs build flag. Remove computed values.

problame force-pushed the master branch from 12340b1 to 9ac05c6 Compare March 13, 2016 00:10

problame added 2 commits March 13, 2016 01:14

Change behavior on errors.

62e4a7d

collector now does not expose metrics if stats cannot be fetched.

Enable ZFS exporter by default and update README.

e92e9a2

problame force-pushed the master branch from 9ac05c6 to e92e9a2 Compare March 13, 2016 00:14

Style fixes.

6c13943

brian-brazil reviewed Mar 13, 2016
View reviewed changes

problame added 4 commits March 13, 2016 11:58

Comments should end in periods.

4d15f0b

Build system fixes.

fdfe4a6

Remove noop zfsInitialize().

3ae58ba

Use struct instantiation instead of constructor for zfsMetrics.

ffc3455

mdlayher reviewed Mar 28, 2016
View reviewed changes

Stylistic fixes.

e8d45a2

mdlayher mentioned this pull request Mar 28, 2016

Question: why does ZoL require root for read-only operations? openzfs/zfs#4468

Closed

juliusv reviewed Mar 28, 2016
View reviewed changes

cbstewart9393 mentioned this pull request Dec 13, 2016

Add ZFS Collector + review feedback from 213 #369

Closed

discordianfish closed this Dec 21, 2016

dominikh mentioned this pull request Sep 13, 2018

ZFS for FreeBSD #1063

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a collector for ZFS, currently focussed on ARC stats. #213

Add a collector for ZFS, currently focussed on ARC stats. #213

problame commented Mar 12, 2016

brian-brazil Mar 12, 2016

brian-brazil commented Mar 12, 2016

problame commented Mar 12, 2016

brian-brazil commented Mar 12, 2016

problame commented Mar 13, 2016

brian-brazil Mar 13, 2016

brian-brazil commented Mar 13, 2016

brian-brazil Mar 13, 2016

mdlayher commented Mar 28, 2016

mdlayher Mar 28, 2016

problame Mar 28, 2016

mdlayher Mar 28, 2016

juliusv Mar 28, 2016

brian-brazil commented Mar 28, 2016

problame commented Mar 28, 2016

mdlayher commented Mar 28, 2016

juliusv Mar 28, 2016

discordianfish commented Sep 13, 2016

mdlayher commented Sep 13, 2016

problame commented Sep 13, 2016

cbstewart9393 commented Nov 21, 2016

problame commented Nov 21, 2016

discordianfish commented Dec 21, 2016

Add a collector for ZFS, currently focussed on ARC stats. #213

Add a collector for ZFS, currently focussed on ARC stats. #213

Conversation

problame commented Mar 12, 2016

Choose a reason for hiding this comment

brian-brazil commented Mar 12, 2016

problame commented Mar 12, 2016

brian-brazil commented Mar 12, 2016

problame commented Mar 13, 2016

Choose a reason for hiding this comment

brian-brazil commented Mar 13, 2016

Choose a reason for hiding this comment

mdlayher commented Mar 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brian-brazil commented Mar 28, 2016

problame commented Mar 28, 2016

mdlayher commented Mar 28, 2016

Choose a reason for hiding this comment

discordianfish commented Sep 13, 2016

mdlayher commented Sep 13, 2016

problame commented Sep 13, 2016

cbstewart9393 commented Nov 21, 2016

problame commented Nov 21, 2016

discordianfish commented Dec 21, 2016