Re: [ovs-dev] [PATCH V4] ovn-controller: reload configured SB probe timer
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 > Date: Wed Mar 30 19:03:10 2016 -0700 > Signed-off-by: Nirapada Ghosh > +/* 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
Re: [ovs-dev] [PATCH V4] ovn-controller: reload configured SB probe timer
On 4/14/16, 3:24 PM, "dev on behalf of ngh...@us.ibm.com" < dev-boun...@openvswitch.org on behalf of 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. Author: Nirapada Ghosh Date: Wed Mar 30 19:03:10 2016 -0700 Signed-off-by: Nirapada Ghosh 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 val
Re: [ovs-dev] [PATCH V4] ovn-controller: reload configured SB probe timer
On 4/14/16, 3:24 PM, "dev on behalf of 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. > >Author: Nirapada Ghosh >Date: Wed Mar 30 19:03:10 2016 -0700 >Signed-off-by: Nirapada Ghosh > >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
[ovs-dev] [PATCH V4] ovn-controller: reload configured SB probe timer
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 Date: Wed Mar 30 19:03:10 2016 -0700 Signed-off-by: Nirapada Ghosh 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-prob