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.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)