| Message ID | 20210619095124.3596487-3-mark.d.gray@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | northd: Refactor Logical Flows for Gateway Router with DNAT/Load Balancers | expand |
On Sat, Jun 19, 2021 at 2:51 AM Mark Gray <mark.d.gray@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:stage-name' will not be updated to reflect the new, correct > name. This will cause the stage name to be incorrectly displayed by some > tools and commands such as `ovn-sbctl dump-flows`. > > This commit, reconciles changes to the stage name as part of build_lflows(). > This is interesting. It means a flow F existed in stage S (named "foo") of ovn-northd version V1, and in version V2, S becomes S + 1 ("foo"), but in the V2's stage S ("bar") there happens to be an exactly same flow like F existing, but the stage name shown in ovn-sbctl dump-flows will be "foo" instead of "bar". Is this the scenario the patch is trying to fix? If so, I think it is better not only fixing the "stage-name" but also other external_ids, including "source" and "stage-hint", for the same reason. Thanks, Han > Suggested-by: Ilya Maximets <i.maximets@ovn.org> > Signed-off-by: Mark Gray <mark.d.gray@redhat.com> > --- > northd/ovn-northd.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index aae7314c8977..d97ab4a5b39c 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -12412,6 +12412,13 @@ 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) { > + const char *stage_name = smap_get_def(&sbflow->external_ids, > + "stage-name", ""); > + 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)); > + } > + > /* This is a valid lflow. Checking if the datapath group needs > * updates. */ > bool update_dp_group = false; > @@ -14197,6 +14204,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); > > ovsdb_idl_add_table(ovnsb_idl_loop.idl, > &sbrec_table_logical_dp_group); > -- > 2.27.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 6/22/21 7:25 AM, Han Zhou wrote: > On Sat, Jun 19, 2021 at 2:51 AM Mark Gray <mark.d.gray@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:stage-name' will not be updated to reflect the new, correct >> name. This will cause the stage name to be incorrectly displayed by some >> tools and commands such as `ovn-sbctl dump-flows`. >> >> This commit, reconciles changes to the stage name as part of > build_lflows(). > This is interesting. It means a flow F existed in stage S (named "foo") of > ovn-northd version V1, and in version V2, S becomes S + 1 ("foo"), but in > the V2's stage S ("bar") there happens to be an exactly same flow like F > existing, but the stage name shown in ovn-sbctl dump-flows will be "foo" > instead of "bar". Is this the scenario the patch is trying to fix? > > If so, I think it is better not only fixing the "stage-name" but also other > external_ids, including "source" and "stage-hint", for the same reason. > Hi Mark, Han, Wouldn't this cause lots of additional string comparisons? What if we restrict ourselves to checking one flow per stage (for each ovn-northd iteration). Would that be enough to detect that we need to update all of them? This might be an ugly hack though. :) Regards, Dumitru
On 6/24/21 5:44 PM, Dumitru Ceara wrote: > On 6/22/21 7:25 AM, Han Zhou wrote: >> On Sat, Jun 19, 2021 at 2:51 AM Mark Gray <mark.d.gray@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:stage-name' will not be updated to reflect the new, correct >>> name. This will cause the stage name to be incorrectly displayed by some >>> tools and commands such as `ovn-sbctl dump-flows`. >>> >>> This commit, reconciles changes to the stage name as part of >> build_lflows(). >> This is interesting. It means a flow F existed in stage S (named "foo") of >> ovn-northd version V1, and in version V2, S becomes S + 1 ("foo"), but in >> the V2's stage S ("bar") there happens to be an exactly same flow like F >> existing, but the stage name shown in ovn-sbctl dump-flows will be "foo" >> instead of "bar". Is this the scenario the patch is trying to fix? >> >> If so, I think it is better not only fixing the "stage-name" but also other >> external_ids, including "source" and "stage-hint", for the same reason. >> > > Hi Mark, Han, > > Wouldn't this cause lots of additional string comparisons? What if we > restrict ourselves to checking one flow per stage (for each ovn-northd > iteration). Would that be enough to detect that we need to update all > of them? > > This might be an ugly hack though. :) Maybe we can check if 'northd_internal_version' changed in SB and re-check all external-ids only in this case? Best regards, Ilya Maximets.
On 6/24/21 5:56 PM, Ilya Maximets wrote: > On 6/24/21 5:44 PM, Dumitru Ceara wrote: >> On 6/22/21 7:25 AM, Han Zhou wrote: >>> On Sat, Jun 19, 2021 at 2:51 AM Mark Gray <mark.d.gray@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:stage-name' will not be updated to reflect the new, correct >>>> name. This will cause the stage name to be incorrectly displayed by some >>>> tools and commands such as `ovn-sbctl dump-flows`. >>>> >>>> This commit, reconciles changes to the stage name as part of >>> build_lflows(). >>> This is interesting. It means a flow F existed in stage S (named "foo") of >>> ovn-northd version V1, and in version V2, S becomes S + 1 ("foo"), but in >>> the V2's stage S ("bar") there happens to be an exactly same flow like F >>> existing, but the stage name shown in ovn-sbctl dump-flows will be "foo" >>> instead of "bar". Is this the scenario the patch is trying to fix? >>> >>> If so, I think it is better not only fixing the "stage-name" but also other >>> external_ids, including "source" and "stage-hint", for the same reason. >>> >> >> Hi Mark, Han, >> >> Wouldn't this cause lots of additional string comparisons? What if we >> restrict ourselves to checking one flow per stage (for each ovn-northd >> iteration). Would that be enough to detect that we need to update all >> of them? >> >> This might be an ugly hack though. :) > > Maybe we can check if 'northd_internal_version' changed in SB and re-check > all external-ids only in this case? > Nice idea! We just need to make sure the ovn_internal_version changes when logical flow stages are added. We do something similar for OVN actions.
On 24/06/2021 18:36, Dumitru Ceara wrote: > On 6/24/21 5:56 PM, Ilya Maximets wrote: >> On 6/24/21 5:44 PM, Dumitru Ceara wrote: >>> On 6/22/21 7:25 AM, Han Zhou wrote: >>>> On Sat, Jun 19, 2021 at 2:51 AM Mark Gray <mark.d.gray@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:stage-name' will not be updated to reflect the new, correct >>>>> name. This will cause the stage name to be incorrectly displayed by some >>>>> tools and commands such as `ovn-sbctl dump-flows`. >>>>> >>>>> This commit, reconciles changes to the stage name as part of >>>> build_lflows(). >>>> This is interesting. It means a flow F existed in stage S (named "foo") of >>>> ovn-northd version V1, and in version V2, S becomes S + 1 ("foo"), but in >>>> the V2's stage S ("bar") there happens to be an exactly same flow like F >>>> existing, but the stage name shown in ovn-sbctl dump-flows will be "foo" >>>> instead of "bar". Is this the scenario the patch is trying to fix? Yes, exactly this. >>>> >>>> If so, I think it is better not only fixing the "stage-name" but also other >>>> external_ids, including "source" and "stage-hint", for the same reason. >>>> >>> >>> Hi Mark, Han, >>> >>> Wouldn't this cause lots of additional string comparisons? What if we >>> restrict ourselves to checking one flow per stage (for each ovn-northd >>> iteration). Would that be enough to detect that we need to update all >>> of them? >>> >>> This might be an ugly hack though. :) >> >> Maybe we can check if 'northd_internal_version' changed in SB and re-check >> all external-ids only in this case? >> > > Nice idea! We just need to make sure the ovn_internal_version changes > when logical flow stages are added. We do something similar for OVN > actions. > I have done this as Ilya suggested. Thanks for the suggestion.
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index aae7314c8977..d97ab4a5b39c 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -12412,6 +12412,13 @@ 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) { + const char *stage_name = smap_get_def(&sbflow->external_ids, + "stage-name", ""); + 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)); + } + /* This is a valid lflow. Checking if the datapath group needs * updates. */ bool update_dp_group = false; @@ -14197,6 +14204,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); ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_logical_dp_group);
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:stage-name' will not be updated to reflect the new, correct name. This will cause the stage name to be incorrectly displayed by some tools and commands such as `ovn-sbctl dump-flows`. This commit, reconciles changes to the stage name as part of build_lflows(). Suggested-by: Ilya Maximets <i.maximets@ovn.org> Signed-off-by: Mark Gray <mark.d.gray@redhat.com> --- northd/ovn-northd.c | 9 +++++++++ 1 file changed, 9 insertions(+)