| Message ID | 80bb82025775e215987d2f15b50835fdc5f3eb56.1651602276.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] northd: fix lflow grouping in build_lb_rules | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On 5/3/22 21:09, Lorenzo Bianconi wrote: > Do not group lflows if the corresponding datapath does not run > copp meters and the previous one has an associated copp entry. > In order to fix the issue reset lflow_ref pointer to NULL if the logical > router/logical flow runs a copp meter. > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- Hi Lorenzo, The change looks good to me. If I'm not wrong this should be: Fixes: deec97274ab8 ("northd: optimize build_lb_rules routine") Would it be possible to add a test for this in ovn-northd.at? Thanks, Dumitru
> On 5/3/22 21:09, Lorenzo Bianconi wrote: > > Do not group lflows if the corresponding datapath does not run > > copp meters and the previous one has an associated copp entry. > > In order to fix the issue reset lflow_ref pointer to NULL if the logical > > router/logical flow runs a copp meter. > > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > --- > > Hi Lorenzo, > > The change looks good to me. > If I'm not wrong this should be: Hi Dumitru, thx for the review :) > > Fixes: deec97274ab8 ("northd: optimize build_lb_rules routine") > > Would it be possible to add a test for this in ovn-northd.at? I guess it is not easy to add a test for it since the issue occurs based on the iteration order and in turn it depends on hashmap iteration. Regards, Lorenzo > > Thanks, > Dumitru > >
On 5/16/22 10:29, Lorenzo Bianconi wrote: >> On 5/3/22 21:09, Lorenzo Bianconi wrote: >>> Do not group lflows if the corresponding datapath does not run >>> copp meters and the previous one has an associated copp entry. >>> In order to fix the issue reset lflow_ref pointer to NULL if the logical >>> router/logical flow runs a copp meter. >>> >>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> >>> --- >> >> Hi Lorenzo, >> >> The change looks good to me. >> If I'm not wrong this should be: > > Hi Dumitru, > > thx for the review :) > >> >> Fixes: deec97274ab8 ("northd: optimize build_lb_rules routine") >> >> Would it be possible to add a test for this in ovn-northd.at? > > I guess it is not easy to add a test for it since the issue occurs based on the > iteration order and in turn it depends on hashmap iteration. > You're right, thanks for the follow up! Acked-by: Dumitru Ceara <dceara@redhat.com>
On Mon, May 16, 2022 at 4:51 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 5/16/22 10:29, Lorenzo Bianconi wrote: > >> On 5/3/22 21:09, Lorenzo Bianconi wrote: > >>> Do not group lflows if the corresponding datapath does not run > >>> copp meters and the previous one has an associated copp entry. > >>> In order to fix the issue reset lflow_ref pointer to NULL if the logical > >>> router/logical flow runs a copp meter. > >>> > >>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > >>> --- > >> > >> Hi Lorenzo, > >> > >> The change looks good to me. > >> If I'm not wrong this should be: > > > > Hi Dumitru, > > > > thx for the review :) > > > >> > >> Fixes: deec97274ab8 ("northd: optimize build_lb_rules routine") > >> > >> Would it be possible to add a test for this in ovn-northd.at? > > > > I guess it is not easy to add a test for it since the issue occurs based on the > > iteration order and in turn it depends on hashmap iteration. > > > > You're right, thanks for the follow up! > > Acked-by: Dumitru Ceara <dceara@redhat.com> I added the Fixes tag and applied the patch to the main branch and backported to branch-22.03 and branch-21.12. Thanks Numan > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/northd/northd.c b/northd/northd.c index a56666297..bcb470043 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -7004,14 +7004,15 @@ build_lb_rules(struct hmap *lflows, struct ovn_northd_lb *lb, if (reject) { meter = copp_meter_get(COPP_REJECT, od->nbs->copp, meter_groups); - } else if (ovn_dp_group_add_with_reference(lflow_ref, od)) { - continue; } - lflow_ref = ovn_lflow_add_at_with_hash(lflows, od, - S_SWITCH_IN_LB, priority, - ds_cstr(match), ds_cstr(action), - NULL, meter, &lb->nlb->header_, - OVS_SOURCE_LOCATOR, hash); + if (meter || !ovn_dp_group_add_with_reference(lflow_ref, od)) { + struct ovn_lflow *lflow = ovn_lflow_add_at_with_hash( + lflows, od, S_SWITCH_IN_LB, priority, + ds_cstr(match), ds_cstr(action), + NULL, meter, &lb->nlb->header_, + OVS_SOURCE_LOCATOR, hash); + lflow_ref = meter ? NULL : lflow; + } } } }
Do not group lflows if the corresponding datapath does not run copp meters and the previous one has an associated copp entry. In order to fix the issue reset lflow_ref pointer to NULL if the logical router/logical flow runs a copp meter. Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- northd/northd.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)