Re: [PATCH net 2/2] selftests: add openvswitch selftest suite

From: Aaron Conole
Date: Tue Oct 25 2022 - 08:15:53 EST

Ilya Maximets <i.maximets@xxxxxxx> writes:

> On 10/20/22 18:33, Ilya Maximets wrote:
>> On 10/20/22 17:32, Aaron Conole wrote:
>>> Hi Ilya,
>>>
>>> Ilya Maximets <i.maximets@xxxxxxx> writes:
>>>
>>>> On 10/19/22 20:30, Aaron Conole wrote:
>>>>> Previous commit resolves a WARN splat that can be difficult to reproduce,
>>>>> but with the ovs-dpctl.py utility, it can be trivial. Introduce a test
>>>>> case which creates a DP, and then downgrades the feature set. This will
>>>>> include a utility 'ovs-dpctl.py' that can be extended to do additional
>>>>> work.
>>>>>
>>>>> Signed-off-by: Aaron Conole <aconole@xxxxxxxxxx>
>>>>> Signed-off-by: Kevin Sprague <ksprague0711@xxxxxxxxx>
>>>>> ---
>>>>> MAINTAINERS | 1 +
>>>>> tools/testing/selftests/Makefile | 1 +
>>>>> .../selftests/net/openvswitch/Makefile | 13 +
>>>>> .../selftests/net/openvswitch/openvswitch.sh | 216 +++++++++
>>>>> .../selftests/net/openvswitch/ovs-dpctl.py | 411 ++++++++++++++++++
>>>>> 5 files changed, 642 insertions(+)
>>>>> create mode 100644 tools/testing/selftests/net/openvswitch/Makefile
>>>>> create mode 100755 tools/testing/selftests/net/openvswitch/openvswitch.sh
>>>>> create mode 100644 tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>>>>>
>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>> index abbe88e1c50b..295a6b0fbe26 100644
>>>>> --- a/MAINTAINERS
>>>>> +++ b/MAINTAINERS
>>>>> @@ -15434,6 +15434,7 @@ S: Maintained
>>>>> W: http://openvswitch.org
>>>>> F: include/uapi/linux/openvswitch.h
>>>>> F: net/openvswitch/
>>>>> +F: tools/testing/selftests/net/openvswitch/
>>>>>
>>>>> OPERATING PERFORMANCE POINTS (OPP)
>>>>> M: Viresh Kumar <vireshk@xxxxxxxxxx>
>>>>
>>>> ...
>>>>
>>>>> +exit ${exitcode}
>>>>> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>>>>> b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>>>>> new file mode 100644
>>>>> index 000000000000..791d76b7adcd
>>>>> --- /dev/null
>>>>> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>>>>> @@ -0,0 +1,411 @@
>>>>> +#!/usr/bin/env python3
>>>>> +# SPDX-License-Identifier: GPL-2.0
>>>>> +
>>>>> +# Controls the openvswitch module. Part of the kselftest suite, but
>>>>> +# can be used for some diagnostic purpose as well.
>>>>> +
>>>>> +import logging
>>>>> +import multiprocessing
>>>>> +import socket
>>>>> +import struct
>>>>> +import sys
>>>>> +
>>>>> +try:
>>>>> + from libnl.attr import NLA_NESTED, NLA_STRING, NLA_U32, NLA_UNSPEC
>>>>> + from libnl.attr import nla_get_string, nla_get_u32
>>>>> + from libnl.attr import nla_put, nla_put_string, nla_put_u32
>>>>> + from libnl.attr import nla_policy
>>>>> +
>>>>> + from libnl.error import errmsg
>>>>> +
>>>>> + from libnl.genl.ctrl import genl_ctrl_resolve
>>>>> + from libnl.genl.genl import genl_connect, genlmsg_parse, genlmsg_put
>>>>> +
>>>>> + from libnl.handlers import nl_cb_alloc, nl_cb_set
>>>>> + from libnl.handlers import NL_CB_CUSTOM, NL_CB_MSG_IN, NL_CB_VALID
>>>>> + from libnl.handlers import NL_OK, NL_STOP
>>>>> +
>>>>> + from libnl.linux_private.netlink import NLM_F_ACK, NLM_F_DUMP
>>>>> + from libnl.linux_private.netlink import NLM_F_REQUEST, NLMSG_DONE
>>>>> +
>>>>> + from libnl.msg import NL_AUTO_SEQ, nlmsg_alloc, nlmsg_hdr
>>>>> +
>>>>> + from libnl.nl import NLMSG_ERROR, nl_recvmsgs_default, nl_send_auto
>>>>> + from libnl.socket_ import nl_socket_alloc, nl_socket_set_cb
>>>>> + from libnl.socket_ import nl_socket_get_local_port
>>>>> +except ModuleNotFoundError:
>>>>> + print("Need to install the python libnl3 library.")
>>>>
>>>>
>>>> Hey, Aaron and Kevin. Selftests sounds like a very important and
>>>> long overdue thing to add. Thanks for working on this!
>>>>
>>>> I have some worries about the libnl3 library though. It doesn't
>>>> seem to be maintained well. It it maintained by a single person,
>>>> it it was at least 3 different single persons over the last 7
>>>> years via forks. It didn't get any significant development done
>>>> since 2015 as well and no commits at all for a last 1.5 years.
>>>> It is not packaged by any major distributions.
>>>
>>> :-/ On my fedora:
>>>
>>> 11:12:24 aconole@RHTPC1VM0NT ~$ dnf search python3-libnl3
>>> Last metadata expiration check: 1 day, 0:25:11 ago on Wed 19 Oct 2022 10:47:21 AM EDT.
>>> ===================== Name Exactly Matched: python3-libnl3 =====================
>>> python3-libnl3.x86_64 : libnl3 binding for Python 3
>>>
>>>
>>> And I can use it:
>>>
>>> 11:18:39 aconole@RHTPC1VM0NT {(6a5c83bdd991...)} ~/git/linux/tools/testing/selftests/net/openvswitch$ sudo python3 ./ovs-dpctl.py show
>>> foop
>>> Lookups: Hit: 0 Missed: 0 Lost: 0
>>> Flows: 0
>>> Masks: Hit: 0 Total: 0
>>> Cache: Hit: 0
>>> Caches:
>>> Masks-cache: size: 256
>>> Port 0: foop (internal)
>>> 11:18:43 aconole@RHTPC1VM0NT {(6a5c83bdd991...)} ~/git/linux/tools/testing/selftests/net/openvswitch$ rpm -qa | grep python3-libnl3
>>> python3-libnl3-3.5.0-6.fc34.x86_64
>>> 11:19:01 aconole@RHTPC1VM0NT {(6a5c83bdd991...)} ~/git/linux/tools/testing/selftests/net/openvswitch$
>>>
>>> Was there some place you did not find it?
>>
>> You're right, I missed that somehow. But this is not an
>> https://github.com/coolshou/libnl3 project. :)
>> These are python bindings for the C libnl library:
>>
>> $ dnf info python3-libnl3
>> Available Packages
>> Name : python3-libnl3
>> Version : 3.7.0
>> Release : 1.fc36
>> Architecture : x86_64
>> Size : 153 k
>> Source : libnl3-3.7.0-1.fc36.src.rpm
>> Repository : updates
>> Summary : libnl3 binding for Python 3
>> URL : http://www.infradead.org/~tgr/libnl/
>> License : LGPLv2
>> Description : Python 3 bindings for libnl3
>
> Actually, I can't find an equivalent package for Ubuntu 22.04.
> And since pip is not an option (pip install libnl3 is a different
> package), there is no way to install it there beside building
> from sources.
>
> Am I still missing something?

