Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP][Greendns] Replace deprecated resolver.query by resolver.resolve #840

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Copy link

codecov bot commented Dec 15, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (d78f8f6) 53% compared to head (7243eeb) 23%.
Report is 2 commits behind head on master.

Files Patch % Lines
eventlet/support/greendns.py 0% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #840     +/-   ##
=======================================
- Coverage      53%    23%    -31%     
=======================================
  Files          88     88             
  Lines        9848   9983    +135     
  Branches     1841   1811     -30     
=======================================
- Hits         5299   2317   -2982     
- Misses       4164   7480   +3316     
+ Partials      385    186    -199     
Flag Coverage Δ
ipv6 23% <0%> (+<1%) ⬆️
py310epolls ?
py310poll ?
py310selects ?
py311epolls ?
py38epolls ?
py38openssl ?
py38poll ?
py38selects ?
py39dnspython1 ?
py39epolls ?
py39poll ?
py39selects ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@4383 4383 changed the title [Greendns] Replace deprecated resolver.query by resolver.resolve [WIP][Greendns] Replace deprecated resolver.query by resolver.resolve Dec 15, 2023
@4383 4383 force-pushed the dns_from_query_to_resolve branch 2 times, most recently from 7cdedd0 to ec88ef2 Compare December 15, 2023 11:31
@4383
Copy link
Member Author

4383 commented Dec 15, 2023

This topic seems a bit more complicated than just replacing a deprecated method by a new one.
I followed the suggestions made in #818, but, local tests and CI jobs started to fail with assignation errors like this one:

FAILED tests/greendns_test.py::TestUdp::test_udp_ipv6 - OSError: [Errno 99] Cannot assign requested address

I need further investigation to see what happens.

The latest build is successful because I submitted a new patch set with an empty commit, just to compare...

@blink1073: Please can you give us more details on the way you made your tests and how you triggered this warning?

@4383
Copy link
Member Author

4383 commented Dec 15, 2023

Moving to the new function lead eventlet to:

>               raise NoNameservers(request=self.request, errors=self.errors)                                          
E               dns.resolver.NoNameservers: All nameservers failed to answer the query machine. IN A: 

@blink1073
Copy link

Here's the full traceback from pymongo:

/opt/python/3.10/lib/python3.10/contextlib.py:135: in __enter__
 [2023/12/15 07:56:56.991]     return next(self.gen)
 [2023/12/15 07:56:56.991] pymongo/pool.py:1656: in checkout
 [2023/12/15 07:56:56.991]     conn = self._get_conn(handler=handler)
 [2023/12/15 07:56:56.991] pymongo/pool.py:1775: in _get_conn
 [2023/12/15 07:56:56.991]     conn = self.connect(handler=handler)
 [2023/12/15 07:56:56.991] pymongo/pool.py:1605: in connect
 [2023/12/15 07:56:56.991]     sock = _configured_socket(self.address, self.opts)
 [2023/12/15 07:56:56.991] pymongo/pool.py:1291: in _configured_socket
 [2023/12/15 07:56:56.991]     sock = _create_connection(address, options)
 [2023/12/15 07:56:56.991] pymongo/pool.py:1244: in _create_connection
 [2023/12/15 07:56:56.991]     for res in socket.getaddrinfo(host, port, family, socket.SOCK_STREAM):
 [2023/12/15 07:56:56.991] .tox/test-eg/lib/python3.10/site-packages/eventlet/support/greendns.py:549: in getaddrinfo
 [2023/12/15 07:56:56.991]     qname, addrs = _getaddrinfo_lookup(host, family, flags)
 [2023/12/15 07:56:56.991] .tox/test-eg/lib/python3.10/site-packages/eventlet/support/greendns.py:511: in _getaddrinfo_lookup
 [2023/12/15 07:56:56.991]     answer = resolve(host, qfamily, False, use_network=use_network)
 [2023/12/15 07:56:56.991] .tox/test-eg/lib/python3.10/site-packages/eventlet/support/greendns.py:456: in resolve
 [2023/12/15 07:56:56.991]     return _proxy.query(name, rdtype, raise_on_no_answer=raises,
 [2023/12/15 07:56:56.991] .tox/test-eg/lib/python3.10/site-packages/eventlet/support/greendns.py:412: in query
 [2023/12/15 07:56:56.991]     return end()
 [2023/12/15 07:56:56.991] .tox/test-eg/lib/python3.10/site-packages/eventlet/support/greendns.py:391: in end
 [2023/12/15 07:56:56.991]     raise result[1]
 [2023/12/15 07:56:56.991] .tox/test-eg/lib/python3.10/site-packages/eventlet/support/greendns.py:372: in step
 [2023/12/15 07:56:56.991]     a = fun(*args, **kwargs)
 [2023/12/15 07:56:56.991] _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
 [2023/12/15 07:56:56.991]     def query(
 [2023/12/15 07:56:56.991]         self,
 [2023/12/15 07:56:56.991]         qname: Union[dns.name.Name, str],
 [2023/12/15 07:56:56.991]         rdtype: Union[dns.rdatatype.RdataType, str] = dns.rdatatype.A,
 [2023/12/15 07:56:56.991]         rdclass: Union[dns.rdataclass.RdataClass, str] = dns.rdataclass.IN,
 [2023/12/15 07:56:56.991]         tcp: bool = False,
 [2023/12/15 07:56:56.991]         source: Optional[str] = None,
 [2023/12/15 07:56:56.991]         raise_on_no_answer: bool = True,
 [2023/12/15 07:56:56.991]         source_port: int = 0,
 [2023/12/15 07:56:56.991]         lifetime: Optional[float] = None,
 [2023/12/15 07:56:56.991]     ) -> Answer:  # pragma: no cover
 [2023/12/15 07:56:56.991]         """Query nameservers to find the answer to the question.
 [2023/12/15 07:56:56.991]         This method calls resolve() with ``search=True``, and is
 [2023/12/15 07:56:56.991]         provided for backwards compatibility with prior versions of
 [2023/12/15 07:56:56.991]         dnspython.  See the documentation for the resolve() method for
 [2023/12/15 07:56:56.991]         further details.
 [2023/12/15 07:56:56.991]         """
 [2023/12/15 07:56:56.991] >       warnings.warn(
 [2023/12/15 07:56:56.991]             "please use dns.resolver.Resolver.resolve() instead",
 [2023/12/15 07:56:56.991]             DeprecationWarning,
 [2023/12/15 07:56:56.991]             stacklevel=2,
 [2023/12/15 07:56:56.991]         )
 [2023/12/15 07:56:56.991] E       DeprecationWarning: please use dns.resolver.Resolver.resolve() instead

Our code that uses dnspython directly is here.

We test eventlet integration by applying this patch and then invoking pytest.main on our full test suite.

@4383
Copy link
Member Author

4383 commented Dec 19, 2023

Thanks for the details. Unit tests should be fixed too, I need to rework my patch.

@@ -400,7 +400,8 @@ def end():
return end()

# Main query
step(self._resolver.query, qname, rdtype, rdclass, tcp, source, raise_on_no_answer=False)
step(self._resolver.resolve, qname, rdtype, rdclass, tcp, source,
raise_on_no_answer=False, search=True)
Copy link
Member Author

@4383 4383 Dec 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolve and query doesn't have the same signature, and internally query call resolve with search=True, while this option is set to None by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use of deprecated dnspython method
2 participants