On Wed, Feb 24, 2016 at 7:34 PM, Huang, Lei <lhua...@ebay.com> wrote:
> Comments line.
>
> On Mon, Feb 22, 2016 at 12:50 AM, Huang Lei <148012...@qq.com> wrote:
>
>> From: Huang Lei <lhua...@ebay.com>
>>
>> For setting the inactivity probe interval of the json session to the OVN
>> southbound database.
>>
>> Signed-off-by: Huang Lei <lhua...@ebay.com>
>> ---
>> lib/jsonrpc.c | 4 +++-
>> lib/ovsdb-idl.c | 10 ++++++++++
>> lib/ovsdb-idl.h | 4 ++++
>> ovn/controller/ovn-controller.8.xml | 14 ++++++++++++++
>> ovn/controller/ovn-controller.c | 33
>> +++++++++++++++++++++++++++++++++
>> 5 files changed, 64 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/jsonrpc.c b/lib/jsonrpc.c
>> index 35428a6..0985d52 100644
>> --- a/lib/jsonrpc.c
>> +++ b/lib/jsonrpc.c
>> @@ -1147,7 +1147,9 @@ void
>> jsonrpc_session_set_probe_interval(struct jsonrpc_session *s,
>> int probe_interval)
>> {
>> - reconnect_set_probe_interval(s->reconnect, probe_interval);
>> + if
>> (stream_or_pstream_needs_probes(reconnect_get_name(s->reconnect))) {
>> + reconnect_set_probe_interval(s->reconnect, probe_interval);
>>
>
> This may cause subtle behavior changes to current
> jsonrpc_session_set_probe_interval() users (ovsdb-idl and jsonrpc-server).
> It may not matter
> much in practice, but would be nice not to introduce them, at least not
> within this patch.
>
> [HL] Is it ok to move the stream_or_pstream_needs_probes() check into
> ovsdb_idl_set_probe_interval(), or simply keep
> jsonrpc_session_set_probe_interval() unchanged? Any suggestion?
>
Personally, I'd do the following:
1. Rename current jsonrpc_session_set_probe_interval to
jsonrpc_session_set_probe_interval_raw(), and change all calling site to
use the new name
2. Reintroduce jsonrpc_session_set_probe_interval() as you have defined,
ovsdb_idl_set_probe_interval() are likely to be the only user for now.
On the other hand, keeping jsonrpc_session_set_probe_interval() as is can
be O.K. since It is simpler and it is consistent how managers table setting
are handled.
At any rate, just pick one and go with it. Ben said he will review this
patch. It is not a lot of code to switch around if he does not like what's
being submitted.
Please add your name to the AUTHORS file if this is your first submission
to OVS. Thanks.
+ }
>> }
>>
>> /* Sets the DSCP value used for 's''s connection to 'dscp'. If this is
>> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
>> index 4cb1c81..022f0cf 100644
>> --- a/lib/ovsdb-idl.c
>> +++ b/lib/ovsdb-idl.c
>> @@ -565,6 +565,16 @@ ovsdb_idl_get_last_error(const struct ovsdb_idl *idl)
>> {
>> return jsonrpc_session_get_last_error(idl->session);
>> }
>> +
>> +/* Sets the "probe interval" for 'idl->session' to 'probe_interval', in
>> + * milliseconds.
>> + */
>> +void
>> +ovsdb_idl_set_probe_interval(const struct ovsdb_idl *idl, int
>> probe_interval)
>> +{
>> + jsonrpc_session_set_probe_interval(idl->session, probe_interval);
>> +}
>> +
>>
>> static unsigned char *
>> ovsdb_idl_get_mode(struct ovsdb_idl *idl,
>> diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h
>> index 136c38c..96d9436 100644
>> --- a/lib/ovsdb-idl.h
>> +++ b/lib/ovsdb-idl.h
>> @@ -68,7 +68,11 @@ 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 *);
>> +
>> +void ovsdb_idl_set_probe_interval(const struct ovsdb_idl *, int
>> probe_interval);
>> +
>>
>> +
>> /* Choosing columns and tables to replicate. */
>>
>> /* Modes with which the IDL can monitor a column.
>> diff --git a/ovn/controller/ovn-controller.8.xml
>> b/ovn/controller/ovn-controller.8.xml
>> index b261af9..6e57fc6 100644
>> --- a/ovn/controller/ovn-controller.8.xml
>> +++ b/ovn/controller/ovn-controller.8.xml
>> @@ -100,6 +100,20 @@
>> </p>
>> </dd>
>>
>> + <dt><code>external_ids:ovn-remote-probe-interval</code></dt>
>> + <dd>
>> + <p>
>> + The inactivity probe interval of the connection to the OVN
>> database,
>> + in milliseconds.
>> + If the value is zero, it disables the connection keepalive
>> feature.
>> + </p>
>> +
>> + <p>
>> + If the value is nonzero, then it will be forced to a value of
>> + at least 1000 ms.
>> + </p>
>> + </dd>
>> +
>> <dt><code>external_ids:ovn-encap-type</code></dt>
>> <dd>
>> <p>
>> diff --git a/ovn/controller/ovn-controller.c
>> b/ovn/controller/ovn-controller.c
>> index 3638342..7a1b966 100644
>> --- a/ovn/controller/ovn-controller.c
>> +++ b/ovn/controller/ovn-controller.c
>> @@ -34,7 +34,9 @@
>> #include "poll-loop.h"
>> #include "fatal-signal.h"
>> #include "lib/hmap.h"
>> +#include "lib/ovsdb-idl.h"
>> #include "lib/vswitch-idl.h"
>> +#include "lib/util.h"
>> #include "smap.h"
>> #include "stream.h"
>> #include "stream-ssl.h"
>> @@ -198,6 +200,32 @@ get_ovnsb_remote(struct ovsdb_idl *ovs_idl)
>> }
>> }
>>
>> +/* Retrieves the OVN Southbound remote's json session probe interval
>> from the
>> + * "external-ids:ovn-remote-probe-interval" key in 'ovs_idl' and returns
>> it.
>> + *
>> + * This function must be called after get_ovnsb_remote().
>> + *
>> + */
>> +static bool
>> +get_ovnsb_remote_probe_interval(struct ovsdb_idl *ovs_idl, int * value)
>>
> No need for a space between "*" and value.
>
>> +{
>> + const struct ovsrec_open_vswitch *cfg =
>> ovsrec_open_vswitch_first(ovs_idl);
>> + if (cfg) {
>> + const char * probe_interval =
>> + smap_get(&cfg->external_ids,
>> "ovn-remote-probe-interval");
>> + if (probe_interval) {
>> + if (str_to_int(probe_interval, 10, value))
>> + {
>> + VLOG_INFO("OVN OVSDB remote probe interval is %d ms",
>> *value);
>> + return true;
>> + }
>> + }
>> + }
>> +
>> + return false;
>> +}
>> +
>> +
>>
> extra blank line.
>
>> int
>> main(int argc, char *argv[])
>> {
>> @@ -261,6 +289,11 @@ main(int argc, char *argv[])
>> ovsdb_idl_create(ovnsb_remote, &sbrec_idl_class, true, true));
>> ovsdb_idl_get_initial_snapshot(ovnsb_idl_loop.idl);
>>
>> + int probe_interval = 0;
>> + if (get_ovnsb_remote_probe_interval(ovs_idl_loop.idl,
>> &probe_interval)) {
>> + ovsdb_idl_set_probe_interval(ovnsb_idl_loop.idl, probe_interval);
>> + }
>> +
>> /* Initialize connection tracking zones. */
>> struct simap ct_zones = SIMAP_INITIALIZER(&ct_zones);
>> unsigned long ct_zone_bitmap[BITMAP_N_LONGS(MAX_CT_ZONES)];
>> --
>> 2.5.4 (Apple Git-61)
>>
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>>
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
