zed.d/pool_import-led.sh: fix for current zpool scripts (and a few bonus touchups)

[nabijaczleweli]

Motivation and Context

#11934 and a few others I noticed

Description

I mean, it's what it says on the tins.

How Has This Been Tested?

Looked at it, and tested on fake data, but I don't have a system with SES enclosures, so it'd be great if someone who does could run statechange-led.sh on pool import :)

The rest should be relative non-brainers.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes. – let's hope none break
• I have run the ZFS Test Suite with this change applied. – CI take my hand
• All commit messages are properly formatted and contain Signed-off-by.

[behlendorf]

I'm fairly certain this script was working but let's definitely make sure. cc: @tonyhutter.

[nabijaczleweli]

Added fix for #11954, too

[behlendorf]

Dropping the printf would mean this function no longer accepts the GUID as either decimal or hex. We should keep that functionality even though the only callers we provide all pass $ZEVENT_POOL_GUID. Others may have written their own scripts which depend on this behavior.

[nabijaczleweli]

urgh, this worked in my tests because GAWK supports hex integers as an extension, but MAWK (and, presumably, POSIX AWK) doesn't seem to

updated to retain the printf

[tonyhutter]

updated to retain the printf

I don't see the printf

[nabijaczleweli]

odd, coulda swore i added it back when i made the comment. ah well, it's definitely here now

[tonyhutter]

I'll take a look at this

[ZNikke]

Ah, my travel down report-issue-lane started with looking at statechange-led and wondering how/why the fallback process of all pool devices was supposed to work.

I have a patch that generalizes the led-handling so statechange-led can light the locate led for OFFLINE devices as well, but I'll hold off on that until this gets landed.

Let me know if this needs more testers, as we have enclosures with blinkenlights...

[tonyhutter]

While reviewing this patch, I realized that the code that code causing the bug (#11934) will never actually get called, so I ended up opening a PR (#11964) to remove that code. My vote would be to use #11964 as the fix, close this PR, and move your "few bonus touchups" to a separate PR.

[tonyhutter]

Spoke too soon - as @nabijaczleweli points out in #11964, removing the process_pool() in isn't going to work for pool_import-led.sh case. I'm looking into a different fix...

[nabijaczleweli]

What other fix? I already wrote one, it's right here. Should be as simple as copying this version into your /etc, exporting an OK pool, futzing with the fault led states, and importing it again to validate it resets them (hopefully; if not, it's trivial to run this branch of the ZEDLET in isolation).

[tonyhutter]

@nabijaczleweli this simplified patch uses zpool status -c upath,fault_led and works on my enclosure. Does it work for you?

diff --git a/cmd/zed/zed.d/statechange-led.sh b/cmd/zed/zed.d/statechange-led.sh
index e656e125d..4b25a55f9 100755
--- a/cmd/zed/zed.d/statechange-led.sh
+++ b/cmd/zed/zed.d/statechange-led.sh
@@ -115,17 +115,24 @@ process_pool()
        pool="$1"
        rc=0
 
-       # Lookup all the current LED values and paths in parallel
        #shellcheck disable=SC2016
-       cmd='echo led_token=$(cat "$VDEV_ENC_SYSFS_PATH/fault"),"$VDEV_ENC_SYSFS_PATH",'
-       out=$($ZPOOL status -vc "$cmd" "$pool" | grep 'led_token=')
+
+       # The output will be the vdevs only (from "grep '/dev/'"):
+       #
+       #    U45     ONLINE       0     0     0   /dev/sdk          0
+       #    U46     ONLINE       0     0     0   /dev/sdm          0
+       #    U47     ONLINE       0     0     0   /dev/sdn          0
+       #    U50     ONLINE       0     0     0  /dev/sdbn          0
+       #
+       out="$(ZPOOL_SCRIPTS_AS_ROOT=1 $ZPOOL status -c upath,fault_led $pool | grep '/dev/')"
 
        #shellcheck disable=SC2034
        echo "$out" | while read -r vdev state read write chksum therest; do
                # Read out current LED value and path
-               tmp=$(echo "$therest" | sed 's/^.*led_token=//g')
-               vdev_enc_sysfs_path=$(echo "$tmp" | awk -F ',' '{print $2}')
-               current_val=$(echo "$tmp" | awk -F ',' '{print $1}')
+               # Get dev name (like 'sda')
+               dev=$(basename $(echo "$therest" |  awk '{print $(NF-1)}'))
+               vdev_enc_sysfs_path=$(realpath /sys/class/block/$dev/device/enclosure_device*)
+               current_val=$(echo "$therest" | awk '{print $NF}')
 
                if [ "$current_val" != "0" ] ; then
                        current_val=1
@@ -171,7 +178,7 @@ if [ -n "$ZEVENT_VDEV_ENC_SYSFS_PATH" ] && [ -n "$ZEVENT_VDEV_STATE_STR" ] ; the
        vdev=$(basename "$ZEVENT_VDEV_PATH")
        check_and_set_led "$ZEVENT_VDEV_ENC_SYSFS_PATH/fault" "$val"
 else
-       # Process the entire pool
+       # Process the entire pool (needed for pool_import-led.sh)
        poolname=$(zed_guid_to_pool "$ZEVENT_POOL_GUID")
        process_pool "$poolname"
 fi

[tonyhutter]

The grammar was correct before. VDEV is an acronym for "Virtual Device" or "Virtual Disk" and acronyms are typically capitalized. "VDEV's" was also correct since it's the possessive form of the noun.

[nabijaczleweli]

This script toggles enclosure fault LEDs on multiple vdevs at the point when a particular vdev's pool experiences a state change, or "toggles vdevs' enclosure fault LEDs when their pool's state changes"

vdev is not an acronym (that'd be "VD"), but, rather, a shorthand-turned-object-class for "virtual device" as ZFS implements it (some would call this a "normal noun"), confer zpoolconcepts(8), or zpoolprops(8), or zpool-status(8), or

[tonyhutter]

What is the point of this change?

[nabijaczleweli]

This can manipulate many fault LEDs at a time and describes a general rule, also making terminology consistent with the rest

[tonyhutter]

What is the point of this change?

[nabijaczleweli]

As noted: seq is a very spotty GNU extension (and also not checked for at the top), so when not available this resolves to for _ in ; do which I assume you can guess why is less than optimal. (Plus, it's literally five numbers, do you really need to spawn a process for it?)

[gdevenyi]

Has checkbashisms said that?

[nabijaczleweli]

Hm, I've misremembered apparently, according to FreeBSD's seq(1) it's had a tumultuous history (as ported from GNU shellutils and/or P9) and should be available on most recent fully-configured UNIXes, but I've also definitely used systems without it in recent years.
Plus, if the portability to small systems doesn't appeal to you, then the performance gains of not using a process to generate 5 numbers that are, in sum, shorter than the process substitution definitely will.

and dunno, haven't run checkbashisms, but it probably won't

[tonyhutter]

I re-tested statechange-led.sh with your latest push and works as expected. Thanks for sticking with this! 👍