| Message ID | f98e8eff11ac4c860922c41b2f04836d@crc.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 05, 2016 at 03:52:30PM +1000, Steven Haigh wrote: > On 2016-05-05 12:32, Steven Haigh wrote: > > Overview > > > > If you're using iSCSI, you can mount a target by multiple Dom0 > > machines on the same target. For non-cluster aware filesystems, this > > can lead to disk corruption and general bad times by all. The iSCSI > > protocol allows the use of persistent reservations as per the SCSI > > disk spec. Low level SCSI commands for locking are handled by the > > sg_persist program (bundled with sg3_utils package in EL). > > > > The aim of this patch is to create a 'locktarget=y' option specified > > within the disk 'target' command for iSCSI to lock the target in > > exclusive mode on VM start with a key generated from the local systems > > IP, and release this lock on the shutdown of the DomU. > > > > Example Config: > > disk = > > ['script=block-iscsi,vdev=xvda,target=iqn=iqn.1986-03.com.sun:02:mytarget,portal=iscsi.example.com,locktarget=y'] > > > > In writing this, I have also re-factored parts of the script to put > > some things in what I believe to be a better place to make expansion > > easier. This is mainly in removing functions that purely call other > > functions with no actual code execution. > > > > Signed-off-by: Steven Haigh <netwiz@crc.id.au> > > > > (on a side note, first time I've submitted a patch to the list and I'm > > currently stuck on a webmail client, so apologies in advance if this > > all goes wrong ;) > > Changes in v2: > Bugfix: Call find_device to locate the /dev/sdX component of the iSCSI > target before trying to run unlock_device(). > > Apologies for this oversight. Thanks for the patch! A couple of comments below. > -- > Steven Haigh > > Email: netwiz@crc.id.au > Web: https://www.crc.id.au > Phone: (03) 9001 6090 - 0412 935 897 > --- block-iscsi 2016-02-10 01:44:19.000000000 +1100 > +++ block-iscsi-lock 2016-05-05 15:42:09.557191235 +1000 > @@ -31,33 +31,37 @@ > echo $1 | sed "s/^\("$2"\)//" > } > > -check_tools() > -{ > - if ! command -v iscsiadm > /dev/null 2>&1; then > - fatal "Unable to find iscsiadm tool" > - fi > - if [ "$multipath" = "y" ] && ! command -v multipath > /dev/null 2>&1; then > - fatal "Unable to find multipath" > - fi > -} > - > # Sets the following global variables based on the params field passed in as > # a parameter: iqn, portal, auth_method, user, multipath, password > parse_target() > { > # set multipath default value > multipath="n" > - for param in $(echo "$1" | tr "," "\n") > - do > + for param in $(echo "$1" | tr "," "\n"); do > case $param in > iqn=*) > iqn=$(remove_label $param "iqn=") > + if ! command -v iscsiadm > /dev/null 2>&1; then > + fatal "Could not find iscsiadm tool." > + fi > ;; > portal=*) > portal=$(remove_label $param "portal=") > ;; > multipath=*) > multipath=$(remove_label $param "multipath=") > + if ! command -v multipath > /dev/null 2>&1; then > + fatal "Multipath selected, but no multipath tools found" > + fi > + ;; > + locktarget=*) > + locktarget=$(remove_label $param "locktarget=") > + if ! command -v sg_persist > /dev/null 2>&1; then > + fatal "Locking requested but no sg_persist found" > + fi > + if ! command -v gethostip > /dev/null 2>&1; then > + fatal "Locking requested but no gethostip found for key generation" > + fi Why don't you just add this to check_tools? In any case, if you want to fold check_tools functionality into parse_target I think it should be done in a separate patch in order for it to be easier to review. IMHO, I prefer to have both functions separated, because it's quite clear what parse_target and check_tools do. > ;; > esac > done > @@ -96,38 +100,29 @@ > fi > } > > -# Attaches the target $iqn in $portal and sets $dev to point to the > -# multipath device > -attach() > -{ > - do_or_die iscsiadm -m node --targetname "$iqn" -p "$portal" --login > /dev/null > - find_device > -} > > -# Discovers targets in $portal and checks that $iqn is one of those targets > -# Also sets the auth parameters to attach the device > -prepare() > +lock_device() > { > - # Check if target is already opened > - iscsiadm -m session 2>&1 | grep -q "$iqn" && fatal "Device already opened" > - # Discover portal targets > - iscsiadm -m discovery -t st -p $portal 2>&1 | grep -q "$iqn" || \ > - fatal "No matching target iqn found" > -} > - > -# Attaches the device and writes xenstore backend entries to connect > -# the device > -add() > -{ > - attach > - write_dev $dev > + ## Lock the iSCSI target as Exclusive Access. > + key=$(gethostip -x $(uname -n)) Isn't there anything else that can be used as a key? What would happen if the host IP changes while the VM is running? Couldn't the iqn (or a hash of it) be used as the key? > + if ! sg_persist -d ${dev} -o -G -S ${key} > /dev/null; then > + unlock_device > + iscsiadm -m node --targetname "$iqn" -p "$portal" --logout > /dev/null > + fatal "iSCSI LOCK: Failed to register with target" > + fi > + if ! sg_persist -d ${dev} -o -R -K ${key} -T 6 > /dev/null; then > + unlock_device > + iscsiadm -m node --targetname "$iqn" -p "$portal" --logout > /dev/null > + fatal "iSCSI LOCK: Failed to set persistent reservation" > + fi > } > > -# Disconnects the device > -remove() > +unlock_device() > { > - find_device > - do_or_die iscsiadm -m node --targetname "$iqn" -p "$portal" --logout > /dev/null > + ## Unlock the iSCSI target. > + key=$(gethostip -x $(uname -n)) > + sg_persist -d ${dev} -o -L -K ${key} -T 6 > /dev/null || true > + sg_persist -d ${dev} -o -G -K ${key} -S 0 > /dev/null || true I'm not sure whether the return code of those functions shouldn't be checked. I know this is the teardown path, so there's not much that can be done here on failure, but it seems like at least the script should check for the return code '3', and then retry the command. According to sg3_utils(8), a '3' means: "the DEVICE reports that it is not ready for the operation requested. The device may be in the process of becoming ready (e.g. spinning up but not at speed) so the utility may work after a wait." > } > > command=$1 > @@ -138,15 +133,28 @@ > > parse_target "$target" > > -check_tools || exit 1 > - > case $command in > add) > - prepare > - add > + # Check if target is already opened > + iscsiadm -m session 2>&1 | grep -q "$iqn" && fatal "Device already opened" > + # Discover portal targets > + iscsiadm -m discovery -t st -p $portal 2>&1 | grep -q "$iqn" || \ > + fatal "No matching target iqn found" > + > + ## Login to the iSCSI target. > + do_or_die iscsiadm -m node --targetname "$iqn" -p "$portal" --login > /dev/null > + find_device > + if [ "$locktarget" = "y" ]; then > + lock_device > + fi > + write_dev $dev > ;; > remove) > - remove > + find_device > + if [ "$locktarget" = "y" ]; then > + unlock_device > + fi > + do_or_die iscsiadm -m node --targetname "$iqn" -p "$portal" --logout > /dev/null You are doing quite of a rework of the script here (by removing/merging a bunch of functions). I think this should be sent as a separate patch, in order to ease the review (mixing new features with code cleanup makes it quite hard to review). Roger.
On 6/05/2016 7:09 PM, Roger Pau Monné wrote: > On Thu, May 05, 2016 at 03:52:30PM +1000, Steven Haigh wrote: >> On 2016-05-05 12:32, Steven Haigh wrote: >>> Overview >>> >>> If you're using iSCSI, you can mount a target by multiple Dom0 >>> machines on the same target. For non-cluster aware filesystems, this >>> can lead to disk corruption and general bad times by all. The iSCSI >>> protocol allows the use of persistent reservations as per the SCSI >>> disk spec. Low level SCSI commands for locking are handled by the >>> sg_persist program (bundled with sg3_utils package in EL). >>> >>> The aim of this patch is to create a 'locktarget=y' option specified >>> within the disk 'target' command for iSCSI to lock the target in >>> exclusive mode on VM start with a key generated from the local systems >>> IP, and release this lock on the shutdown of the DomU. >>> >>> Example Config: >>> disk = >>> ['script=block-iscsi,vdev=xvda,target=iqn=iqn.1986-03.com.sun:02:mytarget,portal=iscsi.example.com,locktarget=y'] >>> >>> In writing this, I have also re-factored parts of the script to put >>> some things in what I believe to be a better place to make expansion >>> easier. This is mainly in removing functions that purely call other >>> functions with no actual code execution. >>> >>> Signed-off-by: Steven Haigh <netwiz@crc.id.au> >>> >>> (on a side note, first time I've submitted a patch to the list and I'm >>> currently stuck on a webmail client, so apologies in advance if this >>> all goes wrong ;) >> >> Changes in v2: >> Bugfix: Call find_device to locate the /dev/sdX component of the iSCSI >> target before trying to run unlock_device(). >> >> Apologies for this oversight. > > Thanks for the patch! A couple of comments below. > >> -- >> Steven Haigh >> >> Email: netwiz@crc.id.au >> Web: https://www.crc.id.au >> Phone: (03) 9001 6090 - 0412 935 897 > >> --- block-iscsi 2016-02-10 01:44:19.000000000 +1100 >> +++ block-iscsi-lock 2016-05-05 15:42:09.557191235 +1000 >> @@ -31,33 +31,37 @@ >> echo $1 | sed "s/^\("$2"\)//" >> } >> >> -check_tools() >> -{ >> - if ! command -v iscsiadm > /dev/null 2>&1; then >> - fatal "Unable to find iscsiadm tool" >> - fi >> - if [ "$multipath" = "y" ] && ! command -v multipath > /dev/null 2>&1; then >> - fatal "Unable to find multipath" >> - fi >> -} >> - >> # Sets the following global variables based on the params field passed in as >> # a parameter: iqn, portal, auth_method, user, multipath, password >> parse_target() >> { >> # set multipath default value >> multipath="n" >> - for param in $(echo "$1" | tr "," "\n") >> - do >> + for param in $(echo "$1" | tr "," "\n"); do >> case $param in >> iqn=*) >> iqn=$(remove_label $param "iqn=") >> + if ! command -v iscsiadm > /dev/null 2>&1; then >> + fatal "Could not find iscsiadm tool." >> + fi >> ;; >> portal=*) >> portal=$(remove_label $param "portal=") >> ;; >> multipath=*) >> multipath=$(remove_label $param "multipath=") >> + if ! command -v multipath > /dev/null 2>&1; then >> + fatal "Multipath selected, but no multipath tools found" >> + fi >> + ;; >> + locktarget=*) >> + locktarget=$(remove_label $param "locktarget=") >> + if ! command -v sg_persist > /dev/null 2>&1; then >> + fatal "Locking requested but no sg_persist found" >> + fi >> + if ! command -v gethostip > /dev/null 2>&1; then >> + fatal "Locking requested but no gethostip found for key generation" >> + fi > > Why don't you just add this to check_tools? In any case, if you want to fold > check_tools functionality into parse_target I think it should be done in a > separate patch in order for it to be easier to review. > > IMHO, I prefer to have both functions separated, because it's quite clear > what parse_target and check_tools do. My thoughts are that it makes more sense to check that the tool required is there depending on the options that are provided. It saves multiple cases that would end up checking if an option is set, then check if the tool is there. Leaving the existing structure would mean that you parse out the target options in parse_target, but then also check which options are set to then check if the required tools are available for the specific options. It seems more logical and efficient to me to merge this together and essentially do the pre-flight checks as we go. It may be an idea to then rename parse_target to something else as a function name - maybe prepare (as that function doesn't exist anymore) to better reflect what it does? >> ;; >> esac >> done >> @@ -96,38 +100,29 @@ >> fi >> } >> >> -# Attaches the target $iqn in $portal and sets $dev to point to the >> -# multipath device >> -attach() >> -{ >> - do_or_die iscsiadm -m node --targetname "$iqn" -p "$portal" --login > /dev/null >> - find_device >> -} >> >> -# Discovers targets in $portal and checks that $iqn is one of those targets >> -# Also sets the auth parameters to attach the device >> -prepare() >> +lock_device() >> { >> - # Check if target is already opened >> - iscsiadm -m session 2>&1 | grep -q "$iqn" && fatal "Device already opened" >> - # Discover portal targets >> - iscsiadm -m discovery -t st -p $portal 2>&1 | grep -q "$iqn" || \ >> - fatal "No matching target iqn found" >> -} >> - >> -# Attaches the device and writes xenstore backend entries to connect >> -# the device >> -add() >> -{ >> - attach >> - write_dev $dev >> + ## Lock the iSCSI target as Exclusive Access. >> + key=$(gethostip -x $(uname -n)) > > Isn't there anything else that can be used as a key? What would happen if > the host IP changes while the VM is running? Couldn't the iqn (or a hash > of it) be used as the key? I thought long and hard about this - as I would like a better method. The key is used to secure the persistent reservation. If another system uses the same key, then they can remove the reservation. My testing seems to indicate that if you provide the same key while asking for a lock, you get it - even if a previous lock was granted to another system. The idea here is that each Dom0 would have a different key - therefore the lock request from a different Dom0 would fail. >> + if ! sg_persist -d ${dev} -o -G -S ${key} > /dev/null; then >> + unlock_device >> + iscsiadm -m node --targetname "$iqn" -p "$portal" --logout > /dev/null >> + fatal "iSCSI LOCK: Failed to register with target" >> + fi >> + if ! sg_persist -d ${dev} -o -R -K ${key} -T 6 > /dev/null; then >> + unlock_device >> + iscsiadm -m node --targetname "$iqn" -p "$portal" --logout > /dev/null >> + fatal "iSCSI LOCK: Failed to set persistent reservation" >> + fi >> } >> >> -# Disconnects the device >> -remove() >> +unlock_device() >> { >> - find_device >> - do_or_die iscsiadm -m node --targetname "$iqn" -p "$portal" --logout > /dev/null >> + ## Unlock the iSCSI target. >> + key=$(gethostip -x $(uname -n)) >> + sg_persist -d ${dev} -o -L -K ${key} -T 6 > /dev/null || true >> + sg_persist -d ${dev} -o -G -K ${key} -S 0 > /dev/null || true > > I'm not sure whether the return code of those functions shouldn't be > checked. I know this is the teardown path, so there's not much that can > be done here on failure, but it seems like at least the script should check > for the return code '3', and then retry the command. According to > sg3_utils(8), a '3' means: > > "the DEVICE reports that it is not ready for the operation requested. The > device may be in the process of becoming ready (e.g. spinning up but not at > speed) so the utility may work after a wait." Possibly - however I came across a problem here... pygrub will add & remove while it gets the grub config file, then add again while firing up the actual VM. This causes a lock - unlock - lock - boot - unlock type workflow. You're right that it may be worthwhile to retry some of these - but I don't quite have a method in mind to do this that I'd be happy with. >> } >> >> command=$1 >> @@ -138,15 +133,28 @@ >> >> parse_target "$target" >> >> -check_tools || exit 1 >> - >> case $command in >> add) >> - prepare >> - add >> + # Check if target is already opened >> + iscsiadm -m session 2>&1 | grep -q "$iqn" && fatal "Device already opened" >> + # Discover portal targets >> + iscsiadm -m discovery -t st -p $portal 2>&1 | grep -q "$iqn" || \ >> + fatal "No matching target iqn found" >> + >> + ## Login to the iSCSI target. >> + do_or_die iscsiadm -m node --targetname "$iqn" -p "$portal" --login > /dev/null >> + find_device >> + if [ "$locktarget" = "y" ]; then >> + lock_device >> + fi >> + write_dev $dev >> ;; >> remove) >> - remove >> + find_device >> + if [ "$locktarget" = "y" ]; then >> + unlock_device >> + fi >> + do_or_die iscsiadm -m node --targetname "$iqn" -p "$portal" --logout > /dev/null > > You are doing quite of a rework of the script here (by removing/merging a > bunch of functions). I think this should be sent as a separate patch, in > order to ease the review (mixing new features with code cleanup makes it > quite hard to review). Yeah I know - and I'm kinda new at this (being my first patch sent here). I also don't have a git clone of the xen repos, or anything else to base this one right now so I end up doing this manually. I'll see if I can break this up a bit more - but my time is somewhat limited atm when I have access to these machines to tinker.
--- block-iscsi 2016-02-10 01:44:19.000000000 +1100 +++ block-iscsi-lock 2016-05-05 15:42:09.557191235 +1000 @@ -31,33 +31,37 @@ echo $1 | sed "s/^\("$2"\)//" } -check_tools() -{ - if ! command -v iscsiadm > /dev/null 2>&1; then - fatal "Unable to find iscsiadm tool" - fi - if [ "$multipath" = "y" ] && ! command -v multipath > /dev/null 2>&1; then - fatal "Unable to find multipath" - fi -} - # Sets the following global variables based on the params field passed in as # a parameter: iqn, portal, auth_method, user, multipath, password parse_target() { # set multipath default value multipath="n" - for param in $(echo "$1" | tr "," "\n") - do + for param in $(echo "$1" | tr "," "\n"); do case $param in iqn=*) iqn=$(remove_label $param "iqn=") + if ! command -v iscsiadm > /dev/null 2>&1; then + fatal "Could not find iscsiadm tool." + fi ;; portal=*) portal=$(remove_label $param "portal=") ;; multipath=*) multipath=$(remove_label $param "multipath=") + if ! command -v multipath > /dev/null 2>&1; then + fatal "Multipath selected, but no multipath tools found" + fi + ;; + locktarget=*) + locktarget=$(remove_label $param "locktarget=") + if ! command -v sg_persist > /dev/null 2>&1; then + fatal "Locking requested but no sg_persist found" + fi + if ! command -v gethostip > /dev/null 2>&1; then + fatal "Locking requested but no gethostip found for key generation" + fi ;; esac done @@ -96,38 +100,29 @@ fi } -# Attaches the target $iqn in $portal and sets $dev to point to the -# multipath device -attach() -{ - do_or_die iscsiadm -m node --targetname "$iqn" -p "$portal" --login > /dev/null - find_device -} -# Discovers targets in $portal and checks that $iqn is one of those targets -# Also sets the auth parameters to attach the device -prepare() +lock_device() { - # Check if target is already opened - iscsiadm -m session 2>&1 | grep -q "$iqn" && fatal "Device already opened" - # Discover portal targets - iscsiadm -m discovery -t st -p $portal 2>&1 | grep -q "$iqn" || \ - fatal "No matching target iqn found" -} - -# Attaches the device and writes xenstore backend entries to connect -# the device -add() -{ - attach - write_dev $dev + ## Lock the iSCSI target as Exclusive Access. + key=$(gethostip -x $(uname -n)) + if ! sg_persist -d ${dev} -o -G -S ${key} > /dev/null; then + unlock_device + iscsiadm -m node --targetname "$iqn" -p "$portal" --logout > /dev/null + fatal "iSCSI LOCK: Failed to register with target" + fi + if ! sg_persist -d ${dev} -o -R -K ${key} -T 6 > /dev/null; then + unlock_device + iscsiadm -m node --targetname "$iqn" -p "$portal" --logout > /dev/null + fatal "iSCSI LOCK: Failed to set persistent reservation" + fi } -# Disconnects the device -remove() +unlock_device() { - find_device - do_or_die iscsiadm -m node --targetname "$iqn" -p "$portal" --logout > /dev/null + ## Unlock the iSCSI target. + key=$(gethostip -x $(uname -n)) + sg_persist -d ${dev} -o -L -K ${key} -T 6 > /dev/null || true + sg_persist -d ${dev} -o -G -K ${key} -S 0 > /dev/null || true } command=$1 @@ -138,15 +133,28 @@ parse_target "$target" -check_tools || exit 1 - case $command in add) - prepare - add + # Check if target is already opened + iscsiadm -m session 2>&1 | grep -q "$iqn" && fatal "Device already opened" + # Discover portal targets + iscsiadm -m discovery -t st -p $portal 2>&1 | grep -q "$iqn" || \ + fatal "No matching target iqn found" + + ## Login to the iSCSI target. + do_or_die iscsiadm -m node --targetname "$iqn" -p "$portal" --login > /dev/null + find_device + if [ "$locktarget" = "y" ]; then + lock_device + fi + write_dev $dev ;; remove) - remove + find_device + if [ "$locktarget" = "y" ]; then + unlock_device + fi + do_or_die iscsiadm -m node --targetname "$iqn" -p "$portal" --logout > /dev/null ;; *) exit 1