On Mon, Jul 10, 2023 at 10:32 AM Terry Wilson <twil...@redhat.com> wrote:
>
> I accidentally forgot to click reply-to-all.
>
> On Fri, Jun 30, 2023 at 10:27 AM Ilya Maximets <i.maxim...@ovn.org> 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 <twil...@redhat.com>
> > >>>>>> ---
> > >>>>>> .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.
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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
