| Message ID | 20200719141509.3129129-1-i.maximets@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] ovs-router: Fix flushing local routes. | expand |
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 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@ucloud.cn> > Fixes: 8e4e45887ec3 ("ofproto-dpif-xlate: makes OVS native tunneling honor tunnel-specified source addresses") > Reported-by: Mike <glovejmm@163.com> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2020-July/373093.html > Signed-off-by: Ilya Maximets <i.maximets@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 Other part looks good to me, thanks William
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@ucloud.cn> >> Fixes: 8e4e45887ec3 ("ofproto-dpif-xlate: makes OVS native tunneling honor tunnel-specified source addresses") >> Reported-by: Mike <glovejmm@163.com> >> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2020-July/373093.html >> Signed-off-by: Ilya Maximets <i.maximets@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 >
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 @@ -25,3 +25,4 @@ m4_include([tests/system-kmod-macros.at]) m4_include([tests/system-traffic.at]) m4_include([tests/system-layer3-tunnels.at]) m4_include([tests/system-interface.at]) +m4_include([tests/system-route.at]) diff --git a/tests/system-route.at b/tests/system-route.at new file mode 100644 index 000000000..1714273e3 --- /dev/null +++ b/tests/system-route.at @@ -0,0 +1,28 @@ +AT_BANNER([system-route]) + +dnl Add an interface, add/del ip address, check that OVS catches route updates. +AT_SETUP([ovs-route - add/remove system route]) +AT_KEYWORDS([route]) +OVS_TRAFFIC_VSWITCHD_START() + +dnl Create tap port. +AT_CHECK([ip tuntap add name p1-route mode tap]) +AT_CHECK([ip link set p1-route up]) +on_exit 'ip link del p1-route' + +dnl Add ip address. +AT_CHECK([ip addr add 10.0.0.17/24 dev p1-route], [0], [stdout]) + +dnl Check that OVS catches route updates. +OVS_WAIT_UNTIL([ovs-appctl ovs/route/show | grep 'p1-route' | sort], [0], [dnl +Cached: 10.0.0.17/24 dev p1-route SRC 10.0.0.17 +Cached: 10.0.0.17/32 dev p1-route SRC 10.0.0.17 local +]) + +dnl Delete ip address. +AT_CHECK([ip addr del 10.0.0.17/24 dev p1-route], [0], [stdout]) +dnl Check that routes was removed from OVS. +OVS_WAIT_UNTIL([test `ovs-appctl ovs/route/show | grep -c 'p1-route'` -eq 0 ]) + +OVS_TRAFFIC_VSWITCHD_STOP +AT_CLEANUP
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 # 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@ucloud.cn> Fixes: 8e4e45887ec3 ("ofproto-dpif-xlate: makes OVS native tunneling honor tunnel-specified source addresses") Reported-by: Mike <glovejmm@163.com> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2020-July/373093.html Signed-off-by: Ilya Maximets <i.maximets@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