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

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.