On Wed, Apr 13, 2022 at 8:48 AM Numan Siddique <[email protected]> wrote: > > On Wed, Apr 13, 2022 at 2:34 AM Han Zhou <[email protected]> wrote: > > > > During incremental flow processing, we track the OVS desired flow > > changes so that we can incrementally install them to OVS. The function > > merge_tracked_flows() is to merge the "delete and add/update" for the > > same flows, to avoid unnecessary changes to OVS when flows are deleted > > but added back in the same run. The function ensures that the flows > > being deleted and added/updated are exactly the same, including actions. > > It only compares the desired flows, assuming that the flow installed to > > OVS is exactly the same as the desired flow being deleted. > > > > However, this assumption can be wrong. If the same flow is already updated > > before "delete and add/update" in the same run, the initial "update" is > > not sent to OVS yet, but the change-tracking entry of that initial > > "update" is already override by the "delete". This would result in lost > > changes to OVS flows. This kind of problems can only be recovered by a > > full recompute in ovn-controller. > > > > This is more likely to cause real problems to branch-21.12 and earlier > > releases, and less likely (if at all) to cause problems in later > > releases, because since 22.03 we avoid repeated processing the same > > logical flow in the same run, but it may still happen in corner cases > > that hasn't been discovered. > > > > This patch fixes the problem by adding the check for installed flows > > before making the decision to merge the "delete and add/update" entries. > > We will not merge if the installed flow doesn't match the desired flow > > finally added/update. > > > > Reported-by: François Rigault > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2071272#c12 > > Reported-by: Numan Siddique <[email protected]> > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2022-April/393256.html > > Fixes: f4e508dd7 ("ofctrl.c: Merge opposite changes of tracked flows before installing.") > > Signed-off-by: Han Zhou <[email protected]> > > > Thanks for the fix. > > Can you please add François Rigault's email id ([email protected]) in > the Reported-by and also add the name to the AUTHORS file before > committing ? > Done
> The patch looks good to me. Since we have the reproducer, I'd > suggest adding this as a test case so that we don't regress it in the > future. > > Even though the test case (without the actual fix) won't fail with the > present main and branch-22.03, it will definitely fail in > branch-21.12. > So I think it's good to add the test case. > Sure. I spent some time and figured out a scenario that reproduces the problem even for the main branch, so I added the test case that covers it. It's worth to note that even the earlier steps are not guaranteed to reproduce the problem (even in 21.12), because it depends on the order of handling the lflows before and after the flood removal. If the orders are different, they would end up with different actions, e.g.: before: conjunction(abc, 2/2), conjunction(xyz, 2/2) after: conjunction(xyz, 2/2), conjunction(abc, 2/2) In this case the merge_tracked_flows() won't merge the delete and add/update (although the two flows are essentially the same), and the problem is not triggered. So in the test case I test the scenario in a loop that removes and re-add the ACLs, which triggers the problem at a very high chance. > With that addressed: > > Acked-by: Numan Siddique <[email protected]> Thanks for the review. I applied the change to main, and backported down to branch-21.03. > > Just one comment with the fix. > > I see that we delete the already installed conj flow (with one > conjunctive action) and then re-add the same flow with the modified > actions. > Which is ok. We could try to modify the flow instead. But it might > make the code more complex and may not be straightforward. > I'm fine with this fix. Just mentioned the observed behaviour. > Thanks for the comment. Yes, ideally we should just modify the flow, but it is risky because there is order in the tracked changes. Maybe it is ok to do it smarter way, but I'd keep it simple and stupid for now, for the scenario is relatively rare, and when this happens it must be a port-group change between 1 and >1, which doesn't seem to make difference in performance. (note that the regular "append" scenario would result in flow "modify" to existing flows without delete+add). Thanks, Han > Thanks > Numan > > > --- > > controller/ofctrl.c | 19 ++++++++++++++----- > > 1 file changed, 14 insertions(+), 5 deletions(-) > > > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > > index 2d8a8ec9c..eb66d0348 100644 > > --- a/controller/ofctrl.c > > +++ b/controller/ofctrl.c > > @@ -2324,7 +2324,20 @@ deleted_flow_lookup(struct hmap *deleted_flows, struct ovn_flow *target) > > && f->cookie == target->cookie > > && ofpacts_equal(f->ofpacts, f->ofpacts_len, target->ofpacts, > > target->ofpacts_len)) { > > - return d; > > + /* del_f must have been installed, otherwise it should have > > + * been removed during track_flow_del. */ > > + ovs_assert(d->installed_flow); > > + > > + /* Now we also need to make sure the desired flow being > > + * added/updated has exact same action and cookie as the installed > > + * flow of d. Otherwise, don't merge them, so that the > > + * installed flow can be updated later. */ > > + struct ovn_flow *f_i = &d->installed_flow->flow; > > + if (f_i->cookie == target->cookie > > + && ofpacts_equal(f_i->ofpacts, f_i->ofpacts_len, > > + target->ofpacts, target->ofpacts_len)) { > > + return d; > > + } > > } > > } > > return NULL; > > @@ -2353,10 +2366,6 @@ merge_tracked_flows(struct ovn_desired_flow_table *flow_table) > > continue; > > } > > > > - /* del_f must have been installed, otherwise it should have been > > - * removed during track_flow_add_or_modify. */ > > - ovs_assert(del_f->installed_flow); > > - > > if (!f->installed_flow) { > > /* f is not installed yet. */ > > replace_installed_to_desired(del_f->installed_flow, del_f, f); > > -- > > 2.30.2 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
