Skip to content

Commit

Permalink
ofctrl.c: Check installed flow when merging tracked flow changes.
Browse files Browse the repository at this point in the history 
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 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.

A test case is added to cover the scenario, which would fail without the
fix.

Reported-by: François Rigault <frigo@amadeus.com>
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2071272#c12
Reported-by: Numan Siddique <numans@ovn.org>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2022-April/393256.html
Fixes: f4e508d ("ofctrl.c: Merge opposite changes of tracked flows before installing.")
Signed-off-by: Han Zhou <hzhou@ovn.org>
(cherry picked from commit 9c6d285)
  • Loading branch information
@hzhou8
hzhou8 committed Apr 13, 2022
1 parent 6410cb7 commit 92cdada
Show file tree
Hide file tree
Showing 3 changed files with 101 additions and 5 deletions.
1 change: 1 addition & 0 deletions AUTHORS.rst
View file
Expand Up @@ -147,6 +147,7 @@ Fabrizio D'Angelo fdangelo@redhat.com
Flavio Fernandes flavio@flaviof.com
Flavio Leitner fbl@redhat.com
Francesco Fusco ffusco@redhat.com
François Rigault frigo@amadeus.com
Frank Wang wangpeihuixyz@126.com
Frédéric Tobias Christ fchrist@live.de
Frode Nordahl frode.nordahl@gmail.com
Expand Down
19 changes: 14 additions & 5 deletions controller/ofctrl.c
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
86 changes: 86 additions & 0 deletions tests/ovn.at
View file
Expand Up @@ -15247,6 +15247,92 @@ OVN_CLEANUP([hv1])
AT_CLEANUP
])

# This test ensures that the incremental flow installation works well when
# handling update->delete->add/update for the same OVS flow.
OVN_FOR_EACH_NORTHD([
AT_SETUP([ACL conjunction append and reprocess])
ovn_start

net_add n1
sim_add hv1
as hv1
check ovs-vsctl add-br br-phys
ovn_attach n1 br-phys 192.168.0.1

# Setup the desired state:
# - Two ACLs, each matches its own port-group (pg1 & pg2), and matches the same
# set of IP addresses.
# - pg1 includes p1, p2, p3
# - pg2 includes p4, p5
check ovn-nbctl ls-add sw
check ovn-nbctl lsp-add sw p1 -- lsp-set-addresses p1 "00:00:00:00:00:02 192.168.0.2"
check ovn-nbctl lsp-add sw p2 -- lsp-set-addresses p2 "00:00:00:00:00:03 192.168.0.3"
check ovn-nbctl lsp-add sw p3 -- lsp-set-addresses p3 "00:00:00:00:00:04 192.168.0.4"
check ovn-nbctl lsp-add sw p4 -- lsp-set-addresses p4 "00:00:00:00:00:05 192.168.0.5"
check ovn-nbctl lsp-add sw p5 -- lsp-set-addresses p5 "00:00:00:00:00:06 192.168.0.6"
check ovn-nbctl pg-add pg1 p1 p2 p3
check ovn-nbctl pg-add pg2 p4 p5
check ovs-vsctl add-port br-int p1 -- set Interface p1 external_ids:iface-id=p1
check ovs-vsctl add-port br-int p2 -- set Interface p2 external_ids:iface-id=p2
check ovs-vsctl add-port br-int p3 -- set Interface p3 external_ids:iface-id=p3
check ovs-vsctl add-port br-int p4 -- set Interface p4 external_ids:iface-id=p4
check ovs-vsctl add-port br-int p5 -- set Interface p5 external_ids:iface-id=p5
check ovn-nbctl acl-add pg1 to-lport 1000 "outport==@pg1 && ip4 && ip4.src == {10.0.0.1, 10.0.0.2}" allow
check ovn-nbctl acl-add pg2 to-lport 1000 "outport==@pg2 && ip4 && ip4.src == {10.0.0.1, 10.0.0.2}" allow
check ovn-nbctl --wait=hv sync

# Now we should have two flows with combined conjunctions.
OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows br-int | \
grep conjunction.*conjunction | wc -l`])


# Test the scenario 10 times to give enough chance to hit the
# "update->delete->add/update" scenario, because we can't decide the order of
# change handling inside ovn-controller.
for i in $(seq 10); do
# Unbind the p3 and p5, the combined conjunctions should be gone.
ovs-vsctl del-port br-int p3
ovs-vsctl del-port br-int p5
OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \
grep conjunction.*conjunction | wc -l`])

# Delete and re-add the ACLs, just to bring some randomness in the lflow
# processing order, so that there is a chance that the order of adding and
# appending are the same before & after the flow deletion, so that the
# generated combined conjunctions are the same before & after the flow
# deletion. (If the order is different, the combined conjunctions order is
# different and the action comparison would fail, so won't trigger the tracked
# flow merging. We want to make sure that we test the merging scenario)
ovn-nbctl acl-del pg1 to-lport 1000 "outport==@pg1 && ip4 && ip4.src == {10.0.0.1, 10.0.0.2}"
ovn-nbctl acl-del pg2 to-lport 1000 "outport==@pg2 && ip4 && ip4.src == {10.0.0.1, 10.0.0.2}"
ovn-nbctl acl-add pg1 to-lport 1000 "outport==@pg1 && ip4 && ip4.src == {10.0.0.1, 10.0.0.2}" allow
ovn-nbctl acl-add pg2 to-lport 1000 "outport==@pg2 && ip4 && ip4.src == {10.0.0.1, 10.0.0.2}" allow
ovn-nbctl --wait=hv sync

# Now re-bind p3 and p5 in the same transaction, so that pg1 and pg2 update are
# handled in the same I-P engine run. The order of pg1 and pg2 can be random.
# If the order is pg2 -> pg1, then it should trigger the OVS flow
# "update->delete->add/update" scenario:
# 1) when pg2 update is handled, the ACL-2 would append conjunctions to
# the conjunction flows of ACL-1
# 2) when pg1 update is handled, it would flood remove flows of both ACL-1 and
# ACL-2, including the "appended" conjunction flows. And then reprocess
# ACL-1 and ACL-2 would re-add and re-append the conjunction flows with
# combined conjunctions.
ovs-vsctl add-port br-int p3 -- set Interface p3 external_ids:iface-id=p3 -- \
add-port br-int p5 -- set Interface p5 external_ids:iface-id=p5
ovn-nbctl --wait=hv sync

# Now making sure we end up with two combined conjunctions.
OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows br-int | \
grep conjunction.*conjunction | wc -l`])

done

OVN_CLEANUP([hv1])
AT_CLEANUP
])

OVN_FOR_EACH_NORTHD([
AT_SETUP([Superseding ACLs with conjunction])
ovn_start
Expand Down

0 comments on commit 92cdada

Please sign in to comment.