Re: [ovs-dev] [PATCH v3] route-table: Filter route changes by interface.

2024-03-26 Thread Cheng Li
On Tue, Mar 26, 2024 at 06:42:02PM +0100, 【外部账号】Ilya Maximets wrote:
> On 3/24/24 13:16, Cheng Li wrote:
> > When ovs host is also a kubernets node, pod creation/deletion may
> > trigger route changes. As a result, ovs run route_table_reset().
> > As ovs do not care the kubernetes pod routes, route_table_reset()
> > is not neccessary.
> > 
> > Signed-off-by: Cheng Li 
> > ---
> 
> Hi, Cheng Li.  Thanks for the patch!
> 
> I'm a little confused by the use case though.  Could you explain
> a bit more why this is a problem (route dump is relatively fast,
> unless there are millions of routes) and how this change helps?
> 
> AFAIU, routes will not change much in kubernetes environment once
> the pod is initially created and configured and the port creation
> will trigger route cache reset anyway.
> 

Hi Ilya, thanks for reviewing this patch.

When calico is used as kubernets cni and IPinIP overlay mode[1] is
enabled, each time a pod created/deleted a route(dev tunl0) is
avertised across all cluster nodes.
```
# ip monitor route
10.233.75.61 via 11.46.8.90 dev tunl0 proto bird onlink
```

If we have a large cluster, route update may happen in high
frequency, which triggers lots of route_table_reset().

In route_table_reset(), all ovs route items are first deleted
then add latest kernel route items. There is a time gap between old
route items all deleted and new route items not ready. During this
gap, upcall/revalidation generate bad datapath flows.  As ovs does not
care kuberntes route changes, seems adding a filter is the simple way
to resolve this issue.

[1] https://docs.tigera.io/calico/latest/networking/configuring/vxlan-ipip

> And we will need to reset the cache when new interfaces are
> added/removed from the filter, because otherwise we'll have
> stale routes in there and the cache may become inconsistent.
> 

Make sense, will fix in next version.

> Best regards, Ilya Maximets
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH v3] route-table: Filter route changes by interface.

2024-03-26 Thread Ilya Maximets
On 3/24/24 13:16, Cheng Li wrote:
> When ovs host is also a kubernets node, pod creation/deletion may
> trigger route changes. As a result, ovs run route_table_reset().
> As ovs do not care the kubernetes pod routes, route_table_reset()
> is not neccessary.
> 
> Signed-off-by: Cheng Li 
> ---

Hi, Cheng Li.  Thanks for the patch!

I'm a little confused by the use case though.  Could you explain
a bit more why this is a problem (route dump is relatively fast,
unless there are millions of routes) and how this change helps?

AFAIU, routes will not change much in kubernetes environment once
the pod is initially created and configured and the port creation
will trigger route cache reset anyway.

And we will need to reset the cache when new interfaces are
added/removed from the filter, because otherwise we'll have
stale routes in there and the cache may become inconsistent.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


[ovs-dev] [PATCH v3] route-table: Filter route changes by interface.

2024-03-24 Thread Cheng Li
When ovs host is also a kubernets node, pod creation/deletion may
trigger route changes. As a result, ovs run route_table_reset().
As ovs do not care the kubernetes pod routes, route_table_reset()
is not neccessary.

Signed-off-by: Cheng Li 
---

Notes:
v2: Add function definition for bsd and stub.
v3: Fix unused-parameter error.

 lib/route-table-bsd.c  |  5 +
 lib/route-table-stub.c |  5 +
 lib/route-table.c  | 41 ++---
 lib/route-table.h  |  1 +
 tests/system-route.at  | 51 ++
 vswitchd/bridge.c  |  3 +++
 vswitchd/vswitch.xml   | 10 +
 7 files changed, 113 insertions(+), 3 deletions(-)

diff --git a/lib/route-table-bsd.c b/lib/route-table-bsd.c
index 34d42cfab..4762e2194 100644
--- a/lib/route-table-bsd.c
+++ b/lib/route-table-bsd.c
@@ -205,3 +205,8 @@ void
 route_table_wait(void)
 {
 }
+
+void
+disable_notify_on_interfaces(const char *ifaces OVS_UNUSED)
+{
+}
diff --git a/lib/route-table-stub.c b/lib/route-table-stub.c
index dd0b096d4..cc13c5191 100644
--- a/lib/route-table-stub.c
+++ b/lib/route-table-stub.c
@@ -48,3 +48,8 @@ void
 route_table_wait(void)
 {
 }
