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