On Fri, Mar 15, 2013 at 2:46 PM, Ansis Atteka <aatt...@nicira.com> wrote:
> On Fri, Mar 15, 2013 at 1:37 PM, Ben Pfaff <b...@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 <jmerr...@vmware.com>
>> Signed-off-by: Ben Pfaff <b...@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 jmed...@juniper.net
>> Janis Hamme janis.ha...@student.kit.edu
>> Jari Sundell sundell.softw...@gmail.com
>> Jed Daniels openvswi...@jeddaniels.com
>> +Jeff Merrick jmerr...@vmware.com
>> Jeongkeun Lee jk...@hp.com
>> Jing Ai ai_jing2...@hotmail.com
>> Joan Cirer j...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
