[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2] RFC Libvirt + Openvswitch



On 02/14/2012 09:26 PM, Laine Stump wrote:
> On 02/14/2012 05:33 PM, Eric Blake wrote:
>> 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.
> I'll fix up the few nits and push it either later tonight or in the
> morning, then :-)

Okay, I fixed all the whitespace nits, put the function rename into a
separate patch, added virnetdevopenvswitch.c to po/POTFILES.in, and
added all three contributors to AUTHORS, then pushed the result (after
successful make check && make syntax-check).

Welcome to libvirt, openvswitch! (and thanks for the contribution,
Ansis, Dan, and Kyle!)


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]