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

On 02/03/2012 05:55 PM, Ansis Atteka wrote: You guys are moving fast! Although I don't necessarily agree with the idea of auto-generated data being added by the parser, there are many prerequisites for this (for example the mac address in network devices and instanceID in virtualportprofiles) - at the end of the parsing function, just look to see if the field in question has been specified, and if not, generate a random value and fill it in before returning from the parser. (as a matter of fact, I point out below that you should be using the existing virtualport parser/formatter, so you can directly lift the code to parse/auto-generate interfaceID from the code that does that for instanceID already). The only problem I can foresee with doing it this way is that it then will cause difficulties when we want to define a <network> that uses openvswitch - following the xml syntax we've come up with for interfaces, that would be indicated by having <forward mode='bridge'> and <virtualport type='openvswitch'/>. The problem is that if we have the parser function auto-generate an interfaceID whenever there is a virtualport with type='openvswitch', the network's copy will have an interfaceID, and that same ID would be returned to all guests that used that network (which breaks the requirement that it be unique for every guest). Maybe somebody who is better at the persistent vs. transient stuff can tell you a nice way to do this during the interface attach instead (you can get a pointer to the persistent config using virDomainLiveConfiHelperMethod(), but that is only called directly by public APIs, so I don't know if it's kosher/necessary to call it for this particular purpose) I haven't looked at what happens if the requested binary doesn't exist on the build system. Does it just set it to tyr "ovs-vsctl" with no path? At least initially, people might be building this on systems which don't have openvswitch installed, but will still want to enable support (e.g., the official Fedora builds prior to openvswitch being available as an official Fedora package). This will allow Fedora to ship a binary that supports openvswitch without needing a rebuild of libvirt, in the case that the user takes the necessary extra steps to install it. Maybe it already does the right thing... You should try to keep lists like this in alphabetical order when it appears that's already the case (as it is here). These two files aren't necessary - what they're doing should just be added to netdev_vport_profile_conf.[ch] Since this is the first place in the code that brings up the "ovsPort", I'll comment about it here. The idea behind re-using the <virtualport> element was 1) to keep down the number of different elements in the XML that the admin must learn about, but also 2) to keep down the volume of source code needed to deal with these elements. Rather than adding a new type of data (and a new parsing function with associated .h and .c files), you should instead just 1) add a "struct { } openvswitch;" to the definition of virNetDevVportProfile (in src/util/virnetdevvportprofile.h. yes, I agree that data is defined in a too-specific location - I think it should be in something more general (or that the other functions there that are specific to 802.1QbX should be defined somewhere else) 2) augment the intelligence of virNetDevVPortProfileParse and virNetDevVPortProfileFormat to populate and output that type of virtualport 3) instead of "ovsPort, include a virNetDevVportProfile in the virDomainNetDef for the case of bridge (and of course, augment the parser/formatter for NetDef to call virNetDevVPortProfile(Parse|Format) at the appropriate time). Once you do this, your new netdev_openvswitch_conf.[ch] files are unnecessary. While you're doing this, I think (again for consistency's sake) you should make interfaceID be a fixed-length uuid in binary format rather than a string (just like instanceID), and call virUUIDFormat() at the appropriate places to turn it into a string when you're going to include it in a commandline. (It's nice that you're filling in support for the ActualNetDef now, even though it isn't yet used - that will make it much easier to add support for <network>s using openvswitch). Again, the code above should be replaced with a call for virNetDevVPortProfileParse(). Just copy the code from the case of actual->type == VIR_DOMAIN_NET_TYPE_DIRECT (but of course use actual->data.bridge.virtPortProfile, instead of actual->data.direct.virtPortProfile). Same as above. Instead of ....bridge.ovsPort, you should have ....bridge.virPortProfile, just as is already there for ...direct.virtPortProfile. Replace with virNetDevVPortProfileFormat(). Same This function can just be removed, and the function virDomainNetGetActualDirectVirtPortProfile: 1) renamed to the more general virDomainNetGetActualVirtPortProfile 2) modified to return the virtportprofile of the bridge if appropriate: virNetDevVPortProfilePtr virDomainNetGetActualVirtPortProfile(virDomainNetDefPtr iface) { switch (iface->type) { case VIR_DOMAIN_NET_TYPE_DIRECT: return iface->data.direct.virtPortProfile; case VIR_DOMAIN_NET_TYPE_BRIDGE: return iface->data.bridge.virtPortProfile; case VIR_DOMAIN_NET_TYPE_NETWORK: if (!iface->data.network.actual) return NULL; switch (iface->data.network.actual->type) { case VIR_DOMAIN_NET_TYPE_DIRECT: return iface->data.network.actual->data.direct.virtPortProfile; case VIR_DOMAIN_NET_TYPE_BRIDGE: return iface->data.network.actual->data.bridge.virtPortProfile; default: return NULL; } default: return NULL; } } Then use that function instead. replace with virNetDevVPortProfilePtr virtPortProfile; Same. Unnecessary. Again, if you appropriately reuse netdev_vport_profile_conf.[ch] as outlined above, you will no longer need to add these two new files. Same as previous file Is there a reason you're allocating this as a string rather than following the existing convention, and just converting it to a string when you need to use it? That would make it identical to instanceID in the 8021Qbg case of the virtualPortProfile (you could actually create the virportprofile openvswitch case for parse and format by just copying the 8021Qbg case and removing the extra fields). To match the naming for linux host bridges, this function should be called virNetDevOpenvswitchRemovePort(). Based on what's happening in this function (not much), I've changed my mind about it, and think that you should instead just be directly calling virNetDevOpenvswitchRemovePort() when appropriate. I notice that you didn't do anything in lxcSetupInterfaceBridged(). It doesn't call virNetDevTapCreateInBridgePort because it doesn't use tap devices. Instead, it calls virNetDevBridgeAddPort(). I guess to be consistent, you should modify that call to instead be: virNetDevVPortProfilePtr vport = virDomainNetGetActualVirtPortProfile(net); if (vport && vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) ret = virNetDevOpenvswitchAddPort(brname, parentVeth, vport); else ret = virNetDevBridgeAddPort(brname, parentVeth); if (ret < 0) goto cleanup; Again, since lxc doesn't use tap devices, it couldn't use this higher-level function (even if we decide to keep it). Instead, you'll want to do: virNetDevVPortProfilePtr vport = virDomainNetGetActualVirtPortProfile(iface); if (vport && vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) ignore_value(virNetDevOpenvswitchRemovePort(brname, parentVeth)); (It almost (but not quite yet) seems like it's worth a helper function virDomainNetGetActualVirtPortProfileType()) Also, although you say it's safe to remove the interface from the switch after deleting it, I would feel better if you moved virNetDevVethDelete to be *after* this code :-) Again, you should call virNetDevOpenvswitchRemovePort as in the example above. (As a matter of fact, this cleanup code looks incomplete to me - I'm not really familiar with the lxc driver, but I think it should also be virNetDevVethDelete too (that's a pre-existing problem, though, and it should be fixed in a separate patch). BTW, you notice that we give the tap device a different MAC address than QEMU is giving to the guest, right? I just want to make sure that openvswitch doesn't assume that all traffic coming from this port has the MAC address of the tap device... virDomainNetGetActualVirtPortProfile(net) Again, since the virNetDevTapDeleteInBridgePort function ends up not actually doing anything with a tap device, I think this should be replaced with a direct call to virNetDevOpenvswitchRemovePort (with appropriate checks). Same as above. Same as above. (hopefully all this repetition is going to make it obvious how best to group together all the net device setup/teardown functions into general-purpose helpers so that future additions/changes can be in just a couple places instead of all over). virDomainNetGetActualVirtPortProfile(net) Again, I don't think this function is necessary - just store the UUID in binary, and call format as necessary when generating the commandline. virNetDevVPortProfilePtr vport) For consistency with the corresponding host bridge function, this should be int virNetDevBridgeRemovePort(const char *brname ATTRIBUTE_UNUSED, const char *ifname) (even though brname is apparently unused) - someday we may want to have this function called from a function pointer for some reason. virNetDevVPortProfilePtr vport) Does the above function succeed when brname contains the name of an ovs device? I was on the border about putting this in here, since it's bringing details about openvswitch into what should be a low level function that doesn't know about much. But on the other hand, this function is by definition like that, since it's mixing bridge and tap functionality, so I still think it's okay (others may disagree with me though). Again, just due to the fact that this doesn't end up actually doing anything to the tap device, I changed my mind and now think it should be eliminated in favor of directly calling virNetDevOpenvswitchRemovePort() directly, when appropriate. I think I recall being told that, while it's okay for something in the conf directory to include/use something in the util directory, the reverse is not true. Fortunately, this will be a non-issue if you're using virNetDevVPortProfilePtr instead, since it's defined in util/virnetdevvportprofile.h.