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 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
