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

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



On 02/03/2012 05:55 PM, Ansis Atteka wrote:
This patch allows libvirt to add interfaces to already
existing Open vSwitch bridges.

You guys are moving fast!


  The following syntax in
domain XML file must be used:

     <interface type='bridge'>
       <mac address='52:54:00:d0:3f:f2'/>
       <source bridge='ovsbr'/>
       <virtualport type='openvswitch'>
         <parameters interfaceid='921a80cd-e6de-5a2e-db9c-ab27f15a6e1d'/>
       </virtualport>
       <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
     </interface>

or if libvirt should auto-generate the interfaceid us following syntax:

     <interface type='bridge'>
       <mac address='52:54:00:d0:3f:f2'/>
       <source bridge='ovsbr'/>
       <virtualport type='openvswitch'>
       </virtualport>
       <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
     </interface>

To create Open vSwitch bridge us following command:

     ovs-vsctl add-br ovsbr

1. This patch has been tested only on Ubuntu 11.10 with KVM
2. Auto-generated interfaceid is not persistent across libvirt reboots

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)


---
  configure.ac                       |    5 ++
  src/Makefile.am                    |    2 +
  src/conf/domain_conf.c             |   46 ++++++++++++++
  src/conf/domain_conf.h             |    6 ++
  src/conf/netdev_openvswitch_conf.c |   94 +++++++++++++++++++++++++++++
  src/conf/netdev_openvswitch_conf.h |   39 ++++++++++++
  src/libvirt_private.syms           |   12 ++++
  src/lxc/lxc_driver.c               |   19 ++++--
  src/network/bridge_driver.c        |    3 +-
  src/qemu/qemu_command.c            |    3 +-
  src/qemu/qemu_hotplug.c            |    7 ++-
  src/qemu/qemu_process.c            |    3 +
  src/uml/uml_conf.c                 |    3 +-
  src/util/uuid.c                    |   19 ++++++
  src/util/uuid.h                    |    1 +
  src/util/virnetdevopenvswitch.c    |  114 ++++++++++++++++++++++++++++++++++++
  src/util/virnetdevopenvswitch.h    |   46 ++++++++++++++
  src/util/virnetdevtap.c            |   32 +++++++++-
  src/util/virnetdevtap.h            |    8 ++-
  19 files changed, 447 insertions(+), 15 deletions(-)
  create mode 100644 src/conf/netdev_openvswitch_conf.c
  create mode 100644 src/conf/netdev_openvswitch_conf.h
  create mode 100644 src/util/virnetdevopenvswitch.c
  create mode 100644 src/util/virnetdevopenvswitch.h

diff --git a/configure.ac b/configure.ac
index 9fb7bfc..dca178f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -213,6 +213,8 @@ AC_PATH_PROG([UDEVSETTLE], [udevsettle], [],
  	[/sbin:/usr/sbin:/usr/local/sbin:$PATH])
  AC_PATH_PROG([MODPROBE], [modprobe], [],
  	[/sbin:/usr/sbin:/usr/local/sbin:$PATH])
+AC_PATH_PROG([OVSVSCTL], [ovs-vsctl], [ovs-vsctl],
+	[/sbin:/usr/sbin:/usr/local/sbin:$PATH])


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...


  AC_DEFINE_UNQUOTED([DNSMASQ],["$DNSMASQ"],
          [Location or name of the dnsmasq program])
@@ -220,6 +222,9 @@ AC_DEFINE_UNQUOTED([RADVD],["$RADVD"],
          [Location or name of the radvd program])
  AC_DEFINE_UNQUOTED([TC],["$TC"],
          [Location or name of the tc profram (see iproute2)])
+AC_DEFINE_UNQUOTED([OVSVSCTL],["$OVSVSCTL"],
+        [Location or name of the ovs-vsctl program])
+
  if test -n "$UDEVADM"; then
    AC_DEFINE_UNQUOTED([UDEVADM],["$UDEVADM"],
          [Location or name of the udevadm program])
diff --git a/src/Makefile.am b/src/Makefile.am
index a3dd847..79a2dde 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -95,6 +95,7 @@ UTIL_SOURCES =							\
  		util/virmacaddr.h util/virmacaddr.c		\
  		util/virnetdev.h util/virnetdev.c		\
  		util/virnetdevbandwidth.h util/virnetdevbandwidth.c \
+		util/virnetdevopenvswitch.h util/virnetdevopenvswitch.c \


