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

Numan Siddique numans at ovn.org
Fri Jan 24 19:07:47 UTC 2020


On Fri, Jan 24, 2020, 21:38 Han Zhou <hzhou at ovn.org> wrote:

> 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,
> ...
>

Thanks for the response.

Acked-by: Numan Siddique <numans at 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 at openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>


More information about the dev mailing list