fix(zfs): dynamic header parsing, ns/ms conversion, live-sample interval - #634
Open
ahoogerhuis wants to merge 8 commits into
Open
fix(zfs): dynamic header parsing, ns/ms conversion, live-sample interval#634ahoogerhuis wants to merge 8 commits into
ahoogerhuis wants to merge 8 commits into
Conversation
zpool iostat -l -q -p -H with no interval argument returns the since-boot cumulative average (per zpool-iostat(8)), not a live sample. This left scrub_wait/trim_wait graphed as non-zero indefinitely after a scrub/trim finished, since the average kept being computed against total ops since boot rather than reflecting actual current pool activity. Add -y (suppress the since-boot summary line) and a 2s/1-count interval so the parsed line reflects current pool state instead.
Two issues found while verifying the parsing for a separate stale-stats fix (bugs/apps-zfs): 1. Field misalignment on newer OpenZFS. The parser assumed a fixed 29-column `zpool iostat -l -q -p -H` output. Versions with the device-rebuild/sequential-resilver feature add a rebuild_wait latency column (right after trim_wait) and a rebuildq_write pend/activ pair (at the very end of the -q block) -- 32 columns total. Since the old parsing was a straight positional list assignment, every field from syncq_read_p onward silently read its neighbor's value once the extra columns showed up, corrupting all 12 queue-depth graphs (sync/async/scrub/trim, read+write, pending+active). Fixed by keeping the stable front block (name through trim_wait, unaffected either way) and branching on the total field count for the queue-depth block, since the two known real-world layouts (29 vs 32 columns) are unambiguous and verified against man zpool-iostat(8) and real sample output. 2. Nanoseconds graphed as milliseconds. `-p` reports time values in nanoseconds (per zpool-iostat(8)), but every wait-time graph (total_wait, disk_wait, syncq_wait, asyncq_wait, scrub_wait, trim_wait) labels its axis 'ms' with no conversion anywhere in the pipeline -- off by a factor of 1,000,000. Fixed by dividing the affected fields by 1_000_000 after the existing '-' -> 0 normalization. Verified both fixes against real zpool iostat -y -l -q -p -H 2 1 output from a real production host (32-column layout) and a synthetic 29-column sample built by removing exactly the rebuild-related fields, confirming identical, correct field mapping either way.
The field-count branching already handles OpenZFS version differences correctly without needing a delayed sample. The 2-second pause per pool adds unnecessary blocking time (4s+ for multi-pool setups) while the instantaneous snapshot is accurate once the layout is detected via field count.
This reverts commit 8242c9d90bf2aadf937eab204c88a6c8b828eada.
Drops -H from both `zpool list` and `zpool iostat -l -q -p`, which was the only reason position ever had to be guessed at -- without it, zpool prints a real header before the data. Pool-list parsing now reads real column names directly into a hash slice. Iostat parsing combines the two-line grouped header (top labels spanning multiple columns, e.g. syncq_read over pend/activ) using the real column boundaries from the dashes separator row plus a name-keyed span table mirroring OpenZFS's own iostat_top_labels structure, then aliases the real on-the-wire names onto this script's existing internal field names. Any future OpenZFS version that adds, removes, or reorders columns -- the exact class of bug this replaces -- is handled automatically instead of silently misaligning fields. Verified against real captured header/data from a live loopback test pool: matches the old parser's output exactly after accounting for its existing cleanup step, a real-derived 29-column layout resolves correctly, and a synthetic never-before-seen layout (reordered groups plus one brand-new column) also resolves correctly by name where the old positional parser would have silently misaligned fields. Co-authored-by: Claude
LibreNMS's usual extend-script convention installs the local file under the same name as the repo filename. With this script called just "zfs", that puts a file literally named zfs on $PATH (e.g. /usr/local/bin/zfs or /etc/snmp/zfs). On a host where that directory precedes /sbin or /usr/sbin in $PATH -- common for /usr/local/bin -- running `zfs list` or `zfs set` at a shell would silently execute this monitoring script instead of the real OpenZFS zfs binary. Caught while deploying to a real device. Renames the file and updates its own POD (NAME, a new section explaining the collision risk, and the SNMPD SETUP EXAMPLES path) to match. The SNMP-visible extend name in snmpd.conf (`extend zfs ...`) is unrelated and unchanged -- it's just a label LibreNMS uses internally, not something executed directly at a shell. No functional change to the script itself; verified snmp/zfs-stats still passes `perl -c` and `perlcritic --severity 5` cleanly, and confirmed no other file in the repo references the old snmp/zfs path (zfs-linux and zfs-freebsd are separate, unrelated scripts and were already named to avoid the same collision). Co-authored-by: Claude
Author
|
There is some more bugs in the L2 section of this script, so this is not worth merging just yet. |
Real upstream core (includes/polling/applications/zfs.inc.php) does an exact-string lookup for l2_bufc_d_asize/l2_bufc_m_asize, but the real kernel ARC stats -- and this script's own generic l2_* passthrough -- use the full names l2_bufc_data_asize/l2_bufc_metadata_asize. The mismatch means core creates no RRD datasource for either field, so the corresponding graphs render broken. Confirmed against real upstream source and real L2ARC data on office-pmox. Add the abbreviated names as aliases alongside the full names (not a rename) so the script's own output stays self-describing while also satisfying core's mismatched expectation. Guarded with defined so hosts without L2ARC don't get these keys injected as undefined. Co-authored-by: Claude
Core (includes/polling/applications/zfs.inc.php) expects this pre-computed ratio but nothing ever calculated it -- unlike the two fields fixed in 85df1b2, this isn't a naming mismatch, the field simply never existed. Confirmed via real RRD data on office-pmox: every row shows -nan for this field specifically. Guards the zero-metadata case explicitly -- a host with L2ARC but genuinely zero cached metadata (or vice versa) is a real case, not hypothetical. Co-authored-by: Claude
Author
|
A few squashed, but there is one unexplained nan left, and need to engineer a way to wiggle all the error/rebuild related values to see if they work. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three related bugs in the
zfsextend script'szpoolparsing, found while adding a new check:zpool iostatwith no interval returns a since-boot average, not a live reading -- scrub/trim wait graphs never dropped to zero after activity ended. Fixed with a real one-shot interval sample.The fixed-position field parser assumed a specific column count. Current OpenZFS adds columns (
rebuild_waitand others), and Perl's positional list assignment meant everything after the insertion point silently read its neighbor's value. Replaced with parsing based on the real column headers, so future OpenZFS versions that add/reorder columns get handled correctly instead of silently misaligning.Time values are documented as nanoseconds; nothing converted them, so every wait-time graph was off by ~1,000,000x. Fixed.
Also renamed the script from
zfstozfs-stats-- the old name would land at a path like/etc/snmp/zfson installation, shadowing the realzfsbinary on$PATH.Verified against real captured
zpooloutput (both known column-count layouts) and a synthetic layout with a new, previously-unseen column, confirming the parser resolves fields by name correctly in a case the old positional parser would have silently gotten wrong. Also deployed and confirmed on a real production host.Developed with Claude's assistance; I've reviewed and take responsibility for the change.