| Message ID | 1578955945-19812-3-git-send-email-hzhou@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,1/3] ovn-controller.c: Fix possible NULL pointer dereference. | expand |
On Tue, Jan 14, 2020 at 4:22 AM Han Zhou <hzhou@ovn.org> wrote: > > Set this option to true will avoid using conditional monitoring in > ovn-controller. Setting it to true helps in environments where > all (or most) workloads need to be reachable from each other, thus > the effectiveness of conditional monitoring is low, but the overhead > of conditional monitoring is high. > > Requested-by: Girish Moodalbail <gmoodalbail@nvidia.com> > Signed-off-by: Han Zhou <hzhou@ovn.org> Hi Han, I am curious what happens when this option is toggled from true to false ? One comment/question below Thanks Numan > --- > controller/ovn-controller.8.xml | 21 ++++++++++++++++++++ > controller/ovn-controller.c | 44 +++++++++++++++++++++++++++++++++++------ > 2 files changed, 59 insertions(+), 6 deletions(-) > > diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml > index a226802..a163ff5 100644 > --- a/controller/ovn-controller.8.xml > +++ b/controller/ovn-controller.8.xml > @@ -98,6 +98,27 @@ > </p> > </dd> > > + <dt><code>external_ids:ovn-monitor-all</code></dt> > + <dd> > + <p> > + A boolean value that tells if <code>ovn-controller</code> should > + monitor all records of tables in <var>ovs-database</var>. If > + set to <code>false</code>, it will conditionally monitor the records > + that is needed in the current chassis. > + </p> > + <p> > + It is more optimal to set it to <code>true</code> in use cases when > + the chassis would anyway need to monitor most of the records in > + <var>ovs-database</var>, which would save the overhead of conditions > + processing, especially for server side. Typically, set it to > + <code>true</code> for environments that all workloads need to be > + reachable from each other. > + </p> > + <p> > + Default value is <var>false</var>. > + </p> > + </dd> > + > <dt><code>external_ids:ovn-remote-probe-interval</code></dt> > <dd> > <p> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index abb1b4c..ccd63b3 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -129,7 +129,8 @@ static void > update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > const struct sbrec_chassis *chassis, > const struct sset *local_ifaces, > - struct hmap *local_datapaths) > + struct hmap *local_datapaths, > + bool monitor_all) > { > /* Monitor Port_Bindings rows for local interfaces and local datapaths. > * > @@ -154,6 +155,19 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > struct ovsdb_idl_condition ce = OVSDB_IDL_CONDITION_INIT(&ce); > struct ovsdb_idl_condition ip_mcast = OVSDB_IDL_CONDITION_INIT(&ip_mcast); > struct ovsdb_idl_condition igmp = OVSDB_IDL_CONDITION_INIT(&igmp); > + > + if (monitor_all) { > + ovsdb_idl_condition_add_clause_true(&pb); > + ovsdb_idl_condition_add_clause_true(&lf); > + ovsdb_idl_condition_add_clause_true(&mb); > + ovsdb_idl_condition_add_clause_true(&mg); > + ovsdb_idl_condition_add_clause_true(&dns); > + ovsdb_idl_condition_add_clause_true(&ce); > + ovsdb_idl_condition_add_clause_true(&ip_mcast); > + ovsdb_idl_condition_add_clause_true(&igmp); > + goto out; > + } > + > sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "patch"); > /* XXX: We can optimize this, if we find a way to only monitor > * ports that have a Gateway_Chassis that point's to our own > @@ -205,6 +219,8 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > uuid); > } > } > + > +out: > sbrec_port_binding_set_condition(ovnsb_idl, &pb); > sbrec_logical_flow_set_condition(ovnsb_idl, &lf); > sbrec_mac_binding_set_condition(ovnsb_idl, &mb); > @@ -428,7 +444,8 @@ get_ofctrl_probe_interval(struct ovsdb_idl *ovs_idl) > /* Retrieves the pointer to the OVN Southbound database from 'ovs_idl' and > * updates 'sbdb_idl' with that pointer. */ > static void > -update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl) > +update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl, > + bool *monitor_all_p) > { > const struct ovsrec_open_vswitch *cfg = ovsrec_open_vswitch_first(ovs_idl); > if (!cfg) { > @@ -445,6 +462,19 @@ update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl) > int interval = smap_get_int(&cfg->external_ids, > "ovn-remote-probe-interval", default_interval); > ovsdb_idl_set_probe_interval(ovnsb_idl, interval); > + > + bool monitor_all = smap_get_bool(&cfg->external_ids, "ovn-monitor-all", > + false); > + if (monitor_all) { > + /* Always call update_sb_monitors when monitor_all is true. > + * Otherwise, don't call it here, because there would be unnecessary > + * extra cost. Instead, it is called after the engine execution only > + * when it is necessary. */ > + update_sb_monitors(ovnsb_idl, NULL, NULL, NULL, true); > + } > + if (monitor_all_p) { > + *monitor_all_p = monitor_all; > + } > } > > static void > @@ -1887,7 +1917,7 @@ main(int argc, char *argv[]) > ovsdb_idl_omit(ovnsb_idl_loop.idl, &sbrec_connection_col_status); > ovsdb_idl_omit(ovnsb_idl_loop.idl, &sbrec_connection_col_target); > > - update_sb_monitors(ovnsb_idl_loop.idl, NULL, NULL, NULL); > + update_sb_monitors(ovnsb_idl_loop.idl, NULL, NULL, NULL, false); > > stopwatch_create(CONTROLLER_LOOP_STOPWATCH_NAME, SW_MS); > > @@ -2009,6 +2039,7 @@ main(int argc, char *argv[]) > /* Main loop. */ > exiting = false; > restart = false; > + bool sb_monitor_all = false; > while (!exiting) { > engine_init_run(); > > @@ -2023,7 +2054,7 @@ main(int argc, char *argv[]) > ovs_cond_seqno = new_ovs_cond_seqno; > } > > - update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl); > + update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl, &sb_monitor_all); > update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl)); > ofctrl_set_probe_interval(get_ofctrl_probe_interval(ovs_idl_loop.idl)); > > @@ -2163,7 +2194,8 @@ main(int argc, char *argv[]) > if (engine_node_changed(&en_runtime_data)) { > update_sb_monitors(ovnsb_idl_loop.idl, chassis, > &runtime_data->local_lports, > - &runtime_data->local_datapaths); > + &runtime_data->local_datapaths, > + sb_monitor_all); If sb_monitor_all is true, do we need to call "update_sb_monitors() given that "update_sb_db() would have called at already ? > } > } > } > @@ -2272,7 +2304,7 @@ main(int argc, char *argv[]) > if (!restart) { > bool done = !ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl); > while (!done) { > - update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl); > + update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl, NULL); > update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl)); > > struct ovsdb_idl_txn *ovs_idl_txn > -- > 2.1.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Fri, Jan 24, 2020 at 1:24 AM Numan Siddique <numans@ovn.org> wrote: > > On Tue, Jan 14, 2020 at 4:22 AM Han Zhou <hzhou@ovn.org> wrote: > > > > Set this option to true will avoid using conditional monitoring in > > ovn-controller. Setting it to true helps in environments where > > all (or most) workloads need to be reachable from each other, thus > > the effectiveness of conditional monitoring is low, but the overhead > > of conditional monitoring is high. > > > > Requested-by: Girish Moodalbail <gmoodalbail@nvidia.com> > > Signed-off-by: Han Zhou <hzhou@ovn.org> > > Hi Han, > > I am curious what happens when this option is toggled from true to false ? Hi, Numan, Thanks for the review. Please see my response below. If toggle from true to false, the monitor condition will be updated to monitor only the interested rows whenever there is a change triggered in engine node runtime_data: if (engine_node_changed(&en_runtime_data)) { update_sb_monitors(ovnsb_idl_loop.idl, chassis, ... > One comment/question below > > Thanks > Numan > > > --- > > controller/ovn-controller.8.xml | 21 ++++++++++++++++++++ > > controller/ovn-controller.c | 44 +++++++++++++++++++++++++++++++++++------ > > 2 files changed, 59 insertions(+), 6 deletions(-) > > > > diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml > > index a226802..a163ff5 100644 > > --- a/controller/ovn-controller.8.xml > > +++ b/controller/ovn-controller.8.xml > > @@ -98,6 +98,27 @@ > > </p> > > </dd> > > > > + <dt><code>external_ids:ovn-monitor-all</code></dt> > > + <dd> > > + <p> > > + A boolean value that tells if <code>ovn-controller</code> should > > + monitor all records of tables in <var>ovs-database</var>. If > > + set to <code>false</code>, it will conditionally monitor the records > > + that is needed in the current chassis. > > + </p> > > + <p> > > + It is more optimal to set it to <code>true</code> in use cases when > > + the chassis would anyway need to monitor most of the records in > > + <var>ovs-database</var>, which would save the overhead of conditions > > + processing, especially for server side. Typically, set it to > > + <code>true</code> for environments that all workloads need to be > > + reachable from each other. > > + </p> > > + <p> > > + Default value is <var>false</var>. > > + </p> > > + </dd> > > + > > <dt><code>external_ids:ovn-remote-probe-interval</code></dt> > > <dd> > > <p> > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index abb1b4c..ccd63b3 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -129,7 +129,8 @@ static void > > update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > > const struct sbrec_chassis *chassis, > > const struct sset *local_ifaces, > > - struct hmap *local_datapaths) > > + struct hmap *local_datapaths, > > + bool monitor_all) > > { > > /* Monitor Port_Bindings rows for local interfaces and local datapaths. > > * > > @@ -154,6 +155,19 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > > struct ovsdb_idl_condition ce = OVSDB_IDL_CONDITION_INIT(&ce); > > struct ovsdb_idl_condition ip_mcast = OVSDB_IDL_CONDITION_INIT(&ip_mcast); > > struct ovsdb_idl_condition igmp = OVSDB_IDL_CONDITION_INIT(&igmp); > > + > > + if (monitor_all) { > > + ovsdb_idl_condition_add_clause_true(&pb); > > + ovsdb_idl_condition_add_clause_true(&lf); > > + ovsdb_idl_condition_add_clause_true(&mb); > > + ovsdb_idl_condition_add_clause_true(&mg); > > + ovsdb_idl_condition_add_clause_true(&dns); > > + ovsdb_idl_condition_add_clause_true(&ce); > > + ovsdb_idl_condition_add_clause_true(&ip_mcast); > > + ovsdb_idl_condition_add_clause_true(&igmp); > > + goto out; > > + } > > + > > sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "patch"); > > /* XXX: We can optimize this, if we find a way to only monitor > > * ports that have a Gateway_Chassis that point's to our own > > @@ -205,6 +219,8 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > > uuid); > > } > > } > > + > > +out: > > sbrec_port_binding_set_condition(ovnsb_idl, &pb); > > sbrec_logical_flow_set_condition(ovnsb_idl, &lf); > > sbrec_mac_binding_set_condition(ovnsb_idl, &mb); > > @@ -428,7 +444,8 @@ get_ofctrl_probe_interval(struct ovsdb_idl *ovs_idl) > > /* Retrieves the pointer to the OVN Southbound database from 'ovs_idl' and > > * updates 'sbdb_idl' with that pointer. */ > > static void > > -update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl) > > +update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl, > > + bool *monitor_all_p) > > { > > const struct ovsrec_open_vswitch *cfg = ovsrec_open_vswitch_first(ovs_idl); > > if (!cfg) { > > @@ -445,6 +462,19 @@ update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl) > > int interval = smap_get_int(&cfg->external_ids, > > "ovn-remote-probe-interval", default_interval); > > ovsdb_idl_set_probe_interval(ovnsb_idl, interval); > > + > > + bool monitor_all = smap_get_bool(&cfg->external_ids, "ovn-monitor-all", > > + false); > > + if (monitor_all) { > > + /* Always call update_sb_monitors when monitor_all is true. > > + * Otherwise, don't call it here, because there would be unnecessary > > + * extra cost. Instead, it is called after the engine execution only > > + * when it is necessary. */ > > + update_sb_monitors(ovnsb_idl, NULL, NULL, NULL, true); > > + } > > + if (monitor_all_p) { > > + *monitor_all_p = monitor_all; > > + } > > } > > > > static void > > @@ -1887,7 +1917,7 @@ main(int argc, char *argv[]) > > ovsdb_idl_omit(ovnsb_idl_loop.idl, &sbrec_connection_col_status); > > ovsdb_idl_omit(ovnsb_idl_loop.idl, &sbrec_connection_col_target); > > > > - update_sb_monitors(ovnsb_idl_loop.idl, NULL, NULL, NULL); > > + update_sb_monitors(ovnsb_idl_loop.idl, NULL, NULL, NULL, false); > > > > stopwatch_create(CONTROLLER_LOOP_STOPWATCH_NAME, SW_MS); > > > > @@ -2009,6 +2039,7 @@ main(int argc, char *argv[]) > > /* Main loop. */ > > exiting = false; > > restart = false; > > + bool sb_monitor_all = false; > > while (!exiting) { > > engine_init_run(); > > > > @@ -2023,7 +2054,7 @@ main(int argc, char *argv[]) > > ovs_cond_seqno = new_ovs_cond_seqno; > > } > > > > - update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl); > > + update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl, &sb_monitor_all); > > update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl)); > > ofctrl_set_probe_interval(get_ofctrl_probe_interval(ovs_idl_loop.idl)); > > > > @@ -2163,7 +2194,8 @@ main(int argc, char *argv[]) > > if (engine_node_changed(&en_runtime_data)) { > > update_sb_monitors(ovnsb_idl_loop.idl, chassis, > > &runtime_data->local_lports, > > - &runtime_data->local_datapaths); > > + &runtime_data->local_datapaths, > > + sb_monitor_all); > > If sb_monitor_all is true, do we need to call "update_sb_monitors() > given that "update_sb_db() would have called at already ? > If sb_monitor_all is true, calling it here results in a non-op. In update_sb_monitors() it just set all condition clauses to true. So to avoid extra if-else I'd just call it here. > > > } > > } > > } > > @@ -2272,7 +2304,7 @@ main(int argc, char *argv[]) > > if (!restart) { > > bool done = !ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl); > > while (!done) { > > - update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl); > > + update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl, NULL); > > update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl)); > > > > struct ovsdb_idl_txn *ovs_idl_txn > > -- > > 2.1.0 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Fri, Jan 24, 2020, 21:38 Han Zhou <hzhou@ovn.org> wrote: > On Fri, Jan 24, 2020 at 1:24 AM Numan Siddique <numans@ovn.org> wrote: > > > > On Tue, Jan 14, 2020 at 4:22 AM Han Zhou <hzhou@ovn.org> wrote: > > > > > > Set this option to true will avoid using conditional monitoring in > > > ovn-controller. Setting it to true helps in environments where > > > all (or most) workloads need to be reachable from each other, thus > > > the effectiveness of conditional monitoring is low, but the overhead > > > of conditional monitoring is high. > > > > > > Requested-by: Girish Moodalbail <gmoodalbail@nvidia.com> > > > Signed-off-by: Han Zhou <hzhou@ovn.org> > > > > Hi Han, > > > > I am curious what happens when this option is toggled from true to false > ? > > Hi, Numan, > > Thanks for the review. Please see my response below. > > If toggle from true to false, the monitor condition will be updated to > monitor only the interested rows whenever there is a change triggered in > engine node runtime_data: > if (engine_node_changed(&en_runtime_data)) { > update_sb_monitors(ovnsb_idl_loop.idl, > chassis, > ... > Thanks for the response. Acked-by: Numan Siddique <numans@ovn.org> Numan > > One comment/question below > > > > Thanks > > Numan > > > > > --- > > > controller/ovn-controller.8.xml | 21 ++++++++++++++++++++ > > > controller/ovn-controller.c | 44 > +++++++++++++++++++++++++++++++++++------ > > > 2 files changed, 59 insertions(+), 6 deletions(-) > > > > > > diff --git a/controller/ovn-controller.8.xml > b/controller/ovn-controller.8.xml > > > index a226802..a163ff5 100644 > > > --- a/controller/ovn-controller.8.xml > > > +++ b/controller/ovn-controller.8.xml > > > @@ -98,6 +98,27 @@ > > > </p> > > > </dd> > > > > > > + <dt><code>external_ids:ovn-monitor-all</code></dt> > > > + <dd> > > > + <p> > > > + A boolean value that tells if <code>ovn-controller</code> > should > > > + monitor all records of tables in <var>ovs-database</var>. > If > > > + set to <code>false</code>, it will conditionally monitor the > records > > > + that is needed in the current chassis. > > > + </p> > > > + <p> > > > + It is more optimal to set it to <code>true</code> in use > cases when > > > + the chassis would anyway need to monitor most of the records > in > > > + <var>ovs-database</var>, which would save the overhead of > conditions > > > + processing, especially for server side. Typically, set it > to > > > + <code>true</code> for environments that all workloads need > to be > > > + reachable from each other. > > > + </p> > > > + <p> > > > + Default value is <var>false</var>. > > > + </p> > > > + </dd> > > > + > > > <dt><code>external_ids:ovn-remote-probe-interval</code></dt> > > > <dd> > > > <p> > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > > index abb1b4c..ccd63b3 100644 > > > --- a/controller/ovn-controller.c > > > +++ b/controller/ovn-controller.c > > > @@ -129,7 +129,8 @@ static void > > > update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > > > const struct sbrec_chassis *chassis, > > > const struct sset *local_ifaces, > > > - struct hmap *local_datapaths) > > > + struct hmap *local_datapaths, > > > + bool monitor_all) > > > { > > > /* Monitor Port_Bindings rows for local interfaces and local > datapaths. > > > * > > > @@ -154,6 +155,19 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > > > struct ovsdb_idl_condition ce = OVSDB_IDL_CONDITION_INIT(&ce); > > > struct ovsdb_idl_condition ip_mcast = > OVSDB_IDL_CONDITION_INIT(&ip_mcast); > > > struct ovsdb_idl_condition igmp = OVSDB_IDL_CONDITION_INIT(&igmp); > > > + > > > + if (monitor_all) { > > > + ovsdb_idl_condition_add_clause_true(&pb); > > > + ovsdb_idl_condition_add_clause_true(&lf); > > > + ovsdb_idl_condition_add_clause_true(&mb); > > > + ovsdb_idl_condition_add_clause_true(&mg); > > > + ovsdb_idl_condition_add_clause_true(&dns); > > > + ovsdb_idl_condition_add_clause_true(&ce); > > > + ovsdb_idl_condition_add_clause_true(&ip_mcast); > > > + ovsdb_idl_condition_add_clause_true(&igmp); > > > + goto out; > > > + } > > > + > > > sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "patch"); > > > /* XXX: We can optimize this, if we find a way to only monitor > > > * ports that have a Gateway_Chassis that point's to our own > > > @@ -205,6 +219,8 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > > > uuid); > > > } > > > } > > > + > > > +out: > > > sbrec_port_binding_set_condition(ovnsb_idl, &pb); > > > sbrec_logical_flow_set_condition(ovnsb_idl, &lf); > > > sbrec_mac_binding_set_condition(ovnsb_idl, &mb); > > > @@ -428,7 +444,8 @@ get_ofctrl_probe_interval(struct ovsdb_idl > *ovs_idl) > > > /* Retrieves the pointer to the OVN Southbound database from 'ovs_idl' > and > > > * updates 'sbdb_idl' with that pointer. */ > > > static void > > > -update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl) > > > +update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl, > > > + bool *monitor_all_p) > > > { > > > const struct ovsrec_open_vswitch *cfg = > ovsrec_open_vswitch_first(ovs_idl); > > > if (!cfg) { > > > @@ -445,6 +462,19 @@ update_sb_db(struct ovsdb_idl *ovs_idl, struct > ovsdb_idl *ovnsb_idl) > > > int interval = smap_get_int(&cfg->external_ids, > > > "ovn-remote-probe-interval", > default_interval); > > > ovsdb_idl_set_probe_interval(ovnsb_idl, interval); > > > + > > > + bool monitor_all = smap_get_bool(&cfg->external_ids, > "ovn-monitor-all", > > > + false); > > > + if (monitor_all) { > > > + /* Always call update_sb_monitors when monitor_all is true. > > > + * Otherwise, don't call it here, because there would be > unnecessary > > > + * extra cost. Instead, it is called after the engine > execution only > > > + * when it is necessary. */ > > > + update_sb_monitors(ovnsb_idl, NULL, NULL, NULL, true); > > > + } > > > + if (monitor_all_p) { > > > + *monitor_all_p = monitor_all; > > > + } > > > } > > > > > > static void > > > @@ -1887,7 +1917,7 @@ main(int argc, char *argv[]) > > > ovsdb_idl_omit(ovnsb_idl_loop.idl, &sbrec_connection_col_status); > > > ovsdb_idl_omit(ovnsb_idl_loop.idl, &sbrec_connection_col_target); > > > > > > - update_sb_monitors(ovnsb_idl_loop.idl, NULL, NULL, NULL); > > > + update_sb_monitors(ovnsb_idl_loop.idl, NULL, NULL, NULL, false); > > > > > > stopwatch_create(CONTROLLER_LOOP_STOPWATCH_NAME, SW_MS); > > > > > > @@ -2009,6 +2039,7 @@ main(int argc, char *argv[]) > > > /* Main loop. */ > > > exiting = false; > > > restart = false; > > > + bool sb_monitor_all = false; > > > while (!exiting) { > > > engine_init_run(); > > > > > > @@ -2023,7 +2054,7 @@ main(int argc, char *argv[]) > > > ovs_cond_seqno = new_ovs_cond_seqno; > > > } > > > > > > - update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl); > > > + update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl, > &sb_monitor_all); > > > update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl)); > > > > ofctrl_set_probe_interval(get_ofctrl_probe_interval(ovs_idl_loop.idl)); > > > > > > @@ -2163,7 +2194,8 @@ main(int argc, char *argv[]) > > > if (engine_node_changed(&en_runtime_data)) { > > > update_sb_monitors(ovnsb_idl_loop.idl, > chassis, > > > > &runtime_data->local_lports, > > > - > &runtime_data->local_datapaths); > > > + > &runtime_data->local_datapaths, > > > + sb_monitor_all); > > > > If sb_monitor_all is true, do we need to call "update_sb_monitors() > > given that "update_sb_db() would have called at already ? > > > > If sb_monitor_all is true, calling it here results in a non-op. In > update_sb_monitors() it just set all condition clauses to true. So to avoid > extra if-else I'd just call it here. > > > > > > } > > > } > > > } > > > @@ -2272,7 +2304,7 @@ main(int argc, char *argv[]) > > > if (!restart) { > > > bool done = !ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl); > > > while (!done) { > > > - update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl); > > > + update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl, NULL); > > > update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl)); > > > > > > struct ovsdb_idl_txn *ovs_idl_txn > > > -- > > > 2.1.0 > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Fri, Jan 24, 2020 at 11:08 AM Numan Siddique <numans@ovn.org> wrote: > > > > On Fri, Jan 24, 2020, 21:38 Han Zhou <hzhou@ovn.org> wrote: >> >> On Fri, Jan 24, 2020 at 1:24 AM Numan Siddique <numans@ovn.org> wrote: >> > >> > On Tue, Jan 14, 2020 at 4:22 AM Han Zhou <hzhou@ovn.org> wrote: >> > > >> > > Set this option to true will avoid using conditional monitoring in >> > > ovn-controller. Setting it to true helps in environments where >> > > all (or most) workloads need to be reachable from each other, thus >> > > the effectiveness of conditional monitoring is low, but the overhead >> > > of conditional monitoring is high. >> > > >> > > Requested-by: Girish Moodalbail <gmoodalbail@nvidia.com> >> > > Signed-off-by: Han Zhou <hzhou@ovn.org> >> > >> > Hi Han, >> > >> > I am curious what happens when this option is toggled from true to false ? >> >> Hi, Numan, >> >> Thanks for the review. Please see my response below. >> >> If toggle from true to false, the monitor condition will be updated to >> monitor only the interested rows whenever there is a change triggered in >> engine node runtime_data: >> if (engine_node_changed(&en_runtime_data)) { >> update_sb_monitors(ovnsb_idl_loop.idl, >> chassis, >> ... > > > Thanks for the response. > > Acked-by: Numan Siddique <numans@ovn.org> > > Numan > >> Thanks Numan. I applied this to master.
diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml index a226802..a163ff5 100644 --- a/controller/ovn-controller.8.xml +++ b/controller/ovn-controller.8.xml @@ -98,6 +98,27 @@ </p> </dd> + <dt><code>external_ids:ovn-monitor-all</code></dt> + <dd> + <p> + A boolean value that tells if <code>ovn-controller</code> should + monitor all records of tables in <var>ovs-database</var>. If + set to <code>false</code>, it will conditionally monitor the records + that is needed in the current chassis. + </p> + <p> + It is more optimal to set it to <code>true</code> in use cases when + the chassis would anyway need to monitor most of the records in + <var>ovs-database</var>, which would save the overhead of conditions + processing, especially for server side. Typically, set it to + <code>true</code> for environments that all workloads need to be + reachable from each other. + </p> + <p> + Default value is <var>false</var>. + </p> + </dd> + <dt><code>external_ids:ovn-remote-probe-interval</code></dt> <dd> <p> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index abb1b4c..ccd63b3 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -129,7 +129,8 @@ static void update_sb_monitors(struct ovsdb_idl *ovnsb_idl, const struct sbrec_chassis *chassis, const struct sset *local_ifaces, - struct hmap *local_datapaths) + struct hmap *local_datapaths, + bool monitor_all) { /* Monitor Port_Bindings rows for local interfaces and local datapaths. * @@ -154,6 +155,19 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, struct ovsdb_idl_condition ce = OVSDB_IDL_CONDITION_INIT(&ce); struct ovsdb_idl_condition ip_mcast = OVSDB_IDL_CONDITION_INIT(&ip_mcast); struct ovsdb_idl_condition igmp = OVSDB_IDL_CONDITION_INIT(&igmp); + + if (monitor_all) { + ovsdb_idl_condition_add_clause_true(&pb); + ovsdb_idl_condition_add_clause_true(&lf); + ovsdb_idl_condition_add_clause_true(&mb); + ovsdb_idl_condition_add_clause_true(&mg); + ovsdb_idl_condition_add_clause_true(&dns); + ovsdb_idl_condition_add_clause_true(&ce); + ovsdb_idl_condition_add_clause_true(&ip_mcast); + ovsdb_idl_condition_add_clause_true(&igmp); + goto out; + } + sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "patch"); /* XXX: We can optimize this, if we find a way to only monitor * ports that have a Gateway_Chassis that point's to our own @@ -205,6 +219,8 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, uuid); } } + +out: sbrec_port_binding_set_condition(ovnsb_idl, &pb); sbrec_logical_flow_set_condition(ovnsb_idl, &lf); sbrec_mac_binding_set_condition(ovnsb_idl, &mb); @@ -428,7 +444,8 @@ get_ofctrl_probe_interval(struct ovsdb_idl *ovs_idl) /* Retrieves the pointer to the OVN Southbound database from 'ovs_idl' and * updates 'sbdb_idl' with that pointer. */ static void -update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl) +update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl, + bool *monitor_all_p) { const struct ovsrec_open_vswitch *cfg = ovsrec_open_vswitch_first(ovs_idl); if (!cfg) { @@ -445,6 +462,19 @@ update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl) int interval = smap_get_int(&cfg->external_ids, "ovn-remote-probe-interval", default_interval); ovsdb_idl_set_probe_interval(ovnsb_idl, interval); + + bool monitor_all = smap_get_bool(&cfg->external_ids, "ovn-monitor-all", + false); + if (monitor_all) { + /* Always call update_sb_monitors when monitor_all is true. + * Otherwise, don't call it here, because there would be unnecessary + * extra cost. Instead, it is called after the engine execution only + * when it is necessary. */ + update_sb_monitors(ovnsb_idl, NULL, NULL, NULL, true); + } + if (monitor_all_p) { + *monitor_all_p = monitor_all; + } } static void @@ -1887,7 +1917,7 @@ main(int argc, char *argv[]) ovsdb_idl_omit(ovnsb_idl_loop.idl, &sbrec_connection_col_status); ovsdb_idl_omit(ovnsb_idl_loop.idl, &sbrec_connection_col_target); - update_sb_monitors(ovnsb_idl_loop.idl, NULL, NULL, NULL); + update_sb_monitors(ovnsb_idl_loop.idl, NULL, NULL, NULL, false); stopwatch_create(CONTROLLER_LOOP_STOPWATCH_NAME, SW_MS); @@ -2009,6 +2039,7 @@ main(int argc, char *argv[]) /* Main loop. */ exiting = false; restart = false; + bool sb_monitor_all = false; while (!exiting) { engine_init_run(); @@ -2023,7 +2054,7 @@ main(int argc, char *argv[]) ovs_cond_seqno = new_ovs_cond_seqno; } - update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl); + update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl, &sb_monitor_all); update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl)); ofctrl_set_probe_interval(get_ofctrl_probe_interval(ovs_idl_loop.idl)); @@ -2163,7 +2194,8 @@ main(int argc, char *argv[]) if (engine_node_changed(&en_runtime_data)) { update_sb_monitors(ovnsb_idl_loop.idl, chassis, &runtime_data->local_lports, - &runtime_data->local_datapaths); + &runtime_data->local_datapaths, + sb_monitor_all); } } } @@ -2272,7 +2304,7 @@ main(int argc, char *argv[]) if (!restart) { bool done = !ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl); while (!done) { - update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl); + update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl, NULL); update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl)); struct ovsdb_idl_txn *ovs_idl_txn
Set this option to true will avoid using conditional monitoring in ovn-controller. Setting it to true helps in environments where all (or most) workloads need to be reachable from each other, thus the effectiveness of conditional monitoring is low, but the overhead of conditional monitoring is high. Requested-by: Girish Moodalbail <gmoodalbail@nvidia.com> Signed-off-by: Han Zhou <hzhou@ovn.org> --- controller/ovn-controller.8.xml | 21 ++++++++++++++++++++ controller/ovn-controller.c | 44 +++++++++++++++++++++++++++++++++++------ 2 files changed, 59 insertions(+), 6 deletions(-)