[ovs-dev] [PATCH ovn v2] ofctrl: Do not link a desired flow twice.

[Dumitru Ceara]

On 2/19/21 1:27 PM, Numan Siddique wrote:
> On Fri, Feb 19, 2021 at 3:59 PM Mark Gray <mark.d.gray at redhat.com> wrote:
>>
>> On 18/02/2021 18:49, Dumitru Ceara wrote:
>>> Depending on the logical flow matches, multiple SB flows can point to
>>> the same desired flow.  If it happens that the desired flow conflicts
>>> with a more restrictive (already installed) flow, then we shouldn't try
>>> to add the desired flow multiple times to the list maintained for the
>>> installed flow.
>>>
>>> Fixes: 6f0b1e02d9ab ("ofctrl: Incremental processing for flow installation by tracking.")
>>> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
>>> ---
>>> Note: I'm quite sure about the "Fixes" tag above, however, I couldn't
>>> reproduce the issue without the following commit:
>>>    dadae4f800cc ("ofctrl.c: Only merge actions for conjunctive flows.")
>>>
>>> v2:
>>> - Address Mark's comments:
>>>    - readd log message removed by accident.
>>>    - add comments to the new test.
>>> ---
>>>   controller/ofctrl.c |  2 +-
>>>   tests/ovn.at        | 23 +++++++++++++++++++++++
>>>   2 files changed, 24 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
>>> index 1c9694a..415d9b7 100644
>>> --- a/controller/ofctrl.c
>>> +++ b/controller/ofctrl.c
>>> @@ -1989,7 +1989,7 @@ update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table,
>>>                    * tracked, so it must have been modified. */
>>>                   installed_flow_mod(&i->flow, &f->flow, msgs);
>>>                   ovn_flow_log(&i->flow, "updating installed (tracked)");
>>> -            } else {
>>> +            } else if (!f->installed_flow) {
>>>                   /* Adding a new flow that conflicts with an existing installed
>>>                    * flow, so add it to the link.  If this flow becomes active,
>>>                    * e.g., it is less restrictive than the previous active flow
>>> diff --git a/tests/ovn.at b/tests/ovn.at
>>> index 344e6bf..3d027c4 100644
>>> --- a/tests/ovn.at
>>> +++ b/tests/ovn.at
>>> @@ -13955,6 +13955,29 @@ AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | ofctl_strip_all | \
>>>    table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction()
>>>   ])
>>>
>>> +# Add another ACL that overlaps with the existing less restrictive ones.
>>> +check ovn-nbctl acl-add ls1 to-lport 3 'udp || ((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
>>> +check ovn-nbctl --wait=hv sync
>>> +
>>> +# Check OVS flows, the same conjunctive flows as above should still be there,
>>> +# with an additional conjunction action.
>>> +#
>>> +# New non-conjunctive flows should be added to match on 'udp'.
>>> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | ofctl_strip_all | \
>>> +   grep "priority=1003" | \
>>> +   sed 's/conjunction([[^)]]*)/conjunction()/g' | sort], [0], [dnl
>>> + table=45, priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
>>> + table=45, priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
>>> + table=45, priority=1003,conj_id=4,ip,metadata=0x1 actions=resubmit(,46)
>>> + table=45, priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(),conjunction(),conjunction()
>>> + table=45, priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(),conjunction(),conjunction()
>>> + table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46)
>>> + table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(),conjunction()
>>> + table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction()
>>> + table=45, priority=1003,udp,metadata=0x1 actions=resubmit(,46)
>>> + table=45, priority=1003,udp6,metadata=0x1 actions=resubmit(,46)
>>> +])
>>> +
>>>   OVN_CLEANUP([hv1])
>>>   AT_CLEANUP
>>>
>>>
>> Acked-by: Mark Gray <mark.d.gray at redhat.com>
>>
>> Also tested it.
> 
> Thanks Dumitru and Mark.

Thanks Mark and Numan!

> 
> I applied this patch to master and branch-20.12.  I'm not sure if it
> is required for 20.09 or not.
> It doesn't apply cleanly though.
> 

I couldn't manage to replicate the issue (or something similar) without 
the following commit which is not ported to 20.09:
   dadae4f800cc ("ofctrl.c: Only merge actions for conjunctive flows.")

I'm pretty sure the problem exists on 20.09 too but it might be very 
hard to hit.

I can prepare a 20.09 backport patch though if you think it's useful.

Regards,
Dumitru