| Message ID | 20240312140411.3446868-1-taoliu828@163.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ofproto-dpif: Fix vxlan with different name del/add failed. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/intel-ovs-compilation | success | test: success |
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Thanks Tao for fixing this. I think the title can be more generic because this problem and fix applies to all tunnel types rather than just VXLAN. On Tue, Mar 12, 2024 at 7:04 AM Tao Liu <taoliu828@163.com> wrote: > > Reproduce: > ovs-vsctl add-port br-int p0 \ > -- set interface p0 type=vxlan options:remote_ip=10.10.10.1 > > sleep 2 > > ovs-vsctl --if-exists del-port p0 \ > -- add-port br-int p1 \ > -- set interface p1 type=vxlan options:remote_ip=10.10.10.1 > ovs-vsctl: Error detected while setting up 'p1': could not add > network device p1 to ofproto (File exists). > > vswitchd log: > bridge|INFO|bridge br-int: added interface p0 on port 1106 > bridge|INFO|bridge br-int: deleted interface p0 on port 1106 > tunnel|WARN|p1: attempting to add tunnel port with same config as port 'p0' (::->10.10.10.1, key=0, legacy_l2, dp port=122) > ofproto|WARN|br-int: could not add port p1 (File exists) > bridge|WARN|could not add network device p1 to ofproto (File exists) > > CallTrace: > bridge_reconfigure > bridge_del_ports > port_destroy > iface_destroy__ > netdev_remove <------ netdev p0 removed > bridge_delete_or_reconfigure_ports > OFPROTO_PORT_FOR_EACH > ofproto_port_dump_next > port_dump_next > port_query_by_name <------ netdev_shash do not contain p0 > ofproto_port_del <------ p0 do not del in ofproto > bridge_add_ports > bridge_add_ports__ > iface_create > iface_do_create > ofproto_port_add <------ p1 add failed > > Fixes: fe83f81df977 ("netdev: Remove netdev from global shash when the user is changing interface configuration.") > Signed-off-by: Tao Liu <taoliu828@163.com> > --- > ofproto/ofproto-dpif.c | 13 +++++++++---- > tests/tunnel.at | 12 ++++++++++++ > 2 files changed, 21 insertions(+), 4 deletions(-) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index f59d69c4d..0cac3050d 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -3905,14 +3905,19 @@ port_query_by_name(const struct ofproto *ofproto_, const char *devname, > > if (sset_contains(&ofproto->ghost_ports, devname)) { > const char *type = netdev_get_type_from_name(devname); > + const struct ofport *ofport = > + shash_find_data(&ofproto->up.port_by_name, devname); > + if (!type && ofport && ofport->netdev) { > + type = netdev_get_type(ofport->netdev); > + } > > /* We may be called before ofproto->up.port_by_name is populated with > * the appropriate ofport. For this reason, we must get the name and > - * type from the netdev layer directly. */ > + * type from the netdev layer directly. > + * When a port deleted, the corresponding netdev is also removed from > + * netdev_shash. netdev_get_type_from_name returns NULL in such case. > + * We should try to get type from ofport->netdev. */ nit: this comment is better to be moved to the above where we are trying to get the type from ofport. Otherwise looks good to me: Acked-by: Han Zhou <hzhou@ovn.org> Tested-by: Han Zhou <hzhou@ovn.org> > if (type) { > - const struct ofport *ofport; > - > - ofport = shash_find_data(&ofproto->up.port_by_name, devname); > ofproto_port->ofp_port = ofport ? ofport->ofp_port : OFPP_NONE; > ofproto_port->name = xstrdup(devname); > ofproto_port->type = xstrdup(type); > diff --git a/tests/tunnel.at b/tests/tunnel.at > index 71e7c2df4..9d539ee6f 100644 > --- a/tests/tunnel.at > +++ b/tests/tunnel.at > @@ -1269,6 +1269,18 @@ OVS_APP_EXIT_AND_WAIT([ovs-vswitchd]) > OVS_APP_EXIT_AND_WAIT([ovsdb-server])] > AT_CLEANUP > > +AT_SETUP([tunnel - re-create port with different name]) > +OVS_VSWITCHD_START( > + [add-port br0 p0 -- set int p0 type=vxlan options:remote_ip=10.10.10.1]) > + > +AT_CHECK([ovs-vsctl --if-exists del-port p0 -- \ > + add-port br0 p1 -- \ > + set int p1 type=vxlan options:remote_ip=10.10.10.1]) > + > +OVS_APP_EXIT_AND_WAIT([ovs-vswitchd]) > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])] > +AT_CLEANUP > + > AT_SETUP([tunnel - SRV6 basic]) > OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=dummy \ > ofport_request=1 \ > -- > 2.31.1 >
On 3/14/24 05:51, Han Zhou wrote: > > Thanks Tao for fixing this. I think the title can be more generic because this problem and fix applies to all tunnel types rather than just VXLAN. > > On Tue, Mar 12, 2024 at 7:04 AM Tao Liu <taoliu828@163.com <mailto:taoliu828@163.com>> wrote: >> >> Reproduce: >> ovs-vsctl add-port br-int p0 \ >> -- set interface p0 type=vxlan options:remote_ip=10.10.10.1 >> >> sleep 2 >> >> ovs-vsctl --if-exists del-port p0 \ >> -- add-port br-int p1 \ >> -- set interface p1 type=vxlan options:remote_ip=10.10.10.1 >> ovs-vsctl: Error detected while setting up 'p1': could not add >> network device p1 to ofproto (File exists). >> >> vswitchd log: >> bridge|INFO|bridge br-int: added interface p0 on port 1106 >> bridge|INFO|bridge br-int: deleted interface p0 on port 1106 >> tunnel|WARN|p1: attempting to add tunnel port with same config as port 'p0' (::->10.10.10.1, key=0, legacy_l2, dp port=122) >> ofproto|WARN|br-int: could not add port p1 (File exists) >> bridge|WARN|could not add network device p1 to ofproto (File exists) >> >> CallTrace: >> bridge_reconfigure >> bridge_del_ports >> port_destroy >> iface_destroy__ >> netdev_remove <------ netdev p0 removed >> bridge_delete_or_reconfigure_ports >> OFPROTO_PORT_FOR_EACH >> ofproto_port_dump_next >> port_dump_next >> port_query_by_name <------ netdev_shash do not contain p0 >> ofproto_port_del <------ p0 do not del in ofproto >> bridge_add_ports >> bridge_add_ports__ >> iface_create >> iface_do_create >> ofproto_port_add <------ p1 add failed >> >> Fixes: fe83f81df977 ("netdev: Remove netdev from global shash when the user is changing interface configuration.") >> Signed-off-by: Tao Liu <taoliu828@163.com <mailto:taoliu828@163.com>> >> --- >> ofproto/ofproto-dpif.c | 13 +++++++++---- >> tests/tunnel.at <http://tunnel.at> | 12 ++++++++++++ >> 2 files changed, 21 insertions(+), 4 deletions(-) >> >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >> index f59d69c4d..0cac3050d 100644 >> --- a/ofproto/ofproto-dpif.c >> +++ b/ofproto/ofproto-dpif.c >> @@ -3905,14 +3905,19 @@ port_query_by_name(const struct ofproto *ofproto_, const char *devname, >> >> if (sset_contains(&ofproto->ghost_ports, devname)) { >> const char *type = netdev_get_type_from_name(devname); >> + const struct ofport *ofport = >> + shash_find_data(&ofproto->up.port_by_name, devname); >> + if (!type && ofport && ofport->netdev) { >> + type = netdev_get_type(ofport->netdev); >> + } >> >> /* We may be called before ofproto->up.port_by_name is populated with >> * the appropriate ofport. For this reason, we must get the name and >> - * type from the netdev layer directly. */ >> + * type from the netdev layer directly. >> + * When a port deleted, the corresponding netdev is also removed from >> + * netdev_shash. netdev_get_type_from_name returns NULL in such case. >> + * We should try to get type from ofport->netdev. */ > > nit: this comment is better to be moved to the above where we are trying to get the type from ofport. > > Otherwise looks good to me: > Acked-by: Han Zhou <hzhou@ovn.org <mailto:hzhou@ovn.org>> > Tested-by: Han Zhou <hzhou@ovn.org <mailto:hzhou@ovn.org>> Thanks, Tao and Han! I fixed the nits and applied the change. Also backported down to 2.17. Best regards, Ilya Maximets.
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index f59d69c4d..0cac3050d 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -3905,14 +3905,19 @@ port_query_by_name(const struct ofproto *ofproto_, const char *devname, if (sset_contains(&ofproto->ghost_ports, devname)) { const char *type = netdev_get_type_from_name(devname); + const struct ofport *ofport = + shash_find_data(&ofproto->up.port_by_name, devname); + if (!type && ofport && ofport->netdev) { + type = netdev_get_type(ofport->netdev); + } /* We may be called before ofproto->up.port_by_name is populated with * the appropriate ofport. For this reason, we must get the name and - * type from the netdev layer directly. */ + * type from the netdev layer directly. + * When a port deleted, the corresponding netdev is also removed from + * netdev_shash. netdev_get_type_from_name returns NULL in such case. + * We should try to get type from ofport->netdev. */ if (type) { - const struct ofport *ofport; - - ofport = shash_find_data(&ofproto->up.port_by_name, devname); ofproto_port->ofp_port = ofport ? ofport->ofp_port : OFPP_NONE; ofproto_port->name = xstrdup(devname); ofproto_port->type = xstrdup(type); diff --git a/tests/tunnel.at b/tests/tunnel.at index 71e7c2df4..9d539ee6f 100644 --- a/tests/tunnel.at +++ b/tests/tunnel.at @@ -1269,6 +1269,18 @@ OVS_APP_EXIT_AND_WAIT([ovs-vswitchd]) OVS_APP_EXIT_AND_WAIT([ovsdb-server])] AT_CLEANUP +AT_SETUP([tunnel - re-create port with different name]) +OVS_VSWITCHD_START( + [add-port br0 p0 -- set int p0 type=vxlan options:remote_ip=10.10.10.1]) + +AT_CHECK([ovs-vsctl --if-exists del-port p0 -- \ + add-port br0 p1 -- \ + set int p1 type=vxlan options:remote_ip=10.10.10.1]) + +OVS_APP_EXIT_AND_WAIT([ovs-vswitchd]) +OVS_APP_EXIT_AND_WAIT([ovsdb-server])] +AT_CLEANUP + AT_SETUP([tunnel - SRV6 basic]) OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=dummy \ ofport_request=1 \
Reproduce: ovs-vsctl add-port br-int p0 \ -- set interface p0 type=vxlan options:remote_ip=10.10.10.1 sleep 2 ovs-vsctl --if-exists del-port p0 \ -- add-port br-int p1 \ -- set interface p1 type=vxlan options:remote_ip=10.10.10.1 ovs-vsctl: Error detected while setting up 'p1': could not add network device p1 to ofproto (File exists). vswitchd log: bridge|INFO|bridge br-int: added interface p0 on port 1106 bridge|INFO|bridge br-int: deleted interface p0 on port 1106 tunnel|WARN|p1: attempting to add tunnel port with same config as port 'p0' (::->10.10.10.1, key=0, legacy_l2, dp port=122) ofproto|WARN|br-int: could not add port p1 (File exists) bridge|WARN|could not add network device p1 to ofproto (File exists) CallTrace: bridge_reconfigure bridge_del_ports port_destroy iface_destroy__ netdev_remove <------ netdev p0 removed bridge_delete_or_reconfigure_ports OFPROTO_PORT_FOR_EACH ofproto_port_dump_next port_dump_next port_query_by_name <------ netdev_shash do not contain p0 ofproto_port_del <------ p0 do not del in ofproto bridge_add_ports bridge_add_ports__ iface_create iface_do_create ofproto_port_add <------ p1 add failed Fixes: fe83f81df977 ("netdev: Remove netdev from global shash when the user is changing interface configuration.") Signed-off-by: Tao Liu <taoliu828@163.com> --- ofproto/ofproto-dpif.c | 13 +++++++++---- tests/tunnel.at | 12 ++++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-)