Change in vdsm[master]: multipath: Configure iscsi_session recovery_tmo

nsoffer at redhat.com nsoffer at redhat.com
Mon Sep 8 09:12:33 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: multipath: Configure iscsi_session recovery_tmo
......................................................................


Patch Set 3:

(6 comments)

http://gerrit.ovirt.org/#/c/32582/3/vdsm/storage/Makefile.am
File vdsm/storage/Makefile.am:

Line 72: 	volume.py
Line 73: 
Line 74: dist_vdsmexec_SCRIPTS = \
Line 75: 	curl-img-wrap \
Line 76: 	multipath-configure-device
> $(NULL)?
Why do we need this?
Line 77: 
Line 78: nodist_vdsmstorage_DATA = \
Line 79: 	lvm.env \
Line 80: 	$(NULL)


http://gerrit.ovirt.org/#/c/32582/3/vdsm/storage/multipath-configure-device.in
File vdsm/storage/multipath-configure-device.in:

Line 7: # (at your option) any later version.  See the files README and
Line 8: # LICENSE_GPL_v2 which accompany this distribution.
Line 9: #
Line 10: 
Line 11: set -e
> usually it is better to exit with custom message instead of having sysadmin
You suggest to replace this with check for each call and logging errors?
Line 12: 
Line 13: # Ensure that multipath devices use no_path_retry fail, instead of
Line 14: # no_path_retry queue, which is hardcoded in multipath configuration for many
Line 15: # devices.


Line 23: # is lost and revert back to iscsid.conf default (120).
Line 24: 
Line 25: timeout=5
Line 26: 
Line 27: for slave in /sys${DEVPATH}/slaves/*; do
> please decide if you are within the ${XXX} or $XXX convention.
ok
Line 28:     path=$(@UDEVADM_PATH@ info --query=path --path="$slave")
Line 29:     session=$(echo "$path" | @SED_PATH@ -rn \
Line 30:               s'|^/devices/platform/host[0-9]+/(session[0-9]+)/.+$|\1|p')
Line 31:     if [ -n "$session" ]; then


Line 24: 
Line 25: timeout=5
Line 26: 
Line 27: for slave in /sys${DEVPATH}/slaves/*; do
Line 28:     path=$(@UDEVADM_PATH@ info --query=path --path="$slave")
> quotes all over please
Do you mean path="$(x y)"?
Line 29:     session=$(echo "$path" | @SED_PATH@ -rn \
Line 30:               s'|^/devices/platform/host[0-9]+/(session[0-9]+)/.+$|\1|p')
Line 31:     if [ -n "$session" ]; then
Line 32:         echo $timeout > "/sys/class/iscsi_session/${session}/recovery_tmo"


Line 26: 
Line 27: for slave in /sys${DEVPATH}/slaves/*; do
Line 28:     path=$(@UDEVADM_PATH@ info --query=path --path="$slave")
Line 29:     session=$(echo "$path" | @SED_PATH@ -rn \
Line 30:               s'|^/devices/platform/host[0-9]+/(session[0-9]+)/.+$|\1|p')
> s' -> 's not that it is that important and working but it is clearer.
s' -> 's - ok

I did not find a better way to get the session from the path. multipath use similar solution, iterating up the path items, until they find an item named "session%d".
Line 31:     if [ -n "$session" ]; then
Line 32:         echo $timeout > "/sys/class/iscsi_session/${session}/recovery_tmo"
Line 33:     fi


http://gerrit.ovirt.org/#/c/32582/3/vdsm/storage/vdsm-multipath.rules
File vdsm/storage/vdsm-multipath.rules:

Line 11: #
Line 12: # This rule runs vdsm-mutipath-configure tool to configure multiapth devices to
Line 13: # work with vdsm.
Line 14: 
Line 15: ACTION=="add|change", ENV{DM_UUID}=="mpath-?*", RUN+="/usr/libexec/vdsm/multipath-configure-device"
> this should be expanded by autoconf.
Would be better, since this script assumes the environment created by udev.


-- 
To view, visit http://gerrit.ovirt.org/32582
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaaa40fa5e6dc86beef501ef4aaefa17c4c1574c1
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list