Ilya Maximets <i.maxim...@ovn.org> writes:
> On 7/20/23 16:55, Aaron Conole wrote:
>> Ilya Maximets <i.maxim...@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.maxim...@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 <acon...@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()
>>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
