Re: [Xen-devel] [PATCH] hotplug/Linux: add iscsi block hotplug script

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2013-04-25 at 13:32 +0100, Roger Pau Monne wrote:
> On 24/04/13 18:48, Ian Campbell wrote:
> > On Wed, 2013-04-24 at 17:31 +0100, Roger Pau Monne wrote:
> >> This hotplug script has been tested with IET and NetBSD iSCSI targets,
> >> without authentication.
> >>
> >> This hotplug script will only work with PV guests not using pygrub.
> > 
> > Thanks. Does this need any docs somewhere, e.g. how to format the target
> > spec?
> > 
> > Is this derived from any of the other block-iscsi scripts floating
> > around on the net?
> > 
> >> Signed-off-by: Roger Pau Monnà <roger.pau@xxxxxxxxxx>
> >> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> >> Cc: Ian Jackson <ian.jackson@xxxxxxxxxx>
> >> ---
> >> Changes due to 4.3 release freeze:
> >>  * We can no longer provide a user/password, because they will be
> >>    stored in xenstore "params" backend node, which can be read by the
> >>    guest.
> >>  * Only works with PV domains because we don't have a hook in libxl to
> >>    pass the block device created by the script to Qemu.
> >>  * Doesn't work with pygrub because we don't call the script until
> >>    pygrub has already been executed, and even if we called it earlier
> >>    we still need a hook in order to provide the right block device to
> >>    pygrub.
> >> Changes since v1:
> >>  * Add -e to script shebang, and use 'set +e' if we know hotplug
> >>    execution might fail.
> >> ---
> >>  tools/hotplug/Linux/Makefile    |    1 +
> >>  tools/hotplug/Linux/block-iscsi |  198 
> >> +++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 199 insertions(+), 0 deletions(-)
> >>  create mode 100644 tools/hotplug/Linux/block-iscsi
> >>
> >> diff --git a/tools/hotplug/Linux/Makefile b/tools/hotplug/Linux/Makefile
> >> index 0605559..98c7738 100644
> >> --- a/tools/hotplug/Linux/Makefile
> >> +++ b/tools/hotplug/Linux/Makefile
> >> @@ -21,6 +21,7 @@ XEN_SCRIPTS += blktap
> >>  XEN_SCRIPTS += xen-hotplug-cleanup
> >>  XEN_SCRIPTS += external-device-migrate
> >>  XEN_SCRIPTS += vscsi
> >> +XEN_SCRIPTS += block-iscsi
> >>  XEN_SCRIPT_DATA = xen-script-common.sh locking.sh logging.sh
> >>  XEN_SCRIPT_DATA += xen-hotplug-common.sh xen-network-common.sh 
> >> vif-common.sh
> >>  XEN_SCRIPT_DATA += block-common.sh
> >> diff --git a/tools/hotplug/Linux/block-iscsi 
> >> b/tools/hotplug/Linux/block-iscsi
> >> new file mode 100644
> >> index 0000000..3cd6e34
> >> --- /dev/null
> >> +++ b/tools/hotplug/Linux/block-iscsi
> >> @@ -0,0 +1,198 @@
> >> +#!/bin/sh -e
> > 
> > Does this rely on any bashisms? Most of the other scripts use bash here,
> > and I'd be a bit worried about bashisms in the common code.
> 
> This script originally didn't use any bashisms, but if I include
> block-common.sh I have to use bash for sure.

I think I would prefer using the library functions over avoiding bash.

If someone hates bash that much they should port the library over
too ;-)

> > 
> >> +        echo "Unable to find iscsiadm tool"
> >> +        return 1
> > 
> > Error paths should use fatal() from xen-hotplug-common.sh so the error
> > is propagated to xenstore and libxl can print it.
> 
> Yes, if we call the script directly from libxl the output form the
> script will be logged by libxl,

Actually logged or just sent to stdout which libxl hasn't interfered
with? or does libxl suck in the stdout of the script? It looks to me
like libxl does the former.

> but since this script can also be called from udev I have to use fatal.

I think even for libxl, since it writes the error node which libxl does
then log.

> > 
> >> +
> >> +# Sets $dev to point to the device associated with the value in iqn
> >> +find_device()
> >> +{
> >> +    while [ ! -e /dev/disk/by-path/*"$iqn"-lun-0 ]; do
> >> +        sleep 0.1
> >> +    done
> > 
> > This loop is potentially infinite? (i.e. if something is broken)
> 
> Yes, I was trusting the timeout in libxl to prevent that, but again
> since this is not using the new calling convention it should work
> properly with udev and I have no idea if udev has a timeout, but we
> shouldn't rely on that.

Good.

> >> +    if [ ! -b "$sddev" ]; then
> >> +        echo "Unable to find attached device path"
> >> +        return 1
> >> +    fi
> >> +    if [ "$multipath" = "y" ]; then
> >> +        mdev=$(multipath -ll "$sddev" | head -1 | awk '{ print $1}')
> >> +        if [ ! -b /dev/mapper/"$mdev" ]; then
> >> +            echo "Unable to find attached device multipath"
> >> +            return 1
> >> +        fi
> >> +        dev="/dev/mapper/$mdev"
> >> +    else
> >> +        dev="$sddev"
> >> +    fi
> >> +    return 0
> >> +}
> >> +
> >> +# Attaches the target $iqn in $portal and sets $dev to point to the
> >> +# multipath device
> >> +attach()
> >> +{
> >> +    set +e
> >> +    iscsiadm -m node --targetname "$iqn" -p "$portal" --login > /dev/null 
> >> 2>&1
> > 
> > Might the error message be useful here? if you don't redirect
> > to /dev/null then you can use exec >>/tmp/hotplug.log tricks to debug
> > things...
> 
> iscsiadm always prints output, even when the target is successfully
> attached, and I haven't found any option to prevent it, so for now I
> guess we will have to redirect it to /dev/null

Does it produce output on stderr even on success too? i.e. can we just
redirect stdout? In a sane world the useful error messages would be on
stderr ;-)

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel