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).
CC: Thomas Graf <tg...@redhat.com>
Co-authored-by: Thomas Graf <tg...@redhat.com>
Signed-off-by: Andy Zhou <az...@nicira.com>
---
V0-V1:
* This patch is prepared as part of OVS Hackathon 2014.
* Ben has provided many valuable inputs on the implementation
approach. The commit message is almost entirely his words
lifted from the Hackathon project idea write up.
* Thomas: Some of the code and ideas are imported from your
12/11/2012 patch titled "ovs-vsctl: Check "ofport" column after
adding a new port", and subsequent mailing list discussions.
Please feel free to review, fix and enhance the patch as
you see fit. More testing will is definitly needed. If make sense,
we can add more unit test cases. The ovs-vsctl manpage needs to be
updated. Please re-post the patch as v2 when you are done with
your changes.
---
tests/ovs-vsctl.at | 8 +++--
utilities/ovs-vsctl.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 96 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..1323d3c 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,59 @@ 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("Unable to add '%s'", 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 +4221,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 +4230,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.7.9.5
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
