On 1/22/21 9:33 AM, [email protected] wrote:
From: Numan Siddique <[email protected]>
When the below ACL is added -
ovn-nbctl acl-add ls1 to-lport 3
'(ip4.src==10.0.0.1 || ip4.src==10.0.0.2) &&
(ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4)' allow
ovn-controller installs the below OF flows
table=45, priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
actions=conjunction(2,1/2)
table=45, priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
actions=conjunction(2,1/2)
table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.2
actions=conjunction(2,2/2)
table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.1
actions=conjunction(2,2/2)
table=45, priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
When a full recompute is triggered, ovn-controller deletes the last
OF flow with the match conj_id=2 and adds the below OF flow
table=45, priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
For subsequent recomputes, the conj_id keeps increasing by 1.
This disrupts the traffic which matches on conjuction action flows.
This patch fixes this issue.
Fixes: 1213bc8270("ovn-controller: Cache logical flow expr matches.")
Signed-off-by: Numan Siddique <[email protected]>
---
Hi Numan,
Good catch!
controller/lflow.c | 7 +++++++
tests/ovn.at | 28 ++++++++++++++++++++++++++++
2 files changed, 35 insertions(+)
diff --git a/controller/lflow.c b/controller/lflow.c
index c02585b1e..a9420a7c4 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -754,6 +754,13 @@ add_matches_to_flow_table(const struct sbrec_logical_flow
*lflow,
&m->match, &conj, &lflow->header_.uuid);
ofpbuf_uninit(&conj);
}
+
+ if (m->match.wc.masks.conj_id) {
+ /* Reset the conj_id back to relative conj id. If caching is
+ * enabled, then processing of the expr match next time (due to
+ * full recompute) will result in the wrong conj_id match flow. */
+ m->match.flow.conj_id -= conj_id_ofs;
+ }
While this works I'm not sure this is the best way to fix the issue.
We're now making the add_matches_to_flow_table() aware of the lflow cache.
In my opinion the problem is that the matches we cache when storing
entries of type LCACHE_T_MATCHES are not fully processed.
One option to fix that is to pass the current conj_ids_ofs to
expr_to_matches() to make sure the generated match objects always use
the "final" conj_id.
This is quite intrusive I guess.
An alternative is to add a new step, something like:
prepare_matches(matches, conj_id_ofs)
{
for_each m in matches:
// adjust m conj_id
if (m->match.wc.masks.conj_id)
m->match.flow.conj_id += conj_id_ofs;
for_each conj in m->conjunctions:
conj->id += conj_id_ofs;
}
We'd also have to remove the parts that update the conj_ids in
add_matches_to_flow_table():
https://github.com/ovn-org/ovn/blob/a2d043d2ee473da8e4a835730d86f16a47fb096b/controller/lflow.c#L712
and
https://github.com/ovn-org/ovn/blob/master/controller/lflow.c#L747
Then we could call prepare_matches() before add_matches_to_flow_table() if:
1. flow cache disabled.
2. if flow cache is enabled and we're caching lflow matches.
Later, when cached matches are used (from step 2 above) we don't need to
change them in any way and we can just call add_matches_to_flow_table()
without having to worry about offsets.
What do you think?
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
