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 <num...@ovn.org> 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 <hz...@ovn.org> --- 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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