+
+void
+disable_notify_on_interfaces(const char *ifaces OVS_UNUSED)
+{
+}
diff --git a/lib/route-table.c b/lib/route-table.c
index f1fe32714..eeff509f0 100644
--- a/lib/route-table.c
+++ b/lib/route-table.c
@@ -33,6 +33,7 @@
 #include "netlink-notifier.h"
 #include "netlink-socket.h"
 #include "openvswitch/ofpbuf.h"
+#include "lib/sset.h"
 #include "ovs-router.h"
 #include "packets.h"
 #include "rtnetlink.h"
@@ -82,6 +83,7 @@ static struct nln_notifier *route6_notifier = NULL;
 static struct nln_notifier *name_notifier = NULL;
 
 static bool route_table_valid = false;
+static struct sset disabled_ifaces = SSET_INITIALIZER(_ifaces);
 
 static void route_table_reset(void);
 static void route_table_handle_msg(const struct route_table_msg *);
@@ -92,6 +94,7 @@ static void route_map_clear(void);
 static void name_table_init(void);
 static void name_table_change(const struct rtnetlink_change *, void *);
 
+
 uint64_t
 route_table_get_change_seq(void)
 {
@@ -354,13 +357,45 @@ route_table_parse(struct ofpbuf *buf, struct 
route_table_msg *change)
 return ipv4 ? RTNLGRP_IPV4_ROUTE : RTNLGRP_IPV6_ROUTE;
 }
 
+void
+disable_notify_on_interfaces(const char *ifaces)
+{
+struct sset tmp_ifaces;
+
+if (ifaces) {
+sset_from_delimited_string(_ifaces, ifaces, ", ");
+} else {
+sset_init(_ifaces);
+}
+if (! sset_equals(_ifaces, _ifaces)) {
+const char *iface;
+struct ds ds = DS_EMPTY_INITIALIZER;
+
+sset_swap(_ifaces, _ifaces);
+SSET_FOR_EACH (iface, _ifaces) {
+ds_put_format(, " %s", iface);
+}
+VLOG_DBG_RL(, "route notify disabled interfaces: [%s]",
+ds_cstr());
+ds_destroy();
+}
+sset_destroy(_ifaces);
+}
+
 static void
-route_table_change(const struct route_table_msg *change OVS_UNUSED,
+route_table_change(const struct route_table_msg *change,
void *aux OVS_UNUSED)
 {
-if (!change || change->relevant) {
-route_table_valid = false;
+if (change) {
+if (!change->relevant) {
+return;
+}
+if (change->rd.ifname[0] != '\0' &&
+sset_contains(_ifaces, change->rd.ifname)) {
+return;
+}
 }
+route_table_valid = false;
 }
 
 static void
diff --git a/lib/route-table.h b/lib/route-table.h
index 3a02d737a..716e5bae0 100644
--- a/lib/route-table.h
+++ b/lib/route-table.h
@@ -33,4 +33,5 @@ void route_table_wait(void);
 bool route_table_fallback_lookup(const struct in6_addr *ip6_dst,
  char name[],
  struct in6_addr *gw6);
+void disable_notify_on_interfaces(const char *ifaces);
 #endif /* route-table.h */
diff --git a/tests/system-route.at b/tests/system-route.at
index c0ecad6cf..039255df7 100644
--- a/tests/system-route.at
+++ b/tests/system-route.at
@@ -128,3 +128,54 @@ OVS_WAIT_UNTIL([test $(ovs-appctl ovs/route/show | grep -c 
'p1-route') -eq 0 ])
 
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
+
+
+dnl Checks that disabled interface doesn't trigger route table refresh.
+AT_SETUP([ovs-route - filter by interface])
+AT_KEYWORDS([route])
+OVS_TRAFFIC_VSWITCHD_START()
+
+dnl Create tap port.
+on_exit 'ip link del p1-route; ip link del p2-route'
+AT_CHECK([ip tuntap add name p1-route mode tap])
+AT_CHECK([ip tuntap add name p2-route mode tap])
+AT_CHECK([ip link set p1-route up])
+AT_CHECK([ip link set p2-route up])
+
+dnl Add ip address.
+AT_CHECK([ip addr add 10.0.0.17/24 dev p1-route], [0], [stdout])
+AT_CHECK([ip addr add 10.0.1.17/24 dev p2-route], [0], [stdout])
+
+dnl Check that OVS catches route updates.
+OVS_WAIT_UNTIL_EQUAL([ovs-appctl ovs/route/show | grep -P 'p(1|2)-route' | 
sort], [dnl
+Cached: 10.0.0.0/24 dev p1-route SRC 10.0.0.17
+Cached: 10.0.0.17/32 dev