From: Andy Zhou <az...@nicira.com>
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 <az...@nicira.com>
Signed-off-by: Thomas Graf <tg...@redhat.com>
---
V2: Improved error message to indicate error occurs during setup and
that additional details can be found in the log.
---
tests/ovs-vsctl.at | 8 +++--
utilities/ovs-vsctl.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 97 insertions(+), 3 deletions(-)
diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
index 851e4d8..e62156b 100644
--- a/tests/ovs-vsctl.at
+++ b/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], [1], [], [dnl
+ovs-vsctl: Unable to add 'reserved_name'
+])
# 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], [1], [], [dnl
+ovs-vsctl: Unable to add 'reserved_name'
+])
# 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
diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
index 21ac777..05aea87 100644
--- a/utilities/ovs-vsctl.c
+++ b/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()
+ * post_db_reload_unexpect_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 ovsdb_idl *idl);
+static void post_db_reload_expect_iface(const char *iface);
+static void post_db_reload_unexpect_iface(const char *iface);
+static void post_db_reload_check_cleanup(void);
+static struct sset neoteric_ifaces;
+
int
main(int argc, char *argv[])
{
@@ -181,6 +209,7 @@ main(int argc, char *argv[])
vlog_set_levels(NULL, VLF_CONSOLE, VLL_WARN);
vlog_set_levels(&VLM_reconnect, VLF_ANY_FACILITY, VLL_WARN);
ovsrec_init();
+ post_db_reload_check_init();
/* Log our arguments. This is often valuable for debugging systems. */
args = process_escape_args(argv);
@@ -1004,6 +1033,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
@@ -1659,6 +1689,7 @@ cmd_add_br(struct vsctl_context *ctx)
bridge_insert_port(br, port);
}
+ post_db_reload_expect_iface(br_name);
vsctl_context_invalidate_cache(ctx);
}
@@ -1672,6 +1703,7 @@ del_port(struct vsctl_context *ctx, struct vsctl_port
*port)
: port->bridge->br_cfg), port->port_cfg);
LIST_FOR_EACH_SAFE (iface, next_iface, ifaces_node, &port->ifaces) {
+ post_db_reload_unexpect_iface(iface->iface_cfg->name);
del_cached_iface(ctx, iface);
}
del_cached_port(ctx, port);
@@ -1699,6 +1731,7 @@ del_bridge(struct vsctl_context *ctx, struct vsctl_bridge
*br)
}
}
+ post_db_reload_unexpect_iface(br->name);
del_cached_bridge(ctx, br);
}
@@ -1952,6 +1985,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(iface_names[i]);
}
port = ovsrec_port_insert(ctx->txn);
@@ -3644,6 +3678,60 @@ post_create(struct vsctl_context *ctx)
}
static void
+post_db_reload_check_init(void)
+{
+ sset_init(&neoteric_ifaces);
+}
+
+static void
+post_db_reload_expect_iface(const char *iface)
+{
+ sset_add_assert(&neoteric_ifaces, iface);
+}
+
+static void
+post_db_reload_unexpect_iface(const char *iface)
+{
+ sset_find_and_delete(&neoteric_ifaces, iface);
+}
+
+static void
+append_dead_iface(struct ds *ds, const char *iface)
+{
+ if (strlen(ds_cstr(ds))) {
+ ds_put_cstr(ds, ", ");
+ }
+ ds_put_format(ds, "%s", iface);
+}
+
+static void
+post_db_reload_do_checks(const struct ovsdb_idl *idl)
+{
+ struct ds dead_ifaces = DS_EMPTY_INITIALIZER;
+ const struct ovsrec_interface *iface;
+
+ OVSREC_INTERFACE_FOR_EACH (iface, idl) {
+ if (sset_contains(&neoteric_ifaces, iface->name) &&
+ (!iface->ofport || *iface->ofport == -1)) {
+ append_dead_iface(&dead_ifaces, iface->name);
+ }
+ }
+
+ if (strlen(ds_cstr(&dead_ifaces))) {
+ vsctl_fatal("Error detected while setting up '%s'. See log for
details.",
+ ds_cstr(&dead_ifaces));
+ }
+
+ ds_destroy(&dead_ifaces);
+}
+
+static void
+post_db_reload_check_cleanup(void)
+{
+ sset_destroy(&neoteric_ifaces);
+}
+
+static void
pre_cmd_destroy(struct vsctl_context *ctx)
{
const char *table_name = ctx->argv[1];
@@ -4134,6 +4222,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(idl);
goto done;
}
}
@@ -4142,6 +4231,7 @@ do_vsctl(const char *args, struct vsctl_command
*commands, size_t n_commands,
}
done: ;
}
+ post_db_reload_check_cleanup();
ovsdb_idl_destroy(idl);
exit(EXIT_SUCCESS);
--
1.8.3.1
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
