[libvirt] [PATCH 1/1] Implement NPIV

[Dave Allan]

Daniel P. Berrange wrote:
> 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.

The above generally sounds like a good idea to me.  I am in the process 
of working out the specifics, but it's a much cleaner fit than trying to 
make this functionality go into pools.

> 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.

That sounds good; a user can create the adapter with the device API and 
then be able to find the associated pool easily via wwpn/wwnn.

> 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

Agreed.

>> <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

That works for me.

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

Dave