Well, I have switched the latest version to using pyroute2 - hopefully
this will be acceptable :)

>>
>>>
>>>> I'm talking about https://github.com/coolshou/libnl3 . Please,
>>>> correct me if that is not the right one. There are too many
>>>> libraies with the name libnl out there... That is also not a great
>>>> sign.
>>>
>>> Yes, this is the project.
>>
>> Doensn't look like it...
>>
>>> We did look at some of the ones you
>>> mentioned, but didn't find much.
>>>
>>> It is a sparse landscape of projects that provide netlink support in
>>> python.
>>>
>>>> The C library libnl (https://github.com/thom311/libnl) seems to
>>>> be well maintained in general. It has experimental python
>>>> bindings which are not really supported much. Python bindings
>>>> received only 2 actual code-changing commits in the last 7 years.
>>>> Both of them are just python 2/3 compatibility changes.
>>>> Maybe that is not that big of a deal since it's not really a
>>>> real python library, but a wrapper on top of a main C library.
>>>> However, I didn't find these python bindings to be packaged in
>>>> distributions. And they seem to be not available in pip as well.
>>>> So, building them is kind of a pain.
>>>
>>> Well, the python libnl3 should be installable via pip3. Ex:
>>>
>>> 11:27:15 aconole@RHTPC1VM0NT ~$ pip3 install libnl3
>>> Defaulting to user installation because normal site-packages is not writeable
>>> Collecting libnl3
>>> Using cached libnl3-0.3.0-py3-none-any.whl (89 kB)
>>> Installing collected packages: libnl3
>>> Successfully installed libnl3-0.3.0
>>
>> And this is https://pypi.org/project/libnl3/, which is the
>> https://github.com/coolshou/libnl3 project. So, by installing
>> libnl3 via pip and installing python3-libnl3 from the fedora
>> you're getting two completely different libraries.
>>
>> So, which one users should use?
>>
>> I can't find python bindings for the C libnl (which is the
>> python3-libnl3 package) in pypi, so it can't be installed
>> with pip.
>>
>>
>>>
>>> So I guess that is worth something.
>>>
>>> At least on Fedora it is installable from distribution as well.
>>>
>>>> There is another option which is pyroute2. It positions itself
>>>> primarily as a netlink library and it does include an
>>>> pyroute2.netlink module indeed:
>>>> https://github.com/svinota/pyroute2/tree/master/pyroute2/netlink
>>>> See the __init__.py for usage examples.
>>>>
>>>> This one looks to me like the most trustworthy. It is actively
>>>> used by everyone in the python networking world, e.g. by OpenStack.
>>>> And it is actively developed and maintained unlike other
>>>> netlink-related python projects. It is also packaged in most of the
>>>> main distributions, so it's easy to install and use. Many people
>>>> already have it installed for other purposes.
>>>>
>>>> TBH, I didn't compare the functionality, but I'd expect that most
>>>> of the things we need are implemented.
>>>>
>>>> What do you think?
>>>
>>> We can certainly look at switching, but having a quick glance, it seems
>>> pyroute2 expects to provide the genl commands as well, so they would
>>> want us to create an ovs module in pyroute2 that includes all of the ovs
>>> family support. Of course, we can always do this just in our module,
>>> but I think it isn't the way pyroute2 project wants to be structured.
>>> More like a library that provides all the command functionality.
>>
>> What I was thinking is to import pyroute2.netlink and the
>> pyroute2.netlink.generic and go from there. But I didn't
>> look too deep on how to actually implement the functionality.
>>
>> The python bindings for the C libnl (python3-libnl3) sounds
>> like a fine option since they are actually packaged in
>> distributions (missed that in my initial reply). However,
>> the fact that you can not install them via pip and actually
>> you will install something but completely different is kind
>> of weird. This has to be at least better documented, so
>> users will know what to install and they will not try to use
>> pip for that.
>>
>>>
>>>> On the other note, I'm not a real python developer, but the code
>>>> looks more like a C than a python even for me. Firstly, I'd say
>>>> that it would be great to maintain some coding style, e.g. by
>>>> checking with flake8 and/or black. See some issues/suggestions
>>>> provided by these tools below.
>>>
>>> Agreed. BTW, on the rhel8 system I developed on:
>>>
>>> [root@wsfd-netdev60 openvswitch]# flake8 ./ovs-dpctl.py
>>> [root@wsfd-netdev60 openvswitch]#
>>>
>>> So, I guess it is probably that I should have used a different system to
>>> do the flake8 checks.
>>
>> Maybe the python version is different... I was running on f36
>> with python 3.10. Also, the list of defaults might be different.
>> flake8 doesn't use default ignore list if one is explicitly provided.
>>
>>>
>>>> Secondly, we shouldd at least use argparse for argument parsing.
>>>> It's part of the standard library since python 3.2, so doens't
>>>> require any special dependencies to be installed.
>>>
>>> Okay - I can switch to argparse. TBH, I haven't kept up with python
>>> standard library for some time.
>>
>> Well, 3.2 was released 11 years ago. :)
>>
>>>
>>>> Some parts of the code can probably be re-written to be more
>>>> "pythonic" as well, but I won't dive into that for now. I didn't
>>>> review the code deep enough for that.
>>>
>>> I have difficulty sometimes understanding what it means to be "Real
>>> Python (tm)" - I don't plan to change things too much. I can certainly
>>> switch to using argparse, but unless you give something you want to
>>> change, I would not change anything.
>>
>> I breifly looked through code and though I don't fully
>> understand what this piece supposed to do:
>>
>> + segment = hdrval.find(":")
>> + if segment == -1:
>> + segment = len(hdrval)
>> + hdrver = int(hdrval[:segment], 0)
>> + if len(hdrval[:segment]):
>> + userfeatures = int(hdrval[:segment], 0)
>>
>> but I have a strong feeling that this part can benefit
>> from use of hdrval.split(':').
>>
>> I won't insist on that too much. :)
>>
>> Best regards, Ilya Maximets.