[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 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 :-)


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