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 ? 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. With that addressed: Acked-by: Numan Siddique <[email protected]> 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 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
