[ovs-dev] [PATCH ovn v7 1/4] northd: update stage-name if changed

Han Zhou hzhou at ovn.org
Thu Jul 8 17:22:43 UTC 2021


On Thu, Jul 8, 2021 at 12:52 AM Mark Gray <mark.d.gray at redhat.com> wrote:
>
> On 08/07/2021 02:10, Han Zhou wrote:
> > On Wed, Jul 7, 2021 at 1:28 AM Mark Gray <mark.d.gray at redhat.com> wrote:
> >>
> >> If a new table is added to a logical flow pipeline, the mapping between
> >> 'external_ids:stage-name' from the 'Logical_Flow' table in the
> >> 'OVN_Southbound' database and the 'stage' number may change for some
> > tables.
> >>
> >> If 'ovn-northd' is started against a populated Southbound database,
> >> 'external_ids' will not be updated to reflect the new, correct
> >> name. This will cause 'external_ids' to be incorrectly displayed by
some
> >> tools and commands such as `ovn-sbctl dump-flows`.
> >>
> >> This commit, reconciles these changes as part of build_lflows() when
> >> 'ovn_internal_version' is updated.
> >>
> >> Suggested-by: Ilya Maximets <i.maximets at ovn.org>
> >> Signed-off-by: Mark Gray <mark.d.gray at redhat.com>
> >> ---
> >>
> >> Notes:
> >>     v2:  Update all 'external_ids' rather than just 'stage-name'
> >>     v4:  Fix line length errors from 0-day
> >>
> >>  lib/ovn-util.c      |  4 ++--
> >>  northd/ovn-northd.c | 43 ++++++++++++++++++++++++++++++++++++++++---
> >>  2 files changed, 42 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> >> index c5af8d1ab340..acf4b1cd6059 100644
> >> --- a/lib/ovn-util.c
> >> +++ b/lib/ovn-util.c
> >> @@ -758,8 +758,8 @@ ip_address_and_port_from_lb_key(const char *key,
char
> > **ip_address,
> >>      return true;
> >>  }
> >>
> >> -/* Increment this for any logical flow changes or if existing OVN
action
> > is
> >> - * modified. */
> >> +/* Increment this for any logical flow changes, if an existing OVN
> > action is
> >> + * modified or a stage is added to a logical pipeline. */
> >>  #define OVN_INTERNAL_MINOR_VER 0
> >>
> >>  /* Returns the OVN version. The caller must free the returned value.
*/
> >> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> >> index 570c6a3efd77..eb25e31b1f7d 100644
> >> --- a/northd/ovn-northd.c
> >> +++ b/northd/ovn-northd.c
> >> @@ -12447,7 +12447,8 @@ build_lflows(struct northd_context *ctx, struct
> > hmap *datapaths,
> >>               struct hmap *ports, struct hmap *port_groups,
> >>               struct hmap *mcgroups, struct hmap *igmp_groups,
> >>               struct shash *meter_groups,
> >> -             struct hmap *lbs, struct hmap *bfd_connections)
> >> +             struct hmap *lbs, struct hmap *bfd_connections,
> >> +             bool ovn_internal_version_changed)
> >>  {
> >>      struct hmap lflows;
> >>
> >> @@ -12559,6 +12560,32 @@ build_lflows(struct northd_context *ctx,
struct
> > hmap *datapaths,
> >>              ovn_stage_build(dp_type, pipeline, sbflow->table_id),
> >>              sbflow->priority, sbflow->match, sbflow->actions,
> > sbflow->hash);
> >>          if (lflow) {
> >> +            if (ovn_internal_version_changed) {
> >> +                const char *stage_name =
> > smap_get_def(&sbflow->external_ids,
> >> +                                                  "stage-name", "");
> >> +                const char *stage_hint =
> > smap_get_def(&sbflow->external_ids,
> >> +                                                  "stage-hint", "");
> >> +                const char *source =
smap_get_def(&sbflow->external_ids,
> >> +                                                  "source", "");
> >> +
> >> +                if (strcmp(stage_name,
ovn_stage_to_str(lflow->stage))) {
> >> +
 sbrec_logical_flow_update_external_ids_setkey(sbflow,
> >> +                     "stage-name", ovn_stage_to_str(lflow->stage));
> >> +                }
> >> +                if (lflow->stage_hint) {
> >> +                    if (strcmp(stage_hint, lflow->stage_hint)) {
> >> +
> >  sbrec_logical_flow_update_external_ids_setkey(sbflow,
> >> +                        "stage-hint", lflow->stage_hint);
> >> +                    }
> >> +                }
> >> +                if (lflow->where) {
> >> +                    if (strcmp(source, lflow->where)) {
> >> +
> >  sbrec_logical_flow_update_external_ids_setkey(sbflow,
> >> +                        "source", lflow->where);
> >> +                    }
> >> +                }
> >> +            }
> >> +
> >>              /* This is a valid lflow.  Checking if the datapath group
> > needs
> >>               * updates. */
> >>              bool update_dp_group = false;
> >> @@ -13390,6 +13417,7 @@ ovnnb_db_run(struct northd_context *ctx,
> >>      struct shash meter_groups = SHASH_INITIALIZER(&meter_groups);
> >>      struct hmap lbs;
> >>      struct hmap bfd_connections = HMAP_INITIALIZER(&bfd_connections);
> >> +    bool ovn_internal_version_changed = true;
> >>
> >>      /* Sync ipsec configuration.
> >>       * Copy nb_cfg from northbound to southbound database.
> >> @@ -13441,7 +13469,13 @@ ovnnb_db_run(struct northd_context *ctx,
> >>      smap_replace(&options, "max_tunid", max_tunid);
> >>      free(max_tunid);
> >>
> >> -    smap_replace(&options, "northd_internal_version",
> > ovn_internal_version);
> >> +    if (!strcmp(ovn_internal_version,
> >> +                smap_get_def(&options, "northd_internal_version",
""))) {
> >> +        ovn_internal_version_changed = false;
> >> +    } else {
> >> +        smap_replace(&options, "northd_internal_version",
> >> +                     ovn_internal_version);
> >> +    }
> >>
> >>      nbrec_nb_global_verify_options(nb);
> >>      nbrec_nb_global_set_options(nb, &options);
> >> @@ -13481,7 +13515,8 @@ ovnnb_db_run(struct northd_context *ctx,
> >>      build_meter_groups(ctx, &meter_groups);
> >>      build_bfd_table(ctx, &bfd_connections, ports);
> >>      build_lflows(ctx, datapaths, ports, &port_groups, &mcast_groups,
> >> -                 &igmp_groups, &meter_groups, &lbs, &bfd_connections);
> >> +                 &igmp_groups, &meter_groups, &lbs, &bfd_connections,
> >> +                 ovn_internal_version_changed);
> >>      ovn_update_ipv6_prefix(ports);
> >>
> >>      sync_address_sets(ctx);
> >> @@ -14351,6 +14386,8 @@ main(int argc, char *argv[])
> >>      add_column_noalert(ovnsb_idl_loop.idl,
> > &sbrec_logical_flow_col_priority);
> >>      add_column_noalert(ovnsb_idl_loop.idl,
> > &sbrec_logical_flow_col_match);
> >>      add_column_noalert(ovnsb_idl_loop.idl,
> > &sbrec_logical_flow_col_actions);
> >> +    ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> >> +                         &sbrec_logical_flow_col_external_ids);
> >>
> >
> > Sorry that I didn't see a reason to monitor external_ids. Did I miss
> > anything?
>
> We need to read and write to this column so I set it. If I do not
> monitor it, I get the following warning:
>
> "2021-07-08T07:49:48.982Z|01628|ovsdb_idl|WARN|cannot partially update
> non-monitored column"

My bad, sorry for the noise. I just applied it to master.

Thanks,
Han

> >
> > Other than this:
> > Acked-by: Han Zhou <hzhou at ovn.org>
> >
> >>      ovsdb_idl_add_table(ovnsb_idl_loop.idl,
> >>                          &sbrec_table_logical_dp_group);
> >> --
> >> 2.27.0
> >>
> >
>


More information about the dev mailing list