| Message ID | 20200228171532.918740-1-blp@ovn.org |
|---|---|
| State | Accepted |
| Commit | 7cc77b301f80a63cd4893198d82be0eef303f731 |
| Headers | show |
| Series | [ovs-dev] ofproto-dpif: Only delete tunnel backer ports along with the dpif. | expand |
Hi Ben,
Thanks for the patch. I can confirm that when applying the patch, tunnel ports are no longer deleted when ovs-vswitchd exits.
I ran a test with a VXLAN tunnel between 2 VMs. The vxlan_sys_4789 interface is not deleted after stopping the OVS daemons.
When I ping one VM from the other, this is what I get without the patch:
--- 10.10.0.3 ping statistics ---
33 packets transmitted, 28 packets received, 15% packet loss
round-trip min/avg/max = 0.342/37.426/1030.260 ms
and this is with the patch:
--- 10.10.0.3 ping statistics ---
32 packets transmitted, 31 packets received, 3% packet loss
round-trip min/avg/max = 0.315/0.777/2.460 ms
When ovs-vswitchd restarts, the vxlan_sys_4789 interface is re-created with a different MAC address and the datapath flows are flushed. This explains the 1 ICMP packet lost.
Thanks,
Antonin
On 2/28/20, 9:15 AM, "Ben Pfaff" <blp@ovn.org> wrote:
The admin can choose whether or not to delete flows from datapaths when
they stop ovs-vswitchd. The goal of not deleting flows it to allow
existing traffic to continue being forwarded until ovs-vswitchd is
restarted. Until now, regardless of this choice, ovs-vswitchd has
always deleted tunnel ports from the datapath. When flows are not
deleted, this nevertheless prevents tunnel traffic from being forwarded.
With this patch, ovs-vswitchd no longer deletes tunnel ports in the
case where it does not delete flows, allowing tunnel traffic to continue
being forwarded.
Reported-by: Antonin Bas <abas@vmware.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
---
I don't have a convenient setup to test this, so I need someone
(Antonin?) to test it for me.
ofproto/ofproto-dpif.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 0222ec82f00a..d56cece95e90 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -698,8 +698,10 @@ close_dpif_backer(struct dpif_backer *backer, bool del)
udpif_destroy(backer->udpif);
- SIMAP_FOR_EACH (node, &backer->tnl_backers) {
- dpif_port_del(backer->dpif, u32_to_odp(node->data), false);
+ if (del) {
+ SIMAP_FOR_EACH (node, &backer->tnl_backers) {
+ dpif_port_del(backer->dpif, u32_to_odp(node->data), false);
+ }
}
simap_destroy(&backer->tnl_backers);
ovs_rwlock_destroy(&backer->odp_to_ofport_lock);
--
2.24.1
Thanks for testing! I'm a little surprised that restarting ovs-vswitchd causes the interface to be deleted and recreated. That might be a bug that should be addressed separately. On Sat, Feb 29, 2020 at 12:16:32AM +0000, Antonin Bas wrote: > Hi Ben, > > Thanks for the patch. I can confirm that when applying the patch, tunnel ports are no longer deleted when ovs-vswitchd exits. > > I ran a test with a VXLAN tunnel between 2 VMs. The vxlan_sys_4789 interface is not deleted after stopping the OVS daemons. > > When I ping one VM from the other, this is what I get without the patch: > --- 10.10.0.3 ping statistics --- > 33 packets transmitted, 28 packets received, 15% packet loss > round-trip min/avg/max = 0.342/37.426/1030.260 ms > > and this is with the patch: > --- 10.10.0.3 ping statistics --- > 32 packets transmitted, 31 packets received, 3% packet loss > round-trip min/avg/max = 0.315/0.777/2.460 ms > > When ovs-vswitchd restarts, the vxlan_sys_4789 interface is re-created with a different MAC address and the datapath flows are flushed. This explains the 1 ICMP packet lost. > > Thanks, > > Antonin > > On 2/28/20, 9:15 AM, "Ben Pfaff" <blp@ovn.org> wrote: > > The admin can choose whether or not to delete flows from datapaths when > they stop ovs-vswitchd. The goal of not deleting flows it to allow > existing traffic to continue being forwarded until ovs-vswitchd is > restarted. Until now, regardless of this choice, ovs-vswitchd has > always deleted tunnel ports from the datapath. When flows are not > deleted, this nevertheless prevents tunnel traffic from being forwarded. > > With this patch, ovs-vswitchd no longer deletes tunnel ports in the > case where it does not delete flows, allowing tunnel traffic to continue > being forwarded. > > Reported-by: Antonin Bas <abas@vmware.com> > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > I don't have a convenient setup to test this, so I need someone > (Antonin?) to test it for me. > > ofproto/ofproto-dpif.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 0222ec82f00a..d56cece95e90 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -698,8 +698,10 @@ close_dpif_backer(struct dpif_backer *backer, bool del) > > udpif_destroy(backer->udpif); > > - SIMAP_FOR_EACH (node, &backer->tnl_backers) { > - dpif_port_del(backer->dpif, u32_to_odp(node->data), false); > + if (del) { > + SIMAP_FOR_EACH (node, &backer->tnl_backers) { > + dpif_port_del(backer->dpif, u32_to_odp(node->data), false); > + } > } > simap_destroy(&backer->tnl_backers); > ovs_rwlock_destroy(&backer->odp_to_ofport_lock); > -- > 2.24.1 > > >
Also: I applied this to master. On Sat, Feb 29, 2020 at 11:06:54AM -0800, Ben Pfaff wrote: > Thanks for testing! > > I'm a little surprised that restarting ovs-vswitchd causes the interface > to be deleted and recreated. That might be a bug that should be > addressed separately. > > On Sat, Feb 29, 2020 at 12:16:32AM +0000, Antonin Bas wrote: > > Hi Ben, > > > > Thanks for the patch. I can confirm that when applying the patch, tunnel ports are no longer deleted when ovs-vswitchd exits. > > > > I ran a test with a VXLAN tunnel between 2 VMs. The vxlan_sys_4789 interface is not deleted after stopping the OVS daemons. > > > > When I ping one VM from the other, this is what I get without the patch: > > --- 10.10.0.3 ping statistics --- > > 33 packets transmitted, 28 packets received, 15% packet loss > > round-trip min/avg/max = 0.342/37.426/1030.260 ms > > > > and this is with the patch: > > --- 10.10.0.3 ping statistics --- > > 32 packets transmitted, 31 packets received, 3% packet loss > > round-trip min/avg/max = 0.315/0.777/2.460 ms > > > > When ovs-vswitchd restarts, the vxlan_sys_4789 interface is re-created with a different MAC address and the datapath flows are flushed. This explains the 1 ICMP packet lost. > > > > Thanks, > > > > Antonin > > > > On 2/28/20, 9:15 AM, "Ben Pfaff" <blp@ovn.org> wrote: > > > > The admin can choose whether or not to delete flows from datapaths when > > they stop ovs-vswitchd. The goal of not deleting flows it to allow > > existing traffic to continue being forwarded until ovs-vswitchd is > > restarted. Until now, regardless of this choice, ovs-vswitchd has > > always deleted tunnel ports from the datapath. When flows are not > > deleted, this nevertheless prevents tunnel traffic from being forwarded. > > > > With this patch, ovs-vswitchd no longer deletes tunnel ports in the > > case where it does not delete flows, allowing tunnel traffic to continue > > being forwarded. > > > > Reported-by: Antonin Bas <abas@vmware.com> > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > --- > > I don't have a convenient setup to test this, so I need someone > > (Antonin?) to test it for me. > > > > ofproto/ofproto-dpif.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > > index 0222ec82f00a..d56cece95e90 100644 > > --- a/ofproto/ofproto-dpif.c > > +++ b/ofproto/ofproto-dpif.c > > @@ -698,8 +698,10 @@ close_dpif_backer(struct dpif_backer *backer, bool del) > > > > udpif_destroy(backer->udpif); > > > > - SIMAP_FOR_EACH (node, &backer->tnl_backers) { > > - dpif_port_del(backer->dpif, u32_to_odp(node->data), false); > > + if (del) { > > + SIMAP_FOR_EACH (node, &backer->tnl_backers) { > > + dpif_port_del(backer->dpif, u32_to_odp(node->data), false); > > + } > > } > > simap_destroy(&backer->tnl_backers); > > ovs_rwlock_destroy(&backer->odp_to_ofport_lock); > > -- > > 2.24.1 > > > > > >
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 0222ec82f00a..d56cece95e90 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -698,8 +698,10 @@ close_dpif_backer(struct dpif_backer *backer, bool del) udpif_destroy(backer->udpif); - SIMAP_FOR_EACH (node, &backer->tnl_backers) { - dpif_port_del(backer->dpif, u32_to_odp(node->data), false); + if (del) { + SIMAP_FOR_EACH (node, &backer->tnl_backers) { + dpif_port_del(backer->dpif, u32_to_odp(node->data), false); + } } simap_destroy(&backer->tnl_backers); ovs_rwlock_destroy(&backer->odp_to_ofport_lock);
The admin can choose whether or not to delete flows from datapaths when they stop ovs-vswitchd. The goal of not deleting flows it to allow existing traffic to continue being forwarded until ovs-vswitchd is restarted. Until now, regardless of this choice, ovs-vswitchd has always deleted tunnel ports from the datapath. When flows are not deleted, this nevertheless prevents tunnel traffic from being forwarded. With this patch, ovs-vswitchd no longer deletes tunnel ports in the case where it does not delete flows, allowing tunnel traffic to continue being forwarded. Reported-by: Antonin Bas <abas@vmware.com> Signed-off-by: Ben Pfaff <blp@ovn.org> --- I don't have a convenient setup to test this, so I need someone (Antonin?) to test it for me. ofproto/ofproto-dpif.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)