[ovs-dev] [PATCH ovn v4 6/6] northd: Do not calculate database sequence numbers incrementally

Mark Gray mrmarkgray at gmail.com
Mon Nov 8 22:46:05 UTC 2021


On Thu, Nov 4, 2021 at 12:02 AM Han Zhou <hzhou at ovn.org> wrote:

> On Wed, Oct 27, 2021 at 10:24 AM Mark Gray <mark.d.gray at redhat.com> wrote:
> >
> > In order to remove the IDL loop variable from the engine context,
> > we do not calculate the database sequence numbers incrementally.
> >
> > Signed-off-by: Mark Gray <mark.d.gray at redhat.com>
> > ---
> >  lib/inc-proc-eng.h       |  2 -
> >  northd/en-northd.c       |  3 +-
> >  northd/inc-proc-northd.c |  2 -
> >  northd/inc-proc-northd.h |  1 -
> >  northd/northd.c          | 84 ++++------------------------------------
> >  northd/northd.h          |  3 +-
> >  northd/ovn-northd.c      | 84 ++++++++++++++++++++++++++++++++++++++--
> >  7 files changed, 90 insertions(+), 89 deletions(-)
> >
> > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h
> > index 1823750c814c..6f3918ae7789 100644
> > --- a/lib/inc-proc-eng.h
> > +++ b/lib/inc-proc-eng.h
> > @@ -73,8 +73,6 @@ struct engine_context {
> >      struct ovsdb_idl_txn *ovnsb_idl_txn;
> >      struct ovsdb_idl_txn *ovnnb_idl_txn;
> >
> > -    struct ovsdb_idl_loop *ovnsb_idl_loop;
> > -
> >      void *client_ctx;
> >  };
> >
> > diff --git a/northd/en-northd.c b/northd/en-northd.c
> > index 36ea890535fe..295a74721678 100644
> > --- a/northd/en-northd.c
> > +++ b/northd/en-northd.c
> > @@ -119,8 +119,7 @@ void en_northd_run(struct engine_node *node, void
> *data)
> >
> >      northd_run(data,
> >                 eng_ctx->ovnnb_idl_txn,
> > -               eng_ctx->ovnsb_idl_txn,
> > -               eng_ctx->ovnsb_idl_loop);
> > +               eng_ctx->ovnsb_idl_txn);
> >      engine_set_node_state(node, EN_UPDATED);
> >
> >  }
> > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> > index 56c05a0fd6f3..44b53432c071 100644
> > --- a/northd/inc-proc-northd.c
> > +++ b/northd/inc-proc-northd.c
> > @@ -248,7 +248,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> >
> >  void inc_proc_northd_run(struct ovsdb_idl_txn *ovnnb_txn,
> >                           struct ovsdb_idl_txn *ovnsb_txn,
> > -                         struct ovsdb_idl_loop *ovnsb_idl_loop,
> >                           bool recompute) {
> >      engine_set_force_recompute(recompute);
> >      engine_init_run();
> > @@ -256,7 +255,6 @@ void inc_proc_northd_run(struct ovsdb_idl_txn
> *ovnnb_txn,
> >      struct engine_context eng_ctx = {
> >          .ovnnb_idl_txn = ovnnb_txn,
> >          .ovnsb_idl_txn = ovnsb_txn,
> > -        .ovnsb_idl_loop = ovnsb_idl_loop,
> >      };
> >
> >      engine_set_context(&eng_ctx);
> > diff --git a/northd/inc-proc-northd.h b/northd/inc-proc-northd.h
> > index 4aeb387b7b0f..1300253791b6 100644
> > --- a/northd/inc-proc-northd.h
> > +++ b/northd/inc-proc-northd.h
> > @@ -10,7 +10,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> >                            struct ovsdb_idl_loop *sb);
> >  void inc_proc_northd_run(struct ovsdb_idl_txn *ovnnb_txn,
> >                           struct ovsdb_idl_txn *ovnsb_txn,
> > -                         struct ovsdb_idl_loop *ovnsb_idl_loop,
> >                           bool recompute);
> >  void inc_proc_northd_cleanup(void);
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 7bccd054cdb4..1e4273ec5e88 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -14502,9 +14502,7 @@ ovnnb_db_run(struct northd_data *data,
> >               struct ovsdb_idl_txn *ovnnb_txn,
> >               struct ovsdb_idl_txn *ovnsb_txn,
> >               struct ovsdb_idl_index *sbrec_chassis_by_name,
> > -             struct ovsdb_idl_index *sbrec_chassis_by_hostname,
> > -             struct ovsdb_idl_loop *sb_loop,
> > -             int64_t loop_start_time)
> > +             struct ovsdb_idl_index *sbrec_chassis_by_hostname)
> >  {
> >      if (!ovnsb_txn || !ovnnb_txn) {
> >          return;
> > @@ -14527,12 +14525,7 @@ ovnnb_db_run(struct northd_data *data,
> >      if (nb->ipsec != sb->ipsec) {
> >          sbrec_sb_global_set_ipsec(sb, nb->ipsec);
> >      }
> > -    if (nb->nb_cfg != sb->nb_cfg) {
> > -        sbrec_sb_global_set_nb_cfg(sb, nb->nb_cfg);
> > -        nbrec_nb_global_set_nb_cfg_timestamp(nb, loop_start_time);
> > -    }
> >      sbrec_sb_global_set_options(sb, &nb->options);
> > -    sb_loop->next_cfg = nb->nb_cfg;
> >
> >      const char *mac_addr_prefix = set_mac_prefix(smap_get(&nb->options,
> >
> "mac_prefix"));
> > @@ -14834,68 +14827,12 @@ handle_port_binding_changes(struct northd_data
> *data,
> >      }
> >  }
> >
> > -/* Updates the sb_cfg and hv_cfg columns in the northbound NB_Global
> table. */
> > -static void
> > -update_northbound_cfg(struct northd_data *data,
> > -                      struct ovsdb_idl_loop *sb_loop,
> > -                      int64_t loop_start_time)
> > -{
> > -    /* Update northbound sb_cfg if appropriate. */
> > -    const struct nbrec_nb_global *nbg = nbrec_nb_global_table_first(
> > -                               data->input.nbrec_nb_global_table);
> > -    int64_t sb_cfg = sb_loop->cur_cfg;
> > -    if (nbg && sb_cfg && nbg->sb_cfg != sb_cfg) {
> > -        nbrec_nb_global_set_sb_cfg(nbg, sb_cfg);
> > -        nbrec_nb_global_set_sb_cfg_timestamp(nbg, loop_start_time);
> > -    }
> > -
> > -    /* Update northbound hv_cfg if appropriate. */
> > -    if (nbg) {
> > -        /* Find minimum nb_cfg among all chassis. */
> > -        const struct sbrec_chassis_private *chassis_priv;
> > -        int64_t hv_cfg = nbg->nb_cfg;
> > -        int64_t hv_cfg_ts = 0;
> > -        SBREC_CHASSIS_PRIVATE_TABLE_FOR_EACH (chassis_priv,
> > -
>  data->input.sbrec_chassis_private_table) {
> > -            const struct sbrec_chassis *chassis = chassis_priv->chassis;
> > -            if (chassis) {
> > -                if (smap_get_bool(&chassis->other_config,
> > -                                  "is-remote", false)) {
> > -                    /* Skip remote chassises. */
> > -                    continue;
> > -                }
> > -            } else {
> > -                static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(1, 1);
> > -                VLOG_WARN_RL(&rl, "Chassis does not exist for "
> > -                             "Chassis_Private record, name: %s",
> > -                             chassis_priv->name);
> > -            }
> > -
> > -            if (chassis_priv->nb_cfg < hv_cfg) {
> > -                hv_cfg = chassis_priv->nb_cfg;
> > -                hv_cfg_ts = chassis_priv->nb_cfg_timestamp;
> > -            } else if (chassis_priv->nb_cfg == hv_cfg &&
> > -                       chassis_priv->nb_cfg_timestamp > hv_cfg_ts) {
> > -                hv_cfg_ts = chassis_priv->nb_cfg_timestamp;
> > -            }
> > -        }
> > -
> > -        /* Update hv_cfg. */
> > -        if (nbg->hv_cfg != hv_cfg) {
> > -            nbrec_nb_global_set_hv_cfg(nbg, hv_cfg);
> > -            nbrec_nb_global_set_hv_cfg_timestamp(nbg, hv_cfg_ts);
> > -        }
> > -    }
> > -}
> > -
> >  /* Handle a fairly small set of changes in the southbound database. */
> >  static void
> >  ovnsb_db_run(struct northd_data *data,
> >               struct ovsdb_idl_txn *ovnnb_txn,
> >               struct ovsdb_idl_txn *ovnsb_txn,
> > -             struct ovsdb_idl_loop *sb_loop,
> > -             struct hmap *ports,
> > -             int64_t loop_start_time)
> > +             struct hmap *ports)
> >  {
> >      if (!ovnnb_txn ||
> >          !ovsdb_idl_has_ever_connected(ovsdb_idl_txn_get_idl(ovnsb_txn)))
> {
> > @@ -14904,30 +14841,23 @@ ovnsb_db_run(struct northd_data *data,
> >
> >      struct shash ha_ref_chassis_map =
> SHASH_INITIALIZER(&ha_ref_chassis_map);
> >      handle_port_binding_changes(data, ovnsb_txn, ports,
> &ha_ref_chassis_map);
> > -    update_northbound_cfg(data, sb_loop, loop_start_time);
> >      if (ovnsb_txn) {
> >          update_sb_ha_group_ref_chassis(data, &ha_ref_chassis_map);
> >      }
> >      shash_destroy(&ha_ref_chassis_map);
> >  }
> >
> > -void
> > -northd_run(struct northd_data *data,
> > -           struct ovsdb_idl_txn *ovnnb_txn,
> > -           struct ovsdb_idl_txn *ovnsb_txn,
> > -           struct ovsdb_idl_loop *sb_loop)
> > +void northd_run(struct northd_data *data,
> > +                struct ovsdb_idl_txn *ovnnb_txn,
> > +                struct ovsdb_idl_txn *ovnsb_txn)
> >  {
> > -    int64_t start_time = time_wall_msec();
> > -
> >      stopwatch_start(OVNNB_DB_RUN_STOPWATCH_NAME, time_msec());
> >      ovnnb_db_run(data, ovnnb_txn, ovnsb_txn,
> >                   data->input.sbrec_chassis_by_name,
> > -                 data->input.sbrec_chassis_by_hostname,
> > -                 sb_loop, start_time);
> > +                 data->input.sbrec_chassis_by_hostname);
> >      stopwatch_stop(OVNNB_DB_RUN_STOPWATCH_NAME, time_msec());
> >      stopwatch_start(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
> > -    ovnsb_db_run(data, ovnnb_txn, ovnsb_txn,
> > -                 sb_loop, &data->ports, start_time);
> > +    ovnsb_db_run(data, ovnnb_txn, ovnsb_txn, &data->ports);
> >      stopwatch_stop(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
> >  }
> >
> > diff --git a/northd/northd.h b/northd/northd.h
> > index 5ceb2c6c6216..113688694cbb 100644
> > --- a/northd/northd.h
> > +++ b/northd/northd.h
> > @@ -79,8 +79,7 @@ struct northd_data {
> >
> >  void northd_run(struct northd_data *data,
> >                  struct ovsdb_idl_txn *ovnnb_txn,
> > -                struct ovsdb_idl_txn *ovnsb_txn,
> > -                struct ovsdb_idl_loop *sb_loop);
> > +                struct ovsdb_idl_txn *ovnsb_txn);
> >  void northd_destroy(struct northd_data *data);
> >  void northd_init(struct northd_data *data);
> >  void northd_indices_create(struct northd_data *data,
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index cce757fb57d9..7b8fa115f14c 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -439,6 +439,79 @@ check_and_add_supported_dhcpv6_opts_to_sb_db(struct
> ovsdb_idl_txn *ovnsb_txn,
> >
> >      hmap_destroy(&dhcpv6_opts_to_add);
> >  }
> > +
> > +/* Updates the nb_cfg, sb_cfg and hv_cfg columns in NB/SB databases. */
> > +static void
> > +update_sequence_numbers(struct ovsdb_idl *ovnnb_idl,
> > +                        struct ovsdb_idl *ovnsb_idl,
> > +                        struct ovsdb_idl_txn *ovnnb_idl_txn,
> > +                        struct ovsdb_idl_txn *ovnsb_idl_txn,
> > +                        struct ovsdb_idl_loop *sb_loop)
> > +{
> > +    int64_t loop_start_time = time_msec();
>
> This patch all looks good except that the loop_start_time is supposed to be
> recorded when the loop starts, i.e. before the northd_run which could take
> a long time. So it needs to be recorded before the I-P engine run and
> passed in here as an argument.
>
>
Good catch :). I will capture it just before inc_proc_northd_run()


> Thanks,
> Han
>
> > +
> > +    /* Create rows in global tables if neccessary */
> > +    const struct nbrec_nb_global *nb = nbrec_nb_global_first(ovnnb_idl);
> > +    if (!nb) {
> > +        nb = nbrec_nb_global_insert(ovnnb_idl_txn);
> > +    }
> > +    const struct sbrec_sb_global *sb = sbrec_sb_global_first(ovnsb_idl);
> > +    if (!sb) {
> > +        sb = sbrec_sb_global_insert(ovnsb_idl_txn);
> > +    }
> > +
> > +    /* Copy nb_cfg from northbound to southbound database.
> > +     * Also set up to update sb_cfg once our southbound transaction
> commits. */
> > +    if (nb->nb_cfg != sb->nb_cfg) {
> > +        sbrec_sb_global_set_nb_cfg(sb, nb->nb_cfg);
> > +        nbrec_nb_global_set_nb_cfg_timestamp(nb, loop_start_time);
> > +    }
> > +    sb_loop->next_cfg = nb->nb_cfg;
> > +
> > +    /* Update northbound sb_cfg if appropriate. */
> > +    int64_t sb_cfg = sb_loop->cur_cfg;
> > +    if (nb && sb_cfg && nb->sb_cfg != sb_cfg) {
> > +        nbrec_nb_global_set_sb_cfg(nb, sb_cfg);
> > +        nbrec_nb_global_set_sb_cfg_timestamp(nb, loop_start_time);
> > +    }
> > +
> > +    /* Update northbound hv_cfg if appropriate. */
> > +    if (nb) {
> > +        /* Find minimum nb_cfg among all chassis. */
> > +        const struct sbrec_chassis_private *chassis_priv;
> > +        int64_t hv_cfg = nb->nb_cfg;
> > +        int64_t hv_cfg_ts = 0;
> > +        SBREC_CHASSIS_PRIVATE_FOR_EACH (chassis_priv, ovnsb_idl) {
> > +            const struct sbrec_chassis *chassis = chassis_priv->chassis;
> > +            if (chassis) {
> > +                if (smap_get_bool(&chassis->other_config,
> > +                                  "is-remote", false)) {
> > +                    /* Skip remote chassises. */
> > +                    continue;
> > +                }
> > +            } else {
> > +                static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(1, 1);
> > +                VLOG_WARN_RL(&rl, "Chassis does not exist for "
> > +                             "Chassis_Private record, name: %s",
> > +                             chassis_priv->name);
> > +            }
> > +
> > +            if (chassis_priv->nb_cfg < hv_cfg) {
> > +                hv_cfg = chassis_priv->nb_cfg;
> > +                hv_cfg_ts = chassis_priv->nb_cfg_timestamp;
> > +            } else if (chassis_priv->nb_cfg == hv_cfg &&
> > +                       chassis_priv->nb_cfg_timestamp > hv_cfg_ts) {
> > +                hv_cfg_ts = chassis_priv->nb_cfg_timestamp;
> > +            }
> > +        }
> > +
> > +        /* Update hv_cfg. */
> > +        if (nb->hv_cfg != hv_cfg) {
> > +            nbrec_nb_global_set_hv_cfg(nb, hv_cfg);
> > +            nbrec_nb_global_set_hv_cfg_timestamp(nb, hv_cfg_ts);
> > +        }
> > +    }
> > +}
> >
> >  static void
> >  add_column_noalert(struct ovsdb_idl *idl,
> > @@ -995,9 +1068,7 @@ main(int argc, char *argv[])
> >              }
> >
> >              if (ovsdb_idl_has_lock(ovnsb_idl_loop.idl)) {
> > -                inc_proc_northd_run(ovnnb_txn, ovnsb_txn,
> > -                                    &ovnsb_idl_loop,
> > -                                    recompute);
> > +                inc_proc_northd_run(ovnnb_txn, ovnsb_txn, recompute);
> >                  recompute = false;
> >                  if (ovnsb_txn) {
> >                      check_and_add_supported_dhcp_opts_to_sb_db(
> > @@ -1007,6 +1078,13 @@ main(int argc, char *argv[])
> >                      check_and_update_rbac(
> >                                   ovnsb_txn, ovnsb_idl_loop.idl);
> >                  }
> > +
> > +                if (ovnnb_txn && ovnsb_txn) {
> > +                    update_sequence_numbers(ovnnb_idl_loop.idl,
> > +                                            ovnsb_idl_loop.idl,
> > +                                            ovnnb_txn, ovnsb_txn,
> > +                                            &ovnsb_idl_loop);
> > +                }
> >              }
> >          } else {
> >              /* ovn-northd is paused
> > --
> > 2.27.0
> >
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list