ovs-vsctl: Improve error reporting

ovs-vsctl is a command-line interface to the Open vSwitch database,
and as such it just modifies the desired Open vSwitch configuration in
the database.  ovs-vswitchd, on the other hand, monitors the database
and implements the actual configuration specified in the database.
This can lead to surprises when the user makes a change to the
database, with ovs-vsctl, that ovs-vswitchd cannot actually
implement. In such a case, the ovs-vsctl command silently succeeds
(because the database was successfully updated) but its desired
effects don't actually take place. One good example of such a change
is attempting to add a port with a misspelled name (e.g. ``ovs-vsctl
add-port br0 fth0'', where fth0 should be eth0); another is creating
a bridge or a port whose name is longer than supported
(e.g. ``ovs-vsctl add-br'' with a 16-character bridge name on
Linux). It can take users a long time to realize the error, because it
requires looking through the ovs-vswitchd log.

The patch improves the situation by checking whether operations that
ovs executes succeed and report an error when
they do not.  This patch only report add-br and add-port
operation errors by examining the `ofport' value that
ovs-vswitchd stores into the database record for the newly created
interface.  Until ovs-vswitchd finishes implementing the new
configuration, this column is empty, and after it finishes it is
either -1 (on failure) or a positive number (on success).

Signed-off-by: Andy Zhou <azhou@nicira.com>
Co-authored-by: Thomas Graf <tgraf@redhat.com>
Signed-off-by: Thomas Graf <tgraf@redhat.com>
Co-authored-by: Ben Pfaff <blp@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>

NEWS

@@ -1,5 +1,7 @@
 Post-v2.1.0
 ---------------------
+   - ovs-vsctl now reports when ovs-vswitchd fails to create a new port or
+     bridge.
    - The "ovsdbmonitor" graphical tool has been removed, because it was
      poorly maintained and not widely used.
    - New "check-ryu" Makefile target for running Ryu tests for OpenFlow

tests/ovs-vsctl.at

@@ -1209,7 +1209,9 @@ m4_foreach(
 [vxlan_system]],
 [
 # Try creating the port
-AT_CHECK([ovs-vsctl add-port br0 reserved_name], [0], [], [])
+AT_CHECK([ovs-vsctl add-port br0 reserved_name], [0], [], [dnl
+ovs-vsctl: Error detected while setting up 'reserved_name'.  See ovs-vswitchd log for details.
+])
 # Detect the warning log message
 AT_CHECK([sed -n "s/^.*\(|bridge|WARN|.*\)$/\1/p" ovs-vswitchd.log], [0], [dnl
 |bridge|WARN|could not create interface reserved_name, name is reserved
@@ -1242,7 +1244,9 @@ m4_foreach(
 [vxlan_system]],
 [
 # Try creating the port
-AT_CHECK([ovs-vsctl add-port br0 reserved_name], [0], [], [])
+AT_CHECK([ovs-vsctl add-port br0 reserved_name], [0], [], [dnl
+ovs-vsctl: Error detected while setting up 'reserved_name'.  See ovs-vswitchd log for details.
+])
 # Detect the warning log message
 AT_CHECK([sed -n "s/^.*\(|bridge|WARN|.*\)$/\1/p" ovs-vswitchd.log], [0], [dnl
 |bridge|WARN|could not create interface reserved_name, name is reserved

utilities/ovs-vsctl.c

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
+ * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -165,6 +165,34 @@ static bool is_condition_satisfied(const struct vsctl_table_class *,
                                    const char *arg,
                                    struct ovsdb_symbol_table *);
 
+/* Post_db_reload_check frame work is to allow ovs-vsctl to do additional
+ * checks after OVSDB transactions are successfully recorded and reload by
+ * ovs-vswitchd.
+ *
+ * For example, When a new interface is added to OVSDB, ovs-vswitchd will
+ * either store a positive values on successful implementing the new
+ * interface, or -1 on failure.
+ *
+ * Unless -no-wait command line option is specified,
+ * post_db_reload_do_checks() is called right after any configuration
+ * changes is picked up (i.e. reload) by ovs-vswitchd. Any error detected
+ * post OVSDB reload is reported as ovs-vsctl errors. OVS-vswitchd logs
+ * more detailed messages about those errors.
+ *
+ * Current implementation only check for Post OVSDB reload failures on new
+ * interface additions with 'add-br' and 'add-port' commands.
+ *
+ * post_db_reload_expect_iface()
+ *
+ * keep track of interfaces to be checked post OVSDB reload. */
+static void post_db_reload_check_init(void);
+static void post_db_reload_do_checks(const struct vsctl_context *);
+static void post_db_reload_expect_iface(const struct ovsrec_interface *);
+
+static struct uuid *neoteric_ifaces;
+static size_t n_neoteric_ifaces;
+static size_t allocated_neoteric_ifaces;
+
 int
 main(int argc, char *argv[])
 {
@@ -1004,6 +1032,7 @@ pre_get_info(struct vsctl_context *ctx)
     ovsdb_idl_add_column(ctx->idl, &ovsrec_port_col_interfaces);
 
     ovsdb_idl_add_column(ctx->idl, &ovsrec_interface_col_name);
+    ovsdb_idl_add_column(ctx->idl, &ovsrec_interface_col_ofport);
 }
 
 static void
@@ -1561,6 +1590,7 @@ cmd_add_br(struct vsctl_context *ctx)
 {
     bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
     const char *br_name, *parent_name;
+    struct ovsrec_interface *iface;
     int vlan;
 
     br_name = ctx->argv[1];
@@ -1614,7 +1644,6 @@ cmd_add_br(struct vsctl_context *ctx)
 
     if (!parent_name) {
         struct ovsrec_port *port;
-        struct ovsrec_interface *iface;
         struct ovsrec_bridge *br;
 
         iface = ovsrec_interface_insert(ctx->txn);
@@ -1633,7 +1662,6 @@ cmd_add_br(struct vsctl_context *ctx)
     } else {
         struct vsctl_bridge *parent;
         struct ovsrec_port *port;
-        struct ovsrec_interface *iface;
         struct ovsrec_bridge *br;
         int64_t tag = vlan;
 
@@ -1659,6 +1687,7 @@ cmd_add_br(struct vsctl_context *ctx)
         bridge_insert_port(br, port);
     }
 
+    post_db_reload_expect_iface(iface);
     vsctl_context_invalidate_cache(ctx);
 }
 
@@ -1952,6 +1981,7 @@ add_port(struct vsctl_context *ctx,
     for (i = 0; i < n_ifaces; i++) {
         ifaces[i] = ovsrec_interface_insert(ctx->txn);
         ovsrec_interface_set_name(ifaces[i], iface_names[i]);
+        post_db_reload_expect_iface(ifaces[i]);
     }
 
     port = ovsrec_port_insert(ctx->txn);
@@ -3643,6 +3673,52 @@ post_create(struct vsctl_context *ctx)
     ds_put_char(&ctx->output, '\n');
 }
 
+static void
+post_db_reload_check_init(void)
+{
+    n_neoteric_ifaces = 0;
+}
+
+static void
+post_db_reload_expect_iface(const struct ovsrec_interface *iface)
+{
+    if (n_neoteric_ifaces >= allocated_neoteric_ifaces) {
+        neoteric_ifaces = x2nrealloc(neoteric_ifaces,
+                                     &allocated_neoteric_ifaces,
+                                     sizeof *neoteric_ifaces);
+    }
+    neoteric_ifaces[n_neoteric_ifaces++] = iface->header_.uuid;
+}
+
+static void
+post_db_reload_do_checks(const struct vsctl_context *ctx)
+{
+    struct ds dead_ifaces = DS_EMPTY_INITIALIZER;
+    size_t i;
+
+    for (i = 0; i < n_neoteric_ifaces; i++) {
+        const struct uuid *uuid;
+
+        uuid = ovsdb_idl_txn_get_insert_uuid(ctx->txn, &neoteric_ifaces[i]);
+        if (uuid) {
+            const struct ovsrec_interface *iface;
+
+            iface = ovsrec_interface_get_for_uuid(ctx->idl, uuid);
+            if (iface && (!iface->ofport || *iface->ofport == -1)) {
+                ds_put_format(&dead_ifaces, "'%s', ", iface->name);
+            }
+        }
+    }
+
+    if (dead_ifaces.length) {
+        dead_ifaces.length -= 2; /* Strip off trailing comma and space. */
+        ovs_error(0, "Error detected while setting up %s.  See ovs-vswitchd "
+                  "log for details.", ds_cstr(&dead_ifaces));
+    }
+
+    ds_destroy(&dead_ifaces);
+}
+
 static void
 pre_cmd_destroy(struct vsctl_context *ctx)
 {
@@ -3999,6 +4075,7 @@ do_vsctl(const char *args, struct vsctl_command *commands, size_t n_commands,
                                 &ovsrec_open_vswitch_col_next_cfg);
     }
 
+    post_db_reload_check_init();
     symtab = ovsdb_symbol_table_create();
     for (c = commands; c < &commands[n_commands]; c++) {
         ds_init(&c->output);
@@ -4055,8 +4132,6 @@ do_vsctl(const char *args, struct vsctl_command *commands, size_t n_commands,
         }
     }
     error = xstrdup(ovsdb_idl_txn_get_error(txn));
-    ovsdb_idl_txn_destroy(txn);
-    txn = the_idl_txn = NULL;
 
     switch (status) {
     case TXN_UNCOMMITTED:
@@ -4134,6 +4209,7 @@ do_vsctl(const char *args, struct vsctl_command *commands, size_t n_commands,
             ovsdb_idl_run(idl);
             OVSREC_OPEN_VSWITCH_FOR_EACH (ovs, idl) {
                 if (ovs->cur_cfg >= next_cfg) {
+                    post_db_reload_do_checks(&ctx);
                     goto done;
                 }
             }
@@ -4142,6 +4218,7 @@ do_vsctl(const char *args, struct vsctl_command *commands, size_t n_commands,
         }
     done: ;
     }
+    ovsdb_idl_txn_destroy(txn);
     ovsdb_idl_destroy(idl);
 
     exit(EXIT_SUCCESS);