fix: pool detection and metrics gathering for ZFS >= 2.1.x #10099

aaronjwood · 2021-11-12T20:50:43Z

Required for all PRs:

Updated associated README.md.
Wrote appropriate unit tests.
Pull request title or commits are in conventional commit format

partially resolves #8862

I moved to ZFS 2.1.1 the other day and noticed my pool metrics broke. After a bit of investigation I found out that things have changed significantly under the hood. I have this diff deployed to one of my servers and can show it working:

aaronjwood · 2021-11-12T20:59:17Z

Did the lint rules change recently? It seems to be flagged a lot of things in the ZFS plugin README that I didn't touch.

srebhan · 2021-11-15T13:06:46Z

Hey @aaronjwood, thanks for looking into this. From your changes it looks like the PR is breaking backward compatibility, i.e. you cannot use a zfs < 2.1.x with the PR (e.g. by changing the proc-file path). Is my understanding correct? And if so, can we somehow test for the present zfs version to be compatible with both formats?

aaronjwood · 2021-11-15T16:34:29Z

Hey @aaronjwood, thanks for looking into this. From your changes it looks like the PR is breaking backward compatibility, i.e. you cannot use a zfs < 2.1.x with the PR (e.g. by changing the proc-file path). Is my understanding correct? And if so, can we somehow test for the present zfs version to be compatible with both formats?

Hi, it should not. I fall back to checking the old kstat path if the new one doesn't exist. There is also a fallback for how the actual file contents are parsed. I've also added some tests that write out a kstat with the new format/filename to exercise the new code paths.

Do you see an issue with it?

srebhan

@aaronjwood I now see your approach and it's fine IMO. Sorry for having missed it in the first pass...

Given that the format changed I suspect/fear that this might happen again in the future. So I
wonder if we should be prepared and approach this in a slightly more general way. How about moving the probing to a probeVersion function returning a format identifier (e.g. 1, 2, 3...). Based on this version we have different gather..._vX() functions for X in {1,2,3,...} that are tailored to the format of this version. This way we would have a clear structure and avoid complex if/else flow handlings dependent on versions. If there is common code, we can factor that into more general functions parameterized by the version-specific functions. What do you think?

plugins/inputs/zfs/zfs_linux.go

aaronjwood · 2021-11-16T16:18:39Z

@aaronjwood I now see your approach and it's fine IMO. Sorry for having missed it in the first pass...

Given that the format changed I suspect/fear that this might happen again in the future. So I
wonder if we should be prepared and approach this in a slightly more general way. How about moving the probing to a probeVersion function returning a format identifier (e.g. 1, 2, 3...). Based on this version we have different gather..._vX() functions for X in {1,2,3,...} that are tailored to the format of this version. This way we would have a clear structure and avoid complex if/else flow handlings dependent on versions. If there is common code, we can factor that into more general functions parameterized by the version-specific functions. What do you think?

No worries. What you suggest sounds good to me. I'll try and find some time in the coming weeks to change this. Things are a bit tight with the holidays coming up :)

aaronjwood · 2021-11-27T01:30:00Z

FWIW I'll try out the changes I pushed up just now over the weekend. I don't have the ability to put a new binary on my server at the moment :)

aaronjwood · 2021-11-28T01:10:10Z

Yep, still looking good on my server.

srebhan

This is an amazing update @aaronjwood! Some small leftovers, but we are almost good to go. I really love the new structure.

plugins/inputs/zfs/zfs_linux.go

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

aaronjwood · 2021-11-30T18:17:04Z

Should be all set now :) Thanks for the reviews. Looking forward to having this upstreamed!

telegraf-tiger · 2021-11-30T18:24:30Z

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Looks like new artifacts were built from this PR.

