vswitchd: Add error column to Interface table to store error condition

Store the error condition of a failed port configuration in a new
column 'error' in the Interface table.

Example:
$ ovs-vsctl add-port br0 test -- \
     set Interface test type=vxlan options:unknown=1
ovs-vsctl: Error detected while setting up 'test'.  [...]

$ ovs-vsctl list Interface test | grep error
error         : "test: could not set configuration (Invalid argument)"

Fixing the error will clear the error column:
$ ovs-vsctl set Interface test options:remote_ip=1.1.1.1
$ ovs-vsctl list Interface test | grep error
error         : []
$

For now, the high level error messages when opening and configuring
the netdev are used. Further patches can extend passing the error
pointer into the individual netdev implementations to allow for more
fine grained error messages to be stored.

Signed-off-by: Thomas Graf <tgraf@redhat.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>

diff --git a/lib/netdev.c b/lib/netdev.c
index f545a51..dd800a4 100644 (file)
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -419,7 +419,7 @@ netdev_ref(const struct netdev *netdev_)
 /* Reconfigures the device 'netdev' with 'args'.  'args' may be empty
  * or NULL if none are needed. */
 int
-netdev_set_config(struct netdev *netdev, const struct smap *args)
+netdev_set_config(struct netdev *netdev, const struct smap *args, char **errp)
     OVS_EXCLUDED(netdev_mutex)
 {
     if (netdev->netdev_class->set_config) {
@@ -429,13 +429,13 @@ netdev_set_config(struct netdev *netdev, const struct smap *args)
         error = netdev->netdev_class->set_config(netdev,
                                                  args ? args : &no_args);
         if (error) {
-            VLOG_WARN("%s: could not set configuration (%s)",
-                      netdev_get_name(netdev), ovs_strerror(error));
+            VLOG_WARN_BUF(errp, "%s: could not set configuration (%s)",
+                          netdev_get_name(netdev), ovs_strerror(error));
         }
         return error;
     } else if (args && !smap_is_empty(args)) {
-        VLOG_WARN("%s: arguments provided to device that is not configurable",
-                  netdev_get_name(netdev));
+        VLOG_WARN_BUF(errp, "%s: arguments provided to device that is not configurable",
+                      netdev_get_name(netdev));
     }
     return 0;
 }

diff --git a/lib/netdev.h b/lib/netdev.h
index 9b35972..a4bd01a 100644 (file)
--- a/lib/netdev.h
+++ b/lib/netdev.h
@@ -147,7 +147,7 @@ void netdev_close(struct netdev *);
 void netdev_parse_name(const char *netdev_name, char **name, char **type);
 
 /* Options. */
-int netdev_set_config(struct netdev *, const struct smap *args);
+int netdev_set_config(struct netdev *, const struct smap *args, char **errp);
 int netdev_get_config(const struct netdev *, struct smap *);
 const struct netdev_tunnel_config *
     netdev_get_tunnel_config(const struct netdev *);

diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c
index ccc55b5..9e879f8 100644 (file)
--- a/utilities/ovs-dpctl.c
+++ b/utilities/ovs-dpctl.c
@@ -349,7 +349,7 @@ dpctl_add_if(int argc OVS_UNUSED, char *argv[])
             goto next;
         }
 
-        error = netdev_set_config(netdev, &args);
+        error = netdev_set_config(netdev, &args, NULL);
         if (error) {
             goto next;
         }
@@ -456,7 +456,7 @@ dpctl_set_if(int argc, char *argv[])
         }
 
         /* Update configuration. */
