fix: pool detection and metrics gathering for ZFS >= 2.1.x by aaronjwood · Pull Request #10099 · influxdata/telegraf

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)

Conversation

aaronjwood commented Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Required for all PRs:

Uh oh!

aaronjwood commented Nov 12, 2021

Uh oh!

srebhan commented Nov 15, 2021

Uh oh!

aaronjwood commented Nov 15, 2021

Uh oh!

srebhan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aaronjwood commented Nov 16, 2021

Uh oh!

aaronjwood commented Nov 27, 2021

Uh oh!

aaronjwood commented Nov 28, 2021

Uh oh!

srebhan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aaronjwood commented Nov 30, 2021

Uh oh!

telegraf-tiger bot commented Nov 30, 2021

Artifact URLs

Uh oh!

srebhan left a comment

Choose a reason for hiding this comment

Uh oh!

MyaLongmire left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aaronjwood commented Nov 12, 2021 •

edited

Loading