On 7/10/23 17:36, Terry Wilson wrote:
> On Mon, Jul 10, 2023 at 10:32 AM Terry Wilson <[email protected]> wrote:
>>
>> I accidentally forgot to click reply-to-all.
>>
>> On Fri, Jun 30, 2023 at 10:27 AM Ilya Maximets <[email protected]> wrote:
>>>
>>> On 6/30/23 16:54, Adrian Moreno wrote:
>>>>
>>>>
>>>> On 6/30/23 14:35, Ilya Maximets wrote:
>>>>> On 6/30/23 14:23, Adrian Moreno wrote:
>>>>>>
>>>>>>
>>>>>> On 6/30/23 13:40, Ilya Maximets wrote:
>>>>>>> On 6/30/23 12:39, Adrian Moreno wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 6/14/23 23:07, Terry Wilson wrote:
>>>>>>>>> This adds a Python version of the async DNS support added in:
>>>>>>>>>
>>>>>>>>> 771680d96 DNS: Add basic support for asynchronous DNS resolving
>>>>>>>>>
>>>>>>>>> The above version uses the unbound C library, and this
>>>>>>>>> implimentation uses the SWIG-wrapped Python version of that.
>>>>>>>>>
>>>>>>>>> In the event that the Python unbound library is not available,
>>>>>>>>> a warning will be logged and the resolve() method will just
>>>>>>>>> return None. For the case where inet_parse_active() is passed
>>>>>>>>> an IP address, it will not try to resolve it, so existing
>>>>>>>>> behavior should be preserved in the case that the unbound
>>>>>>>>> library is unavailable.
>>>>>>>>>
>>>>>>>>> Intentional differences from the C version are as follows:
>>>>>>>>>
>>>>>>>>> OVS_HOSTS_FILE environment variable can bet set to override
>>>>>>>>> the system 'hosts' file. This is primarily to allow testing to
>>>>>>>>> be done without requiring network connectivity.
>>>>>>>>>
>>>>>>>>> Since resolution can still be done via hosts file lookup, DNS
>>>>>>>>> lookups are not disabled when resolv.conf cannot be loaded.
>>>>>>>>>
>>>>>>>>> The Python socket_util module has fallen behind its C equivalent.
>>>>>>>>> The bare minimum change was done to inet_parse_active() to
>>>>>>>>> support
>>>>>>>>> sync/async dns, as there is no equivalent to
>>>>>>>>> parse_sockaddr_components(), inet_parse_passive(), etc. A TODO
>>>>>>>>> was added to bring socket_util.py up to equivalency to the C
>>>>>>>>> version.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Terry Wilson <[email protected]>
>>>>>>>>> ---
>>>>>>>>> .github/workflows/build-and-test.yml | 4 +-
>>>>>>>>> Documentation/intro/install/general.rst | 4 +-
>>>>>>>>> Documentation/intro/install/rhel.rst | 2 +-
>>>>>>>>> Documentation/intro/install/windows.rst | 2 +-
>>>>>>>>> NEWS | 4 +-
>>>>>>>>> debian/control.in | 1 +
>>>>>>>>> m4/openvswitch.m4 | 8 +-
>>>>>>>>> python/TODO.rst | 7 +
>>>>>>>>> python/automake.mk | 2 +
>>>>>>>>> python/ovs/dns_resolve.py | 272
>>>>>>>>> +++++++++++++++++++++++
>>>>>>>>> python/ovs/socket_util.py | 21 +-
>>>>>>>>> python/ovs/stream.py | 2 +-
>>>>>>>>> python/ovs/tests/test_dns_resolve.py | 280
>>>>>>>>> ++++++++++++++++++++++++
>>>>>>>>> python/setup.py | 6 +-
>>>>>>>>> rhel/openvswitch-fedora.spec.in | 2 +-
>>>>>>>>> tests/vlog.at | 2 +
>>>>>>>>> 16 files changed, 601 insertions(+), 18 deletions(-)
>>>>>>>>> create mode 100644 python/ovs/dns_resolve.py
>>>>>>>>> create mode 100644 python/ovs/tests/test_dns_resolve.py
>>>>>>>>>
>>>>>>>>> diff --git a/.github/workflows/build-and-test.yml
>>>>>>>>> b/.github/workflows/build-and-test.yml
>>>>>>>>> index f66ab43b0..47d239f10 100644
>>>>>>>>> --- a/.github/workflows/build-and-test.yml
>>>>>>>>> +++ b/.github/workflows/build-and-test.yml
>>>>>>>>> @@ -183,10 +183,10 @@ jobs:
>>>>>>>>> run: sudo apt update || true
>>>>>>>>> - name: install common dependencies
>>>>>>>>> run: sudo apt install -y ${{ env.dependencies }}
>>>>>>>>> - - name: install libunbound libunwind
>>>>>>>>> + - name: install libunbound libunwind python3-unbound
>>>>>>>>> # GitHub Actions doesn't have 32-bit versions of these
>>>>>>>>> libraries.
>>>>>>>>> if: matrix.m32 == ''
>>>>>>>>> - run: sudo apt install -y libunbound-dev libunwind-dev
>>>>>>>>> + run: sudo apt install -y libunbound-dev libunwind-dev
>>>>>>>>> python3-unbound
>>>>>>>>> - name: install 32-bit libraries
>>>>>>>>> if: matrix.m32 != ''
>>>>>>>>> run: sudo apt install -y gcc-multilib
>>>>>>>>> diff --git a/Documentation/intro/install/general.rst
>>>>>>>>> b/Documentation/intro/install/general.rst
>>>>>>>>> index 42b5682fd..19e360d47 100644
>>>>>>>>> --- a/Documentation/intro/install/general.rst
>>>>>>>>> +++ b/Documentation/intro/install/general.rst
>>>>>>>>> @@ -90,7 +90,7 @@ need the following software:
>>>>>>>>> If libcap-ng is installed, then Open vSwitch will automatically
>>>>>>>>> build with
>>>>>>>>> support for it.
>>>>>>>>>
>>>>>>>>> -- Python 3.4 or later.
>>>>>>>>> +- Python 3.6 or later.
>>>>>>>>>
>>>>>>>>> - Unbound library, from http://www.unbound.net, is optional but
>>>>>>>>> recommended if
>>>>>>>>> you want to enable ovs-vswitchd and other utilities to use DNS
>>>>>>>>> names when
>>>>>>>>> @@ -208,7 +208,7 @@ simply install and run Open vSwitch you require
>>>>>>>>> the following software:
>>>>>>>>> from iproute2 (part of all major distributions and available at
>>>>>>>>> https://wiki.linuxfoundation.org/networking/iproute2).
>>>>>>>>>
>>>>>>>>> -- Python 3.4 or later.
>>>>>>>>> +- Python 3.6 or later.
>>>>>>>>>
>>>>>>>>> On Linux you should ensure that ``/dev/urandom`` exists. To
>>>>>>>>> support TAP
>>>>>>>>> devices, you must also ensure that ``/dev/net/tun`` exists.
>>>>>>>>> diff --git a/Documentation/intro/install/rhel.rst
>>>>>>>>> b/Documentation/intro/install/rhel.rst
>>>>>>>>> index d1fc42021..f2151d890 100644
>>>>>>>>> --- a/Documentation/intro/install/rhel.rst
>>>>>>>>> +++ b/Documentation/intro/install/rhel.rst
>>>>>>>>> @@ -92,7 +92,7 @@ Once that is completed, remove the file
>>>>>>>>> ``/tmp/ovs.spec``.
>>>>>>>>> If python3-sphinx package is not available in your version of
>>>>>>>>> RHEL, you can
>>>>>>>>> install it via pip with 'pip install sphinx'.
>>>>>>>>>
>>>>>>>>> -Open vSwitch requires python 3.4 or newer which is not available in
>>>>>>>>> older
>>>>>>>>> +Open vSwitch requires python 3.6 or newer which is not available in
>>>>>>>>> older
>>>>>>>>> distributions. In the case of RHEL 6.x and its derivatives, one
>>>>>>>>> option is
>>>>>>>>> to install python34 from `EPEL`_.
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/intro/install/windows.rst
>>>>>>>>> b/Documentation/intro/install/windows.rst
>>>>>>>>> index 78f60f35a..fce099d5d 100644
>>>>>>>>> --- a/Documentation/intro/install/windows.rst
>>>>>>>>> +++ b/Documentation/intro/install/windows.rst
>>>>>>>>> @@ -56,7 +56,7 @@ The following explains the steps in some detail.
>>>>>>>>>
>>>>>>>>> 'C:/MinGW /mingw'.
>>>>>>>>>
>>>>>>>>> -- Python 3.4 or later.
>>>>>>>>> +- Python 3.6 or later.
>>>>>>>>>
>>>>>>>>> Install the latest Python 3.x from python.org and verify that
>>>>>>>>> its path is
>>>>>>>>> part of Windows' PATH environment variable.
>>>>>>>>> diff --git a/NEWS b/NEWS
>>>>>>>>> index cfd466663..24c694b8f 100644
>>>>>>>>> --- a/NEWS
>>>>>>>>> +++ b/NEWS
>>>>>>>>> @@ -36,7 +36,9 @@ Post-v3.1.0
>>>>>>>>> process extra privileges when mapping physical
>>>>>>>>> interconnect memory.
>>>>>>>>> - SRv6 Tunnel Protocol
>>>>>>>>> * Added support for userspace datapath (only).
>>>>>>>>> -
>>>>>>>>> + - Python
>>>>>>>>> + * Added async DNS support
>>>>>>>>> + * Dropped support for Python < 3.6
>>>>>>>>>
>>>>>>>>> v3.1.0 - 16 Feb 2023
>>>>>>>>> --------------------
>>>>>>>>> diff --git a/debian/control.in b/debian/control.in
>>>>>>>>> index 19f590d06..64b0a4ce0 100644
>>>>>>>>> --- a/debian/control.in
>>>>>>>>> +++ b/debian/control.in
>>>>>>>>> @@ -287,6 +287,7 @@ Depends:
>>>>>>>>> Suggests:
>>>>>>>>> python3-netaddr,
>>>>>>>>> python3-pyparsing,
>>>>>>>>> + python3-unbound,
>>>>>>>>> Description: Python 3 bindings for Open vSwitch
>>>>>>>>> Open vSwitch is a production quality, multilayer, software-based,
>>>>>>>>> Ethernet virtual switch. It is designed to enable massive network
>>>>>>>>> diff --git a/m4/openvswitch.m4 b/m4/openvswitch.m4
>>>>>>>>> index 14d9249b8..373a7e413 100644
>>>>>>>>> --- a/m4/openvswitch.m4
>>>>>>>>> +++ b/m4/openvswitch.m4
>>>>>>>>> @@ -371,16 +371,16 @@ dnl Checks for valgrind/valgrind.h.
>>>>>>>>> AC_DEFUN([OVS_CHECK_VALGRIND],
>>>>>>>>> [AC_CHECK_HEADERS([valgrind/valgrind.h])])
>>>>>>>>>
>>>>>>>>> -dnl Checks for Python 3.4 or later.
>>>>>>>>> +dnl Checks for Python 3.6 or later.
>>>>>>>>> AC_DEFUN([OVS_CHECK_PYTHON3],
>>>>>>>>> [AC_CACHE_CHECK(
>>>>>>>>> - [for Python 3 (version 3.4 or later)],
>>>>>>>>> + [for Python 3 (version 3.6 or later)],
>>>>>>>>> [ovs_cv_python3],
>>>>>>>>> [if test -n "$PYTHON3"; then
>>>>>>>>> ovs_cv_python3=$PYTHON3
>>>>>>>>> else
>>>>>>>>> ovs_cv_python3=no
>>>>>>>>> - for binary in python3 python3.4 python3.5 python3.6
>>>>>>>>> python3.7; do
>>>>>>>>> + for binary in python3{,.{6..12}}; do
>>>>>>>>> ovs_save_IFS=$IFS; IFS=$PATH_SEPARATOR
>>>>>>>>> for dir in $PATH; do
>>>>>>>>> IFS=$ovs_save_IFS
>>>>>>>>> @@ -397,7 +397,7 @@ else:
>>>>>>>>> done
>>>>>>>>> fi])
>>>>>>>>> if test "$ovs_cv_python3" = no; then
>>>>>>>>> - AC_MSG_ERROR([Python 3.4 or later is required but not found in
>>>>>>>>> $PATH, please install it or set $PYTHON3 to point to it])
>>>>>>>>> + AC_MSG_ERROR([Python 3.6 or later is required but not found in
>>>>>>>>> $PATH, please install it or set $PYTHON3 to point to it])
>>>>>>>>> fi
>>>>>>>>> AC_ARG_VAR([PYTHON3])
>>>>>>>>> PYTHON3=$ovs_cv_python3])
>>>>>>>>> diff --git a/python/TODO.rst b/python/TODO.rst
>>>>>>>>> index 3a53489f1..acc5461e2 100644
>>>>>>>>> --- a/python/TODO.rst
>>>>>>>>> +++ b/python/TODO.rst
>>>>>>>>> @@ -32,3 +32,10 @@ Python Bindings To-do List
>>>>>>>>>
>>>>>>>>> * Support write-only-changed monitor mode (equivalent of
>>>>>>>>> OVSDB_IDL_WRITE_CHANGED_ONLY).
>>>>>>>>> +
>>>>>>>>> +* socket_util:
>>>>>>>>> +
>>>>>>>>> + * Add equivalent fuctions to inet_parse_passive,
>>>>>>>>> parse_sockaddr_components,
>>>>>>>>> + et al. to better support using async dns. The reconnect code will
>>>>>>>>> + currently log a warning when inet_parse_active() returns w/o yet
>>>>>>>>> having
>>>>>>>>> + resolved an address, but will continue to connect and eventually
>>>>>>>>> succeed.
>>>>>>>>> diff --git a/python/automake.mk b/python/automake.mk
>>>>>>>>> index d00911828..82a508787 100644
>>>>>>>>> --- a/python/automake.mk
>>>>>>>>> +++ b/python/automake.mk
>>>>>>>>> @@ -16,6 +16,7 @@ ovs_pyfiles = \
>>>>>>>>> python/ovs/compat/sortedcontainers/sorteddict.py \
>>>>>>>>> python/ovs/compat/sortedcontainers/sortedset.py \
>>>>>>>>> python/ovs/daemon.py \
>>>>>>>>> + python/ovs/dns_resolve.py \
>>>>>>>>> python/ovs/db/__init__.py \
>>>>>>>>> python/ovs/db/custom_index.py \
>>>>>>>>> python/ovs/db/data.py \
>>>>>>>>> @@ -55,6 +56,7 @@ ovs_pyfiles = \
>>>>>>>>>
>>>>>>>>> ovs_pytests = \
>>>>>>>>> python/ovs/tests/test_decoders.py \
>>>>>>>>> + python/ovs/tests/test_dns_resolve.py \
>>>>>>>>> python/ovs/tests/test_filter.py \
>>>>>>>>> python/ovs/tests/test_kv.py \
>>>>>>>>> python/ovs/tests/test_list.py \
>>>>>>>>> diff --git a/python/ovs/dns_resolve.py b/python/ovs/dns_resolve.py
>>>>>>>>> new file mode 100644
>>>>>>>>> index 000000000..7b0f6c266
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/python/ovs/dns_resolve.py
>>>>>>>>> @@ -0,0 +1,272 @@
>>>>>>>>> +# Copyright (c) 2023 Red Hat, Inc.
>>>>>>>>> +#
>>>>>>>>> +# Licensed under the Apache License, Version 2.0 (the "License");
>>>>>>>>> +# you may not use this file except in compliance with the License.
>>>>>>>>> +# You may obtain a copy of the License at:
>>>>>>>>> +#
>>>>>>>>> +# http://www.apache.org/licenses/LICENSE-2.0
>>>>>>>>> +#
>>>>>>>>> +# Unless required by applicable law or agreed to in writing, software
>>>>>>>>> +# distributed under the License is distributed on an "AS IS" BASIS,
>>>>>>>>> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
>>>>>>>>> implied.
>>>>>>>>> +# See the License for the specific language governing permissions and
>>>>>>>>> +# limitations under the License.
>>>>>>>>> +
>>>>>>>>> +import collections
>>>>>>>>> +import enum
>>>>>>>>> +import functools
>>>>>>>>> +import ipaddress
>>>>>>>>> +import os
>>>>>>>>> +import time
>>>>>>>>> +import typing
>>>>>>>>> +
>>>>>>>>> +try:
>>>>>>>>> + import unbound # type: ignore
>>>>>>>>> +except ImportError:
>>>>>>>>> + pass
>>>>>>>>> +
>>>>>>>>> +import ovs.vlog
>>>>>>>>> +
>>>>>>>>> +vlog = ovs.vlog.Vlog("dns_resolve")
>>>>>>>>> +
>>>>>>>>> +
>>>>>>>>> +class ReqState(enum.Enum):
>>>>>>>>> + INVALID = 0
>>>>>>>>> + PENDING = 1
>>>>>>>>> + GOOD = 2
>>>>>>>>> + ERROR = 3
>>>>>>>>> +
>>>>>>>>> +
>>>>>>>>> +class DNSRequest:
>>>>>>>>> +
>>>>>>>>
>>>>>>>> nit: extra space
>>>>>>>>
>>>>>>>>> + def __init__(self, name: str):
>>>>>>>>> + self.name: str = name
>>>>>>>>> + self.state: ReqState = ReqState.INVALID
>>>>>>>>> + self.time: typing.Optional[float] = None
>>>>>>>>> + # set by DNSResolver._callback
>>>>>>>>> + self.result: typing.Optional[str] = None
>>>>>>>>> + self.ttl: typing.Optional[float] = None
>>>>>>>>> +
>>>>>>>>> + @property
>>>>>>>>> + def expired(self):
>>>>>>>>> + return time.time() > self.time + self.ttl
>>>>>>>>> +
>>>>>>>>> + @property
>>>>>>>>> + def is_valid(self):
>>>>>>>>> + return self.state == ReqState.GOOD and not self.expired
>>>>>>>>> +
>>>>>>>>> + def __str__(self):
>>>>>>>>> + return (f"DNSRequest(name={self.name}, state={self.state}, "
>>>>>>>>> + f"time={self.time}, result={self.result})")
>>>>>>>>> +
>>>>>>>>> +
>>>>>>>>> +class DefaultReqDict(collections.defaultdict):
>>>>>>>>> + def __init__(self):
>>>>>>>>> + super().__init__(DNSRequest)
>>>>>>>>> +
>>>>>>>>> + def __missing__(self, key):
>>>>>>>>> + ret = self.default_factory(key)
>>>>>>>>> + self[key] = ret
>>>>>>>>> + return ret
>>>>>>>>> +
>>>>>>>>> +
>>>>>>>>> +class UnboundException(Exception):
>>>>>>>>> + def __init__(self, message, errno):
>>>>>>>>> + try:
>>>>>>>>> + msg = f"{message}: {unbound.ub_strerror(errno)}"
>>>>>>>>> + except NameError:
>>>>>>>>> + msg = message
>>>>>>>>> + super().__init__(msg)
>>>>>>>>> +
>>>>>>>>> +
>>>>>>>>> +def dns_enabled(func):
>>>>>>>>> + @functools.wraps(func)
>>>>>>>>> + def wrapper(self, *args, **kwargs):
>>>>>>>>> + if self.dns_enabled:
>>>>>>>>> + return func(self, *args, **kwargs)
>>>>>>>>> + vlog.err("DNS support requires the python unbound library")
>>>>>>>>> + return wrapper
>>>>>>>>> +
>>>>>>>>
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> +class singleton:
>>>>>>>>> + def __init__(self, klass):
>>>>>>>>> + self._klass = klass
>>>>>>>>> + self._instance = None
>>>>>>>>> +
>>>>>>>>> + def __call__(self, *args, **kwargs):
>>>>>>>>> + if self._instance is None:
>>>>>>>>> + self._instance = self._klass(*args, **kwargs)
>>>>>>>>> + return self._instance
>>>>>>>>> +
>>>>>>>>
>>>>>>>> Having a singleton class that accepts a parameter in the constructor
>>>>>>>> could be a
>>>>>>>> bit confusing. For instance: "d = DNSResolver(is_daemon=True)"
>>>>>>>> might yield a resolver where "is_daemon" is actually "False" if some
>>>>>>>> other part
>>>>>>>> of the code created it before.
>>>>>>>>
>>>>>>>> Woudn't it be cleaner if it just exposed "resove_sync" and
>>>>>>>> "resolve_async" and
>>>>>>>> let users choose how to resolve?
>>
>> Singletons aren't really my favorite thing to use in the first place,
>> but I also didn't really like the idea of it just being a bunch of
>> global variables either. We're following the C lib here as far as
>> dns_resolve_init()/dns_resolve() being the interface, though. It does
>> allow you to re-call dns_resolve_init() with a new is_daemon value and
>> just overwrites all of the global state, which seems kind of weird to
>> me.
>>
>> I can't think of a situation where anyone would ever call
>> dns_resolve_init() or DNSResolver() multiple times in an application
>> (at least with the current implementations). It seems like if we were
>> going to really do that, both the C and Python versions should just
>> stop using global state altogether.
>
> I'm currently working on a solution that removes the singleton and
> keeps the class. Then top-level init/resolve/destroy methods that work
> on a global DNSResolver, but would also allow users to just make
> multiple resolvers if they wanted. It's really rewriting the tests
> that is the pain for this, though. Ultimately, it's most an aesthetic
> issue since I don't imagine anyone will ever write apps that
> destroy/re-init a resolver. But maybe that's just a failure of my
> imagination.
>
> With that said, it looks like we set up the ub_ctx to be able to
> handle async regardless of is_daemon value, so even in the C lib I'm
> not sure why we don't just make resolve_sync/resolve_async both
> available to end users.
The reason is the same as I described below in reply to Adrian's
first suggestion - most of the calls are made from the deep of
OVS libraries and different processes may want different behavior.
E.g. ovsdb-server will want async resolution while ovn-nbctl will
need a synchronous one, because it doesn't have any work to do
menwhile anyway.
So, we either have a global switch in the init()-style function,
or we pass is_async argument through the half of library functions.
Whoever implemented the C version desided to not touch half of
of the libraries and it kind of makes sense.
>
> Terry
>>
>>>>>>> I suppose, the problem is that most of the resolving is happening
>>>>>>> from the inside of OVS libraries. So, it will not be a choice of
>>>>>>> an application.
>>>>>>>
>>>>>>
>>>>>> Not sure I understand. Currently if a user does:
>>>>>> 'DNSResolver(is_daemon=False).resolve("openvswitch.org")', it'll resolve
>>>>>> synchronously and so will all the rest of the resolutions of that
>>>>>> program.
>>>>>>
>>>>>> The internal variable "_is_daemon" is only used to select the way to
>>>>>> resolve.
>>>>>> The same behavior would be achieved if, for instance, "is_daemon" is a
>>>>>> default
>>>>>> argument to "resolve()" with the extra benefit of the first resolution
>>>>>> not
>>>>>> influencing all the rest.
>>>>>
>>>>> My point is that inet_parse_active() is calling:
>>>>>
>>>>> host_name = dns_resolve.resolve(host_name)
>>>>>
>>>>> And we will have to add an argument to this call in order to give user
>>>>> a choice to perform sync or async resolution. As well as add this
>>>>> argument to all the functions that may call inet_parse_active() and all
>>>>> the functions that may call these functions.
>>>>>
>>>>
>>>> If it has the right default value it woudn't impact this particular caller.
>>>>
>>>>> To avoid that we have a global "is_daemon" state.
>>>>>
>>>>> Does that make sense?
>>>>
>>>> Yes. I guess I was thinking in the possibility of the dns resolver being
>>>> used
>>>> outside of inet_parse_active
>>>>
>>>>>
>>>>> Maybe we could remove the class paramater and add a function like
>>>>> dns_resolve.enable_async() instead, so the global state change will
>>>>> be explicit?
>>>>>
>>>>
>>>> That would make it clearer, yes.
>>>
>>> OK.
>>>
>>> Terry, what do you think?
>>>
>>>>
>>>>>>
>>>>>>> Maybe a warning that the parameter is different will be enough?
>>>>>>>> Best regards, Ilya Maximets.
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
