On Thu, Apr 14, 2016 at 12:24:03AM -0700, ngh...@us.ibm.com wrote:
> There are four sessions established from ovn-controller to the following:
> OVN Southbound — JSONRPC based
> Local ovsdb — JSONRPC based
> Local vswitchd — openflow based from ofctrl
> Local vswitchd — openflow based from pinctrl
>
> All of these sessions have their own probe_interval, and currently one
> [SB] of them can be configured using ovn-vsctl command, but that is not
> effective on the fly —in other words, ovn-controller has to be restarted to
> use that probe_timer value, this patch takes care of that.
> For the local ovsdb connection, probe timer is already disabled. For the last
> two connections, they do not need probe_timer as they are over unix domain
> socket. This patch takes care of that as well.
>
> This change has been tested putting logs in several places like in
> ovn-controller.c, lib/rconn.c to make sure the right probe_timer
> values are in effect. Also, by making sure from ovn-controller's
> log file that there is no more reconnect happening due to probe
> under heavy load.
I agree with Lei Huang's comments.
Fails to build:
../lib/rconn.c:342:10: error: implicit declaration of function
'stream_or_pstream_needs_probes' is invalid in C99
[-Werror,-Wimplicit-function-declaration]
In addition, please be careful about style issues. For example, there
should be a space on either side of "=", and a space after each ",".
Author and date should not be part of the commit message:
> Author: Nirapada Ghosh <ngh...@us.ibm.com>
> Date: Wed Mar 30 19:03:10 2016 -0700
> Signed-off-by: Nirapada Ghosh <ngh...@us.ibm.com>
> +/* If SB probe timer is changed using ovs-vsctl command, this function
> + * will set that probe timer value for the session.
> + * cfg: Holding the external-id values read from southbound DB.
> + * sb_idl: pointer to the ovs_idl connection to OVN southbound.
> + */
> +static void
> +set_probe_timer_if_changed(const struct ovsrec_open_vswitch *cfg,
> + const struct ovsdb_idl *sb_idl)
> +{
> + static int probe_int_sb = DEFAULT_PROBE_INTERVAL * 1000;
> + int probe_int_sb_new = probe_int_sb;
> +
> + extract_probe_timer(cfg, "ovn-remote-probe-interval", &probe_int_sb_new);
> + if (probe_int_sb_new != probe_int_sb) {
> + ovsdb_idl_set_probe_interval(sb_idl, probe_int_sb_new);
We generally don't log this kind of thing:
> + VLOG_INFO("OVN SB probe interval changed %d->%d ",
> + probe_int_sb,
> + probe_int_sb_new);
> + probe_int_sb = probe_int_sb_new;
> + }
> +}
> +
> +/* Given key_name, the following function retrieves probe_timer value from
> the
> + * configuration passed, this configuration comes from the "external-ids"
> + * which were configured via ovs-vsctl command.
> + *
> + * cfg: Holding the external-id values read from NB database.
> + * keyname: Name to extract the value for.
> + * ret_value_ptr: Pointer to integer location where the value read
> + * should be copied.
> + * The function returns true if success, keeps the original
> + * value of ret_value_ptr intact in case of a failure.
> + */
> +static bool
> +extract_probe_timer(const struct ovsrec_open_vswitch *cfg,
> + char *key_name,
> + int *ret_value_ptr)
> +{
> + const char *probe_interval= smap_get(&cfg->external_ids, key_name);
This comment seems unnecessary:
> + int ret_value_temp=0; /* Temporary location to hold the value, in case of
> + * failure, str_to_int() sets the ret_value to 0,
> + * which is a valid value of probe_timer. */
> + if ((!probe_interval) ||
> + (!str_to_int(probe_interval, 10, &ret_value_temp))) {
> + VLOG_WARN("OVN OVSDB invalid remote probe interval:%s for %s",
> + probe_interval, key_name);
> + return false;
> + }
> + *ret_value_ptr = ret_value_temp;
> + return true;
> +}
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
