On Wed, Jul 7, 2021 at 1:28 AM Mark Gray <[email protected]> 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 <[email protected]> > Signed-off-by: Mark Gray <[email protected]> > --- > > 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? Other than this: Acked-by: Han Zhou <[email protected]> > ovsdb_idl_add_table(ovnsb_idl_loop.idl, > &sbrec_table_logical_dp_group); > -- > 2.27.0 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
