[libvirt] [PATCH 1/1] Implement NPIV

[Daniel P. Berrange]

On Wed, Apr 08, 2009 at 05:53:57PM -0400, Dave Allan wrote:
> Daniel P. Berrange wrote:
> >>diff --git a/src/storage_conf.h b/src/storage_conf.h
> >>index 4e35ccb..cfd8b14 100644
> >>--- a/src/storage_conf.h
> >>+++ b/src/storage_conf.h
> >>@@ -192,6 +192,12 @@ struct _virStoragePoolSource {
> >>     /* Or an adapter */
> >>     char *adapter;
> >> 
> >>+    /* And an optional WWPN */
> >>+    char *wwpn;
> >>+
> >>+    /* And an optional WWNN */
> >>+    char *wwnn;
> >>+
> >
> >Since adapter, wwpn and wwnn are all associated with each other, I
> >think we should put them all into a union here. eg, replace the
> >current
> >
> >      char *adapter;
> >
> >with
> >
> >     union {
> >        char *name;
> >        char *wwpn;
> >        char *wwnn;
> >     } adapter;
> 
> I don't have completely working code yet, but here's what I'm thinking:
> 
> We can't use a union because wwpn and wwnn will always be in use at the 
> same time, but I agree that we're starting to collect a bunch of related 
> information about adapters that clutters up the pool source struct. 
> I've created a struct along the lines of virStoragePoolSourceHost to 
> contain this information.  It will need some additional fields.

I don't know what I was thinking when I wrote 'union'. I absolutely
meant an anonymous 'struct'. A named struct as you show below is
even better

> 
> /* 
> 
>  * For adapter based pools, info on the relevant adapter 
> 
>  */
> typedef struct _virStoragePoolSourceAdapter virStoragePoolSourceAdapter;
> typedef virStoragePoolSourceAdapter *virStoragePoolSourceAdapterPtr;
> struct _virStoragePoolSourceAdapter {
>     char *name;
>     char *wwpn;
>     char *wwnn;
> };
> 
> Since we might be creating a virtual adapter on a physical adapter that 
> we specify by wwpn/wwnn, we'll need two places to specify wwpn/wwnn in 
> the XML.  I propose:

Hmm, yes, this is what made me think originally that the storage pool
may not be the right place to put the creation/deletion of the vport.

Working through the steps for using NPIV in our APIs...

 - list of HBAs == virNodeListDevices() with  cap='scsi_host'

 - For each HBA
    - Query XML of the  virNodeDevicePtr object
    - Look for 'vport' capability flag

You now have an virNodeDevicePtr object which you  know is able to
create/delete vports, lets say its name is 'host3'.

 - Define config for a new vport, giving <parent> as the name of
   virNodeDevicePtr we just obtained

       <device>
         <parent>host3</parent>
         <capability type='scsi_host'>
            <capability type='fc'>
              <wwpn>88889999aaaabbbb</wwpn>
              <wwnn>4444555566667777</wwnn>
            </capability>
         </capability>
       </device>

 - Create the device with  virNodeDeviceCreate(char *xml);

 - Use the device by defining a SCSI storage pool using the
   <host></host> property of the device we just created

This keeps the notion of hierarchy / parent+child relationship between
HBAs just witin the node device APIs, and avoids replicating it in the
storage APIs.

I still think it would be worth adding a 'wwpn' and 'wwnn' to the
SCSI storage pool XML config - to be used as an alternative to the
'host' parameter, since wwpn+wwnn are guarenteed stable across
machines, whereas 'host' is only unique within scope of a single
machine and even changes across boot.

This does of course mean swe need to add new virNodeDeviceCreate
and virNodeDeviceDestroy  APIs, but I think it is worth doing that
and they can be useful for other areas too. For example, it would 
allow us to have a means to create loopback devices, or create 
NBD devices (via qemu-nbd), which are useful things todo if you
want to get into the innards of file based disks

> <pool type="scsi">
>   <name>npiv</name>
>   <source>
>     <adapter name="host6" wwpn="0000111122223333" wwnn="4444555566667777"/>
>     <vport wwpn="88889999aaaabbbb" wwnn="ccccddddeeeeffff"/>

So, we'd keep the <adapter> line as you show, but would
not need the <vport> line, because the <adater> line would
refer to the wwpn/wwn/host of the vport itself

>   </source>
>   <target>
>     <path>/dev/disk/by-id</path>
>   </target>
> </pool>


Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|