-        error = netdev_set_config(netdev, &args);
+        error = netdev_set_config(netdev, &args, NULL);
         smap_destroy(&args);
         if (error) {
             goto next;

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 618a705..f3575cd 100644 (file)
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -274,7 +274,7 @@ static struct iface *iface_from_ofp_port(const struct bridge *,
                                          ofp_port_t ofp_port);
 static void iface_set_mac(const struct bridge *, const struct port *, struct iface *);
 static void iface_set_ofport(const struct ovsrec_interface *, ofp_port_t ofport);
-static void iface_clear_db_record(const struct ovsrec_interface *if_cfg);
+static void iface_clear_db_record(const struct ovsrec_interface *if_cfg, char *errp);
 static void iface_configure_qos(struct iface *, const struct ovsrec_qos *);
 static void iface_configure_cfm(struct iface *);
 static void iface_refresh_cfm_stats(struct iface *);
@@ -396,6 +396,7 @@ bridge_init(const char *remote)
     ovsdb_idl_omit_alert(idl, &ovsrec_interface_col_cfm_remote_opstate);
     ovsdb_idl_omit_alert(idl, &ovsrec_interface_col_bfd_status);
     ovsdb_idl_omit_alert(idl, &ovsrec_interface_col_lacp_current);
+    ovsdb_idl_omit_alert(idl, &ovsrec_interface_col_error);
     ovsdb_idl_omit(idl, &ovsrec_interface_col_external_ids);
 
     ovsdb_idl_omit_alert(idl, &ovsrec_controller_col_is_connected);
@@ -596,6 +597,8 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
 
             LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
                 iface_set_ofport(iface->cfg, iface->ofp_port);
+                /* Clear eventual previous errors */
+                ovsrec_interface_set_error(iface->cfg, NULL);
                 iface_configure_cfm(iface);
                 iface_configure_qos(iface, port->cfg->qos);
                 iface_set_mac(br, port, iface);
@@ -711,7 +714,7 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
         }
 
         if (strcmp(ofproto_port.type, iface->type)
-            || netdev_set_config(iface->netdev, &iface->cfg->options)) {
+            || netdev_set_config(iface->netdev, &iface->cfg->options, NULL)) {
             /* The interface is the wrong type or can't be configured.
              * Delete it. */
             goto delete;
@@ -1434,9 +1437,9 @@ add_del_bridges(const struct ovsrec_open_vswitch *cfg)
  * Returns 0 if successful, otherwise a positive errno value. */
 static int
 iface_set_netdev_config(const struct ovsrec_interface *iface_cfg,
-                        struct netdev *netdev)
+                        struct netdev *netdev, char **errp)
 {
-    return netdev_set_config(netdev, &iface_cfg->options);
+    return netdev_set_config(netdev, &iface_cfg->options, errp);
 }
 
 /* Opens a network device for 'if_cfg' and configures it.  Adds the network
@@ -1448,7 +1451,8 @@ static int
 iface_do_create(const struct bridge *br,
                 const struct ovsrec_interface *iface_cfg,
                 const struct ovsrec_port *port_cfg,
-                ofp_port_t *ofp_portp, struct netdev **netdevp)
+                ofp_port_t *ofp_portp, struct netdev **netdevp,
+                char **errp)
 {
     struct netdev *netdev = NULL;
     int error;
@@ -1463,12 +1467,12 @@ iface_do_create(const struct bridge *br,
     error = netdev_open(iface_cfg->name,
                         iface_get_type(iface_cfg, br->cfg), &netdev);
     if (error) {
-        VLOG_WARN("could not open network device %s (%s)",
-                  iface_cfg->name, ovs_strerror(error));
+        VLOG_WARN_BUF(errp, "could not open network device %s (%s)",
+                      iface_cfg->name, ovs_strerror(error));
         goto error;
     }
 
-    error = iface_set_netdev_config(iface_cfg, netdev);
+    error = iface_set_netdev_config(iface_cfg, netdev, errp);
     if (error) {
         goto error;
     }
@@ -1509,13 +1513,15 @@ iface_create(struct bridge *br, const struct ovsrec_interface *iface_cfg,
     struct iface *iface;
     ofp_port_t ofp_port;
     struct port *port;
+    char *errp = NULL;
     int error;
 
     /* Do the bits that can fail up front. */
     ovs_assert(!iface_lookup(br, iface_cfg->name));
-    error = iface_do_create(br, iface_cfg, port_cfg, &ofp_port, &netdev);
+    error = iface_do_create(br, iface_cfg, port_cfg, &ofp_port, &netdev, &errp);
     if (error) {
-        iface_clear_db_record(iface_cfg);
+        iface_clear_db_record(iface_cfg, errp);
+        free(errp);
         return false;
     }
 
@@ -3586,10 +3592,11 @@ iface_set_ofport(const struct ovsrec_interface *if_cfg, ofp_port_t ofport)
  * This is appropriate when 'if_cfg''s interface cannot be created or is
  * otherwise invalid. */
 static void
-iface_clear_db_record(const struct ovsrec_interface *if_cfg)
+iface_clear_db_record(const struct ovsrec_interface *if_cfg, char *errp)
 {
     if (!ovsdb_idl_row_is_synthetic(&if_cfg->header_)) {
         iface_set_ofport(if_cfg, OFPP_NONE);
+        ovsrec_interface_set_error(if_cfg, errp);
         ovsrec_interface_set_status(if_cfg, NULL);
         ovsrec_interface_set_admin_state(if_cfg, NULL);
         ovsrec_interface_set_duplex(if_cfg, NULL);

diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
index 006ed23..285cf6d 100644 (file)
--- a/vswitchd/vswitch.ovsschema
+++ b/vswitchd/vswitch.ovsschema
@@ -1,6 +1,6 @@
 {"name": "Open_vSwitch",
- "version": "7.6.0",
- "cksum": "1731605290 20602",
+ "version": "7.7.0",
+ "cksum": "2517737670 20677",
  "tables": {
    "Open_vSwitch": {
      "columns": {
@@ -288,7 +288,9 @@
          "ephemeral": true},
        "mtu": {
          "type": {"key": "integer", "min": 0, "max": 1},
-         "ephemeral": true}},
+         "ephemeral": true},
+       "error": {
+         "type": {"key": "string", "min": 0, "max": 1}}},
      "indexes": [["name"]]},
    "Flow_Table": {
      "columns": {

diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index b37e639..acefed2 100644 (file)
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -1299,6 +1299,13 @@
         address.</p>
       </column>
 
+      <column name="error">
+        If the configuration of the port failed, as indicated by -1 in <ref
+        column="ofport"/>, Open vSwitch sets this column to an error
+        description in human readable form.  Otherwise, Open vSwitch clears
+        this column.
+      </column>
+
       <group title="OpenFlow Port Number">
        <p>
          When a client adds a new interface, Open vSwitch chooses an OpenFlow