[libvirt] [PATCH] [RFC] ISCSI transport protocol support in libvirt

[Daniel Veillard]

On Wed, Jul 21, 2010 at 03:22:36PM +0200, Aurelien ROUGEMONT wrote:
> Hi everyone,

  Hi Aurelien,

> This my first post on this list

  Sorry for coming back so late, I accumulated a bit of backlog and
  since this clearly wasn't for 0.8.3 it ended up on the backburner...

> Context : Some days ago I have decided to use infiniband for ISCSI
> streams. Infiniband adds a new wonderful transport protocol : ISER.
> This protocols is added to the well known the default TCP. (NB: ISER
> = ISCSI + RDMA). I could not see any ISCSI transport protocol
> modification support in libivirt (the default protocol tcp is even
> hardcoded in the regex)
> 
> What i have done :
> - did the shell testing for the iscsiadm
> - the attached patch that corrects 2 typos in the original code and
> switches completely the iscsi transport protocol from tcp to iser
> (which is not ideal at all)

  The typo should be applied separately, no need to wait for them

> What should be done (imho):
> - add iscsi transport protocol support (using my patch as a basis)
> - add a new XML property/whatever_fits_the_projects_policy to the
> storagepool object that allows user to pick the iscsi transport
> protocol (default is tcp)
> 
> I was thinking of having something like :
> 
> <pool type="iscsi">
>         <name>volumename</name>
>         <source>
>           <host name="1.2.3.4"/>
>           <device path="IQNOFTARGET"/>
>           <transport protocol="iser"/>
>         </source>
>         <target>
>           <path>/dev/disk/by-path</path>
>         </target>
> </pool>
> 
> Any comment on this ? Any help on the XML part ?

  the XML addition of a transport element with a protocol attribute
looks fine to me. Values would for now be 'tcp' by default and
optionally 'iser'

> Best regards,
> 
> Aurélien
> 
> 
> NB: the current iscsi transport protocols available are :
> tcp(default), iser, qla4xxx, bnx2, and icxgb3i.

  what about the other protocols ? Can you detect availability ? Can you
automatically set things up from libvirt ?

> PS: i'm still doing extensive testing of my patch
> 
> 
> !DSPAM:4c46f49d90977882820711!

> diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c
> index f34123f..7c21ad7 100644
> --- a/src/storage/storage_backend_iscsi.c
> +++ b/src/storage/storage_backend_iscsi.c
> @@ -113,11 +113,13 @@ virStorageBackendISCSISession(virStoragePoolObjPtr pool,
>       * # iscsiadm --mode session
>       * tcp: [1] 192.168.122.170:3260,1 demo-tgt-b
>       * tcp: [2] 192.168.122.170:3260,1 demo-tgt-a
> +     * iser: [5] 10.111.140.1:3260,1 demo-tgt-d
> +
>       *
>       * Pull out 2nd and 4th fields
>       */
>      const char *regexes[] = {
> -        "^tcp:\\s+\\[(\\S+)\\]\\s+\\S+\\s+(\\S+)\\s*$"
> +        "^iser:\\s+\\[(\\S+)\\]\\s+\\S+\\s+(\\S+)\\s*$"
>      };

 Since regexes are a list of allowed regexps you should just add
    "^iser:\\s+\\[(\\S+)\\]\\s+\\S+\\s+(\\S+)\\s*$"
as the second element in the array

>      int vars[] = {
>          2,
> @@ -230,7 +232,7 @@ virStorageBackendIQNFound(virStoragePoolObjPtr pool,
>  
>  out:
>      if (ret == IQN_MISSING) {
> -        VIR_DEBUG("Could not find interface witn IQN '%s'", iqn);
> +        VIR_DEBUG("Could not find interface with IQN '%s'", iqn);
>      }
>  
>      VIR_FREE(line);
> @@ -293,7 +295,24 @@ virStorageBackendCreateIfaceIQN(virStoragePoolObjPtr pool,
>      if (virRun(cmdargv2, &exitstatus) < 0) {
>          virStorageReportError(VIR_ERR_INTERNAL_ERROR,
>                                _("Failed to run command '%s' to update iscsi interface with IQN '%s'"),
> -                              cmdargv1[0], pool->def->source.initiator.iqn);
> +                              cmdargv2[0], pool->def->source.initiator.iqn);

  Those 2 typo need to be fixed, I pushed this to git, no need to wait

> +        goto out;
> +    }
> +
> +    /* This part switches iscsi transport protocol from default (tcp) to iser
> +     * TODO: add some XML variable variable for enabling/disabling this block*/ 
> +    const char *const cmdargv3[] = {
> +        ISCSIADM, "--mode", "iface", "--interface", &temp_ifacename[0],
> +        "--op", "update", "--name iface.transport_name", "--value iser", NULL
> +    };
> +
> +    /* Note that we ignore the exitstatus.  Older versions of iscsiadm tools
> +     * returned an exit status of > 0, even if they succeeded.  We will just
> +     * rely on whether iface file got updated properly. */
> +    if (virRun(cmdargv3, &exitstatus) < 0) {
> +        virStorageReportError(VIR_ERR_INTERNAL_ERROR,
> +                              _("Failed to run command '%s' to update iscsi interface with IQN '%s'"),
> +                              cmdargv3[0], pool->def->source.initiator.iqn);
>          goto out;
>      }

  Well if you run this here you prevent normal tcp operations it seems.

The big problem is to know when you need to do this, and either this can
be found from the environment settings on the node (possible ?) or
fallback to the user indicating this.
Doing the reading from configuration should not be hard look at the
existing conf code doing it, if needed grab me on IRC for help on the
XML parsing/saving code,

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/