You should try to keep lists like this in alphabetical order when it appears that's already the case (as it is here).


  		util/virnetdevbridge.h util/virnetdevbridge.c	\
  		util/virnetdevmacvlan.c util/virnetdevmacvlan.h	\
  		util/virnetdevtap.h util/virnetdevtap.c		\
@@ -136,6 +137,7 @@ LOCK_DRIVER_SANLOCK_SOURCES = \

  NETDEV_CONF_SOURCES =						\
  		conf/netdev_bandwidth_conf.h conf/netdev_bandwidth_conf.c \
+		conf/netdev_openvswitch_conf.h conf/netdev_openvswitch_conf.c \


These two files aren't necessary - what they're doing should just be added to netdev_vport_profile_conf.[ch]


  		conf/netdev_vport_profile_conf.h conf/netdev_vport_profile_conf.c

  # XML configuration format handling sources
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index aa4b32d..abec371 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -31,6 +31,7 @@
  #include<sys/time.h>
  #include<strings.h>

+#include "internal.h"
  #include "virterror_internal.h"
  #include "datatypes.h"
  #include "domain_conf.h"
@@ -50,6 +51,7 @@
  #include "count-one-bits.h"
  #include "secret_conf.h"
  #include "netdev_vport_profile_conf.h"
+#include "netdev_openvswitch_conf.h"
  #include "netdev_bandwidth_conf.h"

  #define VIR_FROM_THIS VIR_FROM_DOMAIN
@@ -952,6 +954,7 @@ virDomainActualNetDefFree(virDomainActualNetDefPtr def)
      switch (def->type) {
      case VIR_DOMAIN_NET_TYPE_BRIDGE:
          VIR_FREE(def->data.bridge.brname);
+        VIR_FREE(def->data.bridge.ovsPort);

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.

          break;
      case VIR_DOMAIN_NET_TYPE_DIRECT:
          VIR_FREE(def->data.direct.linkdev);
@@ -995,6 +998,7 @@ void virDomainNetDefFree(virDomainNetDefPtr def)
      case VIR_DOMAIN_NET_TYPE_BRIDGE:
          VIR_FREE(def->data.bridge.brname);
          VIR_FREE(def->data.bridge.ipaddr);
+        VIR_FREE(def->data.bridge.ovsPort);
          break;

      case VIR_DOMAIN_NET_TYPE_INTERNAL:
@@ -3737,7 +3741,15 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
      }

