Skip to content

Enclosure LED fixes #5751

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Feb 11, 2017
Merged

Enclosure LED fixes #5751

merged 1 commit into from
Feb 11, 2017

Conversation

tonyhutter
Copy link
Contributor

@tonyhutter tonyhutter commented Feb 6, 2017 •
edited
Loading

Various enclosure LED zed fixes.

Description

  • Pass $VDEV_ENC_SYSFS_PATH to 'zpool [iostat|status] -c' to include
    enclosure LED sysfs path.

  • Set LEDs correctly after import. This includes clearing any erroniously
    set LEDs prior to the import, and setting the LED for any UNAVAIL drives.

  • Include missing symlink for vdev_attach-led.sh in Makefile.am.

  • Print the VDEV path in all-syslog.sh, and fix it so the pool GUID actually
    prints.

Motivation and Context

How Has This Been Tested?

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)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • Change has been approved by a ZFS on Linux member.

@mention-bot
Copy link

@tonyhutter, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dun, @don-brady and @behlendorf to be potential reviewers.

@tonyhutter
Copy link
Contributor Author

This will fix #5716

behlendorf
behlendorf requested changes Feb 7, 2017
View reviewed changes
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach LGTM but the patch causes zpool_status_001_neg and zpool_iostat_005_pos to fail so that'll need to be resolved. The other two test failures can be fixed with a rebase on master.

cmd/zed/zed.d/all-syslog.sh
@@ -6,6 +6,7 @@
. "${ZED_ZEDLET_DIR}/zed-functions.sh"

zed_log_msg "eid=${ZEVENT_EID}" "class=${ZEVENT_SUBCLASS}" \
"${ZEVENT_POOL:+"pool=${ZEVENT_POOL}"}" \
"${ZEVENT_POOL_GUID:+"pool_guid=${ZEVENT_POOL_GUID}"}" \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be useful to log both here if the reason for the change is to minimize ambiguity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had it looking up the pool name in an earlier version of the code, but later decided against it. The problem was that we would get some events before the pool was fully imported, and so we couldn't resolve the pool name. The other issue is that each pool name lookup (zed_guid_to_pool) does a zpool get call, which I thought was a little heavyweight for code that runs on each event.

cmd/zed/zed.d/statechange-led.sh Outdated

# Lookup all the current LED values and paths in parallel
cmd='echo led_token=$(cat "$VDEV_ENC_SYSFS_PATH/fault"),"$VDEV_ENC_SYSFS_PATH",'
out=$(zpool status -vc "$cmd" $pool | grep 'led_token=')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$ZPOOL should be provided in the environment.

cmd/zed/zed.d/zed-functions.sh Outdated

guid=$(printf "%llu" $1)
if [ ! -z "$guid" ] ; then
zpool get -H -ovalue,name guid | awk '$1=='$guid' {print $2}'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same: $ZPOOL

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both $ZPOOLs are fixed in latest push

don-brady
don-brady reviewed Feb 7, 2017
View reviewed changes
cmd/zpool/zpool_iter.c Outdated
if (snprintf(cmd, sizeof (cmd), "VDEV_PATH=%s && VDEV_UPATH=\"%s\" && "
"VDEV_ENC_SYSFS_PATH=\"%s\" && %s", data->path,
data->upath, data->vdev_enc_sysfs_path,
data->cmd) >= sizeof (cmd)) {
/* Our string was truncated */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will snprintf and strdup (below) always handle a NULL vdev_enc_sysfs_path value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, it's fixed in my latest push.

@tonyhutter
Copy link
Contributor Author

I currently don't have access to my VM, so haven't looked into the test failures yet. I'm hoping I get lucky and they go away with my latest push :-)

@behlendorf
Copy link
Contributor

@tonyhutter a rebase on master should get rid of the two reservation test failures.

@tonyhutter
Enclosure LED fixes
fd99878 
- Pass $VDEV_ENC_SYSFS_PATH to 'zpool [iostat|status] -c' to include
  enclosure LED sysfs path.

- Set LEDs correctly after import.  This includes clearing any erroniously
  set LEDs prior to the import, and setting the LED for any UNAVAIL drives.

- Include symlink for vdev_attach-led.sh in Makefile.am.

- Print the VDEV path in all-syslog.sh, and fix it so the pool GUID actually
  prints.
@tonyhutter
Copy link
Contributor Author

Ok, I think I've got everything fixed

don-brady
don-brady approved these changes Feb 10, 2017
View reviewed changes
Copy link
Contributor

@don-brady don-brady left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

behlendorf
behlendorf approved these changes Feb 10, 2017
View reviewed changes
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@behlendorf behlendorf merged commit b291029 into openzfs:master Feb 11, 2017
wli5 pushed a commit to wli5/zfs that referenced this pull request Feb 28, 2017
@tonyhutter @wli5
Enclosure LED fixes
30ba9e1 
- Pass $VDEV_ENC_SYSFS_PATH to 'zpool [iostat|status] -c' to include
  enclosure LED sysfs path.

- Set LEDs correctly after import.  This includes clearing any erroniously
  set LEDs prior to the import, and setting the LED for any UNAVAIL drives.

- Include symlink for vdev_attach-led.sh in Makefile.am.

- Print the VDEV path in all-syslog.sh, and fix it so the pool GUID actually
  prints.    

Reviewed-by: Don Brady <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Tony Hutter <[email protected]>
Closes openzfs#5716 
Closes openzfs#5751
wli5 pushed a commit to wli5/zfs that referenced this pull request Feb 28, 2017
@tonyhutter @wli5
Enclosure LED fixes
6245657 
- Pass $VDEV_ENC_SYSFS_PATH to 'zpool [iostat|status] -c' to include
  enclosure LED sysfs path.

- Set LEDs correctly after import.  This includes clearing any erroniously
  set LEDs prior to the import, and setting the LED for any UNAVAIL drives.

- Include symlink for vdev_attach-led.sh in Makefile.am.

- Print the VDEV path in all-syslog.sh, and fix it so the pool GUID actually
  prints.

Reviewed-by: Don Brady <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Tony Hutter <[email protected]>
Closes openzfs#5716
Closes openzfs#5751
behlendorf pushed a commit to LLNL/zfs that referenced this pull request Mar 1, 2017
@tonyhutter @behlendorf
Enclosure LED fixes
155e953 
- Pass $VDEV_ENC_SYSFS_PATH to 'zpool [iostat|status] -c' to include
  enclosure LED sysfs path.

- Set LEDs correctly after import.  This includes clearing any erroniously
  set LEDs prior to the import, and setting the LED for any UNAVAIL drives.

- Include symlink for vdev_attach-led.sh in Makefile.am.

- Print the VDEV path in all-syslog.sh, and fix it so the pool GUID actually
  prints.    

Reviewed-by: Don Brady <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Tony Hutter <[email protected]>
Closes openzfs#5716 
Closes openzfs#5751
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@behlendorf behlendorf behlendorf approved these changes

@don-brady don-brady don-brady approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tonyhutter @mention-bot @behlendorf @don-brady