| Message ID | 20230719161404.677791-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Commit | 47520b33bdf80d039f25d2faa25f4fcdf55143ea |
| Headers | show |
| Series | [ovs-dev] ofproto-dpif: Fix removal of renamed datapath ports. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
Thanks for the quick patch! Tested-by: Alin-Gabriel Serdean <aserdean@ovn.org> Acked-by: Alin-Gabriel Serdean <aserdean@ovn.org>
Ilya Maximets <i.maximets@ovn.org> writes: > OVS configuration is based on port names and OpenFlow port numbers. > Names are stored in the database and translated later to OF ports. > On the datapath level, each port has a name and a datapath port number. > Port name in the database has to match datapath port name, unless it's > a tunnel port. > > If a datapath port is renamed with 'ip link set DEV name NAME', > ovs-vswitchd will wake up, destroy all the OpenFlow-related structures > and clean other things up. This is because the port no longer > represents the port from a database due to a name difference. > > However, ovs-vswitch will not actually remove the port from the > datapath, because it thinks that this port is no longer there. This > is happening because lookup is performed by name and the name have > changed. As a result we have a port in a datapath that is not related > to any port known to ovs-vswitchd and ovs-vswitchd can't remove it. > This port also occupies a datapath port number and prevents the port > to be added back with a new name. > > Fix that by performing lookup by a datapath port number during the port > destruction. The name was used only to avoid spurious warnings in a > normal case where the port was successfully deleted by other parts of > OVS. Adding an extra flag to avoid these warnings instead. > > Fixes: 02f8d6460afd ("ofproto-dpif: Query port existence by name to prevent warnings.") > Reported-at: https://github.com/openvswitch/ovs-issues/issues/284 > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > lib/dpctl.c | 2 +- > lib/dpif.c | 13 +++++---- > lib/dpif.h | 2 +- > ofproto/ofproto-dpif.c | 14 ++++++---- > tests/system-interface.at | 57 +++++++++++++++++++++++++++++++++++++++ > 5 files changed, 76 insertions(+), 12 deletions(-) > > diff --git a/lib/dpctl.c b/lib/dpctl.c > index 4394653ab..79b82a176 100644 > --- a/lib/dpctl.c > +++ b/lib/dpctl.c > @@ -673,7 +673,7 @@ show_dpif(struct dpif *dpif, struct dpctl_params *dpctl_p) > } > > for (int i = 0; i < n_port_nos; i++) { > - if (dpif_port_query_by_number(dpif, port_nos[i], &dpif_port)) { > + if (dpif_port_query_by_number(dpif, port_nos[i], &dpif_port, true)) { > continue; > } > > diff --git a/lib/dpif.c b/lib/dpif.c > index b1cbf39c4..29041ab91 100644 > --- a/lib/dpif.c > +++ b/lib/dpif.c > @@ -705,13 +705,14 @@ dpif_port_set_config(struct dpif *dpif, odp_port_t port_no, > * initializes '*port' appropriately; on failure, returns a positive errno > * value. > * > - * Retuns ENODEV if the port doesn't exist. > + * Retuns ENODEV if the port doesn't exist. Will not log a warning in this > + * case unless 'warn_if_not_found' is true. > * > * The caller owns the data in 'port' and must free it with > * dpif_port_destroy() when it is no longer needed. */ > int > dpif_port_query_by_number(const struct dpif *dpif, odp_port_t port_no, > - struct dpif_port *port) > + struct dpif_port *port, bool warn_if_not_found) > { > int error = dpif->dpif_class->port_query_by_number(dpif, port_no, port); > if (!error) { > @@ -719,8 +720,10 @@ dpif_port_query_by_number(const struct dpif *dpif, odp_port_t port_no, > dpif_name(dpif), port_no, port->name); > } else { > memset(port, 0, sizeof *port); > - VLOG_WARN_RL(&error_rl, "%s: failed to query port %"PRIu32": %s", > - dpif_name(dpif), port_no, ovs_strerror(error)); > + VLOG_RL(&error_rl, > + (error == ENODEV && !warn_if_not_found) ? VLL_DBG : VLL_WARN, > + "%s: failed to query port %"PRIu32": %s", > + dpif_name(dpif), port_no, ovs_strerror(error)); Do we want to log at all in the case that we didn't set warn_if_not_found? Should we go on the dpmsg_rl instead of error_rl in that case? > } > return error; > } > @@ -788,7 +791,7 @@ dpif_port_get_name(struct dpif *dpif, odp_port_t port_no, > > ovs_assert(name_size > 0); > > - error = dpif_port_query_by_number(dpif, port_no, &port); > + error = dpif_port_query_by_number(dpif, port_no, &port, true); > if (!error) { > ovs_strlcpy(name, port.name, name_size); > dpif_port_destroy(&port); > diff --git a/lib/dpif.h b/lib/dpif.h > index 9e9d0aa1b..0f2dc2ef3 100644 > --- a/lib/dpif.h > +++ b/lib/dpif.h > @@ -463,7 +463,7 @@ void dpif_port_clone(struct dpif_port *, const struct dpif_port *); > void dpif_port_destroy(struct dpif_port *); > bool dpif_port_exists(const struct dpif *dpif, const char *devname); > int dpif_port_query_by_number(const struct dpif *, odp_port_t port_no, > - struct dpif_port *); > + struct dpif_port *, bool warn_if_not_found); > int dpif_port_query_by_name(const struct dpif *, const char *devname, > struct dpif_port *); > int dpif_port_get_name(struct dpif *, odp_port_t port_no, > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index fad7342b0..e22ca757a 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -2161,8 +2161,7 @@ port_destruct(struct ofport *port_, bool del) > struct ofproto_dpif *ofproto = ofproto_dpif_cast(port->up.ofproto); > const char *devname = netdev_get_name(port->up.netdev); > const char *netdev_type = netdev_get_type(port->up.netdev); > - char namebuf[NETDEV_VPORT_NAME_BUFSIZE]; > - const char *dp_port_name; > + struct dpif_port dpif_port; > > ofproto->backer->need_revalidate = REV_RECONFIGURE; > xlate_txn_start(); > @@ -2176,9 +2175,13 @@ port_destruct(struct ofport *port_, bool del) > del = dpif_cleanup_required(ofproto->backer->dpif); > } > > - dp_port_name = netdev_vport_get_dpif_port(port->up.netdev, namebuf, > - sizeof namebuf); > - if (del && dpif_port_exists(ofproto->backer->dpif, dp_port_name)) { > + /* Don't try to delete ports that are not part of the datapath. */ > + if (del && port->odp_port == ODPP_NONE) { > + del = false; > + } > + > + if (del && !dpif_port_query_by_number(ofproto->backer->dpif, > + port->odp_port, &dpif_port, false)) { > /* The underlying device is still there, so delete it. This > * happens when the ofproto is being destroyed, since the caller > * assumes that removal of attached ports will happen as part of > @@ -2186,6 +2189,7 @@ port_destruct(struct ofport *port_, bool del) > if (!port->is_tunnel) { > dpif_port_del(ofproto->backer->dpif, port->odp_port, false); > } > + dpif_port_destroy(&dpif_port); > } else if (del) { > /* The underlying device is already deleted (e.g. tunctl -d). > * Calling dpif_port_remove to do local cleanup for the netdev */ > diff --git a/tests/system-interface.at b/tests/system-interface.at > index 148f011c7..d4ee5c46b 100644 > --- a/tests/system-interface.at > +++ b/tests/system-interface.at > @@ -123,6 +123,63 @@ AT_CHECK([ip link show | grep " genev_sys_[[0-9]]*: .* ovs-system " | diff -u - > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > +AT_SETUP([interface - datapath port rename]) > +OVS_TRAFFIC_VSWITCHD_START() > + > +dnl Not relevant for userspace datapath. > +AT_SKIP_IF([! ovs-appctl dpctl/show | grep -q ovs-system]) > + > +AT_CHECK([ip link add ovs-veth0 type veth peer name ovs-veth1]) > +dnl We will rename ovs-veth0, so removing the peer on exit. > +on_exit 'ip link del ovs-veth1' > + > +AT_CHECK([ovs-vsctl add-port br0 ovs-veth0]) > + > +OVS_WAIT_UNTIL([ip link show | grep -q "ovs-veth0.* ovs-system "]) > + > +AT_CHECK([ovs-appctl dpctl/show | grep port], [0], [dnl > + port 0: ovs-system (internal) > + port 1: br0 (internal) > + port 2: ovs-veth0 > +]) > + > +dnl Rename the interface while attached to OVS. > +AT_CHECK([ip l set ovs-veth0 name ovs-new-port]) > + > +dnl Wait for the port to be detached from the OVS datapath. > +OVS_WAIT_UNTIL([ip link show | grep "ovs-new-port" | grep -v "ovs-system"]) > + > +dnl Check that database indicates the error. > +AT_CHECK([ovs-vsctl get interface ovs-veth0 error], [0], [dnl > +"could not open network device ovs-veth0 (No such device)" > +]) > + > +dnl Check that the port is no longer in the datapath. > +AT_CHECK([ovs-appctl dpctl/show | grep port], [0], [dnl > + port 0: ovs-system (internal) > + port 1: br0 (internal) > +]) > + > +dnl Rename the interface back and check that it is in use again. > +AT_CHECK([ip l set ovs-new-port name ovs-veth0]) > + > +OVS_WAIT_UNTIL([ip link show | grep -q "ovs-veth0.* ovs-system "]) > + > +AT_CHECK([ovs-vsctl get interface ovs-veth0 error], [0], [dnl > +[[]] > +]) > + > +AT_CHECK([ovs-appctl dpctl/show | grep port], [0], [dnl > + port 0: ovs-system (internal) > + port 1: br0 (internal) > + port 2: ovs-veth0 > +]) > + > +OVS_TRAFFIC_VSWITCHD_STOP([" > + /could not open network device ovs-veth0 (No such device)/d > +"]) > +AT_CLEANUP > + > AT_SETUP([interface - current speed]) > AT_SKIP_IF([test $HAVE_ETHTOOL = "no"]) > OVS_TRAFFIC_VSWITCHD_START()
On 7/20/23 16:55, Aaron Conole wrote: > Ilya Maximets <i.maximets@ovn.org> writes: > >> OVS configuration is based on port names and OpenFlow port numbers. >> Names are stored in the database and translated later to OF ports. >> On the datapath level, each port has a name and a datapath port number. >> Port name in the database has to match datapath port name, unless it's >> a tunnel port. >> >> If a datapath port is renamed with 'ip link set DEV name NAME', >> ovs-vswitchd will wake up, destroy all the OpenFlow-related structures >> and clean other things up. This is because the port no longer >> represents the port from a database due to a name difference. >> >> However, ovs-vswitch will not actually remove the port from the >> datapath, because it thinks that this port is no longer there. This >> is happening because lookup is performed by name and the name have >> changed. As a result we have a port in a datapath that is not related >> to any port known to ovs-vswitchd and ovs-vswitchd can't remove it. >> This port also occupies a datapath port number and prevents the port >> to be added back with a new name. >> >> Fix that by performing lookup by a datapath port number during the port >> destruction. The name was used only to avoid spurious warnings in a >> normal case where the port was successfully deleted by other parts of >> OVS. Adding an extra flag to avoid these warnings instead. >> >> Fixes: 02f8d6460afd ("ofproto-dpif: Query port existence by name to prevent warnings.") >> Reported-at: https://github.com/openvswitch/ovs-issues/issues/284 >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- >> lib/dpctl.c | 2 +- >> lib/dpif.c | 13 +++++---- >> lib/dpif.h | 2 +- >> ofproto/ofproto-dpif.c | 14 ++++++---- >> tests/system-interface.at | 57 +++++++++++++++++++++++++++++++++++++++ >> 5 files changed, 76 insertions(+), 12 deletions(-) >> >> diff --git a/lib/dpctl.c b/lib/dpctl.c >> index 4394653ab..79b82a176 100644 >> --- a/lib/dpctl.c >> +++ b/lib/dpctl.c >> @@ -673,7 +673,7 @@ show_dpif(struct dpif *dpif, struct dpctl_params *dpctl_p) >> } >> >> for (int i = 0; i < n_port_nos; i++) { >> - if (dpif_port_query_by_number(dpif, port_nos[i], &dpif_port)) { >> + if (dpif_port_query_by_number(dpif, port_nos[i], &dpif_port, true)) { >> continue; >> } >> >> diff --git a/lib/dpif.c b/lib/dpif.c >> index b1cbf39c4..29041ab91 100644 >> --- a/lib/dpif.c >> +++ b/lib/dpif.c >> @@ -705,13 +705,14 @@ dpif_port_set_config(struct dpif *dpif, odp_port_t port_no, >> * initializes '*port' appropriately; on failure, returns a positive errno >> * value. >> * >> - * Retuns ENODEV if the port doesn't exist. >> + * Retuns ENODEV if the port doesn't exist. Will not log a warning in this >> + * case unless 'warn_if_not_found' is true. >> * >> * The caller owns the data in 'port' and must free it with >> * dpif_port_destroy() when it is no longer needed. */ >> int >> dpif_port_query_by_number(const struct dpif *dpif, odp_port_t port_no, >> - struct dpif_port *port) >> + struct dpif_port *port, bool warn_if_not_found) >> { >> int error = dpif->dpif_class->port_query_by_number(dpif, port_no, port); >> if (!error) { >> @@ -719,8 +720,10 @@ dpif_port_query_by_number(const struct dpif *dpif, odp_port_t port_no, >> dpif_name(dpif), port_no, port->name); >> } else { >> memset(port, 0, sizeof *port); >> - VLOG_WARN_RL(&error_rl, "%s: failed to query port %"PRIu32": %s", >> - dpif_name(dpif), port_no, ovs_strerror(error)); >> + VLOG_RL(&error_rl, >> + (error == ENODEV && !warn_if_not_found) ? VLL_DBG : VLL_WARN, >> + "%s: failed to query port %"PRIu32": %s", >> + dpif_name(dpif), port_no, ovs_strerror(error)); > > Do we want to log at all in the case that we didn't set > warn_if_not_found? It's slightly useful for debug purposes. E.g. it was useful to see that the function was executed while developing this fix. > Should we go on the dpmsg_rl instead of error_rl in > that case? I guess, I just mimicked what dpif_port_query_by_name() is doing. It's currently using error_rl for both cases as well. I suppose, the '/* Not really much point in logging many dpif errors. */' kind of still applies. But we can change to use different limits for different log levels. What do you think? > >> } >> return error; >> } >> @@ -788,7 +791,7 @@ dpif_port_get_name(struct dpif *dpif, odp_port_t port_no, >> >> ovs_assert(name_size > 0); >> >> - error = dpif_port_query_by_number(dpif, port_no, &port); >> + error = dpif_port_query_by_number(dpif, port_no, &port, true); >> if (!error) { >> ovs_strlcpy(name, port.name, name_size); >> dpif_port_destroy(&port); >> diff --git a/lib/dpif.h b/lib/dpif.h >> index 9e9d0aa1b..0f2dc2ef3 100644 >> --- a/lib/dpif.h >> +++ b/lib/dpif.h >> @@ -463,7 +463,7 @@ void dpif_port_clone(struct dpif_port *, const struct dpif_port *); >> void dpif_port_destroy(struct dpif_port *); >> bool dpif_port_exists(const struct dpif *dpif, const char *devname); >> int dpif_port_query_by_number(const struct dpif *, odp_port_t port_no, >> - struct dpif_port *); >> + struct dpif_port *, bool warn_if_not_found); >> int dpif_port_query_by_name(const struct dpif *, const char *devname, >> struct dpif_port *); >> int dpif_port_get_name(struct dpif *, odp_port_t port_no, >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >> index fad7342b0..e22ca757a 100644 >> --- a/ofproto/ofproto-dpif.c >> +++ b/ofproto/ofproto-dpif.c >> @@ -2161,8 +2161,7 @@ port_destruct(struct ofport *port_, bool del) >> struct ofproto_dpif *ofproto = ofproto_dpif_cast(port->up.ofproto); >> const char *devname = netdev_get_name(port->up.netdev); >> const char *netdev_type = netdev_get_type(port->up.netdev); >> - char namebuf[NETDEV_VPORT_NAME_BUFSIZE]; >> - const char *dp_port_name; >> + struct dpif_port dpif_port; >> >> ofproto->backer->need_revalidate = REV_RECONFIGURE; >> xlate_txn_start(); >> @@ -2176,9 +2175,13 @@ port_destruct(struct ofport *port_, bool del) >> del = dpif_cleanup_required(ofproto->backer->dpif); >> } >> >> - dp_port_name = netdev_vport_get_dpif_port(port->up.netdev, namebuf, >> - sizeof namebuf); >> - if (del && dpif_port_exists(ofproto->backer->dpif, dp_port_name)) { >> + /* Don't try to delete ports that are not part of the datapath. */ >> + if (del && port->odp_port == ODPP_NONE) { >> + del = false; >> + } >> + >> + if (del && !dpif_port_query_by_number(ofproto->backer->dpif, >> + port->odp_port, &dpif_port, false)) { >> /* The underlying device is still there, so delete it. This >> * happens when the ofproto is being destroyed, since the caller >> * assumes that removal of attached ports will happen as part of >> @@ -2186,6 +2189,7 @@ port_destruct(struct ofport *port_, bool del) >> if (!port->is_tunnel) { >> dpif_port_del(ofproto->backer->dpif, port->odp_port, false); >> } >> + dpif_port_destroy(&dpif_port); >> } else if (del) { >> /* The underlying device is already deleted (e.g. tunctl -d). >> * Calling dpif_port_remove to do local cleanup for the netdev */ >> diff --git a/tests/system-interface.at b/tests/system-interface.at >> index 148f011c7..d4ee5c46b 100644 >> --- a/tests/system-interface.at >> +++ b/tests/system-interface.at >> @@ -123,6 +123,63 @@ AT_CHECK([ip link show | grep " genev_sys_[[0-9]]*: .* ovs-system " | diff -u - >> OVS_TRAFFIC_VSWITCHD_STOP >> AT_CLEANUP >> >> +AT_SETUP([interface - datapath port rename]) >> +OVS_TRAFFIC_VSWITCHD_START() >> + >> +dnl Not relevant for userspace datapath. >> +AT_SKIP_IF([! ovs-appctl dpctl/show | grep -q ovs-system]) >> + >> +AT_CHECK([ip link add ovs-veth0 type veth peer name ovs-veth1]) >> +dnl We will rename ovs-veth0, so removing the peer on exit. >> +on_exit 'ip link del ovs-veth1' >> + >> +AT_CHECK([ovs-vsctl add-port br0 ovs-veth0]) >> + >> +OVS_WAIT_UNTIL([ip link show | grep -q "ovs-veth0.* ovs-system "]) >> + >> +AT_CHECK([ovs-appctl dpctl/show | grep port], [0], [dnl >> + port 0: ovs-system (internal) >> + port 1: br0 (internal) >> + port 2: ovs-veth0 >> +]) >> + >> +dnl Rename the interface while attached to OVS. >> +AT_CHECK([ip l set ovs-veth0 name ovs-new-port]) >> + >> +dnl Wait for the port to be detached from the OVS datapath. >> +OVS_WAIT_UNTIL([ip link show | grep "ovs-new-port" | grep -v "ovs-system"]) >> + >> +dnl Check that database indicates the error. >> +AT_CHECK([ovs-vsctl get interface ovs-veth0 error], [0], [dnl >> +"could not open network device ovs-veth0 (No such device)" >> +]) >> + >> +dnl Check that the port is no longer in the datapath. >> +AT_CHECK([ovs-appctl dpctl/show | grep port], [0], [dnl >> + port 0: ovs-system (internal) >> + port 1: br0 (internal) >> +]) >> + >> +dnl Rename the interface back and check that it is in use again. >> +AT_CHECK([ip l set ovs-new-port name ovs-veth0]) >> + >> +OVS_WAIT_UNTIL([ip link show | grep -q "ovs-veth0.* ovs-system "]) >> + >> +AT_CHECK([ovs-vsctl get interface ovs-veth0 error], [0], [dnl >> +[[]] >> +]) >> + >> +AT_CHECK([ovs-appctl dpctl/show | grep port], [0], [dnl >> + port 0: ovs-system (internal) >> + port 1: br0 (internal) >> + port 2: ovs-veth0 >> +]) >> + >> +OVS_TRAFFIC_VSWITCHD_STOP([" >> + /could not open network device ovs-veth0 (No such device)/d >> +"]) >> +AT_CLEANUP >> + >> AT_SETUP([interface - current speed]) >> AT_SKIP_IF([test $HAVE_ETHTOOL = "no"]) >> OVS_TRAFFIC_VSWITCHD_START() >
Ilya Maximets <i.maximets@ovn.org> writes: > On 7/20/23 16:55, Aaron Conole wrote: >> Ilya Maximets <i.maximets@ovn.org> writes: >> >>> OVS configuration is based on port names and OpenFlow port numbers. >>> Names are stored in the database and translated later to OF ports. >>> On the datapath level, each port has a name and a datapath port number. >>> Port name in the database has to match datapath port name, unless it's >>> a tunnel port. >>> >>> If a datapath port is renamed with 'ip link set DEV name NAME', >>> ovs-vswitchd will wake up, destroy all the OpenFlow-related structures >>> and clean other things up. This is because the port no longer >>> represents the port from a database due to a name difference. >>> >>> However, ovs-vswitch will not actually remove the port from the >>> datapath, because it thinks that this port is no longer there. This >>> is happening because lookup is performed by name and the name have >>> changed. As a result we have a port in a datapath that is not related >>> to any port known to ovs-vswitchd and ovs-vswitchd can't remove it. >>> This port also occupies a datapath port number and prevents the port >>> to be added back with a new name. >>> >>> Fix that by performing lookup by a datapath port number during the port >>> destruction. The name was used only to avoid spurious warnings in a >>> normal case where the port was successfully deleted by other parts of >>> OVS. Adding an extra flag to avoid these warnings instead. >>> >>> Fixes: 02f8d6460afd ("ofproto-dpif: Query port existence by name to prevent warnings.") >>> Reported-at: https://github.com/openvswitch/ovs-issues/issues/284 >>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>> --- >>> lib/dpctl.c | 2 +- >>> lib/dpif.c | 13 +++++---- >>> lib/dpif.h | 2 +- >>> ofproto/ofproto-dpif.c | 14 ++++++---- >>> tests/system-interface.at | 57 +++++++++++++++++++++++++++++++++++++++ >>> 5 files changed, 76 insertions(+), 12 deletions(-) >>> >>> diff --git a/lib/dpctl.c b/lib/dpctl.c >>> index 4394653ab..79b82a176 100644 >>> --- a/lib/dpctl.c >>> +++ b/lib/dpctl.c >>> @@ -673,7 +673,7 @@ show_dpif(struct dpif *dpif, struct dpctl_params *dpctl_p) >>> } >>> >>> for (int i = 0; i < n_port_nos; i++) { >>> - if (dpif_port_query_by_number(dpif, port_nos[i], &dpif_port)) { >>> + if (dpif_port_query_by_number(dpif, port_nos[i], &dpif_port, true)) { >>> continue; >>> } >>> >>> diff --git a/lib/dpif.c b/lib/dpif.c >>> index b1cbf39c4..29041ab91 100644 >>> --- a/lib/dpif.c >>> +++ b/lib/dpif.c >>> @@ -705,13 +705,14 @@ dpif_port_set_config(struct dpif *dpif, odp_port_t port_no, >>> * initializes '*port' appropriately; on failure, returns a positive errno >>> * value. >>> * >>> - * Retuns ENODEV if the port doesn't exist. >>> + * Retuns ENODEV if the port doesn't exist. Will not log a warning in this >>> + * case unless 'warn_if_not_found' is true. >>> * >>> * The caller owns the data in 'port' and must free it with >>> * dpif_port_destroy() when it is no longer needed. */ >>> int >>> dpif_port_query_by_number(const struct dpif *dpif, odp_port_t port_no, >>> - struct dpif_port *port) >>> + struct dpif_port *port, bool warn_if_not_found) >>> { >>> int error = dpif->dpif_class->port_query_by_number(dpif, port_no, port); >>> if (!error) { >>> @@ -719,8 +720,10 @@ dpif_port_query_by_number(const struct dpif *dpif, odp_port_t port_no, >>> dpif_name(dpif), port_no, port->name); >>> } else { >>> memset(port, 0, sizeof *port); >>> - VLOG_WARN_RL(&error_rl, "%s: failed to query port %"PRIu32": %s", >>> - dpif_name(dpif), port_no, ovs_strerror(error)); >>> + VLOG_RL(&error_rl, >>> + (error == ENODEV && !warn_if_not_found) ? VLL_DBG : VLL_WARN, >>> + "%s: failed to query port %"PRIu32": %s", >>> + dpif_name(dpif), port_no, ovs_strerror(error)); >> >> Do we want to log at all in the case that we didn't set >> warn_if_not_found? > > It's slightly useful for debug purposes. E.g. it was useful to see > that the function was executed while developing this fix. Okay. >> Should we go on the dpmsg_rl instead of error_rl in >> that case? > > I guess, I just mimicked what dpif_port_query_by_name() is doing. > It's currently using error_rl for both cases as well. I suppose, > the '/* Not really much point in logging many dpif errors. */' kind > of still applies. But we can change to use different limits for > different log levels. What do you think? I guess the log should generally be infrequent enough that it shouldn't hit the error rate limit too often - most of the code here gets executed in the "even slower" path. I don't have a strong enough opinion either way. If you decide to change it with a v2, include: Acked-by: Aaron Conole <aconole@redhat.com> >> >>> } >>> return error; >>> } >>> @@ -788,7 +791,7 @@ dpif_port_get_name(struct dpif *dpif, odp_port_t port_no, >>> >>> ovs_assert(name_size > 0); >>> >>> - error = dpif_port_query_by_number(dpif, port_no, &port); >>> + error = dpif_port_query_by_number(dpif, port_no, &port, true); >>> if (!error) { >>> ovs_strlcpy(name, port.name, name_size); >>> dpif_port_destroy(&port); >>> diff --git a/lib/dpif.h b/lib/dpif.h >>> index 9e9d0aa1b..0f2dc2ef3 100644 >>> --- a/lib/dpif.h >>> +++ b/lib/dpif.h >>> @@ -463,7 +463,7 @@ void dpif_port_clone(struct dpif_port *, const struct dpif_port *); >>> void dpif_port_destroy(struct dpif_port *); >>> bool dpif_port_exists(const struct dpif *dpif, const char *devname); >>> int dpif_port_query_by_number(const struct dpif *, odp_port_t port_no, >>> - struct dpif_port *); >>> + struct dpif_port *, bool warn_if_not_found); >>> int dpif_port_query_by_name(const struct dpif *, const char *devname, >>> struct dpif_port *); >>> int dpif_port_get_name(struct dpif *, odp_port_t port_no, >>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >>> index fad7342b0..e22ca757a 100644 >>> --- a/ofproto/ofproto-dpif.c >>> +++ b/ofproto/ofproto-dpif.c >>> @@ -2161,8 +2161,7 @@ port_destruct(struct ofport *port_, bool del) >>> struct ofproto_dpif *ofproto = ofproto_dpif_cast(port->up.ofproto); >>> const char *devname = netdev_get_name(port->up.netdev); >>> const char *netdev_type = netdev_get_type(port->up.netdev); >>> - char namebuf[NETDEV_VPORT_NAME_BUFSIZE]; >>> - const char *dp_port_name; >>> + struct dpif_port dpif_port; >>> >>> ofproto->backer->need_revalidate = REV_RECONFIGURE; >>> xlate_txn_start(); >>> @@ -2176,9 +2175,13 @@ port_destruct(struct ofport *port_, bool del) >>> del = dpif_cleanup_required(ofproto->backer->dpif); >>> } >>> >>> - dp_port_name = netdev_vport_get_dpif_port(port->up.netdev, namebuf, >>> - sizeof namebuf); >>> - if (del && dpif_port_exists(ofproto->backer->dpif, dp_port_name)) { >>> + /* Don't try to delete ports that are not part of the datapath. */ >>> + if (del && port->odp_port == ODPP_NONE) { >>> + del = false; >>> + } >>> + >>> + if (del && !dpif_port_query_by_number(ofproto->backer->dpif, >>> + port->odp_port, &dpif_port, false)) { >>> /* The underlying device is still there, so delete it. This >>> * happens when the ofproto is being destroyed, since the caller >>> * assumes that removal of attached ports will happen as part of >>> @@ -2186,6 +2189,7 @@ port_destruct(struct ofport *port_, bool del) >>> if (!port->is_tunnel) { >>> dpif_port_del(ofproto->backer->dpif, port->odp_port, false); >>> } >>> + dpif_port_destroy(&dpif_port); >>> } else if (del) { >>> /* The underlying device is already deleted (e.g. tunctl -d). >>> * Calling dpif_port_remove to do local cleanup for the netdev */ >>> diff --git a/tests/system-interface.at b/tests/system-interface.at >>> index 148f011c7..d4ee5c46b 100644 >>> --- a/tests/system-interface.at >>> +++ b/tests/system-interface.at >>> @@ -123,6 +123,63 @@ AT_CHECK([ip link show | grep " genev_sys_[[0-9]]*: .* ovs-system " | diff -u - >>> OVS_TRAFFIC_VSWITCHD_STOP >>> AT_CLEANUP >>> >>> +AT_SETUP([interface - datapath port rename]) >>> +OVS_TRAFFIC_VSWITCHD_START() >>> + >>> +dnl Not relevant for userspace datapath. >>> +AT_SKIP_IF([! ovs-appctl dpctl/show | grep -q ovs-system]) >>> + >>> +AT_CHECK([ip link add ovs-veth0 type veth peer name ovs-veth1]) >>> +dnl We will rename ovs-veth0, so removing the peer on exit. >>> +on_exit 'ip link del ovs-veth1' >>> + >>> +AT_CHECK([ovs-vsctl add-port br0 ovs-veth0]) >>> + >>> +OVS_WAIT_UNTIL([ip link show | grep -q "ovs-veth0.* ovs-system "]) >>> + >>> +AT_CHECK([ovs-appctl dpctl/show | grep port], [0], [dnl >>> + port 0: ovs-system (internal) >>> + port 1: br0 (internal) >>> + port 2: ovs-veth0 >>> +]) >>> + >>> +dnl Rename the interface while attached to OVS. >>> +AT_CHECK([ip l set ovs-veth0 name ovs-new-port]) >>> + >>> +dnl Wait for the port to be detached from the OVS datapath. >>> +OVS_WAIT_UNTIL([ip link show | grep "ovs-new-port" | grep -v "ovs-system"]) >>> + >>> +dnl Check that database indicates the error. >>> +AT_CHECK([ovs-vsctl get interface ovs-veth0 error], [0], [dnl >>> +"could not open network device ovs-veth0 (No such device)" >>> +]) >>> + >>> +dnl Check that the port is no longer in the datapath. >>> +AT_CHECK([ovs-appctl dpctl/show | grep port], [0], [dnl >>> + port 0: ovs-system (internal) >>> + port 1: br0 (internal) >>> +]) >>> + >>> +dnl Rename the interface back and check that it is in use again. >>> +AT_CHECK([ip l set ovs-new-port name ovs-veth0]) >>> + >>> +OVS_WAIT_UNTIL([ip link show | grep -q "ovs-veth0.* ovs-system "]) >>> + >>> +AT_CHECK([ovs-vsctl get interface ovs-veth0 error], [0], [dnl >>> +[[]] >>> +]) >>> + >>> +AT_CHECK([ovs-appctl dpctl/show | grep port], [0], [dnl >>> + port 0: ovs-system (internal) >>> + port 1: br0 (internal) >>> + port 2: ovs-veth0 >>> +]) >>> + >>> +OVS_TRAFFIC_VSWITCHD_STOP([" >>> + /could not open network device ovs-veth0 (No such device)/d >>> +"]) >>> +AT_CLEANUP >>> + >>> AT_SETUP([interface - current speed]) >>> AT_SKIP_IF([test $HAVE_ETHTOOL = "no"]) >>> OVS_TRAFFIC_VSWITCHD_START() >>
On 7/20/23 21:18, Aaron Conole wrote: > Ilya Maximets <i.maximets@ovn.org> writes: > >> On 7/20/23 16:55, Aaron Conole wrote: >>> Ilya Maximets <i.maximets@ovn.org> writes: >>> >>>> OVS configuration is based on port names and OpenFlow port numbers. >>>> Names are stored in the database and translated later to OF ports. >>>> On the datapath level, each port has a name and a datapath port number. >>>> Port name in the database has to match datapath port name, unless it's >>>> a tunnel port. >>>> >>>> If a datapath port is renamed with 'ip link set DEV name NAME', >>>> ovs-vswitchd will wake up, destroy all the OpenFlow-related structures >>>> and clean other things up. This is because the port no longer >>>> represents the port from a database due to a name difference. >>>> >>>> However, ovs-vswitch will not actually remove the port from the >>>> datapath, because it thinks that this port is no longer there. This >>>> is happening because lookup is performed by name and the name have >>>> changed. As a result we have a port in a datapath that is not related >>>> to any port known to ovs-vswitchd and ovs-vswitchd can't remove it. >>>> This port also occupies a datapath port number and prevents the port >>>> to be added back with a new name. >>>> >>>> Fix that by performing lookup by a datapath port number during the port >>>> destruction. The name was used only to avoid spurious warnings in a >>>> normal case where the port was successfully deleted by other parts of >>>> OVS. Adding an extra flag to avoid these warnings instead. >>>> >>>> Fixes: 02f8d6460afd ("ofproto-dpif: Query port existence by name to prevent warnings.") >>>> Reported-at: https://github.com/openvswitch/ovs-issues/issues/284 >>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>>> --- >>>> lib/dpctl.c | 2 +- >>>> lib/dpif.c | 13 +++++---- >>>> lib/dpif.h | 2 +- >>>> ofproto/ofproto-dpif.c | 14 ++++++---- >>>> tests/system-interface.at | 57 +++++++++++++++++++++++++++++++++++++++ >>>> 5 files changed, 76 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/lib/dpctl.c b/lib/dpctl.c >>>> index 4394653ab..79b82a176 100644 >>>> --- a/lib/dpctl.c >>>> +++ b/lib/dpctl.c >>>> @@ -673,7 +673,7 @@ show_dpif(struct dpif *dpif, struct dpctl_params *dpctl_p) >>>> } >>>> >>>> for (int i = 0; i < n_port_nos; i++) { >>>> - if (dpif_port_query_by_number(dpif, port_nos[i], &dpif_port)) { >>>> + if (dpif_port_query_by_number(dpif, port_nos[i], &dpif_port, true)) { >>>> continue; >>>> } >>>> >>>> diff --git a/lib/dpif.c b/lib/dpif.c >>>> index b1cbf39c4..29041ab91 100644 >>>> --- a/lib/dpif.c >>>> +++ b/lib/dpif.c >>>> @@ -705,13 +705,14 @@ dpif_port_set_config(struct dpif *dpif, odp_port_t port_no, >>>> * initializes '*port' appropriately; on failure, returns a positive errno >>>> * value. >>>> * >>>> - * Retuns ENODEV if the port doesn't exist. >>>> + * Retuns ENODEV if the port doesn't exist. Will not log a warning in this >>>> + * case unless 'warn_if_not_found' is true. >>>> * >>>> * The caller owns the data in 'port' and must free it with >>>> * dpif_port_destroy() when it is no longer needed. */ >>>> int >>>> dpif_port_query_by_number(const struct dpif *dpif, odp_port_t port_no, >>>> - struct dpif_port *port) >>>> + struct dpif_port *port, bool warn_if_not_found) >>>> { >>>> int error = dpif->dpif_class->port_query_by_number(dpif, port_no, port); >>>> if (!error) { >>>> @@ -719,8 +720,10 @@ dpif_port_query_by_number(const struct dpif *dpif, odp_port_t port_no, >>>> dpif_name(dpif), port_no, port->name); >>>> } else { >>>> memset(port, 0, sizeof *port); >>>> - VLOG_WARN_RL(&error_rl, "%s: failed to query port %"PRIu32": %s", >>>> - dpif_name(dpif), port_no, ovs_strerror(error)); >>>> + VLOG_RL(&error_rl, >>>> + (error == ENODEV && !warn_if_not_found) ? VLL_DBG : VLL_WARN, >>>> + "%s: failed to query port %"PRIu32": %s", >>>> + dpif_name(dpif), port_no, ovs_strerror(error)); >>> >>> Do we want to log at all in the case that we didn't set >>> warn_if_not_found? >> >> It's slightly useful for debug purposes. E.g. it was useful to see >> that the function was executed while developing this fix. > > Okay. > >>> Should we go on the dpmsg_rl instead of error_rl in >>> that case? >> >> I guess, I just mimicked what dpif_port_query_by_name() is doing. >> It's currently using error_rl for both cases as well. I suppose, >> the '/* Not really much point in logging many dpif errors. */' kind >> of still applies. But we can change to use different limits for >> different log levels. What do you think? > > I guess the log should generally be infrequent enough that it shouldn't > hit the error rate limit too often - most of the code here gets executed > in the "even slower" path. I don't have a strong enough opinion either > way. If you decide to change it with a v2, include: > > Acked-by: Aaron Conole <aconole@redhat.com> Sending v2 with 'Ack's included would be a bit awkward for me in terms of how the review process goes, as that would mean that yet another Ack is probably needed. But I could fold the following change in before applying the patch, if you agree: @@ -719,8 +720,13 @@ dpif_port_query_by_number(const struct dpif *dpif, odp_port_t port_no, dpif_name(dpif), port_no, port->name); } else { memset(port, 0, sizeof *port); - VLOG_WARN_RL(&error_rl, "%s: failed to query port %"PRIu32": %s", - dpif_name(dpif), port_no, ovs_strerror(error)); + if (error == ENODEV && !warn_if_not_found) { + VLOG_DBG_RL(&dpmsg_rl, "%s: failed to query port %"PRIu32": %s", + dpif_name(dpif), port_no, ovs_strerror(error)); + } else { + VLOG_WARN_RL(&error_rl, "%s: failed to query port %"PRIu32": %s", + dpif_name(dpif), port_no, ovs_strerror(error)); + } } return error; } --- What do you think? Best regards, Ilya Maximets.
Ilya Maximets <i.maximets@ovn.org> writes: > On 7/20/23 21:18, Aaron Conole wrote: >> Ilya Maximets <i.maximets@ovn.org> writes: >> >>> On 7/20/23 16:55, Aaron Conole wrote: >>>> Ilya Maximets <i.maximets@ovn.org> writes: >>>> >>>>> OVS configuration is based on port names and OpenFlow port numbers. >>>>> Names are stored in the database and translated later to OF ports. >>>>> On the datapath level, each port has a name and a datapath port number. >>>>> Port name in the database has to match datapath port name, unless it's >>>>> a tunnel port. >>>>> >>>>> If a datapath port is renamed with 'ip link set DEV name NAME', >>>>> ovs-vswitchd will wake up, destroy all the OpenFlow-related structures >>>>> and clean other things up. This is because the port no longer >>>>> represents the port from a database due to a name difference. >>>>> >>>>> However, ovs-vswitch will not actually remove the port from the >>>>> datapath, because it thinks that this port is no longer there. This >>>>> is happening because lookup is performed by name and the name have >>>>> changed. As a result we have a port in a datapath that is not related >>>>> to any port known to ovs-vswitchd and ovs-vswitchd can't remove it. >>>>> This port also occupies a datapath port number and prevents the port >>>>> to be added back with a new name. >>>>> >>>>> Fix that by performing lookup by a datapath port number during the port >>>>> destruction. The name was used only to avoid spurious warnings in a >>>>> normal case where the port was successfully deleted by other parts of >>>>> OVS. Adding an extra flag to avoid these warnings instead. >>>>> >>>>> Fixes: 02f8d6460afd ("ofproto-dpif: Query port existence by name to prevent warnings.") >>>>> Reported-at: https://github.com/openvswitch/ovs-issues/issues/284 >>>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>>>> --- >>>>> lib/dpctl.c | 2 +- >>>>> lib/dpif.c | 13 +++++---- >>>>> lib/dpif.h | 2 +- >>>>> ofproto/ofproto-dpif.c | 14 ++++++---- >>>>> tests/system-interface.at | 57 +++++++++++++++++++++++++++++++++++++++ >>>>> 5 files changed, 76 insertions(+), 12 deletions(-) >>>>> >>>>> diff --git a/lib/dpctl.c b/lib/dpctl.c >>>>> index 4394653ab..79b82a176 100644 >>>>> --- a/lib/dpctl.c >>>>> +++ b/lib/dpctl.c >>>>> @@ -673,7 +673,7 @@ show_dpif(struct dpif *dpif, struct dpctl_params *dpctl_p) >>>>> } >>>>> >>>>> for (int i = 0; i < n_port_nos; i++) { >>>>> - if (dpif_port_query_by_number(dpif, port_nos[i], &dpif_port)) { >>>>> + if (dpif_port_query_by_number(dpif, port_nos[i], &dpif_port, true)) { >>>>> continue; >>>>> } >>>>> >>>>> diff --git a/lib/dpif.c b/lib/dpif.c >>>>> index b1cbf39c4..29041ab91 100644 >>>>> --- a/lib/dpif.c >>>>> +++ b/lib/dpif.c >>>>> @@ -705,13 +705,14 @@ dpif_port_set_config(struct dpif *dpif, odp_port_t port_no, >>>>> * initializes '*port' appropriately; on failure, returns a positive errno >>>>> * value. >>>>> * >>>>> - * Retuns ENODEV if the port doesn't exist. >>>>> + * Retuns ENODEV if the port doesn't exist. Will not log a warning in this >>>>> + * case unless 'warn_if_not_found' is true. >>>>> * >>>>> * The caller owns the data in 'port' and must free it with >>>>> * dpif_port_destroy() when it is no longer needed. */ >>>>> int >>>>> dpif_port_query_by_number(const struct dpif *dpif, odp_port_t port_no, >>>>> - struct dpif_port *port) >>>>> + struct dpif_port *port, bool warn_if_not_found) >>>>> { >>>>> int error = dpif->dpif_class->port_query_by_number(dpif, port_no, port); >>>>> if (!error) { >>>>> @@ -719,8 +720,10 @@ dpif_port_query_by_number(const struct dpif *dpif, odp_port_t port_no, >>>>> dpif_name(dpif), port_no, port->name); >>>>> } else { >>>>> memset(port, 0, sizeof *port); >>>>> - VLOG_WARN_RL(&error_rl, "%s: failed to query port %"PRIu32": %s", >>>>> - dpif_name(dpif), port_no, ovs_strerror(error)); >>>>> + VLOG_RL(&error_rl, >>>>> + (error == ENODEV && !warn_if_not_found) ? VLL_DBG : VLL_WARN, >>>>> + "%s: failed to query port %"PRIu32": %s", >>>>> + dpif_name(dpif), port_no, ovs_strerror(error)); >>>> >>>> Do we want to log at all in the case that we didn't set >>>> warn_if_not_found? >>> >>> It's slightly useful for debug purposes. E.g. it was useful to see >>> that the function was executed while developing this fix. >> >> Okay. >> >>>> Should we go on the dpmsg_rl instead of error_rl in >>>> that case? >>> >>> I guess, I just mimicked what dpif_port_query_by_name() is doing. >>> It's currently using error_rl for both cases as well. I suppose, >>> the '/* Not really much point in logging many dpif errors. */' kind >>> of still applies. But we can change to use different limits for >>> different log levels. What do you think? >> >> I guess the log should generally be infrequent enough that it shouldn't >> hit the error rate limit too often - most of the code here gets executed >> in the "even slower" path. I don't have a strong enough opinion either >> way. If you decide to change it with a v2, include: >> >> Acked-by: Aaron Conole <aconole@redhat.com> > > Sending v2 with 'Ack's included would be a bit awkward for me in terms of > how the review process goes, as that would mean that yet another Ack is > probably needed. But I could fold the following change in before applying > the patch, if you agree: > > @@ -719,8 +720,13 @@ dpif_port_query_by_number(const struct dpif *dpif, odp_port_t port_no, > dpif_name(dpif), port_no, port->name); > } else { > memset(port, 0, sizeof *port); > - VLOG_WARN_RL(&error_rl, "%s: failed to query port %"PRIu32": %s", > - dpif_name(dpif), port_no, ovs_strerror(error)); > + if (error == ENODEV && !warn_if_not_found) { > + VLOG_DBG_RL(&dpmsg_rl, "%s: failed to query port %"PRIu32": %s", > + dpif_name(dpif), port_no, ovs_strerror(error)); > + } else { > + VLOG_WARN_RL(&error_rl, "%s: failed to query port %"PRIu32": %s", > + dpif_name(dpif), port_no, ovs_strerror(error)); > + } > } > return error; > } > --- > > What do you think? ACK - fine by me. > Best regards, Ilya Maximets.
On 7/31/23 21:31, Aaron Conole wrote: > Ilya Maximets <i.maximets@ovn.org> writes: > >> On 7/20/23 21:18, Aaron Conole wrote: >>> Ilya Maximets <i.maximets@ovn.org> writes: >>> >>>> On 7/20/23 16:55, Aaron Conole wrote: >>>>> Ilya Maximets <i.maximets@ovn.org> writes: >>>>> >>>>>> OVS configuration is based on port names and OpenFlow port numbers. >>>>>> Names are stored in the database and translated later to OF ports. >>>>>> On the datapath level, each port has a name and a datapath port number. >>>>>> Port name in the database has to match datapath port name, unless it's >>>>>> a tunnel port. >>>>>> >>>>>> If a datapath port is renamed with 'ip link set DEV name NAME', >>>>>> ovs-vswitchd will wake up, destroy all the OpenFlow-related structures >>>>>> and clean other things up. This is because the port no longer >>>>>> represents the port from a database due to a name difference. >>>>>> >>>>>> However, ovs-vswitch will not actually remove the port from the >>>>>> datapath, because it thinks that this port is no longer there. This >>>>>> is happening because lookup is performed by name and the name have >>>>>> changed. As a result we have a port in a datapath that is not related >>>>>> to any port known to ovs-vswitchd and ovs-vswitchd can't remove it. >>>>>> This port also occupies a datapath port number and prevents the port >>>>>> to be added back with a new name. >>>>>> >>>>>> Fix that by performing lookup by a datapath port number during the port >>>>>> destruction. The name was used only to avoid spurious warnings in a >>>>>> normal case where the port was successfully deleted by other parts of >>>>>> OVS. Adding an extra flag to avoid these warnings instead. >>>>>> >>>>>> Fixes: 02f8d6460afd ("ofproto-dpif: Query port existence by name to prevent warnings.") >>>>>> Reported-at: https://github.com/openvswitch/ovs-issues/issues/284 >>>>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>>>>> --- >>>>>> lib/dpctl.c | 2 +- >>>>>> lib/dpif.c | 13 +++++---- >>>>>> lib/dpif.h | 2 +- >>>>>> ofproto/ofproto-dpif.c | 14 ++++++---- >>>>>> tests/system-interface.at | 57 +++++++++++++++++++++++++++++++++++++++ >>>>>> 5 files changed, 76 insertions(+), 12 deletions(-) >>>>>> >>>>>> diff --git a/lib/dpctl.c b/lib/dpctl.c >>>>>> index 4394653ab..79b82a176 100644 >>>>>> --- a/lib/dpctl.c >>>>>> +++ b/lib/dpctl.c >>>>>> @@ -673,7 +673,7 @@ show_dpif(struct dpif *dpif, struct dpctl_params *dpctl_p) >>>>>> } >>>>>> >>>>>> for (int i = 0; i < n_port_nos; i++) { >>>>>> - if (dpif_port_query_by_number(dpif, port_nos[i], &dpif_port)) { >>>>>> + if (dpif_port_query_by_number(dpif, port_nos[i], &dpif_port, true)) { >>>>>> continue; >>>>>> } >>>>>> >>>>>> diff --git a/lib/dpif.c b/lib/dpif.c >>>>>> index b1cbf39c4..29041ab91 100644 >>>>>> --- a/lib/dpif.c >>>>>> +++ b/lib/dpif.c >>>>>> @@ -705,13 +705,14 @@ dpif_port_set_config(struct dpif *dpif, odp_port_t port_no, >>>>>> * initializes '*port' appropriately; on failure, returns a positive errno >>>>>> * value. >>>>>> * >>>>>> - * Retuns ENODEV if the port doesn't exist. >>>>>> + * Retuns ENODEV if the port doesn't exist. Will not log a warning in this >>>>>> + * case unless 'warn_if_not_found' is true. >>>>>> * >>>>>> * The caller owns the data in 'port' and must free it with >>>>>> * dpif_port_destroy() when it is no longer needed. */ >>>>>> int >>>>>> dpif_port_query_by_number(const struct dpif *dpif, odp_port_t port_no, >>>>>> - struct dpif_port *port) >>>>>> + struct dpif_port *port, bool warn_if_not_found) >>>>>> { >>>>>> int error = dpif->dpif_class->port_query_by_number(dpif, port_no, port); >>>>>> if (!error) { >>>>>> @@ -719,8 +720,10 @@ dpif_port_query_by_number(const struct dpif *dpif, odp_port_t port_no, >>>>>> dpif_name(dpif), port_no, port->name); >>>>>> } else { >>>>>> memset(port, 0, sizeof *port); >>>>>> - VLOG_WARN_RL(&error_rl, "%s: failed to query port %"PRIu32": %s", >>>>>> - dpif_name(dpif), port_no, ovs_strerror(error)); >>>>>> + VLOG_RL(&error_rl, >>>>>> + (error == ENODEV && !warn_if_not_found) ? VLL_DBG : VLL_WARN, >>>>>> + "%s: failed to query port %"PRIu32": %s", >>>>>> + dpif_name(dpif), port_no, ovs_strerror(error)); >>>>> >>>>> Do we want to log at all in the case that we didn't set >>>>> warn_if_not_found? >>>> >>>> It's slightly useful for debug purposes. E.g. it was useful to see >>>> that the function was executed while developing this fix. >>> >>> Okay. >>> >>>>> Should we go on the dpmsg_rl instead of error_rl in >>>>> that case? >>>> >>>> I guess, I just mimicked what dpif_port_query_by_name() is doing. >>>> It's currently using error_rl for both cases as well. I suppose, >>>> the '/* Not really much point in logging many dpif errors. */' kind >>>> of still applies. But we can change to use different limits for >>>> different log levels. What do you think? >>> >>> I guess the log should generally be infrequent enough that it shouldn't >>> hit the error rate limit too often - most of the code here gets executed >>> in the "even slower" path. I don't have a strong enough opinion either >>> way. If you decide to change it with a v2, include: >>> >>> Acked-by: Aaron Conole <aconole@redhat.com> >> >> Sending v2 with 'Ack's included would be a bit awkward for me in terms of >> how the review process goes, as that would mean that yet another Ack is >> probably needed. But I could fold the following change in before applying >> the patch, if you agree: >> >> @@ -719,8 +720,13 @@ dpif_port_query_by_number(const struct dpif *dpif, odp_port_t port_no, >> dpif_name(dpif), port_no, port->name); >> } else { >> memset(port, 0, sizeof *port); >> - VLOG_WARN_RL(&error_rl, "%s: failed to query port %"PRIu32": %s", >> - dpif_name(dpif), port_no, ovs_strerror(error)); >> + if (error == ENODEV && !warn_if_not_found) { >> + VLOG_DBG_RL(&dpmsg_rl, "%s: failed to query port %"PRIu32": %s", >> + dpif_name(dpif), port_no, ovs_strerror(error)); >> + } else { >> + VLOG_WARN_RL(&error_rl, "%s: failed to query port %"PRIu32": %s", >> + dpif_name(dpif), port_no, ovs_strerror(error)); >> + } >> } >> return error; >> } >> --- >> >> What do you think? > > ACK - fine by me. Thanks, Aaron and Alin! Applied and backported down to 2.17. Best regards, Ilya Maximets.
diff --git a/lib/dpctl.c b/lib/dpctl.c index 4394653ab..79b82a176 100644 --- a/lib/dpctl.c +++ b/lib/dpctl.c @@ -673,7 +673,7 @@ show_dpif(struct dpif *dpif, struct dpctl_params *dpctl_p) } for (int i = 0; i < n_port_nos; i++) { - if (dpif_port_query_by_number(dpif, port_nos[i], &dpif_port)) { + if (dpif_port_query_by_number(dpif, port_nos[i], &dpif_port, true)) { continue; } diff --git a/lib/dpif.c b/lib/dpif.c index b1cbf39c4..29041ab91 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -705,13 +705,14 @@ dpif_port_set_config(struct dpif *dpif, odp_port_t port_no, * initializes '*port' appropriately; on failure, returns a positive errno * value. * - * Retuns ENODEV if the port doesn't exist. + * Retuns ENODEV if the port doesn't exist. Will not log a warning in this + * case unless 'warn_if_not_found' is true. * * The caller owns the data in 'port' and must free it with * dpif_port_destroy() when it is no longer needed. */ int dpif_port_query_by_number(const struct dpif *dpif, odp_port_t port_no, - struct dpif_port *port) + struct dpif_port *port, bool warn_if_not_found) { int error = dpif->dpif_class->port_query_by_number(dpif, port_no, port); if (!error) { @@ -719,8 +720,10 @@ dpif_port_query_by_number(const struct dpif *dpif, odp_port_t port_no, dpif_name(dpif), port_no, port->name); } else { memset(port, 0, sizeof *port); - VLOG_WARN_RL(&error_rl, "%s: failed to query port %"PRIu32": %s", - dpif_name(dpif), port_no, ovs_strerror(error)); + VLOG_RL(&error_rl, + (error == ENODEV && !warn_if_not_found) ? VLL_DBG : VLL_WARN, + "%s: failed to query port %"PRIu32": %s", + dpif_name(dpif), port_no, ovs_strerror(error)); } return error; } @@ -788,7 +791,7 @@ dpif_port_get_name(struct dpif *dpif, odp_port_t port_no, ovs_assert(name_size > 0); - error = dpif_port_query_by_number(dpif, port_no, &port); + error = dpif_port_query_by_number(dpif, port_no, &port, true); if (!error) { ovs_strlcpy(name, port.name, name_size); dpif_port_destroy(&port); diff --git a/lib/dpif.h b/lib/dpif.h index 9e9d0aa1b..0f2dc2ef3 100644 --- a/lib/dpif.h +++ b/lib/dpif.h @@ -463,7 +463,7 @@ void dpif_port_clone(struct dpif_port *, const struct dpif_port *); void dpif_port_destroy(struct dpif_port *); bool dpif_port_exists(const struct dpif *dpif, const char *devname); int dpif_port_query_by_number(const struct dpif *, odp_port_t port_no, - struct dpif_port *); + struct dpif_port *, bool warn_if_not_found); int dpif_port_query_by_name(const struct dpif *, const char *devname, struct dpif_port *); int dpif_port_get_name(struct dpif *, odp_port_t port_no, diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index fad7342b0..e22ca757a 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -2161,8 +2161,7 @@ port_destruct(struct ofport *port_, bool del) struct ofproto_dpif *ofproto = ofproto_dpif_cast(port->up.ofproto); const char *devname = netdev_get_name(port->up.netdev); const char *netdev_type = netdev_get_type(port->up.netdev); - char namebuf[NETDEV_VPORT_NAME_BUFSIZE]; - const char *dp_port_name; + struct dpif_port dpif_port; ofproto->backer->need_revalidate = REV_RECONFIGURE; xlate_txn_start(); @@ -2176,9 +2175,13 @@ port_destruct(struct ofport *port_, bool del) del = dpif_cleanup_required(ofproto->backer->dpif); } - dp_port_name = netdev_vport_get_dpif_port(port->up.netdev, namebuf, - sizeof namebuf); - if (del && dpif_port_exists(ofproto->backer->dpif, dp_port_name)) { + /* Don't try to delete ports that are not part of the datapath. */ + if (del && port->odp_port == ODPP_NONE) { + del = false; + } + + if (del && !dpif_port_query_by_number(ofproto->backer->dpif, + port->odp_port, &dpif_port, false)) { /* The underlying device is still there, so delete it. This * happens when the ofproto is being destroyed, since the caller * assumes that removal of attached ports will happen as part of @@ -2186,6 +2189,7 @@ port_destruct(struct ofport *port_, bool del) if (!port->is_tunnel) { dpif_port_del(ofproto->backer->dpif, port->odp_port, false); } + dpif_port_destroy(&dpif_port); } else if (del) { /* The underlying device is already deleted (e.g. tunctl -d). * Calling dpif_port_remove to do local cleanup for the netdev */ diff --git a/tests/system-interface.at b/tests/system-interface.at index 148f011c7..d4ee5c46b 100644 --- a/tests/system-interface.at +++ b/tests/system-interface.at @@ -123,6 +123,63 @@ AT_CHECK([ip link show | grep " genev_sys_[[0-9]]*: .* ovs-system " | diff -u - OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([interface - datapath port rename]) +OVS_TRAFFIC_VSWITCHD_START() + +dnl Not relevant for userspace datapath. +AT_SKIP_IF([! ovs-appctl dpctl/show | grep -q ovs-system]) + +AT_CHECK([ip link add ovs-veth0 type veth peer name ovs-veth1]) +dnl We will rename ovs-veth0, so removing the peer on exit. +on_exit 'ip link del ovs-veth1' + +AT_CHECK([ovs-vsctl add-port br0 ovs-veth0]) + +OVS_WAIT_UNTIL([ip link show | grep -q "ovs-veth0.* ovs-system "]) + +AT_CHECK([ovs-appctl dpctl/show | grep port], [0], [dnl + port 0: ovs-system (internal) + port 1: br0 (internal) + port 2: ovs-veth0 +]) + +dnl Rename the interface while attached to OVS. +AT_CHECK([ip l set ovs-veth0 name ovs-new-port]) + +dnl Wait for the port to be detached from the OVS datapath. +OVS_WAIT_UNTIL([ip link show | grep "ovs-new-port" | grep -v "ovs-system"]) + +dnl Check that database indicates the error. +AT_CHECK([ovs-vsctl get interface ovs-veth0 error], [0], [dnl +"could not open network device ovs-veth0 (No such device)" +]) + +dnl Check that the port is no longer in the datapath. +AT_CHECK([ovs-appctl dpctl/show | grep port], [0], [dnl + port 0: ovs-system (internal) + port 1: br0 (internal) +]) + +dnl Rename the interface back and check that it is in use again. +AT_CHECK([ip l set ovs-new-port name ovs-veth0]) + +OVS_WAIT_UNTIL([ip link show | grep -q "ovs-veth0.* ovs-system "]) + +AT_CHECK([ovs-vsctl get interface ovs-veth0 error], [0], [dnl +[[]] +]) + +AT_CHECK([ovs-appctl dpctl/show | grep port], [0], [dnl + port 0: ovs-system (internal) + port 1: br0 (internal) + port 2: ovs-veth0 +]) + +OVS_TRAFFIC_VSWITCHD_STOP([" + /could not open network device ovs-veth0 (No such device)/d +"]) +AT_CLEANUP + AT_SETUP([interface - current speed]) AT_SKIP_IF([test $HAVE_ETHTOOL = "no"]) OVS_TRAFFIC_VSWITCHD_START()
OVS configuration is based on port names and OpenFlow port numbers. Names are stored in the database and translated later to OF ports. On the datapath level, each port has a name and a datapath port number. Port name in the database has to match datapath port name, unless it's a tunnel port. If a datapath port is renamed with 'ip link set DEV name NAME', ovs-vswitchd will wake up, destroy all the OpenFlow-related structures and clean other things up. This is because the port no longer represents the port from a database due to a name difference. However, ovs-vswitch will not actually remove the port from the datapath, because it thinks that this port is no longer there. This is happening because lookup is performed by name and the name have changed. As a result we have a port in a datapath that is not related to any port known to ovs-vswitchd and ovs-vswitchd can't remove it. This port also occupies a datapath port number and prevents the port to be added back with a new name. Fix that by performing lookup by a datapath port number during the port destruction. The name was used only to avoid spurious warnings in a normal case where the port was successfully deleted by other parts of OVS. Adding an extra flag to avoid these warnings instead. Fixes: 02f8d6460afd ("ofproto-dpif: Query port existence by name to prevent warnings.") Reported-at: https://github.com/openvswitch/ovs-issues/issues/284 Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- lib/dpctl.c | 2 +- lib/dpif.c | 13 +++++---- lib/dpif.h | 2 +- ofproto/ofproto-dpif.c | 14 ++++++---- tests/system-interface.at | 57 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 76 insertions(+), 12 deletions(-)