(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).


      if (actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
+        xmlNodePtr virtPortNode = virXPathNode("./virtualport", ctxt);
          actual->data.bridge.brname = virXPathString("string(./source[1]/@bridge)", ctxt);
+        if (virtPortNode&&  virXMLPropString(virtPortNode, "type")&&
+                STREQ(virXMLPropString(virtPortNode, "type"), "openvswitch")) {
+            if (!(actual->data.bridge.ovsPort =
+                  virNetDevOpenvswitchPortParse(virtPortNode))) {
+                goto error;
+            }
+        }

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


      } else if (actual->type == VIR_DOMAIN_NET_TYPE_DIRECT) {
          xmlNodePtr virtPortNode;

@@ -3817,6 +3829,7 @@ virDomainNetDefParseXML(virCapsPtr caps,
      char *linkstate = NULL;
      virNWFilterHashTablePtr filterparams = NULL;
      virNetDevVPortProfilePtr virtPort = NULL;
+    virNetDevOpenvswitchPortPtr ovsPort = NULL;
      virDomainActualNetDefPtr actual = NULL;
      xmlNodePtr oldnode = ctxt->node;
      int ret;
@@ -3870,6 +3883,13 @@ virDomainNetDefParseXML(virCapsPtr caps,
                         xmlStrEqual(cur->name, BAD_CAST "virtualport")) {
                  if (!(virtPort = virNetDevVPortProfileParse(cur)))
                      goto error;
+            } else if ((ovsPort == NULL)&&
+                       ((def->type == VIR_DOMAIN_NET_TYPE_BRIDGE)&&
+                       xmlStrEqual(cur->name, BAD_CAST "virtualport")&&
+                       virXMLPropString(cur, "type")&&
+                       STREQ(virXMLPropString(cur, "type"), "openvswitch"))) {
+                if (!(ovsPort = virNetDevOpenvswitchPortParse(cur)))
+                    goto error;

Same as above.

              } else if ((network == NULL)&&
                         ((def->type == VIR_DOMAIN_NET_TYPE_SERVER) ||
                          (def->type == VIR_DOMAIN_NET_TYPE_CLIENT) ||
@@ -4005,6 +4025,8 @@ virDomainNetDefParseXML(virCapsPtr caps,
              def->data.bridge.ipaddr = address;
              address = NULL;
          }
+        def->data.bridge.ovsPort = ovsPort;
+        ovsPort = NULL;


Instead of ....bridge.ovsPort, you should have ....bridge.virPortProfile, just as is already there for ...direct.virtPortProfile.


          break;

      case VIR_DOMAIN_NET_TYPE_CLIENT:
@@ -10427,6 +10449,12 @@ virDomainActualNetDefFormat(virBufferPtr buf,
      case VIR_DOMAIN_NET_TYPE_BRIDGE:
          virBufferEscapeString(buf, "<source bridge='%s'/>\n",
                                def->data.bridge.brname);
+        if (def->data.bridge.ovsPort) {
+            virBufferAdjustIndent(buf, 6);
+            if (virNetDevOpenvswitchPortFormat(def->data.bridge.ovsPort, buf)<  0)
+                return -1;
+            virBufferAdjustIndent(buf, -6);
+        }


Replace with virNetDevVPortProfileFormat().


          break;

      case VIR_DOMAIN_NET_TYPE_DIRECT:
@@ -10514,6 +10542,12 @@ virDomainNetDefFormat(virBufferPtr buf,
          if (def->data.bridge.ipaddr)
              virBufferAsprintf(buf, "<ip address='%s'/>\n",
                                def->data.bridge.ipaddr);
+        if (def->data.bridge.ovsPort) {
+            virBufferAdjustIndent(buf, 6);
+            if (virNetDevOpenvswitchPortFormat(def->data.bridge.ovsPort, buf)<  0)
+                return -1;
+            virBufferAdjustIndent(buf, -6);
+        }

Same

          break;

      case VIR_DOMAIN_NET_TYPE_SERVER:
@@ -13851,6 +13885,18 @@ virDomainNetGetActualBandwidth(virDomainNetDefPtr iface)
      return iface->bandwidth;
  }

+virNetDevOpenvswitchPortPtr
+virDomainNetGetActualOpenvswitchPortPtr(virDomainNetDefPtr iface)
+{
+    if (iface->type != VIR_DOMAIN_NET_TYPE_BRIDGE)
+        return NULL;
+    if (iface->data.bridge.ovsPort)
+        return iface->data.bridge.ovsPort;
+    if (!iface->data.network.actual)
+        return NULL;
+    return iface->data.network.actual->data.bridge.ovsPort;
+}
+

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.


  /* Return listens[ii] from the appropriate union for the graphics
   * type, or NULL if this is an unsuitable type, or the index is out of
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 0a2795d..b7eb7d1 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -41,6 +41,7 @@
  # include "virnetdevmacvlan.h"
  # include "sysinfo.h"
  # include "virnetdevvportprofile.h"
+# include "virnetdevopenvswitch.h"
  # include "virnetdevbandwidth.h"

  /* Different types of hypervisor */
@@ -594,6 +595,7 @@ struct _virDomainActualNetDef {
      union {
          struct {
              char *brname;
+            virNetDevOpenvswitchPortPtr ovsPort;

replace with

            virNetDevVPortProfilePtr virtPortProfile;

          } bridge;
          struct {
              char *linkdev;
@@ -645,6 +647,7 @@ struct _virDomainNetDef {
          struct {
              char *brname;
              char *ipaddr;
+            virNetDevOpenvswitchPortPtr ovsPort;

Same.

          } bridge;
          struct {
              char *name;
@@ -1877,6 +1880,9 @@ virDomainNetGetActualDirectVirtPortProfile(virDomainNetDefPtr iface);
  virNetDevBandwidthPtr
  virDomainNetGetActualBandwidth(virDomainNetDefPtr iface);

+virNetDevOpenvswitchPortPtr
+virDomainNetGetActualOpenvswitchPortPtr(virDomainNetDefPtr iface);
+

Unnecessary.


  int virDomainControllerInsert(virDomainDefPtr def,
                                virDomainControllerDefPtr controller);
  void virDomainControllerInsertPreAlloced(virDomainDefPtr def,
diff --git a/src/conf/netdev_openvswitch_conf.c b/src/conf/netdev_openvswitch_conf.c
new file mode 100644


Again, if you appropriately reuse netdev_vport_profile_conf.[ch] as outlined above, you will no longer need to add these two new files.


index 0000000..ddae3aa
--- /dev/null
+++ b/src/conf/netdev_openvswitch_conf.c
@@ -0,0 +1,94 @@
+/*
+ * Copyright (C) 2012 Nicira, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307  USA
+ *
+ * Authors:
+ *     Dan Wendlandt<dan nicira com>
+ *     Kyle Mestery<kmestery cisco com>
+ *     Ansis Atteka<aatteka nicira com>
+ */
+
+#include<config.h>
+
+#include "netdev_openvswitch_conf.h"
+#include "virterror_internal.h"
+#include "memory.h"
+#include "uuid.h"
+
+#define VIR_FROM_THIS VIR_FROM_NONE
+#define virNetDevError(code, ...)                                       \
+    virReportErrorHelper(VIR_FROM_THIS, code, __FILE__,                 \
+                         __FUNCTION__, __LINE__, __VA_ARGS__)
+
+
+virNetDevOpenvswitchPortPtr
+virNetDevOpenvswitchPortParse(xmlNodePtr node)
+{
+    char *InterfaceID = NULL;
+    virNetDevOpenvswitchPortPtr ovsPort = NULL;
+    xmlNodePtr cur = node->children;
+
+    if (VIR_ALLOC(ovsPort)<  0) {
+        virReportOOMError();
+        goto error;
+    }
+
+    while (cur != NULL) {
+        if (xmlStrEqual(cur->name, BAD_CAST "parameters")) {
+            InterfaceID = virXMLPropString(cur, "interfaceid");
+            break;
+        }
+        cur = cur->next;
+    }
+
+    if (InterfaceID == NULL || (strlen(InterfaceID) == 0)) {
+        // interfaceID does not have to be a UUID,
+        // but a UUID is a reasonable default
+        if (virUUIDGenerateStr(ovsPort->InterfaceID)) {
+            virNetDevError(VIR_ERR_XML_ERROR, "%s",
+                        _("cannot generate a random uuid for interfaceid"));
+            goto error;
+        }
+    } else {
+        if (virStrcpyStatic(ovsPort->InterfaceID, InterfaceID) == NULL) {
+            virNetDevError(VIR_ERR_XML_ERROR, "%s",
+                           _("InterfaceID parameter too long"));
+            goto error;
+        }
+    }
+
+cleanup:
+    return ovsPort;
+
+error:
+    VIR_FREE(ovsPort);
+    goto cleanup;
+}
+
+
+int
+virNetDevOpenvswitchPortFormat(virNetDevOpenvswitchPortPtr ovsPort,
+                            virBufferPtr buf)
+{
+    if (ovsPort == NULL)
+        return 0;
+
+    virBufferAsprintf(buf, "<virtualport type='openvswitch'>\n");
+    virBufferAsprintf(buf, "<parameters interfaceid='%s'/>\n",
+                      ovsPort->InterfaceID);
+    virBufferAddLit(buf, "</virtualport>\n");
+    return 0;
+}
diff --git a/src/conf/netdev_openvswitch_conf.h b/src/conf/netdev_openvswitch_conf.h
new file mode 100644

Same as previous file

index 0000000..fd8e079
--- /dev/null
+++ b/src/conf/netdev_openvswitch_conf.h
@@ -0,0 +1,39 @@
+/*
+ * Copyright (C) 2012 Nicira, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307  USA
+ *
+ * Authors:
+ *     Dan Wendlandt<dan nicira com>
+ *     Kyle Mestery<kmestery cisco com>
+ *     Ansis Atteka<aatteka nicira com>
+ */
+
+#ifndef __VIR_NETDEV_OPENVSWITCH_CONF_H__
+# define __VIR_NETDEV_OPENVSWITCH_CONF_H__
+
+# include "internal.h"
+# include "virnetdevopenvswitch.h"
+# include "buf.h"
+# include "xml.h"
+
+virNetDevOpenvswitchPortPtr
+virNetDevOpenvswitchPortParse(xmlNodePtr node);
+
+int
+virNetDevOpenvswitchPortFormat(virNetDevOpenvswitchPortPtr ovsPort,
+                               virBufferPtr buf);
+
+#endif /* __VIR_NETDEV_OPENVSWITCH_CONF_H__ */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d6ad36c..0666d5c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -379,6 +379,7 @@ virDomainNetGetActualBridgeName;
  virDomainNetGetActualDirectDev;
  virDomainNetGetActualDirectMode;
  virDomainNetGetActualDirectVirtPortProfile;
+virDomainNetGetActualOpenvswitchPortPtr;
  virDomainNetGetActualType;
  virDomainNetIndexByMac;
  virDomainNetInsert;
@@ -738,6 +739,10 @@ virShrinkN;
  virNetDevBandwidthFormat;
  virNetDevBandwidthParse;

+# netdev_openvswitch_conf.h
+virNetDevOpenvswitchPortFormat;
+virNetDevOpenvswitchPortParse;
+

  # netdev_vportprofile_conf.h
  virNetDevVPortProfileFormat;
@@ -1145,6 +1150,7 @@ virGetHostUUID;
  virSetHostUUIDStr;
  virUUIDFormat;
  virUUIDGenerate;
+virUUIDGenerateStr;

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


  virUUIDParse;


@@ -1237,10 +1243,16 @@ virNetDevMacVLanCreateWithVPortProfile;
  virNetDevMacVLanDeleteWithVPortProfile;


+# virnetdevopenvswitch.h
+virNetDevOpenvswitchAddPort;
+virNetDevOpenvswitchDelPort;

To match the naming for linux host bridges, this function should be called virNetDevOpenvswitchRemovePort().


+
+
  # virnetdevtap.h
  virNetDevTapCreate;
  virNetDevTapCreateInBridgePort;
  virNetDevTapDelete;
+virNetDevTapDeleteInBridgePort;

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.




  # virnetdevveth.h
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index b712da4..1fab77a 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c

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;

@@ -56,6 +56,7 @@
  #include "domain_nwfilter.h"
  #include "network/bridge_driver.h"
  #include "virnetdev.h"
+#include "virnetdevtap.h"
  #include "virnodesuspend.h"
  #include "virtime.h"
  #include "virtypedparam.h"
@@ -1132,10 +1133,12 @@ static void lxcVmCleanup(lxc_driver_t *driver,
      priv->monitorWatch = -1;

      for (i = 0 ; i<  vm->def->nnets ; i++) {
-        ignore_value(virNetDevSetOnline(vm->def->nets[i]->ifname, false));
-        ignore_value(virNetDevVethDelete(vm->def->nets[i]->ifname));
-
-        networkReleaseActualDevice(vm->def->nets[i]);
+        virDomainNetDefPtr iface = vm->def->nets[i];
+        ignore_value(virNetDevSetOnline(iface->ifname, false));
+        ignore_value(virNetDevVethDelete(iface->ifname));
+        ignore_value(virNetDevTapDeleteInBridgePort(iface->ifname,
+                       virDomainNetGetActualOpenvswitchPortPtr(iface)));

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

+        networkReleaseActualDevice(iface);
      }

      virDomainConfVMNWFilterTeardown(vm);
@@ -1377,8 +1380,12 @@ static int lxcSetupInterfaces(virConnectPtr conn,

  cleanup:
      if (ret != 0) {
-        for (i = 0 ; i<  def->nnets ; i++)
-            networkReleaseActualDevice(def->nets[i]);
+        for (i = 0 ; i<  def->nnets ; i++) {
+            virDomainNetDefPtr iface = def->nets[i];
+            ignore_value(virNetDevTapDeleteInBridgePort(iface->ifname,
+                           virDomainNetGetActualOpenvswitchPortPtr(iface)));

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


+            networkReleaseActualDevice(iface);
+        }
      }
      return ret;
  }
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 57ebb9f..1423d3f 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1765,7 +1765,8 @@ networkStartNetworkVirtual(struct network_driver *driver,
              goto err0;
          }
          if (virNetDevTapCreateInBridgePort(network->def->bridge,
-&macTapIfName, network->def->mac, 0, false, NULL)<  0) {
+&macTapIfName, network->def->mac, 0,
+                           false, NULL, NULL)<  0) {
              VIR_FREE(macTapIfName);
              goto err0;
          }
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0e26df1..d6a81f1 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -247,7 +247,8 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
      memcpy(tapmac, net->mac, VIR_MAC_BUFLEN);
      tapmac[0] = 0xFE; /* Discourage bridge from using TAP dev MAC */

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...


      err = virNetDevTapCreateInBridgePort(brname,&net->ifname, tapmac,
-                                         vnet_hdr, true,&tapfd);
+                             vnet_hdr, true,&tapfd,
+                             virDomainNetGetActualOpenvswitchPortPtr(net));

virDomainNetGetActualVirtPortProfile(net)

      virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd>= 0);
      if (err<  0) {
          if (template_ifname)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index b4870be..85a004d 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -40,6 +40,7 @@
  #include "qemu_cgroup.h"
  #include "locking/domain_lock.h"
  #include "network/bridge_driver.h"
+#include "virnetdevtap.h"

  #define VIR_FROM_THIS VIR_FROM_QEMU

@@ -837,7 +838,8 @@ cleanup:

          if (iface_connected)
              virDomainConfNWFilterTeardown(net);
-
+        ignore_value(virNetDevTapDeleteInBridgePort(net->ifname,
+                       virDomainNetGetActualOpenvswitchPortPtr(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).


          networkReleaseActualDevice(net);
      }

@@ -1937,7 +1939,8 @@ int qemuDomainDetachNetDevice(struct qemud_driver *driver,
                                   detach->ifname);
          }
      }
-
+    ignore_value(virNetDevTapDeleteInBridgePort(detach->ifname,
+                   virDomainNetGetActualOpenvswitchPortPtr(detach)));


Same as above.


      networkReleaseActualDevice(detach);
      if (vm->def->nnets>  1) {
          memmove(vm->def->nets + i,
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 2d92d66..14eeef8 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -62,6 +62,7 @@
  #include "network/bridge_driver.h"
  #include "uuid.h"
  #include "virtime.h"
+#include "virnetdevtap.h"

  #define VIR_FROM_THIS VIR_FROM_QEMU

@@ -3731,6 +3732,8 @@ void qemuProcessStop(struct qemud_driver *driver,
          /* release the physical device (or any other resources used by
           * this interface in the network driver
           */
+        ignore_value(virNetDevTapDeleteInBridgePort(net->ifname,
+                       virDomainNetGetActualOpenvswitchPortPtr(net)));

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).
          networkReleaseActualDevice(net);
      }

diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c
index 316d558..b8610ea 100644
--- a/src/uml/uml_conf.c
+++ b/src/uml/uml_conf.c
@@ -142,7 +142,8 @@ umlConnectTapDevice(virConnectPtr conn,
      memcpy(tapmac, net->mac, VIR_MAC_BUFLEN);
      tapmac[0] = 0xFE; /* Discourage bridge from using TAP dev MAC */
      if (virNetDevTapCreateInBridgePort(bridge,&net->ifname, tapmac,
-                                       0, true, NULL)<  0) {
+                       0, true, NULL,
+                       virDomainNetGetActualOpenvswitchPortPtr(net))<  0) {

virDomainNetGetActualVirtPortProfile(net)

          if (template_ifname)
              VIR_FREE(net->ifname);
          goto error;
diff --git a/src/util/uuid.c b/src/util/uuid.c
index 23822ec..d8188b7 100644
--- a/src/util/uuid.c
+++ b/src/util/uuid.c
@@ -116,6 +116,25 @@ virUUIDGenerate(unsigned char *uuid)
  }

  /**
+ * virUUIDGenerateString:


Again, I don't think this function is necessary - just store the UUID in binary, and call format as necessary when generating the commandline.


+ * @uuid: string of VIR_UUID_STRING_BUFLEN characters to store the UUID
+ *
+ *
+ * Generates a randomized unique identifier
+ * Returns 0 in case of success and -1 in case of failure
+ */
+int
+virUUIDGenerateStr(char *uuidstr)
+{
+    unsigned char uuid[VIR_UUID_BUFLEN];
+
+    if (virUUIDGenerate(uuid)<  0)
+        return -1;
+    virUUIDFormat(uuid, uuidstr);
+    return 0;
+}
+
+/**
   * virUUIDParse:
   * @uuidstr: zero terminated string representation of the UUID
   * @uuid: array of VIR_UUID_BUFLEN bytes to store the raw UUID
diff --git a/src/util/uuid.h b/src/util/uuid.h
index 7dbfad5..1ae7efb 100644
--- a/src/util/uuid.h
+++ b/src/util/uuid.h
@@ -30,6 +30,7 @@ int virGetHostUUID(unsigned char *host_uuid) ATTRIBUTE_NONNULL(1);
  int virUUIDIsValid(unsigned char *uuid);

  int virUUIDGenerate(unsigned char *uuid);
+int virUUIDGenerateStr(char *struuid);

  int virUUIDParse(const char *uuidstr,
                   unsigned char *uuid)
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
new file mode 100644
index 0000000..c5bef8d
--- /dev/null
+++ b/src/util/virnetdevopenvswitch.c
@@ -0,0 +1,114 @@
+/*
+ * Copyright (C) 2012 Nicira, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307  USA
+ *
+ * Authors:
+ *     Dan Wendlandt<dan nicira com>
+ *     Kyle Mestery<kmestery cisco com>
+ *     Ansis Atteka<aatteka nicira com>
+ */
+
+#include<config.h>
+
+#include "virnetdevopenvswitch.h"
+#include "command.h"
+#include "memory.h"
+#include "virterror_internal.h"
+#include "ignore-value.h"
+#include "virmacaddr.h"
+
+#define VIR_FROM_THIS VIR_FROM_NONE
+
+/**
+ * virNetDevOpenvswitchAddPort:
+ * @brname: the bridge name
+ * @ifname: the network interface name
+ * @macaddr: the mac address of the virtual interface
+ *
+ * Add an interface to the OVS bridge
+ *
+ * Returns 0 in case of success or -1 in case of failure.
+ */
+int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
+                                   const unsigned char *macaddr,
+                                   virNetDevOpenvswitchPortPtr ovsport)
virNetDevVPortProfilePtr vport)


{
+    int ret = -1;
+    virCommandPtr cmd = NULL;
+    char macaddrstr[VIR_MAC_STRING_BUFLEN];
+    char *attachedmac_ex_id = NULL;
+    char *ifaceid_ex_id = NULL;
+
+    virMacAddrFormat(macaddr, macaddrstr);
+
+    if (virAsprintf(&attachedmac_ex_id, "external-ids:attached-mac=\"%s\"",
+                    macaddrstr)<  0)
+        goto cleanup;
+    if (virAsprintf(&ifaceid_ex_id, "external-ids:iface-id=\"%s\"",
+                    ovsport->InterfaceID)<  0)
+        goto cleanup;
+
+    cmd = virCommandNew(OVSVSCTL);
+    virCommandAddArgList(cmd, "--", "--may-exist", "add-port",
+                        brname, ifname,
+                        "--", "set", "Interface", ifname, attachedmac_ex_id,
+                        "--", "set", "Interface", ifname, ifaceid_ex_id,
+                        "--", "set", "Interface", ifname,
+                        "external-ids:iface-status=active",
+                        NULL);
+
+    if (virCommandRun(cmd, NULL)<  0) {
+        virReportSystemError(VIR_ERR_INTERNAL_ERROR,
+                             _("Unable to add port %s to OVS bridge %s"),
+                             ifname, brname);
+        goto cleanup;
+    }
+    ret = 0;
+
+    cleanup:
+        VIR_FREE(attachedmac_ex_id);
+        VIR_FREE(ifaceid_ex_id);
+        virCommandFree(cmd);
+        return ret;
+}
+
+/**
+ * virNetDevOpenvswitchDelPort:
+ * @ifname: the network interface name
+ *
+ * Deletes an interface from a OVS bridge
+ *
+ * Returns 0 in case of success or -1 in case of failure.
+ */
+int virNetDevOpenvswitchDelPort(const char *ifname)

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.


+{
+    int ret = -1;
+    virCommandPtr cmd = NULL;
+
+    cmd = virCommandNew(OVSVSCTL);
+    virCommandAddArgList(cmd, "--", "--if-exists", "del-port", ifname, NULL);
+
+    if (virCommandRun(cmd, NULL)<  0) {
+        virReportSystemError(VIR_ERR_INTERNAL_ERROR,
+                             _("Unable to delete port %s from OVS"), ifname);
+        goto cleanup;
+    }
+    ret = 0;
+
+    cleanup:
+        virCommandFree(cmd);
+        return ret;
+}
diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h
new file mode 100644
index 0000000..210546b
--- /dev/null
+++ b/src/util/virnetdevopenvswitch.h
@@ -0,0 +1,46 @@
+/*
+ * Copyright (C) 2012 Nicira, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307  USA
+ *
+ * Authors:
+ *     Dan Wendlandt<dan nicira com>
+ *     Kyle Mestery<kmestery cisco com>
+ *     Ansis Atteka<aatteka nicira com>
+ */
+
+#ifndef __VIR_NETDEV_OPENVSWITCH_H__
+# define __VIR_NETDEV_OPENVSWITCH_H__
+
+# include "internal.h"
+# include "util.h"
+
+typedef struct _virNetDevOpenvswitchPort virNetDevOpenvswitchPort;
+typedef virNetDevOpenvswitchPort *virNetDevOpenvswitchPortPtr;
+struct _virNetDevOpenvswitchPort {
+    char InterfaceID[VIR_UUID_STRING_BUFLEN];
+};
+
+int virNetDevOpenvswitchAddPort(const char *brname,
+                                const char *ifname,
+                                const unsigned char *macaddr,
+                                virNetDevOpenvswitchPortPtr ovsport)
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+    ATTRIBUTE_RETURN_CHECK;
+
+int virNetDevOpenvswitchDelPort(const char *ifname)
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
+
+#endif /* __VIR_NETDEV_OPENVSWITCH_H__ */
diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
index 2ed53c6..975bb85 100644
--- a/src/util/virnetdevtap.c
+++ b/src/util/virnetdevtap.c
@@ -25,6 +25,7 @@
  #include "virnetdevtap.h"
  #include "virnetdev.h"
  #include "virnetdevbridge.h"
+#include "virnetdevopenvswitch.h"
  #include "virterror_internal.h"
  #include "virfile.h"
  #include "virterror_internal.h"
@@ -249,6 +250,7 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED)
   * @macaddr: desired MAC address (VIR_MAC_BUFLEN long)
   * @vnet_hdr: whether to try enabling IFF_VNET_HDR
   * @tapfd: file descriptor return value for the new tap device
+ * @ovsport: Open vSwitch specific configuration
   *
   * This function creates a new tap device on a bridge. @ifname can be either
   * a fixed name or a name template with '%d' for dynamic name allocation.
@@ -265,7 +267,8 @@ int virNetDevTapCreateInBridgePort(const char *brname,
                                     const unsigned char *macaddr,
                                     int vnet_hdr,
                                     bool up,
-                                   int *tapfd)
+                                   int *tapfd,
+                                   virNetDevOpenvswitchPortPtr ovsport)

virNetDevVPortProfilePtr vport)


  {
      if (virNetDevTapCreate(ifname, vnet_hdr, tapfd)<  0)
          return -1;
@@ -286,8 +289,13 @@ int virNetDevTapCreateInBridgePort(const char *brname,
      if (virNetDevSetMTUFromDevice(*ifname, brname)<  0)
          goto error;

Does the above function succeed when brname contains the name of an ovs device?


-    if (virNetDevBridgeAddPort(brname, *ifname)<  0)
-        goto error;
+    if (ovsport) {
+        if (virNetDevOpenvswitchAddPort(brname, *ifname, macaddr, ovsport)<  0)
+            goto error;
+    } else {
+        if (virNetDevBridgeAddPort(brname, *ifname)<  0)
+            goto error;
+    }

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



      if (virNetDevSetOnline(*ifname, up)<  0)
          goto error;
@@ -299,3 +307,21 @@ int virNetDevTapCreateInBridgePort(const char *brname,

      return errno;
  }
+
+/**
+ * virNetDevTapDeleteInBridgePort:
+ * @ifname: the interface name (or name template)
+ * @ovsport: Open vSwitch specific configuration
+ *
+ * This function detaches tap device from a bridge.
+ *
+ * Returns 0 in case of success or -1 on failure
+ */
+int virNetDevTapDeleteInBridgePort(char *ifname,
+                                   virNetDevOpenvswitchPortPtr ovsport)
+{
+    int ret = 0;
+    if (ovsport)
+        ret = virNetDevOpenvswitchDelPort(ifname);
+    return ret;
+}

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.


diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h
index fb35ac5..5b16570 100644
--- a/src/util/virnetdevtap.h
+++ b/src/util/virnetdevtap.h
@@ -24,6 +24,7 @@
  # define __VIR_NETDEV_TAP_H__

  # include "internal.h"
+# include "conf/domain_conf.h"

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.



  int virNetDevTapCreate(char **ifname,
                         int vnet_hdr,
@@ -38,8 +39,13 @@ int virNetDevTapCreateInBridgePort(const char *brname,
                                     const unsigned char *macaddr,
                                     int vnet_hdr,
                                     bool up,
-                                   int *tapfd)
+                                   int *tapfd,
+                                   virNetDevOpenvswitchPortPtr ovsport)
      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
      ATTRIBUTE_RETURN_CHECK;

+int virNetDevTapDeleteInBridgePort(char *ifname,
+                                   virNetDevOpenvswitchPortPtr ovsport)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
+
  #endif /* __VIR_NETDEV_TAP_H__ */


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