Not sure if I suppose to ack this patch since I am one of authors. I did review v4 in detail and tested it .
Just in case it is useful, here is my ack. Acked-by: Andy Zhou <az...@nicira.com> On Thu, Mar 27, 2014 at 9:02 PM, Ben Pfaff <b...@nicira.com> wrote: > 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> > Co-authored-by: Thomas Graf <tg...@redhat.com> > Signed-off-by: Thomas Graf <tg...@redhat.com> > Co-authored-by: Ben Pfaff <b...@nicira.com> > Signed-off-by: Ben Pfaff <b...@nicira.com> > --- > v3->v4: See http://openvswitch.org/pipermail/dev/2014-March/038164.html for > changes. > > tests/ovs-vsctl.at | 8 +++-- > utilities/ovs-vsctl.c | 87 > ++++++++++++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 88 insertions(+), 7 deletions(-) > > diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at > index 851e4d8..440bf1a 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 21ac777..4fcee77 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); > @@ -3644,6 +3674,52 @@ post_create(struct vsctl_context *ctx) > } > > 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) > { > const char *table_name = ctx->argv[1]; > @@ -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); > -- > 1.8.5.3 > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
