[ovs-dev] [PATCH] ovs-router: Fix flushing local routes.

[Ilya Maximets]

On 7/21/20 6:30 AM, William Tu wrote:
> On Sun, Jul 19, 2020 at 04:15:09PM +0200, Ilya Maximets wrote:
>> Since commit 8e4e45887ec3, priority of 'local' route entries no
>> longer matches with 'plen'.  This should be taken into account
>> while flushing cached routes, otherwise they will remain in OVS
>> even after removing them from the system:
>>
>>   # ifconfig eth0 11.1
>>   # ovs-appctl ovs/route/show
>>     --- A new route synchronized from kernel route table ---
>>     Cached: 11.0.0.1/32 dev eth0 SRC 11.0.0.1 local
> question:
> Why ifconfig eth0 11.1 generates a 11.0.0.1 route?

ifconfig translates 11.1 to 11.0.0.1 ip address.  But I'm not
sure why exactly.  OTOH, 'ip addr add' translates 11.1 to 11.1.0.0.
I'll update this example to not bring any confusion.

> 
>>   # ifconfig eth0 0
>>   # ovs-appctl ovs/route/show
>>     -- the new route entry is still in ovs route table ---
>>     Cached: 11.0.0.1/32 dev eth0 SRC 11.0.0.1 local
>>
>> CC: wenxu <wenxu at ucloud.cn>
>> Fixes: 8e4e45887ec3 ("ofproto-dpif-xlate: makes OVS native tunneling honor tunnel-specified source addresses")
>> Reported-by: Mike <glovejmm at 163.com>
>> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2020-July/373093.html
>> Signed-off-by: Ilya Maximets <i.maximets at ovn.org>
>> ---
>>  lib/ovs-router.c               |  2 +-
>>  tests/automake.mk              |  3 ++-
>>  tests/system-kmod-testsuite.at |  1 +
>>  tests/system-route.at          | 28 ++++++++++++++++++++++++++++
>>  4 files changed, 32 insertions(+), 2 deletions(-)
>>  create mode 100644 tests/system-route.at
>>
>> diff --git a/lib/ovs-router.c b/lib/ovs-router.c
>> index bfb2b7071..09b81c6e5 100644
>> --- a/lib/ovs-router.c
>> +++ b/lib/ovs-router.c
>> @@ -505,7 +505,7 @@ ovs_router_flush(void)
>>      ovs_mutex_lock(&mutex);
>>      classifier_defer(&cls);
>>      CLS_FOR_EACH(rt, cr, &cls) {
>> -        if (rt->priority == rt->plen) {
>> +        if (rt->priority == rt->plen || rt->local) {
>>              rt_entry_delete__(&rt->cr);
>>          }
>>      }
>> diff --git a/tests/automake.mk b/tests/automake.mk
>> index cbba5b170..0f0562b19 100644
>> --- a/tests/automake.mk
>> +++ b/tests/automake.mk
>> @@ -149,7 +149,8 @@ OVSDB_CLUSTER_TESTSUITE_AT = \
>>  SYSTEM_KMOD_TESTSUITE_AT = \
>>  	tests/system-common-macros.at \
>>  	tests/system-kmod-testsuite.at \
>> -	tests/system-kmod-macros.at
>> +	tests/system-kmod-macros.at \
>> +	tests/system-route.at
>>  
>>  SYSTEM_USERSPACE_TESTSUITE_AT = \
>>  	tests/system-userspace-testsuite.at \
>> diff --git a/tests/system-kmod-testsuite.at b/tests/system-kmod-testsuite.at
>> index 3de0290c0..ff2ecd0ba 100644
>> --- a/tests/system-kmod-testsuite.at
>> +++ b/tests/system-kmod-testsuite.at
> 
> Why adding to system-kmod?
> I thought this is needed only for system-userspace-testsuite.at

Yes, you're right.  This makes more sense inside system-userspace
testsuite.  I put it here because I thought that we're disabling
system routes in system userspace testsuite too, but we're not.

I'll move it.

> 
> Other part looks good to me, thanks
> William
>