Add ZFS collector + review feedback from PRs 213 and 369 #410
Conversation
|
Note: Looks like CI is failing on the darwin build. Would an acceptable workaround for now be to prevent the ZFS component from being enabled by default, at least until FreeBSD compatibility is added? I'm open to alternatives, but am not sure what they would be with the way the code is written at the moment. |
|
@joehandzik Yes, zfs doesn't need to get built at all on darwin. But it looks like the build tag for that is already there.. Confused why it's trying to build that. |
|
@discordianfish I had the same thought, but I am pretty new to both Go and circleci so I am willing to assume that I'm doing something wrong. I'll poke around and see if I can fix it, but I don't really have any bright ideas at the moment. |
|
@discordianfish I fixed it. The build tags for collector/zfs.go were below the 'package collector' line, which means that they get ignored (I guess?). |
| zfsArcstatsProcpath = "spl/kstat/zfs/arcstats" | ||
| ) | ||
|
|
||
| func (c *zfsCollector) zfsAvailable() (err error) { |
There was a problem hiding this comment.
It looks like this function is obsolete and can be removed.
|
@SuperQ Removed it just now. Thanks! |
|
Looking good, please add |
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.
This patch makes stylistic changes to error strings, unexports method names by lower casing them, removes unused dataSetMetric, and adds copyright/licence information. Signed-Off-By: Corey Stewart <stewa169@purdue.edu>
This also involves removing zfs_zpool code for now. Signed-Off-By: Corey Stewart <stewa169@purdue.edu> Signed-Off-By: Joe Handzik <joseph.t.handzik@hpe.com>
Enables fixture test and updates e2e-output.txt. Signed-Off-By: Joe Handzik <joseph.t.handzik@hpe.com>
Signed-Off-By: Joe Handzik <joseph.t.handzik@hpe.com>
|
Cool, just need to ad the e2e fixture changes and it should pass tests again. |
|
@SuperQ Ok, added a commit to enable the zfs plugin in the e2e tests...but now the travis-ci build fails. circleci still passes. I can't figure out what failed in the travis build, but it failed even after rebasing in everything from master. The only thing that looks remotely like a failure (to me) in the logs is the warning about megacli being deprecated. Any ideas? |
|
@SuperQ Ok, finally figured it out and did what you said, looks like everything works again. Thanks for the help! |
|
Great, thanks a lot for the contribution! |
|
@discordianfish And thanks to you for dealing with all the thrashing on this contribution. Future contributions won't require three different PRs, I promise! |
Apologies for creating another PR, my intern is no longer available to complete the work. This is the last time this would need to happen (I'll get this over the finish line). Here is what changes have been factored in based on the feedback from PR 369:
I think this took all feedback into account, but please let me know if there's more needed. Thanks!