[ovs-dev] [PATCH 3/3] ovn-controller.c: Support option "ovn-monitor-all".

Han Zhou hzhou at ovn.org
Fri Jan 24 16:07:37 UTC 2020


On Fri, Jan 24, 2020 at 1:24 AM Numan Siddique <numans at ovn.org> wrote:
>
> On Tue, Jan 14, 2020 at 4:22 AM Han Zhou <hzhou at 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 at nvidia.com>
> > Signed-off-by: Han Zhou <hzhou at 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 at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >


More information about the dev mailing list