[libvirt] [PATCH 5/5] virISCSIScanTargets: Allow making targets persistent

Tue Jul 3 13:08:43 UTC 2018

On 07/03/2018 02:42 PM, John Ferlan wrote:
> 
> 
> On 07/03/2018 01:08 AM, Michal Prívozník wrote:
>> On 07/03/2018 01:40 AM, John Ferlan wrote:
>>>
>>>
>>> On 06/29/2018 11:01 AM, Michal Privoznik wrote:
>>>> After new iSCSI interface is successfully set up, we issue
>>>
>>> s/new/a new/
>>> s/issue/issue a/
>>>
>>>> sendtargets command. However, after 56057900dc53df490d we don't
>>>> update the host config which in turn makes login fail because
>>>> iscsiadm is unable to find any matching record for the interface.
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>>> ---
>>>>  src/storage/storage_backend_iscsi.c |  1 +
>>>>  src/util/viriscsi.c                 | 21 ++++++++++++++++++---
>>>>  src/util/viriscsi.h                 |  1 +
>>>>  tests/viriscsitest.c                |  3 ++-
>>>>  4 files changed, 22 insertions(+), 4 deletions(-)
>>>>
>>>
>>> Like the previous patch - is there a specific bug or something that led
>>> you down this path?  Can you show an example of the existing code that's
>>> creating a bad command line and generating an error and then how this
>>> fixes things.  It's not like we have tests and for this stuff it's
>>> really nice to have plenty of examples.
>>
>> So here is the run without my patches:
>>
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session
>> iscsiadm: No active sessions.
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal $PORTAL:3260,1 --targetname $TARGET --op new
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal $PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.authmethod --value CHAP
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal $PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.username --value $USER
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal $PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.password --value $PASS
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface --interface libvirt-iface-03316143 --op new
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface --interface libvirt-iface-03316143 --op update --name iface.initiatorname --value $INITIATOR
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode discovery --type sendtargets --portal $PORTAL:3260,1 --op nonpersistent
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal $PORTAL:3260,1 --targetname $TARGET --login --interface libvirt-iface-03316143
>> error : virCommandWait:2600 : internal error: Child process (iscsiadm --mode node --portal $PORTAL:3260,1 --targetname $TARGET --login --interface libvirt-iface-03316143) unexpected exit status 21:
>> iscsiadm: No records found
>> iscsiadm: No records found
>>
>>
>> And with my patches:
>>
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session
>> iscsiadm: No active sessions.
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal $PORTAL:3260,1 --targetname $TARGET --op new
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal $PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.authmethod --value CHAP
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal $PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.username --value $USER
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal $PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.password --value $PASS
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface --interface libvirt-iface-28727243 --op new
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface --interface libvirt-iface-28727243 --op update --name iface.initiatorname --value $INITIATOR
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode discovery --type sendtargets --portal $PORTAL:3260,1 --interface libvirt-iface-28727243
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal $PORTAL:3260,1 --targetname $TARGET --login --interface libvirt-iface-28727243
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session -r 1 -R
>>
>>
> 
> So the example helps me understand - thanks! The difference of discovery
> being:
> 
>   fail: iscsiadm --mode discovery --type sendtargets \
>                  --portal $PORTAL:3260,1 \
>                  --op nonpersistent
> 
>   succ: iscsiadm --mode discovery --type sendtargets \
>                  --portal $PORTAL:3260,1 \
>                  --interface libvirt-iface-28727243
> 
> So, what commit 56057900d did to "help" or "fix" auto-login for sessions
> that do not "belong to" libvirt is being called out as causing problems
> for the initiatoriqn sessions. Prior to that commit the command was:
> 
>     iscsiadm --mode discovery --type sendtargets --portal $PORTAL:3260,1
> 
> So, I have to wonder "how" or "if" discovery using an initiatoriqn
> really worked since there wasn't an --interface argument. 

Maybe it did. Maybe iscsiadm used to use all interfaces (which okay,
sounds unlikely) for sendtargets. Or it remembered that we added a
special interface for $PORTAL and preferred that. I have no idea. But it
is clearly not working now.

> Not sure I
> have the patience/desire to go through the history of refactors and
> adjustments since 6aabcb5 though ;-). Although it does appear that the
> original code would make a "node" during it's connection period rather
> than doing a sendtargets using the --interface option (and whether that
> option existed at the time is a different search through a different set
> of code).
> 
> Given all of what's been discovered here - I think patch 4 and 5 should
> be combined. 

Well, I can merge them. It's just that I wanted these virISCSI* helpers
to be generic enough so that when one day somebody wants to persists
targets for default connection they can do that. I view @ifacename and
@persist arguments as orthogonal. It's only the usage of APIs that makes
the arguments mutually exclusive (or tied together or what).

> And rather than adding a new "bool persist" parameter that
> only is true when initiatoriqn is passed, let's use the fact that
> initiatoriqn was passed in order to:
> 
> if (ifacename)
>     virCommandAddArgList(cmd, "--interface", ifacename, NULL);
> else
>     virCommandAddArgList(cmd, "--op", "nonpersistent", NULL);
> 
> Then as long as non initiatoriqn sessions still work, even those without
> a libvirt pool associated - we should be able to declare victory. Not
> sure "if" non libvirt pool initiatoriqn sessions would be affected by
> all this.

They shouldn't. That is why libvirt creates its own interface.

> 
> John
> 
> BTW: Based on that commit, I wonder if you can modify viriscsitest.c a
> bit in order to generate the initiatoriqn output as well via a change to
> testIscsiadmCb.
> 

I'll look into it.

Michal