From c3ccfe984933ca8df56d551cb3ae7e0bf3d119b8 Mon Sep 17 00:00:00 2001 From: Andy Zhou Date: Thu, 27 Mar 2014 17:10:31 +0100 Subject: [PATCH] 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 Co-authored-by: Thomas Graf Signed-off-by: Thomas Graf Co-authored-by: Ben Pfaff Signed-off-by: Ben Pfaff --- NEWS | 2 + tests/ovs-vsctl.at | 8 +++- utilities/ovs-vsctl.c | 87 ++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 90 insertions(+), 7 deletions(-) diff --git a/NEWS b/NEWS index 5f4632ca6..4f88e3b8e 100644 --- a/NEWS +++ b/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 diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at index 851e4d8dd..440bf1ae9 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], [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 diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c index 21ac777e2..4fcee77ae 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() + * + * 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); -- 2.43.0