On 2/24/23 14:06, Simon Horman wrote: > On Thu, Feb 23, 2023 at 05:39:27PM +0100, Ilya Maximets wrote: >> Tunnel OpenFlow ports do not exist in the datapath, instead there is a >> tunnel backing interface that serves all the tunnels of the same type. >> For example, if the geneve port 'my_tunnel' is added to OVS, it will >> create 'geneve_sys_6041' datapath port, if it doesn't already exist, >> and use this port as a tunnel output. >> >> However, while creating/opening a new datapath after re-start, >> ovs-vswitchd only has a list of names of OpenFlow interfaces. And it >> thinks that each datapath port, that is not on the list, is a stale >> port that needs to be removed. This is obviously not correct for >> tunnel backing interfaces that can serve multiple tunnel ports and do >> not match OpenFlow port names. >> >> This is causing removal and re-creation of all the tunnel backing >> interfaces in the datapath on OVS restart, causing disruption in >> existing connections. >> >> It's hard to tell by only having a name of the interface if this >> interface is a tunnel backing interface, or someone just named a >> normal interface this way. So, instead of trying to determine that, >> not removing any interfaces at all, while we don't know types of >> actual ports we need. >> >> Assuming that all the ports that are currently not in the list of OF >> ports are tunnel backing ports. Later, revalidation of tunnel backing >> ports in type_run() will determine which ports are still needed and >> which should be removed. >> >> It's OK to add even a non-tunnel stale ports into tnl_backers, they >> will be cleaned up the same way as stale tunnel backers. >> >> Reported-at: >> https://mail.openvswitch.org/pipermail/ovs-discuss/2023-February/052215.html >> Signed-off-by: Ilya Maximets <[email protected]> > > Thanks Ilya, > > this looks good to me. > I have a few comments below, but those notwithstanding, > > Reviewed-by: Simon Horman <[email protected]> > > ... > >> @@ -729,8 +723,6 @@ open_dpif_backer(const char *type, struct dpif_backer >> **backerp) >> struct dpif_port_dump port_dump; >> struct dpif_port port; >> struct shash_node *node; >> - struct ovs_list garbage_list; >> - struct odp_garbage *garbage; >> >> struct sset names; >> char *backer_name; >> @@ -792,25 +784,23 @@ open_dpif_backer(const char *type, struct dpif_backer >> **backerp) >> dpif_flow_flush(backer->dpif); >> } >> >> - /* Loop through the ports already on the datapath and remove any >> - * that we don't need anymore. */ >> - ovs_list_init(&garbage_list); >> + /* Loop through the ports already on the datapath and find ones that are >> + * not on the initial OpenFlow ports list. These are stale ports, that >> we >> + * do not need anymore, or tunnel backing interfaces, that do not >> generally >> + * match the name of OpenFlow tunnel ports, or both. Adding all of >> them to > > nit: s/Adding/Add/
OK. > >> + * the list of tunnel backers. type_run() will garbage collect those >> that >> + * are not active tunnel backing interfaces during revalidation. */ > > ... > >> diff --git a/tests/system-interface.at b/tests/system-interface.at >> index 784bada12..6cd8ae802 100644 >> --- a/tests/system-interface.at >> +++ b/tests/system-interface.at >> @@ -63,3 +63,62 @@ AT_CHECK([ >> [stdout], [Device "br-p1" does not exist.] >> ) >> AT_CLEANUP >> + >> +AT_SETUP([interface - datapath ports garbage collection]) >> +OVS_CHECK_GENEVE() >> +OVS_TRAFFIC_VSWITCHD_START() >> + >> +dnl Not relevant for userspace datapath. >> +AT_SKIP_IF([! ovs-appctl dpctl/show | grep -q ovs-system]) >> + >> +AT_CHECK([ovs-vsctl add-port br0 tunnel_port dnl >> + -- set Interface tunnel_port dnl >> + type=geneve options:remote_ip=flow options:key=123]) >> + >> +AT_CHECK([ip link add ovs-veth0 type veth peer name ovs-veth1]) >> +on_exit 'ip link del ovs-veth0' >> + >> +AT_CHECK([ovs-vsctl add-port br0 ovs-veth0]) >> + >> +OVS_WAIT_UNTIL([ip link show | grep ovs-system | grep -q genev]) >> + >> +dnl Store the output of ip link for geneve port to compare ifindex later. >> +AT_CHECK([ip link show | grep ovs-system | grep genev > geneve.0]) >> + >> +AT_CHECK([ovs-appctl dpctl/show | grep port], [0], [dnl >> + port 0: ovs-system (internal) >> + port 1: br0 (internal) >> + port 2: genev_sys_6081 (geneve: packet_type=ptap) >> + port 3: ovs-veth0 >> +]) >> + >> +OVS_APP_EXIT_AND_WAIT_BY_TARGET([ovs-vswitchd], [ovs-vswitchd.pid]) >> + >> +dnl Check that geneve backing interface is still in the datapath. >> +AT_CHECK([ip link show | grep ovs-system | grep genev | diff -u - geneve.0]) >> + >> +dnl Remove the veth port from the database while ovs-vswitchd is down. >> +AT_CHECK([ovs-vsctl --no-wait del-port ovs-veth0]) >> + >> +dnl Check that it is still tied to the OVS datapath. >> +AT_CHECK([ip link show | grep ovs-system | grep -q ovs-veth0]) >> + >> +dnl Bring ovs-vswitchd back up. >> +AT_CHECK([ovs-vswitchd --detach --no-chdir --pidfile --log-file -vdpif:dbg], >> + [0], [], [stderr]) >> + >> +dnl Wait for the veth port to be removed from the datapath. >> +OVS_WAIT_WHILE([ip link show | grep ovs-system | grep -q ovs-veth0]) >> + >> +AT_CHECK([ovs-appctl dpctl/show | grep port], [0], [dnl >> + port 0: ovs-system (internal) >> + port 1: br0 (internal) >> + port 2: genev_sys_6081 (geneve: packet_type=ptap) >> +]) >> + >> +dnl Check that geneve backing interface is still in the datapath and it >> wasn't >> +dnl re-created, i.e. the ifindex is the same. >> +AT_CHECK([ip link show | grep ovs-system | grep genev | diff -u - geneve.0]) >> + >> +OVS_TRAFFIC_VSWITCHD_STOP >> +AT_CLEANUP > > Its a personal preference, which in this case perhaps doesn't improve > anything, but I do like to minimise calls to grep. Sure. Doesn't hurt. I folded in the change below and applied the patch. Thanks! Since it's actually a bug fix, also backported down to 2.17. Best regards, Ilya Maximets. > > diff --git a/tests/system-interface.at b/tests/system-interface.at > index 6cd8ae802..3bf339582 100644 > --- a/tests/system-interface.at > +++ b/tests/system-interface.at > @@ -80,10 +80,10 @@ on_exit 'ip link del ovs-veth0' > > AT_CHECK([ovs-vsctl add-port br0 ovs-veth0]) > > -OVS_WAIT_UNTIL([ip link show | grep ovs-system | grep -q genev]) > +OVS_WAIT_UNTIL([ip link show | grep -q " genev_sys_[[0-9]]*: .* ovs-system > "]) > > dnl Store the output of ip link for geneve port to compare ifindex later. > -AT_CHECK([ip link show | grep ovs-system | grep genev > geneve.0]) > +AT_CHECK([ip link show | grep " genev_sys_[[0-9]]*: .* ovs-system " > > geneve.0]) > > AT_CHECK([ovs-appctl dpctl/show | grep port], [0], [dnl > port 0: ovs-system (internal) > @@ -95,20 +95,20 @@ AT_CHECK([ovs-appctl dpctl/show | grep port], [0], [dnl > OVS_APP_EXIT_AND_WAIT_BY_TARGET([ovs-vswitchd], [ovs-vswitchd.pid]) > > dnl Check that geneve backing interface is still in the datapath. > -AT_CHECK([ip link show | grep ovs-system | grep genev | diff -u - geneve.0]) > +AT_CHECK([ip link show | grep " genev_sys_[[0-9]]*: .* ovs-system " | diff > -u - geneve.0]) > > dnl Remove the veth port from the database while ovs-vswitchd is down. > AT_CHECK([ovs-vsctl --no-wait del-port ovs-veth0]) > > dnl Check that it is still tied to the OVS datapath. > -AT_CHECK([ip link show | grep ovs-system | grep -q ovs-veth0]) > +AT_CHECK([ip link show ovs-veth0 | grep -q ovs-system]) > > dnl Bring ovs-vswitchd back up. > AT_CHECK([ovs-vswitchd --detach --no-chdir --pidfile --log-file -vdpif:dbg], > [0], [], [stderr]) > > dnl Wait for the veth port to be removed from the datapath. > -OVS_WAIT_WHILE([ip link show | grep ovs-system | grep -q ovs-veth0]) > +OVS_WAIT_WHILE([ip link show ovs-veth0 | grep -q ovs-system]) > > AT_CHECK([ovs-appctl dpctl/show | grep port], [0], [dnl > port 0: ovs-system (internal) > @@ -118,7 +118,7 @@ AT_CHECK([ovs-appctl dpctl/show | grep port], [0], [dnl > > dnl Check that geneve backing interface is still in the datapath and it > wasn't > dnl re-created, i.e. the ifindex is the same. > -AT_CHECK([ip link show | grep ovs-system | grep genev | diff -u - geneve.0]) > +AT_CHECK([ip link show | grep " genev_sys_[[0-9]]*: .* ovs-system " | diff > -u - geneve.0]) > > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
