[ovs-dev] [PATCH ovn] ovn-controller: Propagate nb_cfg to the local OVS DB.

Numan Siddique numans at ovn.org
Mon Nov 16 11:02:36 UTC 2020


On Thu, Nov 12, 2020 at 9:25 PM Dumitru Ceara <dceara at redhat.com> wrote:
>
> The NB.NB_Global.nb_cfg value gets propagated to
> Chassis_Private.nb_cfg (and then to NB.NB_Global.hv_cfg) as soon as
> ovn-controller has finished installing OVS flows corresponding to
> the NB DB state.
>
> However, if the CMS runs monitoring applications on the chassis itself,
> in order to detect that the NB changes have been applied, it has to
> connect to the NB/SB database.  In a scaled deployment this additional
> connection might induce performance issues.
>
> In order to avoid that we now (also) propagate nb_cfg to the local OVS
> DB, in table Open_vSwitch, as external_id "ovn-nb-cfg".
>
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>

Hi Dumitru,

Thanks for the patch. Overall the patch LGTM. I have a couple of comments.

1. I think it  needs to be documented that the ovn-controller writes
'ovn-nb-cfg' to the local ovsdb.
2. I think it's better if the ovn-controller writes the ovn-nb-cfg to
the integration bridge's external_ids column.
   We already have Ihar's multiple ovn-controller support patch for
review and it would conflict for this usecase.

Thanks
Numan

> ---
>  controller/ovn-controller.c | 53 ++++++++++++++++++++++++++++++++++-----------
>  tests/ovn-controller.at     | 21 ++++++++++++++++++
>  2 files changed, 61 insertions(+), 13 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index a06cae3..e05afc2 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -85,6 +85,8 @@ static unixctl_cb_func debug_delay_nb_cfg_report;
>
>  #define CONTROLLER_LOOP_STOPWATCH_NAME "ovn-controller-flow-generation"
>
> +#define OVS_NB_CFG_NAME "ovn-nb-cfg"
> +
>  static char *parse_options(int argc, char *argv[]);
>  OVS_NO_RETURN static void usage(void);
>
> @@ -733,6 +735,41 @@ get_nb_cfg(const struct sbrec_sb_global_table *sb_global_table)
>      return sb ? sb->nb_cfg : 0;
>  }
>
> +/* Propagates the local cfg seqno, 'cur_cfg', to the chassis_private record
> + * and to the local OVS DB.
> + */
> +static void
> +store_nb_cfg(struct ovsdb_idl_txn *sb_txn, struct ovsdb_idl_txn *ovs_txn,
> +             const struct sbrec_chassis_private *chassis,
> +             const struct ovsrec_open_vswitch *ovs_cfg,
> +             unsigned int delay_nb_cfg_report,
> +             int64_t cur_cfg)
> +{
> +    if (!cur_cfg) {
> +        return;
> +    }
> +
> +    if (sb_txn && chassis && cur_cfg != chassis->nb_cfg) {
> +        sbrec_chassis_private_set_nb_cfg(chassis, cur_cfg);
> +        sbrec_chassis_private_set_nb_cfg_timestamp(chassis, time_wall_msec());
> +
> +        if (delay_nb_cfg_report) {
> +            VLOG_INFO("Sleep for %u sec", delay_nb_cfg_report);
> +            xsleep(delay_nb_cfg_report);
> +        }
> +    }
> +
> +    if (ovs_txn && ovs_cfg &&
> +            cur_cfg != smap_get_ullong(&ovs_cfg->external_ids,
> +                                       OVS_NB_CFG_NAME, 0)) {
> +        char *cur_cfg_str = xasprintf("%"PRId64, cur_cfg);
> +        ovsrec_open_vswitch_update_external_ids_setkey(ovs_cfg,
> +                                                       OVS_NB_CFG_NAME,
> +                                                       cur_cfg_str);
> +        free(cur_cfg_str);
> +    }
> +}
> +
>  static const char *
>  get_transport_zones(const struct ovsrec_open_vswitch_table *ovs_table)
>  {
> @@ -2632,19 +2669,9 @@ main(int argc, char *argv[])
>                  engine_set_force_recompute(false);
>              }
>
> -            if (ovnsb_idl_txn && chassis_private) {
> -                int64_t cur_cfg = ofctrl_get_cur_cfg();
> -                if (cur_cfg && cur_cfg != chassis_private->nb_cfg) {
> -                    sbrec_chassis_private_set_nb_cfg(chassis_private, cur_cfg);
> -                    sbrec_chassis_private_set_nb_cfg_timestamp(
> -                        chassis_private, time_wall_msec());
> -                    if (delay_nb_cfg_report) {
> -                        VLOG_INFO("Sleep for %u sec", delay_nb_cfg_report);
> -                        xsleep(delay_nb_cfg_report);
> -                    }
> -                }
> -            }
> -
> +            store_nb_cfg(ovnsb_idl_txn, ovs_idl_txn, chassis_private,
> +                         ovsrec_open_vswitch_first(ovs_idl_loop.idl),
> +                         delay_nb_cfg_report, ofctrl_get_cur_cfg());
>
>              if (pending_pkt.conn) {
>                  struct ed_type_addr_sets *as_data =
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 014a977..f672bc9 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -411,3 +411,24 @@ AT_CHECK([ovn-nbctl --timeout=1 --wait=hv sync])
>
>  OVN_CLEANUP([hv])
>  AT_CLEANUP
> +
> +AT_SETUP([ovn -- nb_cfg sync to OVS])
> +ovn_start
> +
> +net_add n1
> +sim_add hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +
> +# Wait for ovn-controller to register in the SB.
> +wait_row_count Chassis 1
> +
> +# Increment nb_cfg.
> +check ovn-nbctl --wait=hv sync
> +
> +# And check that it gets propagated to OVS external_ids.
> +as hv1
> +OVS_WAIT_UNTIL([ovs-vsctl get Open_vSwitch . external_ids:ovn-nb-cfg], [0], [1])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list