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

Numan Siddique numans at ovn.org
Fri Jan 24 09:24:02 UTC 2020


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 ?
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 at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list