Expand this list to get them here ! 🐯

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	freebsd_amd64.tar.gz	windows_i386.zip
armel.deb	armv6hl.rpm	freebsd_armv7.tar.gz
armhf.deb	i386.rpm	freebsd_i386.tar.gz
i386.deb	ppc64le.rpm	linux_amd64.tar.gz
mips.deb	s390x.rpm	linux_arm64.tar.gz
mipsel.deb	x86_64.rpm	linux_armel.tar.gz
ppc64el.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_s390x.tar.gz
		static_linux_amd64.tar.gz

srebhan

Looks good to me. Thanks for the nice work @aaronjwood!

MyaLongmire

Thank you for this pr!

(cherry picked from commit 666bfe3)

…a#10099)

* origin/master: (133 commits) chore: restart service if it is already running and upgraded via RPM (influxdata#9970) feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10237) fix: Handle duplicate registration of protocol-buffer files gracefully. (influxdata#10188) fix(http_listener_v2): fix panic on close (influxdata#10132) feat: add Vault input plugin (influxdata#10198) feat: support aws managed service for prometheus (influxdata#10202) fix: Make telegraf compile on Windows with golang 1.16.2 (influxdata#10246) Update changelog feat: Modbus add per-request tags (influxdata#10231) fix: Implement NaN and inf handling for elasticsearch output (influxdata#10196) feat: add nomad input plugin (influxdata#10106) fix: Print loaded plugins and deprecations for once and test (influxdata#10205) fix: eliminate MIB dependency for ifname processor (influxdata#10214) feat: Optimize locking for SNMP MIBs loading. (influxdata#10206) feat: Add SMART plugin concurrency configuration option, nvme-cli v1.14+ support and lint fixes. (influxdata#10150) feat: update configs (influxdata#10236) fix(inputs/kube_inventory): set TLS server name config properly (influxdata#9975) fix: Sudden close of Telegraf caused by OPC UA input plugin (influxdata#10230) fix: bump github.com/eclipse/paho.mqtt.golang from 1.3.0 to 1.3.5 (influxdata#9913) fix: json_v2 parser timestamp setting (influxdata#10221) fix: ensure graylog spec fields not prefixed with '_' (influxdata#10209) docs: remove duplicate links in CONTRIBUTING.md (influxdata#10218) fix: pool detection and metrics gathering for ZFS >= 2.1.x (influxdata#10099) fix: parallelism fix for ifname processor (influxdata#10007) chore: Forbids "log" package only for aggregators, inputs, outputs, parsers and processors (influxdata#10191) docs: address documentation gap when running telegraf in k8s (influxdata#10215) feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10211) fix: mqtt topic extracting no longer requires all three fields (influxdata#10208) fix: windows service - graceful shutdown of telegraf (influxdata#9616) feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10201) feat: Modbus support multiple slaves (gateway feature) (influxdata#9279) fix: Revert unintented corruption of the Makefile from influxdata#10200. (influxdata#10203) chore: remove triggering update-config bot in CI (influxdata#10195) Update changelog feat: Implement deprecation infrastructure (influxdata#10200) fix: extra lock on init for safety (influxdata#10199) fix: resolve influxdata#10027 (influxdata#10112) fix: register bigquery to output plugins influxdata#10177 (influxdata#10178) fix: sysstat use unique temp file vs hard-coded (influxdata#10165) refactor: snmp to use gosmi (influxdata#9518) ...

telegraf-tiger bot added the fix pr to fix corresponding bug label Nov 12, 2021

aaronjwood changed the title ~~Fix pool detection and metrics gathering for ZFS >= 2.1.x~~ fix (inputs/zfs): pool detection and metrics gathering for ZFS >= 2.1.x Nov 12, 2021

aaronjwood force-pushed the zfs2x branch from c8d4f63 to 549967a Compare November 12, 2021 21:14

sspaink mentioned this pull request Nov 15, 2021

fix(inputs/zfs): resolve README.md linter issues #10109

Merged

srebhan reviewed Nov 16, 2021

View reviewed changes

plugins/inputs/zfs/zfs_linux.go Outdated Show resolved Hide resolved

plugins/inputs/zfs/zfs_linux.go Outdated Show resolved Hide resolved

plugins/inputs/zfs/zfs_linux.go Outdated Show resolved Hide resolved

srebhan self-assigned this Nov 17, 2021

aaronjwood force-pushed the zfs2x branch 3 times, most recently from 090ab3c to dd60a24 Compare November 27, 2021 01:06

aaronjwood changed the title ~~fix (inputs/zfs): pool detection and metrics gathering for ZFS >= 2.1.x~~ fix: pool detection and metrics gathering for ZFS >= 2.1.x Nov 27, 2021

aaronjwood force-pushed the zfs2x branch 2 times, most recently from 995a6c5 to 1c77f17 Compare November 27, 2021 01:13

fix: pool detection and metrics gathering for ZFS >= 2.1.x

67d9052

aaronjwood force-pushed the zfs2x branch from 1c77f17 to 67d9052 Compare November 27, 2021 01:27

srebhan reviewed Nov 30, 2021

View reviewed changes

plugins/inputs/zfs/zfs_linux.go Outdated Show resolved Hide resolved

plugins/inputs/zfs/zfs_linux.go Outdated Show resolved Hide resolved

plugins/inputs/zfs/zfs_linux.go Show resolved Hide resolved

aaronjwood and others added 2 commits November 30, 2021 10:03

Update plugins/inputs/zfs/zfs_linux.go

6fb34b3

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

Update plugins/inputs/zfs/zfs_linux.go

9cb3501

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

srebhan approved these changes Dec 1, 2021

View reviewed changes

srebhan added plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. labels Dec 1, 2021

MyaLongmire approved these changes Dec 6, 2021

View reviewed changes

MyaLongmire merged commit 666bfe3 into influxdata:master Dec 6, 2021

MyaLongmire pushed a commit that referenced this pull request Dec 8, 2021

fix: pool detection and metrics gathering for ZFS >= 2.1.x (#10099)

711c97f

(cherry picked from commit 666bfe3)

powersj pushed a commit to powersj/telegraf that referenced this pull request Jan 21, 2022

fix: pool detection and metrics gathering for ZFS >= 2.1.x (influxdat…

db3398c

…a#10099)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: pool detection and metrics gathering for ZFS >= 2.1.x #10099

fix: pool detection and metrics gathering for ZFS >= 2.1.x #10099

aaronjwood commented Nov 12, 2021 •

edited

Loading

aaronjwood commented Nov 12, 2021

srebhan commented Nov 15, 2021

aaronjwood commented Nov 15, 2021

srebhan left a comment

aaronjwood commented Nov 16, 2021

aaronjwood commented Nov 27, 2021

aaronjwood commented Nov 28, 2021

srebhan left a comment

aaronjwood commented Nov 30, 2021

telegraf-tiger bot commented Nov 30, 2021

Artifact URLs

srebhan left a comment

MyaLongmire left a comment

fix: pool detection and metrics gathering for ZFS >= 2.1.x #10099

fix: pool detection and metrics gathering for ZFS >= 2.1.x #10099

Conversation

aaronjwood commented Nov 12, 2021 • edited Loading

Required for all PRs:

aaronjwood commented Nov 12, 2021

srebhan commented Nov 15, 2021

aaronjwood commented Nov 15, 2021

srebhan left a comment

Choose a reason for hiding this comment

aaronjwood commented Nov 16, 2021

aaronjwood commented Nov 27, 2021

aaronjwood commented Nov 28, 2021

srebhan left a comment

Choose a reason for hiding this comment

aaronjwood commented Nov 30, 2021

telegraf-tiger bot commented Nov 30, 2021

Artifact URLs

srebhan left a comment

Choose a reason for hiding this comment

MyaLongmire left a comment

Choose a reason for hiding this comment

aaronjwood commented Nov 12, 2021 •

edited

Loading