[ovs-dev] [PATCH ovn v4 6/6] northd: Do not calculate database sequence numbers incrementally
Han Zhou
hzhou at ovn.org
Thu Nov 4 00:01:30 UTC 2021
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.
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
>
More information about the dev
mailing list