[ovs-dev] [PATCH] ovs-vsctl: Try connecting only once for active connections by default.

Ben Pfaff blp at nicira.com
Sat Mar 16 01:21:05 UTC 2013


Oops, I misinterpreted your other email as "looks good". Sorry about that.
I will fix up your comments with extra commits.
On Mar 15, 2013 4:13 PM, "Ansis Atteka" <aatteka at nicira.com> wrote:

> On Fri, Mar 15, 2013 at 2:46 PM, Ansis Atteka <aatteka at nicira.com> wrote:
> > On Fri, Mar 15, 2013 at 1:37 PM, Ben Pfaff <blp at nicira.com> wrote:
> >> Until now, ovs-vsctl has kept trying to the database server until it
> >> succeeded or the timeout expired (if one was specified with --timeout).
> >> This meant that if ovsdb-server wasn't running, then ovs-vsctl would
> hang.
> >> The result was that almost every ovs-vsctl invocation in scripts
> specified
> >> a timeout on the off-chance that the database server might not be
> running.
> >> But it's difficult to choose a good timeout.  A timeout that is too
> short
> >> can cause spurious failures.  A timeout that is too long causes long
> delays
> >> if the server really isn't running.
> >>
> >> This commit should alleviate this problem.  It changes ovs-vsctl's
> behavior
> >> so that, if it fails to connect to the server, it exits unsuccessfully.
> >> This makes --timeout obsolete for the purpose of avoiding a hang if the
> >> database server isn't running.  (--timeout is still useful to avoid a
> hang
> >> if ovsdb-server is running but ovs-vswitchd is not, for ovs-vsctl
> commands
> >> that modify the database.  --no-wait also avoids that issue.)
> >>
> >> Bug #2393.
> >> Bug #15594.
> >> Reported-by: Jeff Merrick <jmerrick at vmware.com>
> >> Signed-off-by: Ben Pfaff <blp at nicira.com>
> >> ---
> >>  AUTHORS                                            |    1 +
> >>  NEWS                                               |    6 +++
> >>  debian/ifupdown.sh                                 |    4 +-
> >>  lib/jsonrpc.c                                      |   24 +++++++++-
> >>  lib/jsonrpc.h                                      |    5 +-
> >>  lib/ovsdb-idl.c                                    |   19 +++++++-
> >>  lib/ovsdb-idl.h                                    |    6 ++-
> >>  lib/reconnect.c                                    |    5 +-
> >>  ovsdb/jsonrpc-server.c                             |    2 +-
> >>  tests/interface-reconfigure.at                     |    4 +-
> >>  tests/ovs-monitor-ipsec.at                         |    2 +-
> >>  tests/ovs-vsctl.at                                 |   48
> +++++++++++++++++---
> >>  tests/ovs-xapi-sync.at                             |    2 +-
> >>  tests/test-ovsdb.c                                 |    4 +-
> >>  utilities/bugtool/ovs-bugtool-vsctl-show           |    4 +-
> >>  utilities/ovs-ctl.in                               |    2 +-
> >>  utilities/ovs-save                                 |    6 +-
> >>  utilities/ovs-vsctl.8.in                           |   14 ++++++
> >>  utilities/ovs-vsctl.c                              |   23 +++++++++-
> >>  vswitchd/bridge.c                                  |    2 +-
> >>  .../etc_xapi.d_plugins_openvswitch-cfg-update      |    4 +-
> >>  ...ensource_libexec_InterfaceReconfigureVswitch.py |    4 +-
> >>  ..._lib_xsconsole_plugins-base_XSFeatureVSwitch.py |    4 +-
> >>  23 files changed, 155 insertions(+), 40 deletions(-)
> >>
> >> diff --git a/AUTHORS b/AUTHORS
> >> index 9613142..ed263a1 100644
> >> --- a/AUTHORS
> >> +++ b/AUTHORS
> >> @@ -142,6 +142,7 @@ Jan Medved              jmedved at juniper.net
> >>  Janis Hamme             janis.hamme at student.kit.edu
> >>  Jari Sundell            sundell.software at gmail.com
> >>  Jed Daniels             openvswitch at jeddaniels.com
> >> +Jeff Merrick            jmerrick at vmware.com
> >>  Jeongkeun Lee           jklee at hp.com
> >>  Jing Ai                 ai_jing2000 at hotmail.com
> >>  Joan Cirer              joan at ev0.net
> >> diff --git a/NEWS b/NEWS
> >> index 7914807..cc96880 100644
> >> --- a/NEWS
> >> +++ b/NEWS
> >> @@ -1,5 +1,11 @@
> >>  post-v1.10.0
> >>  ---------------------
> >> +    - ovs-vsctl:
> >> +      * Previously ovs-vsctl would retry connecting to the database
> forever,
> >> +        causing it to hang if ovsdb-server was not running.  Now,
> ovs-vsctl
> >> +        only tries once by default (use --retry to try forever).  This
> change
> >> +        means that you may want to remove uses of --timeout to avoid
> hangs
> >> +        in ovs-vsctl calls.g
> > It seems that "g" is not necessary here.
> >
> >>      - Stable bond mode has been removed.
> >>      - The autopath action has been removed.
> >>      - New support for the data encapsulation format of the LISP tunnel
> >> diff --git a/debian/ifupdown.sh b/debian/ifupdown.sh
> >> index f6a7750..f728f7c 100755
> >> --- a/debian/ifupdown.sh
> >> +++ b/debian/ifupdown.sh
> >> @@ -1,6 +1,6 @@
> >>  #! /bin/sh
> >>
> >> -# Copyright (c) 2012 Nicira, Inc.
> >> +# Copyright (c) 2012, 2013 Nicira, Inc.
> >>  #
> >>  # Licensed under the Apache License, Version 2.0 (the "License");
> >>  # you may not use this file except in compliance with the License.
> >> @@ -22,7 +22,7 @@ if [ -z "${IF_OVS_TYPE}" ]; then
> >>  fi
> >>
> >>  ovs_vsctl() {
> >> -    ovs-vsctl --timeout=5 "$@"
> >> +    ovs-vsctl "$@"
> >>  }
> >>
> >>  if (ovs_vsctl --version) > /dev/null 2>&1; then :; else
> >> diff --git a/lib/jsonrpc.c b/lib/jsonrpc.c
> >> index 50073b6..1ea5398 100644
> >> --- a/lib/jsonrpc.c
> >> +++ b/lib/jsonrpc.c
> >> @@ -1,5 +1,5 @@
> >>  /*
> >> - * Copyright (c) 2009, 2010, 2011, 2012 Nicira, Inc.
> >> + * Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> >>   *
> >>   * Licensed under the Apache License, Version 2.0 (the "License");
> >>   * you may not use this file except in compliance with the License.
> >> @@ -744,6 +744,7 @@ struct jsonrpc_session {
> >>      struct jsonrpc *rpc;
> >>      struct stream *stream;
> >>      struct pstream *pstream;
> >> +    int last_error;
> >>      unsigned int seqno;
> >>      uint8_t dscp;
> >>  };
> >> @@ -752,14 +753,18 @@ struct jsonrpc_session {
> >>   * acceptable to stream_open() or pstream_open().
> >>   *
> >>   * If 'name' is an active connection method, e.g. "tcp:127.1.2.3", the
> new
> >> - * jsonrpc_session connects and reconnects, with back-off, to 'name'.
> >> + * jsonrpc_session connects to 'name'.  If 'retry' is true, then the
> new
> >> + * session connects and reconnects to 'name', with backoff.  If
> 'retry' is
> >> + * false, the new session will only try to connect once and after a
> connection
> >> + * failure or a disconnection jsonrpc_session_is_alive() will return
> false for
> >> + * the new session.
> >>   *
> >>   * If 'name' is a passive connection method, e.g. "ptcp:", the new
> >>   * jsonrpc_session listens for connections to 'name'.  It maintains at
> most one
> >>   * connection at any given time.  Any new connection causes the
> previous one
> >>   * (if any) to be dropped. */
> >>  struct jsonrpc_session *
> >> -jsonrpc_session_open(const char *name)
> >> +jsonrpc_session_open(const char *name, bool retry)
> >>  {
> >>      struct jsonrpc_session *s;
> >>
> >> @@ -772,9 +777,13 @@ jsonrpc_session_open(const char *name)
> >>      s->pstream = NULL;
> >>      s->seqno = 0;
> >>      s->dscp = 0;
> >> +    s->last_error = 0;
> >>
> >>      if (!pstream_verify_name(name)) {
> >>          reconnect_set_passive(s->reconnect, true, time_msec());
> >> +    } else if (!retry) {
> >> +        reconnect_set_max_tries(s->reconnect, 1);
> >> +        reconnect_set_backoff(s->reconnect, INT_MAX, INT_MAX);
> >>      }
> >>
> >>      if (!stream_or_pstream_needs_probes(name)) {
> >> @@ -847,6 +856,8 @@ jsonrpc_session_connect(struct jsonrpc_session *s)
> >>          error = jsonrpc_stream_open(name, &s->stream, s->dscp);
> >>          if (!error) {
> >>              reconnect_connecting(s->reconnect, time_msec());
> >> +        } else {
> >> +            s->last_error = error;
> >>          }
> >>      } else {
> >>          error = s->pstream ? 0 : jsonrpc_pstream_open(name,
> &s->pstream,
> >> @@ -908,6 +919,7 @@ jsonrpc_session_run(struct jsonrpc_session *s)
> >>          if (error) {
> >>              reconnect_disconnected(s->reconnect, time_msec(), error);
> >>              jsonrpc_session_disconnect(s);
> >> +            s->last_error = error;
> >>          }
> >>      } else if (s->stream) {
> >>          int error;
> >> @@ -1061,6 +1073,12 @@ jsonrpc_session_get_status(const struct
> jsonrpc_session *s)
> >>      return s && s->rpc ? jsonrpc_get_status(s->rpc) : 0;
> >>  }
> >>
> >> +int
> >> +jsonrpc_session_get_last_error(const struct jsonrpc_session *s)
> >> +{
> >> +    return s->last_error;
> >> +}
> >> +
> >>  void
> >>  jsonrpc_session_get_reconnect_stats(const struct jsonrpc_session *s,
> >>                                      struct reconnect_stats *stats)
> >> diff --git a/lib/jsonrpc.h b/lib/jsonrpc.h
> >> index b5acf89..0ae205d 100644
> >> --- a/lib/jsonrpc.h
> >> +++ b/lib/jsonrpc.h
> >> @@ -1,5 +1,5 @@
> >>  /*
> >> - * Copyright (c) 2009, 2010, 2012 Nicira, Inc.
> >> + * Copyright (c) 2009, 2010, 2012, 2013 Nicira, Inc.
> >>   *
> >>   * Licensed under the Apache License, Version 2.0 (the "License");
> >>   * you may not use this file except in compliance with the License.
> >> @@ -98,7 +98,7 @@ struct json *jsonrpc_msg_to_json(struct jsonrpc_msg
> *);
> >>
> >>  /* A JSON-RPC session with reconnection. */
> >>
> >> -struct jsonrpc_session *jsonrpc_session_open(const char *name);
> >> +struct jsonrpc_session *jsonrpc_session_open(const char *name, bool
> retry);
> >>  struct jsonrpc_session *jsonrpc_session_open_unreliably(struct jsonrpc
> *,
> >>                                                          uint8_t);
> >>  void jsonrpc_session_close(struct jsonrpc_session *);
> >> @@ -117,6 +117,7 @@ bool jsonrpc_session_is_alive(const struct
> jsonrpc_session *);
> >>  bool jsonrpc_session_is_connected(const struct jsonrpc_session *);
> >>  unsigned int jsonrpc_session_get_seqno(const struct jsonrpc_session *);
> >>  int jsonrpc_session_get_status(const struct jsonrpc_session *);
> >> +int jsonrpc_session_get_last_error(const struct jsonrpc_session *);
> >>  void jsonrpc_session_get_reconnect_stats(const struct jsonrpc_session
> *,
> >>                                           struct reconnect_stats *);
> >>
> >> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> >> index 0c644a5..116aa86 100644
> >> --- a/lib/ovsdb-idl.c
> >> +++ b/lib/ovsdb-idl.c
> >> @@ -157,6 +157,9 @@ static void ovsdb_idl_parse_lock_notify(struct
> ovsdb_idl *,
> >>   * 'class'.  (Ordinarily 'class' is compiled from an OVSDB schema
> automatically
> >>   * by ovsdb-idlc.)
> >>   *
> >> + * Passes 'retry' to jsonrpc_session_open().  See that function for
> >> + * documentation.
> >> + *
> >>   * If 'monitor_everything_by_default' is true, then everything in the
> remote
> >>   * database will be replicated by default.  ovsdb_idl_omit() and
> >>   * ovsdb_idl_omit_alert() may be used to selectively drop some columns
> from
> >> @@ -168,7 +171,7 @@ static void ovsdb_idl_parse_lock_notify(struct
> ovsdb_idl *,
> >>   */
> >>  struct ovsdb_idl *
> >>  ovsdb_idl_create(const char *remote, const struct ovsdb_idl_class
> *class,
> >> -                 bool monitor_everything_by_default)
> >> +                 bool monitor_everything_by_default, bool retry)
> >>  {
> >>      struct ovsdb_idl *idl;
> >>      uint8_t default_mode;
> >> @@ -180,7 +183,7 @@ ovsdb_idl_create(const char *remote, const struct
> ovsdb_idl_class *class,
> >>
> >>      idl = xzalloc(sizeof *idl);
> >>      idl->class = class;
> >> -    idl->session = jsonrpc_session_open(remote);
> >> +    idl->session = jsonrpc_session_open(remote, retry);
> >>      shash_init(&idl->table_by_name);
> >>      idl->tables = xmalloc(class->n_tables * sizeof *idl->tables);
> >>      for (i = 0; i < class->n_tables; i++) {
> >> @@ -412,6 +415,18 @@ ovsdb_idl_verify_write_only(struct ovsdb_idl *idl)
> >>  {
> >>      idl->verify_write_only = true;
> >>  }
> >> +
> >> +bool
> >> +ovsdb_idl_is_alive(const struct ovsdb_idl *idl)
> >> +{
> >> +    return jsonrpc_session_is_alive(idl->session);
> >> +}
> >> +
> >> +int
> >> +ovsdb_idl_get_last_error(const struct ovsdb_idl *idl)
> >> +{
> >> +    return jsonrpc_session_get_last_error(idl->session);
> >> +}
> >>
> >>  static unsigned char *
> >>  ovsdb_idl_get_mode(struct ovsdb_idl *idl,
> >> diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h
> >> index b428501..6b5e198 100644
> >> --- a/lib/ovsdb-idl.h
> >> +++ b/lib/ovsdb-idl.h
> >> @@ -44,7 +44,8 @@ struct uuid;
> >>
> >>  struct ovsdb_idl *ovsdb_idl_create(const char *remote,
> >>                                     const struct ovsdb_idl_class *,
> >> -                                   bool monitor_everything_by_default);
> >> +                                   bool monitor_everything_by_default,
> >> +                                   bool retry);
> >>  void ovsdb_idl_destroy(struct ovsdb_idl *);
> >>
> >>  void ovsdb_idl_run(struct ovsdb_idl *);
> >> @@ -58,6 +59,9 @@ unsigned int ovsdb_idl_get_seqno(const struct
> ovsdb_idl *);
> >>  bool ovsdb_idl_has_ever_connected(const struct ovsdb_idl *);
> >>  void ovsdb_idl_force_reconnect(struct ovsdb_idl *);
> >>  void ovsdb_idl_verify_write_only(struct ovsdb_idl *);
> >> +
> >> +bool ovsdb_idl_is_alive(const struct ovsdb_idl *);
> >> +int ovsdb_idl_get_last_error(const struct ovsdb_idl *);
> >>
> >>  /* Choosing columns and tables to replicate. */
> >>
> >> diff --git a/lib/reconnect.c b/lib/reconnect.c
> >> index 40cc7fc..b914ef6 100644
> >> --- a/lib/reconnect.c
> >> +++ b/lib/reconnect.c
> >> @@ -1,5 +1,5 @@
> >>  /*
> >> - * Copyright (c) 2008, 2009, 2010, 2012 Nicira, Inc.
> >> + * Copyright (c) 2008, 2009, 2010, 2012, 2013 Nicira, Inc.
> >>   *
> >>   * Licensed under the Apache License, Version 2.0 (the "License");
> >>   * you may not use this file except in compliance with the License.
> >> @@ -207,7 +207,8 @@ reconnect_get_max_tries(struct reconnect *fsm)
> >>
> >>  /* Configures the backoff parameters for 'fsm'.  'min_backoff' is the
> minimum
> >>   * number of milliseconds, and 'max_backoff' is the maximum, between
> connection
> >> - * attempts.
> >> + * attempts.  The current backoff is also the duration that 'fsm' is
> willing to
> >> + * wait for a given connection to succeed or fail.
> >>   *
> >>   * 'min_backoff' must be at least 1000, and 'max_backoff' must be
> greater than
> >>   * or equal to 'min_backoff'.
> >> diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
> >> index 16c4a76..6e0a6f6 100644
> >> --- a/ovsdb/jsonrpc-server.c
> >> +++ b/ovsdb/jsonrpc-server.c
> >> @@ -210,7 +210,7 @@ ovsdb_jsonrpc_server_add_remote(struct
> ovsdb_jsonrpc_server *svr,
> >>      shash_add(&svr->remotes, name, remote);
> >>
> >>      if (!listener) {
> >> -        ovsdb_jsonrpc_session_create(remote,
> jsonrpc_session_open(name));
> >> +        ovsdb_jsonrpc_session_create(remote,
> jsonrpc_session_open(name, true));
> >>      }
> >>      return remote;
> >>  }
> >> diff --git a/tests/interface-reconfigure.at b/tests/
> interface-reconfigure.at
> >> index 59507ff..fdfe12e 100644
> >> --- a/tests/interface-reconfigure.at
> >> +++ b/tests/interface-reconfigure.at
> >> @@ -709,7 +709,7 @@ configure_datapath: bridge      - xenbr2
> >>  configure_datapath: physical    - [u'eth2']
> >>  configure_datapath: extra ports - []
> >>  configure_datapath: extra bonds - []
> >> -/usr/bin/ovs-vsctl --timeout=5 -vconsole:off get-fail-mode xenbr2
> >> +/usr/bin/ovs-vsctl -vconsole:off get-fail-mode xenbr2
> >>  Applying changes to /etc/sysconfig/network-scripts/route-xenbr2
> configuration
> >>  Applying changes to /etc/sysconfig/network configuration
> >>  Applying changes to /etc/sysconfig/network-scripts/ifcfg-xenbr2
> configuration
> >> @@ -724,7 +724,7 @@ Applying changes to
> /etc/sysconfig/network-scripts/ifcfg-xenbr2 configuration
> >>      set Bridge xenbr2 fail_mode=secure
> >>      remove Bridge xenbr2 other_config disable-in-band
> >>      br-set-external-id xenbr2 xs-network-uuids
> d08c8749-0c8f-9e8d-ce25-fd364661ee99
> >> -/usr/bin/ovs-vsctl --timeout=5 -vconsole:off get interface eth2 ofport
> >> +/usr/bin/ovs-vsctl -vconsole:off get interface eth2 ofport
> >>  /usr/bin/ovs-ofctl add-flow xenbr2
> idle_timeout=0,priority=0,in_port=5,arp,nw_proto=1,actions=local
> >>  /usr/bin/ovs-ofctl add-flow xenbr2
> idle_timeout=0,priority=0,in_port=local,arp,dl_src=00:15:17:a0:29:80,actions=5
> >>  /usr/bin/ovs-ofctl add-flow xenbr2
> idle_timeout=0,priority=0,in_port=5,dl_dst=00:15:17:a0:29:80,actions=local
> >> diff --git a/tests/ovs-monitor-ipsec.at b/tests/ovs-monitor-ipsec.at
> >> index e66c943..bd150cf 100644
> >> --- a/tests/ovs-monitor-ipsec.at
> >> +++ b/tests/ovs-monitor-ipsec.at
> >> @@ -33,7 +33,7 @@ chmod +x usr/sbin/setkey
> >>  touch etc/racoon/certs/ovs-stale.pem
> >>
> >>  ovs_vsctl () {
> >> -    ovs-vsctl --timeout=5 --no-wait -vreconnect:emer --db=unix:socket
> "$@"
> >> +    ovs-vsctl --no-wait -vreconnect:emer --db=unix:socket "$@"
> >>  }
> >>  trim () {  # Removes blank lines and lines starting with # from input.
> >>      sed -e '/^#/d' -e '/^[       ]*$/d' "$@"
> >> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> >> index 326d5a4..c5c3f47 100644
> >> --- a/tests/ovs-vsctl.at
> >> +++ b/tests/ovs-vsctl.at
> >> @@ -15,17 +15,17 @@ dnl RUN_OVS_VSCTL(COMMAND, ...)
> >>  dnl
> >>  dnl Executes each ovs-vsctl COMMAND.
> >>  m4_define([RUN_OVS_VSCTL],
> >> -  [m4_foreach([command], [$@], [ovs-vsctl --timeout=5 --no-wait
> -vreconnect:emer --db=unix:socket command
> >> +  [m4_foreach([command], [$@], [ovs-vsctl --no-wait -vreconnect:emer
> --db=unix:socket command
> >>  ])])
> >>  m4_define([RUN_OVS_VSCTL_ONELINE],
> >> -  [m4_foreach([command], [$@], [ovs-vsctl --timeout=5 --no-wait
> -vreconnect:emer --db=unix:socket --oneline -- command
> >> +  [m4_foreach([command], [$@], [ovs-vsctl --no-wait -vreconnect:emer
> --db=unix:socket --oneline -- command
> >>  ])])
> >>
> >>  dnl RUN_OVS_VSCTL_TOGETHER(COMMAND, ...)
> >>  dnl
> >>  dnl Executes each ovs-vsctl COMMAND in a single run of ovs-vsctl.
> >>  m4_define([RUN_OVS_VSCTL_TOGETHER],
> >> -  [ovs-vsctl --timeout=5 --no-wait -vreconnect:emer --db=unix:socket
> --oneline dnl
> >> +  [ovs-vsctl --no-wait -vreconnect:emer --db=unix:socket --oneline dnl
> >>  m4_foreach([command], [$@], [ -- command])])
> >>
> >>  dnl CHECK_BRIDGES([BRIDGE, PARENT, VLAN], ...)
> >> @@ -140,6 +140,40 @@ m4_define([CHECK_IFACES],
> >>                 [], [OVS_VSCTL_CLEANUP])])])
> >>
> >>  dnl
> ----------------------------------------------------------------------
> >> +AT_BANNER([ovs-vsctl unit tests])
> >> +
> >> +AT_SETUP([ovs-vsctl connection retry])
> >> +OVS_RUNDIR=$PWD; export OVS_RUNDIR
> >> +
> >> +dnl Without --retry, there should be no retry for active connections.
> >> +AT_CHECK([ovs-vsctl --db=unix:foo --timeout=10 -vreconnect:emer --
> init],
> >> +  [1], [], [stderr])
> >> +AT_CHECK([[sed 's/([^()]*)/(...reason...)/' stderr]], [0],
> >> +  [ovs-vsctl: unix:foo: database connection failed (...reason...)
> >> +])
> >> +
> >> +dnl With --retry, we should retry for active connections.
> >> +AT_CHECK(
> >> +  [ovs-vsctl --db=unix:foo --timeout=1 --retry -vreconnect:emer
> -vPATTERN:console:'%c|%p|%m' -- init
> >> +   echo $? > status],
> >> +  [0], [], [stderr])
> >> +AT_CHECK([grep -c 'terminating with signal' stderr], [0], [1
> >> +])
> >> +AT_CHECK([kill -l `cat status`], [0], [ALRM
> >> +])
> >> +
> >> +dnl Without --retry, we should retry for passive connections.
> >> +AT_CHECK(
> >> +  [ovs-vsctl --db=punix:foo --timeout=1 -vreconnect:emer
> -vPATTERN:console:'%c|%p|%m' -- init
> >> +   echo $? > status],
> >> +  [0], [], [stderr])
> >> +AT_CHECK([grep -c 'terminating with signal' stderr], [0], [1
> >> +])
> >> +AT_CHECK([kill -l `cat status`], [0], [ALRM
> >> +])
> >> +AT_CLEANUP
> >> +
> >> +dnl
> ----------------------------------------------------------------------
> >>  AT_BANNER([ovs-vsctl unit tests -- real bridges])
> >>
> >>  AT_SETUP([add-br a])
> >> @@ -833,7 +867,7 @@ AT_CHECK(
> >>
> >>  ])
> >>  m4_define([VSCTL_CHECK_FIND],
> >> -  [AT_CHECK([echo `ovs-vsctl --bare --timeout=5 --no-wait
> -vreconnect:emer --db=unix:socket -- --columns=name find bridge '$1' |
> sort`], [0], [$2
> >> +  [AT_CHECK([echo `ovs-vsctl --bare --no-wait -vreconnect:emer
> --db=unix:socket -- --columns=name find bridge '$1' | sort`], [0], [$2
> >>  ])])
> >>
> >>  # Arithmetic relational operators without keys.
> >> @@ -1044,19 +1078,19 @@ AT_SETUP([unreferenced record warnings])
> >>  AT_KEYWORDS([ovs-vsctl])
> >>  OVS_VSCTL_SETUP
> >>  AT_CHECK(
> >> -  [ovs-vsctl -vPATTERN:console:'%c|%p|%m' --timeout=5 --no-wait
> -vreconnect:emer --db=unix:socket \
> >> +  [ovs-vsctl -vPATTERN:console:'%c|%p|%m' --no-wait -vreconnect:emer
> --db=unix:socket \
> >>       -- create Bridge name=br0 | $srcdir/uuidfilt.pl],
> >>    [0], [<0>
> >>  ], [vsctl|WARN|applying "create" command to table Bridge without --id
> option will have no effect
> >>  ], [OVS_VSCTL_CLEANUP])
> >>  AT_CHECK(
> >> -  [ovs-vsctl -vPATTERN:console:'%c|%p|%m' --timeout=5 --no-wait
> -vreconnect:emer --db=unix:socket \
> >> +  [ovs-vsctl -vPATTERN:console:'%c|%p|%m' --no-wait -vreconnect:emer
> --db=unix:socket \
> >>       -- --id=@br0 create Bridge name=br0 | $srcdir/uuidfilt.pl],
> >>    [0], [<0>
> >>  ], [vsctl|WARN|row id "@br0" was created but no reference to it was
> inserted, so it will not actually appear in the database
> >>  ], [OVS_VSCTL_CLEANUP])
> >>  AT_CHECK(
> >> -  [ovs-vsctl -vPATTERN:console:'%c|%p|%m' --timeout=5 --no-wait
> -vreconnect:emer --db=unix:socket \
> >> +  [ovs-vsctl -vPATTERN:console:'%c|%p|%m' --no-wait -vreconnect:emer
> --db=unix:socket \
> >>       -- --id=@eth0_iface create Interface name=eth0 \
> >>       -- --id=@eth0 create Port name=eth0 interfaces=@eth0_iface \
> >>       -- --id=@m0 create Mirror name=m0 output_port=@eth0 \
> >> diff --git a/tests/ovs-xapi-sync.at b/tests/ovs-xapi-sync.at
> >> index 29d4737..b55eecd 100644
> >> --- a/tests/ovs-xapi-sync.at
> >> +++ b/tests/ovs-xapi-sync.at
> >> @@ -22,7 +22,7 @@ mkdir var var/run
> >>  touch var/run/xapi_init_complete.cookie
> >>
> >>  ovs_vsctl () {
> >> -    ovs-vsctl --timeout=5 --no-wait -vreconnect:emer --db=unix:socket
> "$@"
> >> +    ovs-vsctl --no-wait -vreconnect:emer --db=unix:socket "$@"
> >>  }
> >>
> >>  # Start ovsdb-server.
> >> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
> >> index d448109..81b5f9f 100644
> >> --- a/tests/test-ovsdb.c
> >> +++ b/tests/test-ovsdb.c
> >> @@ -1,5 +1,5 @@
> >>  /*
> >> - * Copyright (c) 2009, 2010, 2011, 2012 Nicira, Inc.
> >> + * Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> >>   *
> >>   * Licensed under the Apache License, Version 2.0 (the "License");
> >>   * you may not use this file except in compliance with the License.
> >> @@ -1880,7 +1880,7 @@ do_idl(int argc, char *argv[])
> >>
> >>      idltest_init();
> >>
> >> -    idl = ovsdb_idl_create(argv[1], &idltest_idl_class, true);
> >> +    idl = ovsdb_idl_create(argv[1], &idltest_idl_class, true, true);
> >>      if (argc > 2) {
> >>          struct stream *stream;
> >>
> >> diff --git a/utilities/bugtool/ovs-bugtool-vsctl-show
> b/utilities/bugtool/ovs-bugtool-vsctl-show
> >> index fe433d8..26e46e5 100755
> >> --- a/utilities/bugtool/ovs-bugtool-vsctl-show
> >> +++ b/utilities/bugtool/ovs-bugtool-vsctl-show
> >> @@ -14,6 +14,6 @@
> >>  # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
> >>  # USA
> >>  #
> >> -# Copyright (C) 2012 Nicira, Inc.
> >> +# Copyright (C) 2012, 2013 Nicira, Inc.
> >>
> >> -ovs-vsctl --timeout=5 show
> >> +ovs-vsctl show
> >> diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
> >> index 6bad187..5ad5f26 100755
> >> --- a/utilities/ovs-ctl.in
> >> +++ b/utilities/ovs-ctl.in
> >> @@ -57,7 +57,7 @@ insert_mod_if_required () {
> >>  }
> >>
> >>  ovs_vsctl () {
> >> -    ovs-vsctl --no-wait --timeout=5 "$@"
> >> +    ovs-vsctl --no-wait "$@"
> >>  }
> >>
> >>  ovsdb_tool () {
> >> diff --git a/utilities/ovs-save b/utilities/ovs-save
> >> index 16ac879..b46f98d 100755
> >> --- a/utilities/ovs-save
> >> +++ b/utilities/ovs-save
> >> @@ -1,6 +1,6 @@
> >>  #! /bin/sh
> >>
> >> -# Copyright (c) 2011 Nicira, Inc.
> >> +# Copyright (c) 2011, 2013 Nicira, Inc.
> >>  #
> >>  # Licensed under the Apache License, Version 2.0 (the "License");
> >>  # you may not use this file except in compliance with the License.
> >> @@ -177,7 +177,7 @@ save_flows () {
> >>  }
> >>
> >>  ovs_vsctl () {
> >> -    ovs-vsctl --no-wait --timeout=1 "$@"
> >> +    ovs-vsctl --no-wait "$@"
> >>  }
> >>
> >>  save_ofports ()
> >> @@ -191,7 +191,7 @@ save_ofports ()
> >>          count=0
> >>          for iface in `ovs_vsctl list-ifaces ${bridge}`; do
> >>              ofport=`ovs_vsctl get interface ${iface} ofport`
> >> -            [ "${count}" -eq 0 ] && cmd="ovs-vsctl --no-wait
> --timeout=1"
> >> +            [ "${count}" -eq 0 ] && cmd="ovs-vsctl --no-wait"
> >>              cmd="${cmd} -- --if-exists set interface "${iface}" \
> >>                       ofport_request="${ofport}""
> >>
> >> diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
> >> index cf6cf88..85149a9 100644
> >> --- a/utilities/ovs-vsctl.8.in
> >> +++ b/utilities/ovs-vsctl.8.in
> >> @@ -127,6 +127,20 @@ to approximately \fIsecs\fR seconds.  If the
> timeout expires,
> >>  would normally happen only if the database cannot be contacted, or if
> >>  the system is overloaded.)
> >>  .
> >> +.IP "\fB\-\-retry\fR"
> >> +Without this option, if \fBovs\-vsctl\fR connects outward to the
> >> +database server (the default) then \fBovs\-vsctl\fR will try to
> >> +connect once and exit with an error if the connection fails (which
> >> +usually means that \fBovsdb\-server\fR is not running).
> >> +.IP
> >> +With this option, or if \fB\-\-db\fR specifies that \fBovs\-vsctl\fR
> >> +should listen for an incoming connection from the database server,
> >> +then \fBovs\-vsctl\fR will wait for a connection to the database
> >> +forever.
> >> +.IP
> >> +Regardless of this setting, \fB\-\-timeout\fR always limits how long
> >> +\fBovs\-vsctl\fR will wait.
> >> +.
> >>  .SS "Table Formatting Options"
> >>  These options control the format of output from the \fBlist\fR and
> >>  \fBfind\fR commands.
> >> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> >> index 1ba8588..5388f3d 100644
> >> --- a/utilities/ovs-vsctl.c
> >> +++ b/utilities/ovs-vsctl.c
> >> @@ -114,6 +114,16 @@ static bool wait_for_reload = true;
> >>  /* --timeout: Time to wait for a connection to 'db'. */
> >>  static int timeout;
> >>
> >> +/* --retry: If true, ovs-vsctl will retry connecting to the database
> forever.
> >> + * If false and --db says to use an active connection method (e.g.
> "unix:",
> >> + * "tcp:", "ssl:"), then ovs-vsctl will try to connect once and exit
> with an
> >> + * error if the database server cannot be contacted (e.g. ovsdb-server
> is not
> >> + * running).
> >> + *
> >> + * Regardless of this setting, --timeout always limits how long
> ovs-vsctl will
> >> + * wait. */
> >> +static bool retry;
> >> +
> >>  /* Format for table output. */
> >>  static struct table_style table_style = TABLE_STYLE_DEFAULT;
> >>
> >> @@ -186,7 +196,7 @@ main(int argc, char *argv[])
> >>      }
> >>
> >>      /* Initialize IDL. */
> >> -    idl = the_idl = ovsdb_idl_create(db, &ovsrec_idl_class, false);
> >> +    idl = the_idl = ovsdb_idl_create(db, &ovsrec_idl_class, false,
> retry);
> >>      run_prerequisites(commands, n_commands, idl);
> >>
> >>      /* Execute the commands.
> >> @@ -199,6 +209,11 @@ main(int argc, char *argv[])
> >>      seqno = ovsdb_idl_get_seqno(idl);
> >>      for (;;) {
> >>          ovsdb_idl_run(idl);
> >> +        if (!ovsdb_idl_is_alive(idl)) {
> >> +            int retval = ovsdb_idl_get_last_error(idl);
> >> +            vsctl_fatal("%s: database connection failed (%s)",
> >> +                        db, ovs_retval_to_string(retval));
> >> +        }
> >>
> >>          if (seqno != ovsdb_idl_get_seqno(idl)) {
> >>              seqno = ovsdb_idl_get_seqno(idl);
> >> @@ -247,6 +262,7 @@ parse_options(int argc, char *argv[], struct shash
> *local_options)
> >>          OPT_DRY_RUN,
> >>          OPT_PEER_CA_CERT,
> >>          OPT_LOCAL,
> >> +        OPT_RETRY,
> >>          VLOG_OPTION_ENUMS,
> >>          TABLE_OPTION_ENUMS
> >>      };
> >> @@ -257,6 +273,7 @@ parse_options(int argc, char *argv[], struct shash
> *local_options)
> >>          {"dry-run", no_argument, NULL, OPT_DRY_RUN},
> >>          {"oneline", no_argument, NULL, OPT_ONELINE},
> >>          {"timeout", required_argument, NULL, 't'},
> >> +        {"retry", no_argument, NULL, OPT_RETRY},
> >>          {"help", no_argument, NULL, 'h'},
> >>          {"version", no_argument, NULL, 'V'},
> >>          VLOG_LONG_OPTIONS,
> >> @@ -384,6 +401,10 @@ parse_options(int argc, char *argv[], struct shash
> *local_options)
> >>              }
> >>              break;
> >>
> >> +        case OPT_RETRY:
> >> +            retry = true;
> >> +            break;
> >> +
> >>          VLOG_OPTION_HANDLERS
> >>          TABLE_OPTION_HANDLERS(&table_style)
> >>
> >> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> >> index 1fd431c..9449879 100644
> >> --- a/vswitchd/bridge.c
> >> +++ b/vswitchd/bridge.c
> >> @@ -318,7 +318,7 @@ void
> >>  bridge_init(const char *remote)
> >>  {
> >>      /* Create connection to database. */
> >> -    idl = ovsdb_idl_create(remote, &ovsrec_idl_class, true);
> >> +    idl = ovsdb_idl_create(remote, &ovsrec_idl_class, true, true);
> >>      idl_seqno = ovsdb_idl_get_seqno(idl);
> >>      ovsdb_idl_set_lock(idl, "ovs_vswitchd");
> >>      ovsdb_idl_verify_write_only(idl);
> >> diff --git a/xenserver/etc_xapi.d_plugins_openvswitch-cfg-update
> b/xenserver/etc_xapi.d_plugins_openvswitch-cfg-update
> >> index 02927f8..2df838a 100755
> >> --- a/xenserver/etc_xapi.d_plugins_openvswitch-cfg-update
> >> +++ b/xenserver/etc_xapi.d_plugins_openvswitch-cfg-update
> >> @@ -4,7 +4,7 @@
> >>  # ovs-vswitchd configuration that are managed in the xapi database when
> >>  # integrated with Citrix management tools.
> >>
> >> -# Copyright (C) 2009, 2010, 2011, 2012 Nicira, Inc.
> >> +# Copyright (C) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> >>  #
> >>  # Licensed under the Apache License, Version 2.0 (the "License");
> >>  # you may not use this file except in compliance with the License.
> >> @@ -213,7 +213,7 @@ def setControllerCfg(controller):
> >>                     "--", "set-manager", 'ssl:' + controller + ':6632'])
> >>
> >>  def vswitchCfgQuery(action_args):
> >> -    cmd = [vsctl, "--timeout=5", "-vconsole:off"] + action_args
> >> +    cmd = [vsctl, "-vconsole:off"] + action_args
> >>      output = subprocess.Popen(cmd,
> stdout=subprocess.PIPE).communicate()
> >>      if len(output) == 0 or output[0] == None:
> >>          output = ""
> >> diff --git
> a/xenserver/opt_xensource_libexec_InterfaceReconfigureVswitch.py
> b/xenserver/opt_xensource_libexec_InterfaceReconfigureVswitch.py
> >> index 971f918..1379fb4 100644
> >> --- a/xenserver/opt_xensource_libexec_InterfaceReconfigureVswitch.py
> >> +++ b/xenserver/opt_xensource_libexec_InterfaceReconfigureVswitch.py
> >> @@ -1,5 +1,5 @@
> >>  # Copyright (c) 2008,2009,2011 Citrix Systems, Inc.
> >> -# Copyright (c) 2009,2010,2011,2012 Nicira, Inc.
> >> +# Copyright (c) 2009,2010,2011,2012,2013 Nicira, Inc.
> >>  #
> >>  # This program is free software; you can redistribute it and/or modify
> >>  # it under the terms of the GNU Lesser General Public License as
> published
> >> @@ -721,7 +721,7 @@ class DatapathVswitch(Datapath):
> >>
> >>  def vswitchCfgQuery(action_args):
> >>      cmd = ['%s/usr/bin/ovs-vsctl' % root_prefix(),
> >> -        '--timeout=5', '-vconsole:off'] + action_args
> >> +           '-vconsole:off'] + action_args
> >>      output = subprocess.Popen(cmd,
> stdout=subprocess.PIPE).communicate()
> >>      if len(output) == 0 or output[0] == None:
> >>          output = ""
> >> diff --git
> a/xenserver/usr_lib_xsconsole_plugins-base_XSFeatureVSwitch.py
> b/xenserver/usr_lib_xsconsole_plugins-base_XSFeatureVSwitch.py
> >> index f62eaa8..06a7a16 100644
> >> --- a/xenserver/usr_lib_xsconsole_plugins-base_XSFeatureVSwitch.py
> >> +++ b/xenserver/usr_lib_xsconsole_plugins-base_XSFeatureVSwitch.py
> >> @@ -1,4 +1,4 @@
> >> -# Copyright (c) 2007-2011 Citrix Systems Inc.
> >> +# Copyright (c) 2007-2011, 2013 Citrix Systems Inc.
> >>  # Copyright (c) 2009,2010,2011,2012 Nicira, Inc.
> > Not sure, but did not you have to increase year for Nicira, Inc. line?
> >>  #
> >>  # This program is free software; you can redistribute it and/or modify
> >> @@ -86,7 +86,7 @@ class VSwitchConfig:
> >>      @staticmethod
> >>      def Get(action):
> >>          try:
> >> -            arg = [vsctl, "--timeout=30", "-vconsole:off"] +
> action.split()
> >> +            arg = [vsctl, "-vconsole:off"] + action.split()
> >>              output = ShellPipe(arg).Stdout()
> >>          except StandardError, e:
> >>              XSLogError("config retrieval error: " + str(e))
> >> --
> >
> >
> > Perhaps I missed it, but did you add explanation for "--retry" flag to
> > usage()? I see documentation only in ovs-vsctl man page.
> >
> > I will try to apply this patch locally and test it out. It will take
> > another 30 minutes.
> >
> > Thanks,
> > Ansis
>
> I saw few other occurrences where ovs-vsctl is being called with
> --timeout, Though, I am not sure how safe or necessary it would be to
> remove --timeout everywhere at this time. Also I am wondering, if
> someone depended on default behaviour (without --retry flag) to be
> "retry forever". We will see this soon. Probably libvirt will be able
> to make use of this "retry once" approach.
>
> Besides those 3 small things I already commented this patch looks good to
> me!
>
> Thanks,
> Ansis
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130315/7697870f/attachment-0003.html>


More information about the dev mailing list