[libvirt] [RFC] Multi-IQN proposal

[Dave Allan]

Shyam_Iyer at Dell.com wrote:
> Would this proposal be acceptable ?

In principle, I think what you're proposing is reasonable, and is 
certainly contemplated by the iSCSI specs.

> Example XML schema for an iSCSI storage pool created --
> 
>  <pool type="iscsi">
>   <name>dell</name>
>   <uuid>cf354733-b01b-8870-2040-94888640e24d</uuid>
>   <capacity>0</capacity>
>   <allocation>0</allocation>
>   <available>0</available>
> - <source>
>   <initiator iqnname = "<initiator IQN1>">
>   <initiator iqnname = "<initiator IQN2>">
>   ........................................
>   ........................................
>   <host name="<iSCSI target hostname or Target IP address>" />
>   <device path="<iSCSI Target IQN name>" />
>   </source>
> - <target>
>   <path>/dev/disk/by-path</path>
> - <permissions>
>   <mode>0700</mode>
>   <owner>0</owner>
>   <group>0</group>
>   </permissions>
>   </target>
>   </pool>

I think you have the initiator name specified in the right place in the 
XML.  I would make the initiator iqn an element rather than an 
attribute, since your proposal contemplates adding additional initiator 
specific information later, and stylistically I think elements will be 
cleaner.  That gives:

<initiator>
	<iqn>iqn.foo1.bar.baz</iqn>
	<iqn>iqn.foo2.bar.baz</iqn>
	<iqn>iqn.foo3.bar.baz</iqn>
</initiator>

> Each initiator iqn name can be parsed to create the unique sessions.

You should propose specifically how you see the sessions being set up. 
Each pool currently describes something that basically resembles a 
session, so your proposal modifies that paradigm a bit.  Another 
possible way to implement what you describe would be to allow zero or 
one initiator tags within a pool.  If no initiator tag is specified, the 
system will use the system default; if a tag is specified, the system 
will attempt to use the information contained in it.  The more I think 
about it, the more I like that approach since it keeps the pool paradigm 
unmodified.

> This should solve the following possibilities --
> 
> * possibility of multiple IQNs for a single Guest
> * option for Guest's own BIOS & initiator to use these IQNs (iSCSI in
> Guest)
> * option for hypervisor's initiator to use these IQNs on behalf of the
> guest
> (Multi-IQN)

How is this different from the first possibility?

> 
> 
> Compile tested only. Needs beatification.

I didn't go over the code closely, but I didn't see anything that struck 
me as completely off base.  I think it's more important to get the 
details of how this information will be used worked out at this point 
than to get the code finalized.

Dave

> diff --git a/src/storage_conf.c b/src/storage_conf.c
> index cb063cc..915e897 100644
> --- a/src/storage_conf.c
> +++ b/src/storage_conf.c
> @@ -116,6 +116,7 @@ enum {
>      VIR_STORAGE_POOL_SOURCE_DIR     = (1<<2),
>      VIR_STORAGE_POOL_SOURCE_ADAPTER = (1<<3),
>      VIR_STORAGE_POOL_SOURCE_NAME    = (1<<4),
> +    VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN  = (1<<5),
>  };
> 
> 
> @@ -537,6 +538,33 @@ virStoragePoolDefParseXML(virConnectPtr conn,
>              goto cleanup;
>          }
>      }
> +
> +/* Read the conf here */
> +    if (options->flags & VIR_STORAGE_POOL_SOURCE_INITATOR_IQN) {
> +       int niqn, i;
> +
> +       if ((niqn = virXPathNodeSet(conn, "./source/host/initiator",
> ctxt, &nodeset)) < 0) {
> +            virStorageReportError(conn, VIR_ERR_XML_ERROR,
> +                        "%s", _("cannot extract storage pool source
> devices"));
> +            goto cleanup;
> +        }
> +       if (VIR_ALLOC_N(ret->source.niqn, niqn) < 0) {
> +            VIR_FREE(nodeset);
> +            virReportOOMError(conn);
> +            goto cleanup;
> +        }
> +       for (i = 0 ; i < niqn ; i++ ) {
> +           xmlChar *name = xmlGetProp(nodeset[i], BAD_CAST "iqnname");
> +            if (path == NULL) {
> +                VIR_FREE(nodeset);
> +                virStorageReportError(conn, VIR_ERR_XML_ERROR,
> +                        "%s", _("missing storage pool source device
> path"));
> +                goto cleanup;
> +            }
> +           ret->source.initiator[i].iqnname  = (char  *)name;
> +       }
> +    }
>      if (options->flags & VIR_STORAGE_POOL_SOURCE_DEVICE) {
>          xmlNodePtr *nodeset = NULL;
>          int nsource, i;
> diff --git a/src/storage_conf.h b/src/storage_conf.h
> index 421d305..c2504be 100644
> --- a/src/storage_conf.h
> +++ b/src/storage_conf.h
> @@ -202,7 +202,12 @@ struct _virStoragePoolSourceDevice {
>      } geometry;
>  };
> 
> -
> +/* Initiator Attributes */
> +typedef virStoragePoolSourceInitiatorAttr
> *virStoragePoolSourceInitiatorAttrPtr;
> +struct virStoragePoolSourceInitiatorAttr {
> +    char *iqnname;
> +    /* We could put other initiator specific things here */
> +}
> 
>  typedef struct _virStoragePoolSource virStoragePoolSource;
>  typedef virStoragePoolSource *virStoragePoolSourcePtr;
> @@ -214,6 +219,9 @@ struct _virStoragePoolSource {
>      int ndevice;
>      virStoragePoolSourceDevicePtr devices;
> 
> +    /*And either one or more initiators*/
> +    virStoragePoolSourceInitiatorAttrPtr initiator;
> +
>      /* Or a directory */
>      char *dir;
> 
> 
> --
> Libvir-list mailing list
> Libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list