[ovs-dev] [PATCH ovn] ovn-northd-ddlog: Fix startup race updating nb_cfg_timestamp.

Han Zhou hzhou at ovn.org
Mon Mar 8 18:47:40 UTC 2021


On Fri, Mar 5, 2021 at 5:24 PM Ben Pfaff <blp at ovn.org> wrote:
>
> The DDlog code used by ovn-northd-ddlog relies on always having a value
> (exactly one of them) in the NbCfgTimestamp relation.  The C code
> inserts and updates this value.  It's supposed to insert the initial
> value the first time that it processes any update from the northbound
> database.
>
> However, it actually only did this if the first update it processed was
> from the northbound database--if it was from the southbound database,
> it forgot to do so even when the first northbound update showed up.
> The DDlog code would then go kind of berzerk creating and deleting the
> NB_Global record until something actually happened to increment nb_cfg
> (such as "ovn-nbctl --wait=sb sync").
>
> This fixes the problem by only recording that a value was inserted to
> NbCfgTimestamp when the first update from the northbound database is
> received, same as originally intended.
>
> Signed-off-by: Ben Pfaff <blp at ovn.org>
> ---
>  northd/ovn-northd-ddlog.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/northd/ovn-northd-ddlog.c b/northd/ovn-northd-ddlog.c
> index aa0ea73e401d..f373bb129a89 100644
> --- a/northd/ovn-northd-ddlog.c
> +++ b/northd/ovn-northd-ddlog.c
> @@ -445,8 +445,10 @@ northd_parse_updates(struct northd_ctx *ctx, struct
ovs_list *updates)
>      if (ddlog_commit(ctx->ddlog)) {
>          goto error;
>      }
> -    old_nb_cfg = new_nb_cfg;
> -    old_nb_cfg_timestamp = new_nb_cfg_timestamp;
> +    if (ctx->has_timestamp_columns) {
> +        old_nb_cfg = new_nb_cfg;
> +        old_nb_cfg_timestamp = new_nb_cfg_timestamp;
> +    }

Hi Ben, sorry that I didn't understand this patch. It seems this change
wouldn't change the behavior. It only adds the if condition before
assigning old_nb_cfg and old_nb_cfg_timestamp. If
ctx->has_timestamp_columns is true, it is the same behavior as before; if
ctx->has_timestamp_columns is false, new_nb_cfg and new_nb_cfg_timestamp
wouldn't change (in the previous if-block not shown in this diff), then
before this change the old_nb_cfg and old_nb_cfg_timestamp would just be
re-assigned to their original values (a no-op) - same effect as this
change. Did I misunderstand anything? I also couldn't see any link between
this change and the commit message.

Thanks,
Han

>
>      /* This update may have implications for the other side, so
>       * immediately wake to check for more changes to be applied. */
> --
> 2.29.2
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list