[libvirt] [PATCH v2] RFC Libvirt + Openvswitch

[Eric Blake]

On 02/13/2012 10:56 PM, Laine Stump wrote:

> But because an interfaceid is auto-generated by the parser when one
> isn't given, and a common parser function for virtualport is used by the
> domain and network parsers, this will result in an interfaceid being
> filled in when the network is defined, which makes no sense for a
> <network> definition, since each interface that uses this network is
> supposed to have its own unique interfaceid. However, the code decides
> that the bridge is an openvswitch by looking for the presence of a
> virtualport of type='openvswitch'.
> 
> I can see a couple ways out of this:
> 
> 1) we could add an arg to virNetDevVPortProfileParse:
> 
> virNetDevVPortProfilePtr
> virNetDevVPortProfileParse(xmlNodePtr node, unsigned int flags);
> 
> with a possible flag being VIR_NETDEV_VPORT_AUTOGEN
> 
> If that flag is given, interfaceid and instanceid will be auto-generated
> if necessary during parse, otherwise they will be left blank.

That actually sounds reasonable to me.  We've been bit more than once by
autogenerating stuff too eagerly (most recently, Osier's patches for WWN
auto-generation had the issue that part of the WWN is the OUI which
should be hypervisor-specific, but the parser is supposed to be
hypervisor-agnostic; other examples are that detach-device of a partial
XML <interface> snippet without a MAC address would autogenerate a new
MAC address, then fail to detach the actual intended interface).

> 
> 2) Rather than deciding which type of bridge we're dealing with by
> looking at the config, we could try to do it by direct examination of
> the system.
...
> 
> So, I'm wondering if there's a simple way to confirm that a device
> *isn't* an openvswitch without relying on calling any utilities that are
> part of the ovs package.

I'm not sure how easy or hard this would be.  At any rate,

> 
> I'm thinking it may be okay to push this code more or less as-is though,
> since it works properly for the case of straight <interface
> type='bridge'/>, and the changes I'm talking about are refinements that
> are backward compatible, and are only necessary when we add support for
> <network>s that use openvswitch. Any other libvirt regulars have an
> opinion on this?

in answer to this question, I think that you are correct - anything we
do to further expand the code (whether option 1 of adding a flag to
control whether auto-generation happens, or option 2 of probing the
system device, or some other option 3 or even combination) can deal with
the added complexity at that time.

>> @@ -13832,15 +13854,27 @@ virDomainNetGetActualDirectMode(virDomainNetDefPtr iface)
>>  }
>>  
>>  virNetDevVPortProfilePtr
>> -virDomainNetGetActualDirectVirtPortProfile(virDomainNetDefPtr iface)
>> +virDomainNetGetActualVirtPortProfile(virDomainNetDefPtr iface)
> 
> 
> In hindsight, renaming this function would have been better done in a
> separate pre-requisite patch, just to avoid clutter in this patch.

Yes, I highly agree that separating renames, code motion, and actual new
code into separate patches almost always makes life easier, both in the
initial reviews, and in later efforts to backport patches to older
stable releases.

> I didn't see any major problems in this version, so I'm inclined to ACK
> it conditioned on the nits being fixed (they're simple enough I could
> just take care of them while pushing it). The one thing I would like to
> get other opinions of before pushing is problem with auto-generating the
> interfaceid (just in case there's a better way to fix it that would
> required making a change now).

I think we're safe pushing now.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120214/096d78c6/attachment-0001.sig>