[ovs-dev] [PATCH v4 2/2] ovs-vsctl: Improve error reporting

Andy Zhou azhou at nicira.com
Fri Mar 28 07:13:45 UTC 2014


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 <azhou at nicira.com>

On Thu, Mar 27, 2014 at 9:02 PM, Ben Pfaff <blp at nicira.com> wrote:
> From: Andy Zhou <azhou at 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 <azhou at nicira.com>
> Co-authored-by: Thomas Graf <tgraf at redhat.com>
> Signed-off-by: Thomas Graf <tgraf at redhat.com>
> Co-authored-by: Ben Pfaff <blp at nicira.com>
> Signed-off-by: Ben Pfaff <blp at 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
>



More information about the dev mailing list