[ovs-dev] [PATCH ovn v7 1/4] northd: update stage-name if changed
Mark Gray
mark.d.gray at redhat.com
Thu Jul 8 07:52:40 UTC 2021
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"
>
> 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