| Message ID | 1460618643-41761-1-git-send-email-nghosh@us.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
On 4/14/16, 3:24 PM, "dev on behalf of nghosh@us.ibm.com" <dev-bounces@openvswitch.org on behalf of nghosh@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. > >Author: Nirapada Ghosh <nghosh@us.ibm.com> >Date: Wed Mar 30 19:03:10 2016 -0700 >Signed-off-by: Nirapada Ghosh <nghosh@us.ibm.com> > >diff --git a/lib/rconn.c b/lib/rconn.c >index 6de4c63..54f3745 100644 >--- a/lib/rconn.c >+++ b/lib/rconn.c >@@ -339,6 +339,9 @@ rconn_connect(struct rconn *rc, const char *target, const char *name) > rconn_disconnect__(rc); > rconn_set_target__(rc, target, name); > rc->reliable = true; >+ if (!stream_or_pstream_needs_probes()) { stream_or_pstream_needs_probes() is called without argument ‘target’, compile gives a warning: warning: implicit declaration of function ` stream_or_stream_needs_probs’ … Header file stream.h should be included. >+ rc->probe_interval =0; Need a whitespace at right side of ‘=‘. >+ } > reconnect(rc); > ovs_mutex_unlock(&rc->mutex); > } >diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c >index 7c68c9d..92e9543 100644 >--- a/ovn/controller/ovn-controller.c >+++ b/ovn/controller/ovn-controller.c >@@ -56,6 +56,13 @@ static unixctl_cb_func ovn_controller_exit; > static unixctl_cb_func ct_zone_list; > > #define DEFAULT_BRIDGE_NAME "br-int" >+#define DEFAULT_PROBE_INTERVAL 5 >+ >+static void set_probe_timer_if_changed(const struct ovsrec_open_vswitch *cfg, >+ const struct ovsdb_idl *sb_idl); >+static bool extract_probe_timer(const struct ovsrec_open_vswitch *cfg, >+ char *key_name, >+ int *ret_value); > > static void parse_options(int argc, char *argv[]); > OVS_NO_RETURN static void usage(void); >@@ -217,32 +224,6 @@ 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) >-{ >- const struct ovsrec_open_vswitch *cfg = ovsrec_open_vswitch_first(ovs_idl); >- if (!cfg) { >- return false; >- } >- >- const char *probe_interval = >- smap_get(&cfg->external_ids, "ovn-remote-probe-interval"); >- if (probe_interval) { >- if (str_to_int(probe_interval, 10, value)) { >- return true; >- } >- >- VLOG_WARN("Invalid value for OVN remote probe interval: %s", >- probe_interval); >- } >- >- return false; >-} >- > int > main(int argc, char *argv[]) > { >@@ -306,10 +287,12 @@ 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); >- } >+ const struct ovsrec_open_vswitch *cfg = >+ ovsrec_open_vswitch_first(ovs_idl_loop.idl); >+ if (!cfg) { >+ return false; >+ } >+ set_probe_timer_if_changed(cfg,ovnsb_idl_loop.idl); > > /* Initialize connection tracking zones. */ > struct simap ct_zones = SIMAP_INITIALIZER(&ct_zones); >@@ -327,6 +310,7 @@ main(int argc, char *argv[]) > .ovnsb_idl = ovnsb_idl_loop.idl, > .ovnsb_idl_txn = ovsdb_idl_loop_run(&ovnsb_idl_loop), > }; >+ set_probe_timer_if_changed(cfg,ovnsb_idl_loop.idl); Need a whitespace after comma. > > /* Contains "struct local_datpath" nodes whose hash values are the > * tunnel_key of datapaths with at least one local port binding. */ >@@ -561,3 +545,55 @@ ct_zone_list(struct unixctl_conn *conn, int argc OVS_UNUSED, > unixctl_command_reply(conn, ds_cstr(&ds)); > ds_destroy(&ds); > } >+ >+/* 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); >+ 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. Above style of the param doc strings is not found in other code in ovs, you may need to change it. Not sure, just look around and find a suitable way ;) >+ */ >+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); >+ 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. */ Wrap ‘=‘ with whitespace :) >+ 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; >+} Have tested the patch, works very well. BR Huang Lei > >_______________________________________________ >dev mailing list >dev@openvswitch.org >http://openvswitch.org/mailman/listinfo/dev
On Thu, Apr 14, 2016 at 12:24:03AM -0700, nghosh@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 <nghosh@us.ibm.com> > Date: Wed Mar 30 19:03:10 2016 -0700 > Signed-off-by: Nirapada Ghosh <nghosh@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.
diff --git a/lib/rconn.c b/lib/rconn.c index 6de4c63..54f3745 100644 --- a/lib/rconn.c +++ b/lib/rconn.c @@ -339,6 +339,9 @@ rconn_connect(struct rconn *rc, const char *target, const char *name) rconn_disconnect__(rc); rconn_set_target__(rc, target, name); rc->reliable = true; + if (!stream_or_pstream_needs_probes()) { + rc->probe_interval =0; + } reconnect(rc); ovs_mutex_unlock(&rc->mutex); } diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c index 7c68c9d..92e9543 100644 --- a/ovn/controller/ovn-controller.c +++ b/ovn/controller/ovn-controller.c @@ -56,6 +56,13 @@ static unixctl_cb_func ovn_controller_exit; static unixctl_cb_func ct_zone_list; #define DEFAULT_BRIDGE_NAME "br-int" +#define DEFAULT_PROBE_INTERVAL 5 + +static void set_probe_timer_if_changed(const struct ovsrec_open_vswitch *cfg, + const struct ovsdb_idl *sb_idl); +static bool extract_probe_timer(const struct ovsrec_open_vswitch *cfg, + char *key_name, + int *ret_value); static void parse_options(int argc, char *argv[]); OVS_NO_RETURN static void usage(void); @@ -217,32 +224,6 @@ 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) -{ - const struct ovsrec_open_vswitch *cfg = ovsrec_open_vswitch_first(ovs_idl); - if (!cfg) { - return false; - } - - const char *probe_interval = - smap_get(&cfg->external_ids, "ovn-remote-probe-interval"); - if (probe_interval) { - if (str_to_int(probe_interval, 10, value)) { - return true; - } - - VLOG_WARN("Invalid value for OVN remote probe interval: %s", - probe_interval); - } - - return false; -} - int main(int argc, char *argv[]) { @@ -306,10 +287,12 @@ 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); - } + const struct ovsrec_open_vswitch *cfg = + ovsrec_open_vswitch_first(ovs_idl_loop.idl); + if (!cfg) { + return false; + } + set_probe_timer_if_changed(cfg,ovnsb_idl_loop.idl); /* Initialize connection tracking zones. */ struct simap ct_zones = SIMAP_INITIALIZER(&ct_zones); @@ -327,6 +310,7 @@ main(int argc, char *argv[]) .ovnsb_idl = ovnsb_idl_loop.idl, .ovnsb_idl_txn = ovsdb_idl_loop_run(&ovnsb_idl_loop), }; + set_probe_timer_if_changed(cfg,ovnsb_idl_loop.idl); /* Contains "struct local_datpath" nodes whose hash values are the * tunnel_key of datapaths with at least one local port binding. */ @@ -561,3 +545,55 @@ ct_zone_list(struct unixctl_conn *conn, int argc OVS_UNUSED, unixctl_command_reply(conn, ds_cstr(&ds)); ds_destroy(&ds); } + +/* 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); + 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); + 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; +}
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. Author: Nirapada Ghosh <nghosh@us.ibm.com> Date: Wed Mar 30 19:03:10 2016 -0700 Signed-off-by: Nirapada Ghosh <nghosh@us.ibm.com>