[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